Add support for AWS_CONTAINER_CREDENTIALS_FULL_URI (EKS Pod Identity Agent)#449
Open
LockedThread wants to merge 5 commits intodurch:masterfrom
Open
Add support for AWS_CONTAINER_CREDENTIALS_FULL_URI (EKS Pod Identity Agent)#449LockedThread wants to merge 5 commits intodurch:masterfrom
LockedThread wants to merge 5 commits intodurch:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds feature-gated container authorization token retrieval and extends Credentials::from_container_credentials_provider to support ECS RELATIVE_URI and EKS FULL_URI metadata endpoints, optionally sending an Authorization header. Includes tests and test utilities to validate token sourcing and precedence. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant CP as CredentialsProvider
participant TS as TokenSource\n(File / Env)
participant MS as MetadataServer\n(ECS RELATIVE_URI / EKS FULL_URI)
App->>CP: from_container_credentials_provider()
alt FULL_URI path
CP->>TS: get_container_authorization_token()
TS-->>CP: token (optional)
CP->>MS: GET FULL_URI with Authorization header
else RELATIVE_URI path
CP->>MS: GET RELATIVE_URI (no auth)
end
MS-->>CP: credential JSON
CP-->>App: Credentials
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
Brownian Motion (Brass)Recommendation: Refactor Summary: PR adds support for EKS Pod Identity, but complexity may outweigh benefits. Highlights
Unknowns
Next actions
Reflection questions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
aws-creds/src/credentials.rs (2)
344-350: Add a rustdoc example forfrom_container_credentials_provider.The updated public API docs explain precedence and auth behavior, but they still lack a concrete usage example.
As per coding guidelines, "Provide documentation examples for all public APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aws-creds/src/credentials.rs` around lines 344 - 350, Add a rustdoc example for the public function from_container_credentials_provider that shows how to construct and use the provider: demonstrate calling from_container_credentials_provider(), await-ing/getting credentials, and illustrate the precedence order (AWS_CONTAINER_CREDENTIALS_RELATIVE_URI checked before AWS_CONTAINER_CREDENTIALS_FULL_URI) and how an Authorization header is supplied from AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE or AWS_CONTAINER_AUTHORIZATION_TOKEN; mark the example with the #[cfg(feature = "http-credentials")] note in the docs and include a short snippet that sets the relevant env vars (or token file) and retrieves credentials so users can copy/paste to verify behavior.
196-201: Avoid silently masking token-file misconfiguration.If
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILEis set but unreadable at Line 197, the code silently falls back. For explicit file configuration, returning an error is usually safer and easier to debug.Proposed refactor
- fn get_container_authorization_token() -> Option<String> { + fn get_container_authorization_token() -> Result<Option<String>, CredentialsError> { if let Ok(path) = env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE") { - if let Ok(token) = std::fs::read_to_string(path) { - return Some(token.trim_end().to_string()); - } + let token = std::fs::read_to_string(path)?; + return Ok(Some(token.trim_end().to_string())); } - env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN").ok() + Ok(env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN").ok()) }- let token = get_container_authorization_token(); + let token = get_container_authorization_token()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aws-creds/src/credentials.rs` around lines 196 - 201, The code silently falls back to AWS_CONTAINER_AUTHORIZATION_TOKEN when AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE is set but std::fs::read_to_string fails; change the logic so that if AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE is present but unreadable you return/propagate an explicit error (rather than silently falling back) — detect env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE") being Ok and, on read_to_string error, return a Result::Err (or otherwise propagate a io::Error with context) referencing the failing std::fs::read_to_string call and the AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE variable instead of proceeding to env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN").ok().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aws-creds/src/credentials.rs`:
- Around line 669-709: The three tests
(test_get_container_authorization_token_from_file,
test_get_container_authorization_token_from_env,
test_get_container_authorization_token_file_precedence) mutate process-global
env vars without synchronizing on CONTAINER_CREDENTIALS_TEST_LOCK; update each
test to acquire the same lock at the start (e.g. let _guard_lock =
CONTAINER_CREDENTIALS_TEST_LOCK.lock().unwrap();) so they use the shared mutex
while setting/removing env vars via EnvGuard and calling
get_container_authorization_token(), preventing races in parallel test runs.
- Around line 352-365: The code currently accepts any env-provided URL in the
url variable and may forward auth_token to it; parse and validate the URL before
making the request by calling Url::parse(&full_uri) (or equivalent) and ensure
the scheme is http or https and the host is in a trusted allowlist (reject
otherwise, e.g. return Err(CredentialsError::NotContainer)). Only call
get_container_authorization_token() and attach the Authorization header to the
request created by apply_timeout(attohttpc::get(&url)) if the parsed URL passes
validation; do not attach the token for untrusted or invalid URIs.
---
Nitpick comments:
In `@aws-creds/src/credentials.rs`:
- Around line 344-350: Add a rustdoc example for the public function
from_container_credentials_provider that shows how to construct and use the
provider: demonstrate calling from_container_credentials_provider(),
await-ing/getting credentials, and illustrate the precedence order
(AWS_CONTAINER_CREDENTIALS_RELATIVE_URI checked before
AWS_CONTAINER_CREDENTIALS_FULL_URI) and how an Authorization header is supplied
from AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE or
AWS_CONTAINER_AUTHORIZATION_TOKEN; mark the example with the #[cfg(feature =
"http-credentials")] note in the docs and include a short snippet that sets the
relevant env vars (or token file) and retrieves credentials so users can
copy/paste to verify behavior.
- Around line 196-201: The code silently falls back to
AWS_CONTAINER_AUTHORIZATION_TOKEN when AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE is
set but std::fs::read_to_string fails; change the logic so that if
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE is present but unreadable you
return/propagate an explicit error (rather than silently falling back) — detect
env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE") being Ok and, on
read_to_string error, return a Result::Err (or otherwise propagate a io::Error
with context) referencing the failing std::fs::read_to_string call and the
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE variable instead of proceeding to
env::var("AWS_CONTAINER_AUTHORIZATION_TOKEN").ok().
Brownian Motion (Brass)Recommendation: Refactor Summary: PR adds support for EKS Pod Identity, but complexity may outweigh benefits. Highlights
Unknowns
Next actions
Reflection questions
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
aws-creds/src/credentials.rs (1)
344-350: Add a rustdoc example for this public API.The method-level docs explain behavior well, but this public API still lacks an example snippet. Adding a short
no_runexample for RELATIVE_URI/FULL_URI usage would satisfy the project doc standard.Suggested doc addition
/// Load credentials from container metadata (ECS task role or EKS Pod Identity). /// /// Checks `AWS_CONTAINER_CREDENTIALS_RELATIVE_URI` first (ECS), then /// `AWS_CONTAINER_CREDENTIALS_FULL_URI` (EKS Pod Identity Agent, etc.). /// When using `FULL_URI`, optionally sends an `Authorization` header from /// `AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE` or `AWS_CONTAINER_AUTHORIZATION_TOKEN`. + /// + /// # Example + /// + /// ```no_run + /// use awscreds::Credentials; + /// + /// // In ECS, set AWS_CONTAINER_CREDENTIALS_RELATIVE_URI. + /// // In EKS Pod Identity, set AWS_CONTAINER_CREDENTIALS_FULL_URI + /// // (optionally with AWS_CONTAINER_AUTHORIZATION_TOKEN[_FILE]). + /// let creds = Credentials::from_container_credentials_provider().unwrap(); + /// ```As per coding guidelines, "Provide documentation examples for all public APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aws-creds/src/credentials.rs` around lines 344 - 350, Add a rustdoc example (annotated with no_run) to the public method Credentials::from_container_credentials_provider() demonstrating typical usage for ECS (set AWS_CONTAINER_CREDENTIALS_RELATIVE_URI) and EKS Pod Identity (set AWS_CONTAINER_CREDENTIALS_FULL_URI and optional AWS_CONTAINER_AUTHORIZATION_TOKEN or AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE), returning a Credentials instance; place the example in the doc comment for the same function (keeping the existing #[cfg(feature = "http-credentials")] guard) and ensure it follows the project's doc style (short, shows environment variables and a call to Credentials::from_container_credentials_provider() and unwrap()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@aws-creds/src/credentials.rs`:
- Around line 344-350: Add a rustdoc example (annotated with no_run) to the
public method Credentials::from_container_credentials_provider() demonstrating
typical usage for ECS (set AWS_CONTAINER_CREDENTIALS_RELATIVE_URI) and EKS Pod
Identity (set AWS_CONTAINER_CREDENTIALS_FULL_URI and optional
AWS_CONTAINER_AUTHORIZATION_TOKEN or AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE),
returning a Credentials instance; place the example in the doc comment for the
same function (keeping the existing #[cfg(feature = "http-credentials")] guard)
and ensure it follows the project's doc style (short, shows environment
variables and a call to Credentials::from_container_credentials_provider() and
unwrap()).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the container credentials provider to support
AWS_CONTAINER_CREDENTIALS_FULL_URIand the optional authorization token environment variables (AWS_CONTAINER_AUTHORIZATION_TOKEN,AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE). This allows applications to obtain credentials when running with AWS EKS Pod Identity Agent or other credential endpoints that use a full URI and bearer token (e.g. AWS Greengrass Token Exchange, custom sidecars).Why this is important
http://169.254.170.23/v1/credentials). The agent expects the client to set:AWS_CONTAINER_CREDENTIALS_FULL_URIto that URLAWS_CONTAINER_AUTHORIZATION_TOKEN_FILE(orAWS_CONTAINER_AUTHORIZATION_TOKEN) with the pod identity tokenFULL_URIsupport,Credentials::default()(and the rest of the chain) never considers the container provider when only these env vars are set, so pods using EKS Pod Identity getNoCredentialsand cannot call S3 or other AWS services with this crate.Prior work
This builds on the container credentials support added in Priorize ECS/EKS credentials over EC2's when available (PR #441). That PR introduced
from_container_credentials_provider()and prioritized it over EC2 instance metadata, but only forAWS_CONTAINER_CREDENTIALS_RELATIVE_URI(used by ECS task IAM roles). This change adds theFULL_URIpath and authorization token handling so both ECS-style and EKS Pod Identity–style container credentials are supported by the same provider.Changes
aws-creds/src/credentials.rsfrom_container_credentials_provider()now:AWS_CONTAINER_CREDENTIALS_RELATIVE_URIif set (unchanged; buildshttp://169.254.170.2{path}).AWS_CONTAINER_CREDENTIALS_FULL_URIif set.FULL_URI, the request optionally includes anAuthorizationheader:AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE(file path) if set.AWS_CONTAINER_AUTHORIZATION_TOKEN.get_container_authorization_token()implements the token lookup (file precedence over env).#[cfg(feature = "http-credentials")]):test_container_credentials_not_container_when_no_env– neither URI set →NotContainer.test_container_credentials_relative_uri_precedence– both set → RELATIVE_URI is used.test_container_credentials_full_uri_used_when_no_relative– only FULL_URI set → that URL is used.test_get_container_authorization_token_from_file– token read from file.test_get_container_authorization_token_from_env– token from env var.test_get_container_authorization_token_file_precedence– file overrides env.Environment variables (after this PR)
AWS_CONTAINER_CREDENTIALS_RELATIVE_URI169.254.170.2(ECS task role).AWS_CONTAINER_CREDENTIALS_FULL_URIAWS_CONTAINER_AUTHORIZATION_TOKEN_FILEAuthorizationheader.AWS_CONTAINER_AUTHORIZATION_TOKENAuthorizationheader.Behavior matches the AWS SDK / container credential provider (RELATIVE_URI precedence, FULL_URI, optional auth token).
Testing
cargo test -p aws-creds --features http-credentials– all new and existing tests pass.This change is
Summary by CodeRabbit
New Features
Improvements
Tests