fix(aws): use default credential chain#403
Conversation
Signed-off-by: Oleg Leizerov <oleizerov@nvidia.com>
Signed-off-by: Oleg Leizerov <oleizerov@nvidia.com>
Greptile SummaryThis PR fixes the AWS provider so that it uses the AWS SDK v2 default credential chain when no explicit credentials are supplied, instead of unconditionally calling the EC2 instance-role IMDS provider. This unblocks EKS Pod Identity and IRSA, whose credentials are injected into the pod environment and resolved by the SDK chain but were previously bypassed.
Confidence Score: 5/5Safe to merge — the change removes a custom credential-fetching loop and delegates entirely to the well-tested AWS SDK default chain, with explicit creds still taking highest priority and a pre-flight 401 guard added for early failure detection. The logic simplification is straightforward: explicit credentials continue to work as before, and the removal of the manual EC2 IMDS loop is replaced by an already-battle-tested SDK mechanism. The pre-flight Retrieve() check is a net improvement for operator visibility. New tests cover all branching paths including credential expiry and refresh. No existing behavior for users supplying explicit credentials is changed. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Loader called with cfg.Creds] --> B{len creds == 0?}
B -->|Yes| C[credsProvider = nil\nlog: SDK default chain]
B -->|No| D[decodeCredentials]
D -->|Error| E[HTTP 400 Bad Request]
D -->|OK| F[NewStaticCredentialsProvider\ncredsProvider = static]
C --> G[clientFactory per region]
F --> G
G --> H[loadAWSConfig\nconfig.WithRegion]
H -->|credsProvider != nil| I[config.WithCredentialsProvider\nstatic creds]
H -->|credsProvider == nil| J[SDK default chain\nenv → shared config → IRSA\n→ Pod Identity → IMDS]
I --> K[ec2.NewFromConfig\nClient.credentials = awsCfg.Credentials]
J --> K
K --> L{client.credentials != nil?}
L -->|Yes| M[credentials.Retrieve]
M -->|Error| N[HTTP 401 Unauthorized]
M -->|OK| O[DescribeInstanceTopology]
L -->|No| O
O -->|Error| P[HTTP 502 Bad Gateway]
O -->|OK| Q[Return topology]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Loader called with cfg.Creds] --> B{len creds == 0?}
B -->|Yes| C[credsProvider = nil\nlog: SDK default chain]
B -->|No| D[decodeCredentials]
D -->|Error| E[HTTP 400 Bad Request]
D -->|OK| F[NewStaticCredentialsProvider\ncredsProvider = static]
C --> G[clientFactory per region]
F --> G
G --> H[loadAWSConfig\nconfig.WithRegion]
H -->|credsProvider != nil| I[config.WithCredentialsProvider\nstatic creds]
H -->|credsProvider == nil| J[SDK default chain\nenv → shared config → IRSA\n→ Pod Identity → IMDS]
I --> K[ec2.NewFromConfig\nClient.credentials = awsCfg.Credentials]
J --> K
K --> L{client.credentials != nil?}
L -->|Yes| M[credentials.Retrieve]
M -->|Error| N[HTTP 401 Unauthorized]
M -->|OK| O[DescribeInstanceTopology]
L -->|No| O
O -->|Error| P[HTTP 502 Bad Gateway]
O -->|OK| Q[Return topology]
Reviews (2): Last reviewed commit: "fix(aws): preserve credential error stat..." | Re-trigger Greptile |
|
/ok-to-test 4473a9e |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-403.docs.buildwithfern.com/topograph |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Solid refactor — moving to the SDK default credential chain is the right call, and the new tests genuinely exercise precedence and auto-refresh rather than asserting SDK tautologies. Two behavior changes worth a quick confirm below.
- One precedence change worth confirming is intentional: the old resolver only consulted request creds, then AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, then the EC2 instance role — it never read ~/.aws/config, ~/.aws/credentials, or AWS_PROFILE. The full default chain places shared-config/profile (and IRSA) ahead of the instance role, so a host with stray ~/.aws state or an exported AWS_PROFILE that previously fell through to instance-role creds will now silently authenticate as that profile identity. The docs cover the env / Pod Identity case; worth extending the caveat to shared-config/profile too. (pkg/providers/aws/provider.go:126)
- With no explicit creds, credential-resolution failure is now deferred to the API call: the old ec2rolecreds path returned 401 at load time when instance-role creds couldn't be retrieved, whereas an unresolvable default chain now surfaces as the 502 from DescribeInstanceTopology. #177 deliberately curated these status codes, so worth deciding whether a no-credentials failure should still present as 401 rather than 502. (pkg/providers/aws/provider.go:104)
| SecretAccessKey: secretAccessKey, | ||
| Token: sessionToken, | ||
| }, nil | ||
| return config.LoadDefaultConfig(ctx, opts...) |
There was a problem hiding this comment.
One precedence change worth confirming is intentional: the old resolver only consulted request creds, then AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, then the EC2 instance role — it never read ~/.aws/config, ~/.aws/credentials, or AWS_PROFILE. The full default chain places shared-config/profile (and IRSA) ahead of the instance role, so a host with stray ~/.aws state or an exported AWS_PROFILE that previously fell through to instance-role creds will now silently authenticate as that profile identity. The docs cover the env / Pod Identity case; worth extending the caveat to shared-config/profile too.
There was a problem hiding this comment.
Yes, the expanded precedence is intentional because the goal is to use the standard SDK chain. I’ll make the warning explicit for AWS_PROFILE and shared config/credentials files.
| func getCredentialsProvider(creds map[string]any) (aws.CredentialsProvider, *httperr.Error) { | ||
| if len(creds) == 0 { | ||
| klog.Infof("Using AWS SDK default credential chain") | ||
| return nil, nil |
There was a problem hiding this comment.
With no explicit creds, credential-resolution failure is now deferred to the API call: the old ec2rolecreds path returned 401 at load time when instance-role creds couldn't be retrieved, whereas an unresolvable default chain now surfaces as the 502 from DescribeInstanceTopology. #177 deliberately curated these status codes, so worth deciding whether a no-credentials failure should still present as 401 rather than 502.
There was a problem hiding this comment.
Good catch. The status-code change was not intentional. I’ll preserve the previous 401 for credential-resolution failures while keeping EC2 API authorization errors as 502.
Signed-off-by: Oleg Leizerov <oleizerov@nvidia.com>
3 participants
Description
The AWS provider currently calls the EC2 instance-role credential provider whenever Topograph-specific credentials are absent. That bypasses the AWS SDK default credential chain, so EKS Pod Identity and IRSA credentials injected into the Topograph pod are ignored and the EC2 node role is used instead.
This change:
ec2:DescribeInstanceTopologypermission.Tests cover invalid and valid explicit credentials, explicit-over-environment precedence, environment credentials, EKS container credentials with an authorization token file, and refresh after expiration.
Validation
make qualifyAWS integration test
Validated commit
4473a9eon thenkx-slinky-gb300-dev-01EKS cluster:topographServiceAccount and its EKS Pod Identity association with the dedicatedaehiqzudem-topographIAM role.AWS_EC2_METADATA_DISABLED=trueto prevent fallback to the EC2 node role.The request used the AWS SDK default credential chain, extracted topology for 35 instances, updated the test ConfigMap, and completed with HTTP 200. No authorization errors occurred.
Checklist
git commit -s).