-
Notifications
You must be signed in to change notification settings - Fork 183
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
Support EKS Pod Identity credentials #3416
Conversation
9032e5e
to
a414ee1
Compare
This looks great! Someone from our team will do a detailed review next week. The one missing piece is a e2e integration test. You can see examples here: https://github.com/smithy-lang/smithy-rs/tree/6e3e010e912874262b37727d900d7fae170efaed/aws/rust-runtime/aws-config/test-data/default-provider-chain/ecs_credentials_invalid_profile |
8b1ed5f
to
41c72e0
Compare
done @rcoh |
41c72e0
to
111307e
Compare
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.
This is a really high quality contribution, thank you! ❤️
There are just a couple of things that need to be changed before we can merge.
.await | ||
.expect("localhost is the loopback interface"); | ||
validate_full_uri("http://169.254.170.2.backname.io:8888/creds", dns.clone()) |
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.
This definitely works, but I'm a little hesitant to have our tests rely on backname.io since it adds some level of risk to our release process if backname.io happens to go down for a period of time. Could you refactor the test to use a fake ResolveDns
trait implementation instead of the real one?
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.
this test is skipped because it already relies on dns resolution (of amazon.com)
see comment // ignored by default because it relies on actual DNS resolution
Is there some release process that is unskipping it?
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.
Oh, nope. I just didn't see that comment. Looks good then.
This brings this provider in line with other sdks by supporting AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE as well as permitting http IPs to be non-loopback if they are the EKS pod identity IPs. Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Signed-off-by: Jack Kleeman <jackkleeman@gmail.com>
Co-authored-by: John DiSanti <john@vinylsquid.com>
9186416
to
9b353f7
Compare
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.
Looks great. Thank you!
Motivation and Context
I would like to support EKS Pod Identity credentials in the Rust SDKs
Description
This brings the ECS provider in line with other sdks (eg, Go) by supporting AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE as well as permitting http IPs to be non-loopback if they are the EKS pod identity IPs.
Testing
I have added various new unit tests, and I have updated the existing integration test to also create pods with eks pod identity creds, which I have used to test in a real EKS cluster as well.
Checklist
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.