Skip to content

fix(credentials/aws): resolve Self account (empty role ARN) to ambient creds (closes #605) - #612

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/issue-605-self-account-resolver
May 20, 2026
Merged

fix(credentials/aws): resolve Self account (empty role ARN) to ambient creds (closes #605)#612
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/issue-605-self-account-resolver

Conversation

@cristim

@cristim cristim commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • resolveRoleARNProvider returned a hard error when AWSRoleARN=="" (the "Self" account shape), while the scheduler's collectAWSForAccount treated the same shape as the ambient-creds case, causing every approve action against the CUDly host account to fail.
  • Fixed by using opts.AmbientProvider (new AWSResolveOptions field) when AWSRoleARN is empty; falls back to a descriptive error when ambient is nil.
  • Threaded awsCfg.Credentials as AmbientAWSCreds through purchase.Manager (new field) and the commitment-opts closure in server/app.go so both callers supply a valid ambient provider.

Test plan

  • TestResolveAWSCredentialProvider_SelfAccount_WithAmbient - Self shape + ambient returns ambient, no error
  • TestResolveAWSCredentialProvider_SelfAccount_NilAmbient - Self shape + nil ambient returns descriptive error
  • TestResolveAWSCredentialProvider_RoleARN_NoARN_NilAmbient - back-compat wrapper (nil ambient) returns descriptive error
  • Existing role_arn-with-ARN path regression: all 83 credential tests pass
  • All purchase tests pass (132)
  • Full build clean (go build ./...)

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for Self account configuration using ambient AWS credentials, eliminating cross-account role ARN requirements in certain scenarios.
  • Bug Fixes

    • Improved error messages during AWS credential resolution for better troubleshooting and diagnostics.
  • Tests

    • Expanded test coverage for Self account credential resolution scenarios.

Review Change Stack

…t creds (closes #605)

When an account is registered with auth_mode=role_arn and aws_role_arn=""
(the "Self" shape), the collector already treats it as an ambient-creds
case (collectAWSForAccount early-returns to ambient when AWSRoleARN=="").
The executor's resolver path through resolveRoleARNProvider rejected the
same shape with a hard error, causing every approve action against the
CUDly host account to fail.

Fix: pass AWSResolveOptions.AmbientProvider through the role_arn switch
case; resolveRoleARNProvider returns it directly when AWSRoleARN is empty,
falling back to a descriptive error when ambient is nil. Thread ambient
creds (awsCfg.Credentials) through purchase.Manager and the commitment-opts
closure in server/app.go so the two callers that previously used the back-
compat wrapper now supply a valid ambient provider.

Tests: positive (Self+ambient returns ambient, no error), negative
(Self+nil ambient returns descriptive error), regression (existing
role_arn-with-ARN path unchanged, all 83 credential tests pass).
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/now Drop other things impact/all-users Affects every user effort/s Hours type/bug Defect labels May 20, 2026
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 08b744fb-d130-4ee2-a3c7-f4e918086558

📥 Commits

Reviewing files that changed from the base of the PR and between bc81d15 and 2a41db3.

📒 Files selected for processing (6)
  • internal/credentials/resolver.go
  • internal/credentials/resolver_extra_test.go
  • internal/credentials/resolver_test.go
  • internal/purchase/execution.go
  • internal/purchase/manager.go
  • internal/server/app.go

📝 Walkthrough

Walkthrough

This PR enables AWS credential resolution for "Self" accounts—those configured with role_arn auth mode but no role ARN—by threading an injected ambient AWS credentials provider through the resolver, purchase manager, and server configuration. The resolver now returns the ambient provider when the role ARN is empty and available, or returns a descriptive error when it is not.

Changes

Self-Account Ambient Credential Resolution

Layer / File(s) Summary
Resolver contract and empty ARN handling
internal/credentials/resolver.go
AWSResolveOptions gains AmbientProvider field. ResolveAWSCredentialProviderWithOpts passes it to resolveRoleARNProvider, which now returns the ambient provider for empty AWSRoleARN when available, or errors with a descriptive message otherwise.
Resolver test coverage
internal/credentials/resolver_test.go, internal/credentials/resolver_extra_test.go
Test suite replaces the old missing-ARN test with three new tests: one for back-compat wrapper with nil ambient, one for successful self-account resolution with ambient, and one for nil ambient with self-account. Bastion test updated to expect the ambient-unavailable error message.
Purchase manager configuration
internal/purchase/manager.go
ManagerConfig and Manager struct each gain an AmbientAWSCreds field to accept and store the ambient provider. NewManager initializes it. AWS SDK import added.
Purchase execution using ambient provider
internal/purchase/execution.go
resolveAWSProvider switches to ResolveAWSCredentialProviderWithOpts, passing the manager's stored ambient provider through AWSResolveOptions.
Server app integration
internal/server/app.go
reinitializeAfterConnect wires awsCfg.Credentials into the purchase manager via AmbientAWSCreds. Commitment-options credential resolution uses the same ambient-provider options.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • LeanerCloud/CUDly#605: Implements the core feature of plumbing an ambient AWS credentials provider to resolve self-account credential scenarios instead of erroring on empty role ARN.

Possibly related PRs

  • LeanerCloud/CUDly#233: Both PRs modify multi-account AWS credential resolution in internal/purchase; the referenced PR adds regression tests for purchase execution isolation when credential resolution fails per account.

Poem

🐰 A rabbit hops through credentials anew,
"Self" accounts now trusted, through and through,
Ambient providers flow down the chain,
No more cross-account role-ARN pain!
Where role ARNs sleep, these creds shall reign.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding support for resolving 'Self' accounts (empty role ARN) by falling back to ambient AWS credentials, which directly addresses issue #605.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-605-self-account-resolver

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 2bba813 into feat/multicloud-web-frontend May 20, 2026
4 checks passed
@cristim
cristim deleted the fix/issue-605-self-account-resolver branch May 20, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/all-users Affects every user priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/now Drop other things

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant