-
Notifications
You must be signed in to change notification settings - Fork 637
Prefer SSM Credentials over EC2Role Credentials for ECS-A (External=true). #4788
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
Conversation
48a1bad
to
8d539af
Compare
I think I'm out of my depth on the Linux Unit Test Failures, I'm running on Linux and seeing that same tests pass:
and
I did check the go version:
So I don't quite match 1.24 .3 versus 1.24.6, but I'd be very surprised if it's that. Is there any known thing I need to make sure my environment matches to get the same results as the test environment? It's also possible I'm just very naive, I'm not seeing these tests in the output when I run |
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.
Thanks for your contribution!
…SSM over EC2 Creds
Sorry I tried to just accept your recommendations in the GitHub UI, but that (obviously) didn't run |
Prefer SSM Credentials over EC2Role Credentials for ECS-A (External=true).
Summary
This change updates the Linux credential provider chain to prefer the
RotatingSharedCredentialsProvider
overEC2RoleProvider
. The motivation is that, when running ECS-A on an EC2 instance that has been registered with SSM, the EC2 Role credentials were being used whenever the EC2 instance had an IAM role. This caused theRegisterContainerInstance
attempt to fail with the error "The identity document and identity document signature were not valid".Implementation details
This change updates the Linux credential provider chain, and was based off this stale PR: #4155. I tried to follow the feedback provided there, by combining the Linux and Windows files into one, and using the same order of precedence for both. I cannot say I fully understand the implications there, I was just trying to follow instructions.
I also would love a little bit of confirmation from someone who knows better, that this won't break people. The scenario I'm concerned about is if a user had used an EC2 instance as a container instance for an ECS Cluster, but had incorrectly labeled it External. I couldn't figure it out in just the day today, but I'm worried that would "work" because the EC2 Role was being used, and this change would make it stop "working" because the SSM cred is going to be selected instead. Hopefully that already didn't work, and there's no way this change can break anyone, but I did want to call it out, just in case.
Testing
make test
- this actually failed on my machine, but only with some version number differences coming fromamazon-ecs-cni-plugins
thing. I also ran all the unit tests in theecs-agent
package withgo test -tags=unit ./...
and by clicking the button on my IDE, all passed.I had to update one test, because it had different conditions for Windows and Linux, which are now the same, and added one test, to make sure the
RotatingSharedCredentialsProvider
was used ahead of theEC2RoleProvider
when External=true.And here's the one test that did fail on me:
New tests cover the changes: Yes
Description for the changelog
Enhancement - Use same precedence for credential providers in Windows and Linux
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No
Does this PR include the addition of new environment variables in the README?
No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.