-
Notifications
You must be signed in to change notification settings - Fork 230
Priorize ECS/EKS credentials over EC2's when available #441
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
base: master
Are you sure you want to change the base?
Conversation
- Move container credentials logic out of the IMDSv1 function - Priorize it over IMDSv2 like before durch#378
WalkthroughSupport for container-based AWS credential retrieval is added via a new Changes
Sequence DiagramsequenceDiagram
participant Code as Credential Resolver
participant Env as Environment
participant Container as Container Metadata<br/>(if available)
participant EC2 as EC2 Metadata<br/>(if available)
Code->>Env: Check AWS_CONTAINER_CREDENTIALS_RELATIVE_URI
alt Container environment detected
Code->>Container: Fetch credentials from<br/>container endpoint
Container-->>Code: Return credentials
else Not in container
Code->>Env: Check for EC2 IAM role
Code->>EC2: Fetch role credentials
EC2-->>Code: Return credentials
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
aws-creds/src/error.rs (1)
7-8: NewNotContainervariant is consistent with existing error model, but note semver impactThe new
NotContainercase mirrorsNotEc2nicely and gives a clear signal for non-container environments. Just be aware this is a public enum, so adding a variant can break downstream exhaustivematches onCredentialsError; if you care about strict semver for this crate, consider calling this out in the changelog or marking the enum#[non_exhaustive]in a future breaking release.aws-creds/src/credentials.rs (1)
330-350: Container credentials provider looks correct; consider minor robustness improvementsThe implementation matches the ECS task credentials model well:
- It keys off
AWS_CONTAINER_CREDENTIALS_RELATIVE_URIand returnsNotContainerwhen unset, which is exactly what you need for chaining.- It targets
http://169.254.170.2{RELATIVE_URI}and reusesCredentialsFromInstanceMetadata, aligning the JSON shape with the existing IMDS parsing.- The mapping into
Credentialsis consistent with the IMDS flows (security_tokenpopulated fromtoken,session_tokenleftNone).Two optional follow-ups you might consider (non-blocking):
Support
AWS_CONTAINER_CREDENTIALS_FULL_URIas well
AWS also exposes container credentials via a full URI env var; handling that as a first branch and falling back toRELATIVE_URIwould make this more broadly compatible.Reuse the existing HTTP helper for consistency
You could slightly simplify and standardize behavior by going throughhttp_get(or a small shared helper) instead of inliningapply_timeout(attohttpc::get(...)).send()?, mirroring how STS requests are made.If you’d like, I can sketch a small refactor that adds FULL_URI support while keeping behavior unchanged for existing callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
aws-creds/src/credentials.rs(2 hunks)aws-creds/src/error.rs(1 hunks)
🔇 Additional comments (2)
aws-creds/src/credentials.rs (2)
296-301: Credential resolution order now correctly prefers ECS/EKS over EC2 instance metadataPlacing
from_container_credentials_provider()betweenfrom_sts_env("aws-creds")and the IMDSv2/IMDSv1 calls achieves the stated goal: whenAWS_CONTAINER_CREDENTIALS_RELATIVE_URIis present, container credentials win over EC2 instance metadata; when it’s absent, the chain naturally falls through to IMDSv2 then IMDSv1. The error mapping toNoCredentialsat the end keeps the external behavior stable.
353-369: IMDSv1 path cleanly separated from container logic and guarded byis_ec2The new
from_instance_metadataimplementation is focused solely on EC2 IMDSv1:
- Early
is_ec2check withNotEc2preserves the previous “don’t hit metadata off-EC2” behavior.- Role name and credentials are fetched via the standard IMDSv1 URLs, using the same
CredentialsFromInstanceMetadatastruct as other flows.- Container-specific handling is gone from here, which avoids conflating ECS/EKS with raw EC2 metadata and matches the new container provider design.
This separation aligns well with the PR goal and keeps the IMDSv1 code straightforward.
This change is
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.