-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix https container-credentials endpoints to match other AWS SDKs #3584
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
Fix https container-credentials endpoints to match other AWS SDKs #3584
Conversation
|
Hi @blairdrummond, Thanks for bringing this to our attention! It does appear we have some drift from the Container credential provider specification we use across SDKs. The initial implementation was one of AWS' earliest ones and predates support for arbitrary HTTPS endpoints. I think we'd be happy to add this but will likely need to make a few minor changes to your implementation to be fully compliant. Would you be alright if I add a couple commits onto this PR so we can get it merged? |
|
Absolutely! Please feel free to add commits @nateprewitt and let me know if there's anything I can assist with 🙏 |
|
Hey @nateprewitt ! Not to nag, but lmk if there's anything I can do to assist! |
|
Thanks for checking in @blairdrummond! We're in the middle of re:Invent prep right now but this is at the top of my list once that's wrapped up. I can let you know if we need anything else. |
|
Checking in as promised. I started the work to get the missing tests we have internally that should have shipped with this feature. The lift to get those working is larger than I originally thought. In order to get this fixed for you now, I'm just adding a changelog and we can ship it. We'll do the follow up work to get the test suite more inline with where it should be. Thanks again for bringing this to our attention, @blairdrummond! I'll merge this once tests finish successfully. |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3584 +/- ##
===========================================
- Coverage 93.17% 92.71% -0.46%
===========================================
Files 68 68
Lines 15411 15561 +150
===========================================
+ Hits 14359 14428 +69
- Misses 1052 1133 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @nateprewitt !! |
|
No problem! This should go out in the next release. |
Relates to #2515
Description of changes:
I discovered that the Container Credential Provider options behave differently in boto compared to all other AWS SDKs that I've checked. When supplying an
AWS_CONTAINER_CREDENTIALS_FULL_URIendpoint, the other SDKs allow anyhttpsendpoint, whereasbotodoes not allow any URL unless it's one of the special addresses (even if it's HTTPS).The tests in this repo were aware of this behaviour, but I think this code pre-dates most other SDK implementations, which seem to have provided a new convention. I have updated the tests to address this. You can see below the implementations of this behaviour in the other major SDKs
Note on #2515
If using HTTPS on
host.docker.internal, then #2515 would be resolved. This wouldn't be out-of-the-box though without using some CA certs on the client-side. (That might be fine though)