-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
amazon-ssm-agent: Add dynamically-linked agent binaries #22
Conversation
# Files copied by tmpfiles.d | ||
install -d %{buildroot}%{_cross_tmpfilesdir} | ||
install -p -m 0644 %{S:3} %{buildroot}%{_cross_tmpfilesdir}/amazon-ssm-agent-tmpfiles.conf | ||
|
||
install -d %{buildroot}%{_cross_factorydir}%{_cross_sysconfdir}/amazon/ssm | ||
install -m 0644 %{S:2} %{buildroot}%{_cross_factorydir}%{_cross_sysconfdir}/amazon/ssm/amazon-ssm-agent.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned, no need to include a default config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned, no need to include a default config file.
Why not? Does this config file effectively end up just listing what would be the defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reason for including the custom config amazon-ssm-agent.json
and the tmpfiles.d to install it was to make it as easy as possible for downstream users to change this config (and our new setting UseShell
) as necessary.
But I also understand the case that we do not want to include this in the core-kit itself. I'm fine with either approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned, no need to include a default config file.
Why not? Does this config file effectively end up just listing what would be the defaults?
My reasoning was that it wouldn't be possible for downstreams to override this configuration if we provide it through the package, right? RPM will complain if the downstreams provide the same file twice (unless you know a magic trick in RPM to prevent that). What I thought was that we could provide the factory file, with reasonable defaults, and let downstreams decide whether they want to use it with a tmpfiles.d
file. If they want to provide their own, they can provide their own factory and their own tmpfiles.d
conf file so that they copy the correct file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was that it wouldn't be possible for downstreams to override this configuration if we provide it through the package, right?
This config file amazon-ssm-agent.json
is essentially the default configs + our new config for UseShell
. So if downstreams want override, they can modify this file (or provide their own amazon-ssm-agent.json
).
What I thought was that we could provide the factory file
Can you clarify what you mean by "factory file"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @arnaldo2792 offline. We are keeping amazon-ssm-agent.json
in %{_cross_factorydir}%{_cross_sysconfdir}
. Customers can opt to take this config by adding a tmpfiles.d for that target or create/install their own config.
packages/amazon-ssm-agent/0001-agent-Add-config-to-make-shell-optional.patch
Outdated
Show resolved
Hide resolved
packages/amazon-ssm-agent/0001-agent-Add-config-to-make-shell-optional.patch
Show resolved
Hide resolved
Signed-off-by: Kush Upadhyay <kushupad@amazon.com>
New commit makes the following major changes:
|
Issue number: #19
Closes #19
Description of changes:
Making the following modifications to the
amazon-ssm-agent
package:amazon-ssm-agent.spec
bash
plugin
subpackages which vendor the original statically-linked agent binaries0001-agent-Add-config-to-make-shell-optional.patch
UseShell
which is a boolean set tofalse
by default. If this config isfalse
, thenbash
is not used when running commands -- instead usingexec
. Iftrue
, it usesbash
like the default Agent.amazon-ssm-agent.json
UseShell
setting along with defaults installed in%{_cross_factorydir}%{_cross_sysconfdir}
. Depending on whether they want to usebash
, downstream users can opt to take this config by adding a tmpfiles.d for that target or create and install their own config.amazon-ssm-agent.service
RestartSec
lowered from 90 to 5. Journal logs show no unexpected behavior as a result of this reduction in time.Also updating the
ecs-agent.spec
in theecs-agent
package to use the newssm-agent-plugin
subpackage.Testing done:
Launched an
aws-dev
variant instance with theamazon-ssm-agent
package added and host-containers disabled.When it is built with
UseShell
set tofalse
, running a command with a bash script fails:While other commands still succeed. Example running
apiclient get os
:When it is built with
UseShell
set totrue
, the bash script runs successfully as expected.Regarding the subpackaging of the statically-linked binaries, I launched an
aws-ecs-1
variant instance with theamazon-ssm-agent-plugin
package added and verified it is able to join a cluster + ECS exec also works.Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.