Skip to content

fix(aws): use default credential chain#403

Open
olegleyz wants to merge 4 commits into
NVIDIA:mainfrom
olegleyz:fix/aws-default-credential-chain
Open

fix(aws): use default credential chain#403
olegleyz wants to merge 4 commits into
NVIDIA:mainfrom
olegleyz:fix/aws-default-credential-chain

Conversation

@olegleyz

@olegleyz olegleyz commented Jul 15, 2026

Copy link
Copy Markdown

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:

  • keeps request/config-file credentials as the highest-priority explicit override;
  • leaves the credentials provider unset otherwise, allowing the AWS SDK to resolve environment, shared config, web identity, container/Pod Identity, and EC2 IMDS credentials;
  • preserves the SDK credentials cache so temporary credentials refresh automatically;
  • documents the default chain, EKS Pod Identity setup, and the least-privilege ec2:DescribeInstanceTopology permission.

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 qualify
  • AWS provider race tests repeated 10 times

AWS integration test

Validated commit 4473a9e on the nkx-slinky-gb300-dev-01 EKS cluster:

  1. Deployed an isolated Topograph API using the topograph ServiceAccount and its EKS Pod Identity association with the dedicated aehiqzudem-topograph IAM role.
  2. Set AWS_EC2_METADATA_DISABLED=true to prevent fallback to the EC2 node role.
  3. Triggered a full topology generation request.

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

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • All commits are signed off per DCO (git commit -s).

Signed-off-by: Oleg Leizerov <oleizerov@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Signed-off-by: Oleg Leizerov <oleizerov@nvidia.com>
@olegleyz
olegleyz marked this pull request as ready for review July 16, 2026 00:38
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where the AWS provider always fell back to the EC2 instance-role credential provider (ec2rolecreds) when Topograph-specific credentials were absent, bypassing EKS Pod Identity, IRSA, and other SDK-managed credential sources. The fix removes that manual fallback and delegates to config.LoadDefaultConfig without an explicit credentials provider, which invokes the full AWS SDK default credential chain including the credential cache.

  • provider.go: getCredentials is replaced by getCredentialsProvider, which returns a StaticCredentialsProvider for explicit creds or nil for the default chain; loadAWSConfig conditionally attaches the static provider so the SDK picks the right chain.
  • instance_topology.go: Adds an early credential validation step before the EC2 API call; returns HTTP 401 when credentials cannot be retrieved rather than surfacing a potentially confusing 502.
  • provider_test.go: New test suite covers invalid credentials, valid explicit credentials, explicit-over-environment precedence, environment credential resolution, container/Pod Identity credential fetch, and automatic refresh after expiry.

Confidence Score: 5/5

Safe to merge — the change correctly delegates credential resolution to the AWS SDK default chain, removing a hand-rolled EC2 IMDS fallback that blocked EKS Pod Identity and IRSA. No existing credential paths are removed; explicit static credentials and environment variables still work through the SDK chain.

The refactor is small and well-contained: two provider files change, the new behaviour is fully exercised by five new integration-style tests, and the change was validated end-to-end on an EKS cluster with EC2 metadata disabled. The old busy-wait retry loop and its race-prone timing constant are cleanly removed.

No files require special attention.

Important Files Changed

Filename Overview
pkg/providers/aws/provider.go Core credential logic refactored: explicit static provider or nil (default SDK chain); loadAWSConfig helper conditionally attaches the provider; old getCredentialsFromProvider/ec2rolecreds loop removed cleanly.
pkg/providers/aws/instance_topology.go Adds a per-region credential pre-check that short-circuits with HTTP 401 when Retrieve() fails; EC2 API call proceeds only when credentials are retrievable.
pkg/providers/aws/provider_test.go Comprehensive new tests added: invalid credentials, valid explicit creds, explicit-over-env precedence, environment credentials, container credentials with auth token file, and automatic refresh after expiry.
pkg/providers/aws/imds.go Minor cleanup: removes the unused tokenTimeDelay constant and its time import since the busy-wait retry loop in getCredentialsFromProvider was deleted.
docs/providers/aws.md Documentation updated to reflect least-privilege IAM permission, two auth modes, credential precedence, and EKS Pod Identity setup instructions.
CHANGELOG.md Changelog entry added describing the credential chain fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Loader called with cfg.Creds] --> B{Creds empty?}
    B -- Yes --> C[return nil provider\nSDK default chain]
    B -- No --> D[Decode explicit credentials]
    D --> E{Decode error?}
    E -- Yes --> F[HTTP 400 Bad Request]
    E -- No --> G[StaticCredentialsProvider]
    C --> H[loadAWSConfig without provider]
    G --> I[loadAWSConfig with static provider]
    H --> J[SDK default chain\nenv, shared config, web identity,\ncontainer/Pod Identity, EC2 IMDS]
    I --> K[Static provider overrides SDK chain]
    J --> L[Client with awsCfg.Credentials set]
    K --> L
    L --> M[generateRegionInstanceTopology]
    M --> N{credentials != nil?}
    N -- Yes --> O[credentials.Retrieve]
    O --> P{Retrieve error?}
    P -- Yes --> Q[HTTP 401 Unauthorized]
    P -- No --> R[EC2 DescribeInstanceTopology]
    N -- No --> R
    R --> S{EC2 error?}
    S -- Yes --> T[HTTP 502 Bad Gateway]
    S -- No --> U[Process topology output]
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/aws-default..." | Re-trigger Greptile

@dmitsh

dmitsh commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

/ok-to-test 4473a9e

@github-actions

Copy link
Copy Markdown
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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)
  2. 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...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@olegleyz
olegleyz requested a review from ArangoGutierrez July 22, 2026 18:06
@olegleyz olegleyz self-assigned this Jul 22, 2026
Signed-off-by: Oleg Leizerov <olegleyz@gmail.com>

@ArangoGutierrez ArangoGutierrez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second-round review — both items from the previous round are addressed:

  1. The credential-precedence caveat now documents that environment and shared-profile sources (AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, AWS_PROFILE, ~/.aws/config, ~/.aws/credentials) can take priority over EKS Pod Identity or an EC2 instance role.
  2. A missing/unresolvable credential now surfaces as 401 again, via the pre-flight Retrieve check before the EC2 call, with tests covering the 401 and 502 paths plus a real credential-refresh test against a fake container-credentials endpoint.

No new issues found — checked for bugs, Go conventions, test quality, security, and CLAUDE.md compliance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants