Suggest profiles when no auth config; clean up legacy keyring entries#2154
Merged
Suggest profiles when no auth config; clean up legacy keyring entries#2154
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads AtmosConfiguration through the auth-list flow to suggest profiles when no auth is configured, adds realm propagation into global auth copies, and implements cleanup of legacy pre-realm keyring entries and credential files after authentication or credential storage. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (auth list)
participant Cmd as Cobra Command
participant Atmos as AtmosConfig
participant Profile as ProfileManager
participant User as Terminal
CLI->>Cmd: invoke auth list
Cmd->>Atmos: loadAuthManagerForList(cmd)
Atmos-->>CLI: (authManager, atmosConfig)
CLI->>Profile: suggestProfilesForAuth(atmosConfig)
Profile-->>CLI: profiles[]
alt profiles found
CLI->>User: return ErrAuthNotConfigured with hints (profile names, --profile)
else no profiles
CLI->>User: continue without suggestion
end
sequenceDiagram
participant Auth as AuthManager
participant Chain as AuthChain/Steps
participant Keyring as CredentialStore
participant FS as Filesystem
participant Logger as Logger
Auth->>Chain: Authenticate()
Chain-->>Auth: credentials (session or non-session)
alt non-session credentials stored
Auth->>Keyring: store credentials
Auth->>Auth: deleteLegacyKeyringEntry(alias)
Auth->>FS: deleteLegacyCredentialFiles()
Logger-->>Auth: log deletions or non-existence
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/auth/realm_mismatch_test.go (1)
295-349: These cleanup cases want one table-driven test.The three new tests only vary by realm/store setup and expected outcome. Folding them into a single table will make future realm cases much cheaper to add and keep this file aligned with the repo’s test style.
As per coding guidelines, "
**/*_test.go: Use table-driven tests for testing multiple scenarios in Go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/realm_mismatch_test.go` around lines 295 - 349, Combine the three tests into one table-driven test that iterates scenarios for different realm/store setups and expected outcomes: create a slice of test cases with fields like name, store data (using realmAwareTestStore), realm (realm.RealmInfo), and expectedError bool; loop over cases with t.Run and inside call m := &manager{credentialStore: tc.store, realm: tc.realm} followed by m.deleteLegacyKeyringEntry("my-identity") and assertions that check Retrieve results based on tc.expectedError; keep the existing test logic (checks for deletion/no-op/nil store not panicking) and reuse identifiers from the diff such as deleteLegacyKeyringEntry, realmAwareTestStore, manager, and realm.RealmInfo so the refactor is purely structural and preserves behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth_list.go`:
- Around line 403-417: The helper loadAuthManagerForList currently calls
cfg.InitCliConfig with an empty schema.ConfigAndStacksInfo, ignoring global
selection flags; change the function signature to accept the cobra command (cmd)
and use flags.ParseGlobalFlags(cmd, v) to populate a ConfigAndStacksInfo
instance, then pass that populated struct into cfg.InitCliConfig so --profile,
--config, --base-path, etc. are respected; keep existing error wrapping and the
subsequent createAuthManager(&atmosConfig.Auth, atmosConfig.CliConfigPath) usage
unchanged and add a regression test for the --profile case.
In `@pkg/auth/realm_mismatch_test.go`:
- Around line 361-406: Remove the hand-rolled realmAwareTestStore and instead
use a generated mock for the CredentialStore interface (or extend the existing
generated test double); add or update a //go:generate mockgen directive to
produce a mock for the CredentialStore, then modify the tests to configure the
mock's Store, Retrieve, Delete, List, IsExpired and Type method
expectations/returns (using types.ICredentials where needed) to simulate
per-realm behavior rather than keeping realmAwareTestStore with methods Store,
Retrieve, Delete, List, IsExpired, Type in the test file.
In `@pkg/auth/realm_mismatch.go`:
- Around line 130-133: The current Delete error handling in the
m.credentialStore.Delete(alias, "") call treats all errors as "No legacy keyring
entry to delete"; change it to detect the store's not-found condition and only
log the "no legacy keyring entry" message for that case, while logging a
distinct, detailed error (including the error value) for any other failures so
real keyring/backend errors are visible; update the branch around
m.credentialStore.Delete(...) to check for the store-specific not-found sentinel
(or call like errors.Is(err, store.ErrNotFound) / err ==
credentialstore.ErrNotFound) and call log.Debug(...) for not-found but
log.Error(...) (with alias and err) for other errors.
---
Nitpick comments:
In `@pkg/auth/realm_mismatch_test.go`:
- Around line 295-349: Combine the three tests into one table-driven test that
iterates scenarios for different realm/store setups and expected outcomes:
create a slice of test cases with fields like name, store data (using
realmAwareTestStore), realm (realm.RealmInfo), and expectedError bool; loop over
cases with t.Run and inside call m := &manager{credentialStore: tc.store, realm:
tc.realm} followed by m.deleteLegacyKeyringEntry("my-identity") and assertions
that check Retrieve results based on tc.expectedError; keep the existing test
logic (checks for deletion/no-op/nil store not panicking) and reuse identifiers
from the diff such as deleteLegacyKeyringEntry, realmAwareTestStore, manager,
and realm.RealmInfo so the refactor is purely structural and preserves behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 385c631a-2975-4ea2-963a-085a8d162bcd
📒 Files selected for processing (6)
cmd/auth_list.gocmd/auth_list_test.gopkg/auth/manager_chain.gopkg/auth/manager_whoami.gopkg/auth/realm_mismatch.gopkg/auth/realm_mismatch_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2154 +/- ##
==========================================
+ Coverage 77.40% 77.43% +0.02%
==========================================
Files 962 962
Lines 91318 91406 +88
==========================================
+ Hits 70686 70779 +93
+ Misses 16551 16543 -8
- Partials 4081 4084 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/auth/manager.go`:
- Around line 234-240: The docstring for deleteLegacyKeyringEntry in
realm_mismatch.go is inaccurate about when it runs; update the comment to
reflect that deleteLegacyKeyringEntry (and deleteLegacyCredentialFiles) are
invoked early in Authenticate (before authenticateChain and buildWhoamiInfo) and
therefore remove legacy entries unconditionally rather than only "after
credentials have been successfully stored under the current realm." Edit the
docstring to state the actual contract: these functions perform proactive
cleanup at the start of the Authenticate flow and may remove legacy credentials
even if subsequent authentication fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a0bf080-b163-45a2-8680-75a03a829a1c
📒 Files selected for processing (3)
pkg/auth/manager.gopkg/auth/realm_mismatch.gopkg/auth/realm_mismatch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/realm_mismatch.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/auth_list.go (1)
402-417:⚠️ Potential issue | 🟠 MajorThis helper still drops
--profile.
schema.ConfigAndStacksInfois still assembled by hand and only forwards three selection flags.atmos auth list --profile <name>will keep loading the base config here and can incorrectly trigger the new suggestion path. Please use the shared global-flag parser instead of copying a subset.Based on learnings, "
cfg.InitCliConfigmust receive aschema.ConfigAndStacksInfopopulated viaflags.ParseGlobalFlags(cmd, v)becauseLoadConfigreads config selection fields directly from that struct; passing an empty struct makes--base-path,--config,--config-path, and--profilesilently ignored."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_list.go` around lines 402 - 417, loadAuthManagerForList builds schema.ConfigAndStacksInfo manually and omits the --profile (and other global) flags, causing cfg.InitCliConfig to receive an incomplete struct; replace the manual assembly with the shared global-flag parser by calling flags.ParseGlobalFlags(cmd, &configAndStacksInfo) (or the equivalent used project-wide) so schema.ConfigAndStacksInfo is fully populated (including profile, base-path, config, config-path) before calling cfg.InitCliConfig in loadAuthManagerForList.
🧹 Nitpick comments (1)
pkg/auth/realm_mismatch_test.go (1)
334-355: Please cover the lock-file and directory cleanup paths too.
deleteLegacyCredentialFilesnow removescredentials.lock,config.lock, and then prunes empty directories, but this test only asserts the two main files. A couple more assertions here would close the remaining coverage gap around the new behavior.As per coding guidelines, "Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/realm_mismatch_test.go` around lines 334 - 355, The test TestDeleteLegacyCredentialFiles_CleansUpNoRealmFiles only checks deletion of credentials and config; enhance it to also create credentials.lock and config.lock in providerDir before calling m.deleteLegacyCredentialFiles() and assert those lock files are removed (os.IsNotExist), and finally assert that the providerDir (or the empty ancestor directories like atmos/aws/my-provider) have been pruned by checking that filepath.Join(parent, "atmos", "aws", "my-provider") (or its parents) does not exist after cleanup; reference the manager.deleteLegacyCredentialFiles function and the files named "credentials.lock" and "config.lock" when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth_list.go`:
- Around line 155-161: The suggestion gating is using the filtered results
(filteredProviders/filteredIdentities) after applyFilters, which can be empty
even when auth is configured; change the condition so
suggestProfilesForAuth(atmosConfig) is invoked based on the unfiltered auth
config (e.g., check atmosConfig.Auth.Providers and atmosConfig.Auth.Identities
or the original providers/identities variables) instead of
filteredProviders/filteredIdentities, ensuring profiles are suggested when any
providers or identities exist in the config.
In `@pkg/auth/realm_mismatch.go`:
- Around line 123-177: The helpers deleteLegacyKeyringEntry and
deleteLegacyCredentialFiles are being invoked before authenticateChain, which
prematurely purges legacy credentials even if authentication or writing the new
realm-scoped credentials fails; move the calls so they run only after a
successful authentication and after the code that writes the new realm-scoped
credentials completes (i.e., remove any pre-auth calls to
deleteLegacyKeyringEntry and deleteLegacyCredentialFiles and invoke them in the
post-success path immediately after authenticateChain returns successfully and
after the new credential write routine finishes).
---
Duplicate comments:
In `@cmd/auth_list.go`:
- Around line 402-417: loadAuthManagerForList builds schema.ConfigAndStacksInfo
manually and omits the --profile (and other global) flags, causing
cfg.InitCliConfig to receive an incomplete struct; replace the manual assembly
with the shared global-flag parser by calling flags.ParseGlobalFlags(cmd,
&configAndStacksInfo) (or the equivalent used project-wide) so
schema.ConfigAndStacksInfo is fully populated (including profile, base-path,
config, config-path) before calling cfg.InitCliConfig in loadAuthManagerForList.
---
Nitpick comments:
In `@pkg/auth/realm_mismatch_test.go`:
- Around line 334-355: The test
TestDeleteLegacyCredentialFiles_CleansUpNoRealmFiles only checks deletion of
credentials and config; enhance it to also create credentials.lock and
config.lock in providerDir before calling m.deleteLegacyCredentialFiles() and
assert those lock files are removed (os.IsNotExist), and finally assert that the
providerDir (or the empty ancestor directories like atmos/aws/my-provider) have
been pruned by checking that filepath.Join(parent, "atmos", "aws",
"my-provider") (or its parents) does not exist after cleanup; reference the
manager.deleteLegacyCredentialFiles function and the files named
"credentials.lock" and "config.lock" when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1ab234d-99b1-49f6-b293-5963add9c070
📒 Files selected for processing (3)
cmd/auth_list.gopkg/auth/realm_mismatch.gopkg/auth/realm_mismatch_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
pkg/auth/manager.go (1)
279-285:⚠️ Potential issue | 🟠 MajorKeep the target identity out of this cleanup loop for keyring-backed creds.
Line 283 still iterates over
identityName, but inpkg/auth/manager_whoami.go, Lines 43-51,buildWhoamiInfoonly removes that legacy alias afterStore(identityName, creds, m.realm.Value)succeeds. For non-session credentials, this can still delete the fallback entry before the new realm-scoped write lands.Suggested fix
m.deleteLegacyCredentialFiles() for _, step := range chain { + if step == identityName && !isSessionToken(finalCreds) { + continue + } m.deleteLegacyKeyringEntry(step) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager.go` around lines 279 - 285, The cleanup loop is removing the target identity's legacy keyring entry prematurely; change the loop that calls m.deleteLegacyKeyringEntry(step) so it skips the current identityName (the target identity used in buildWhoamiInfo/Store) — i.e., if step == identityName then continue — ensuring the target's legacy alias is not deleted before Store(identityName, creds, m.realm.Value) completes; keep m.deleteLegacyCredentialFiles() as-is.cmd/auth_list.go (1)
403-418:⚠️ Potential issue | 🟠 Major
loadAuthManagerForListstill drops--profile.Line 407 hand-copies only
base-path,config, andconfig-pathintoConfigAndStacksInfo;ProfilesFromArgnever gets populated.atmos auth list --profile <name>can still load the base config and then suggest the same profile the user already selected. Please use the standard global flag parser here instead of open-coding a subset.Based on learnings,
cfg.InitCliConfigmust receive aschema.ConfigAndStacksInfopopulated viaflags.ParseGlobalFlags(cmd, v)becauseLoadConfigreads config selection fields directly from that struct; passing an empty or partial struct makes--base-path,--config,--config-path, and--profilesilently ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_list.go` around lines 403 - 418, loadAuthManagerForList currently hand-copies a subset of flags into a schema.ConfigAndStacksInfo and therefore drops --profile; replace the manual flag handling with the standard parser by calling flags.ParseGlobalFlags(cmd, &configAndStacksInfo) (or the equivalent global flag parsing helper) to populate schema.ConfigAndStacksInfo before calling cfg.InitCliConfig so ProfilesFromArg (and other global flags) are respected; update references to configAndStacksInfo, loadAuthManagerForList, cfg.InitCliConfig, and flags.ParseGlobalFlags accordingly.pkg/auth/realm_mismatch.go (2)
131-133:⚠️ Potential issue | 🟡 MinorSurface real keyring cleanup failures.
Right now every delete miss/backend failure lands in the same debug path. Please keep the expected “already gone” case at debug, but promote unexpected keyring/backend errors so migration cleanup failures stay visible.
Based on learnings, "Provide clear error messages to users, include troubleshooting hints when appropriate, and log detailed errors for debugging."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/realm_mismatch.go` around lines 131 - 133, The delete call in m.credentialStore.Delete (pkg/auth/realm_mismatch.go) currently logs all failures at debug; change it to distinguish the expected "already gone" case from real backend errors: detect the not-found condition using the credential store's sentinel error (e.g., keyring.ErrNotFound or a provided IsNotFound/IsNotExist helper) or by matching the well-known error text, and keep that case at Debug with alias context; for any other error log at Error (or Warn) level with full error details and alias so migration cleanup failures are visible and actionable.
145-177:⚠️ Potential issue | 🟠 MajorDon’t purge every legacy provider tree after one login.
This helper removes legacy files for all
aws/<provider>/...directories, even though a successful auth only refreshes one root provider chain. That means logging into one provider can erase fallback files for unrelated providers that have not been re-cached under the current realm yet. Please scope this to the provider that just wrote fresh state, or move it behind a provider-owned cleanup hook.Based on learnings, "The auth manager in pkg/auth/manager.go should remain cloud-agnostic and not contain AWS-specific logic or references to specific cloud providers. Keep the manager generic and extensible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/realm_mismatch.go` around lines 145 - 177, The deleteLegacyCredentialFiles method on manager currently deletes every provider subtree under awsDirNameForMismatch, which can purge unrelated providers after a single login; change it to only target the specific provider that just refreshed state by accepting (or using) a provider identifier instead of iterating all entries — e.g., update deleteLegacyCredentialFiles to take a providerName (or make it a method on the provider/credential owner) and only remove files under filepath.Join(awsDirNameForMismatch, providerName), or alternatively remove this AWS-specific logic from manager and call a provider-owned cleanup hook from the provider implementation; also remove direct AWS-specific symbols from the generic auth manager so it remains cloud-agnostic (use an injected provider cleanup interface rather than awsDirNameForMismatch inside manager).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/auth_list.go`:
- Around line 403-418: loadAuthManagerForList currently hand-copies a subset of
flags into a schema.ConfigAndStacksInfo and therefore drops --profile; replace
the manual flag handling with the standard parser by calling
flags.ParseGlobalFlags(cmd, &configAndStacksInfo) (or the equivalent global flag
parsing helper) to populate schema.ConfigAndStacksInfo before calling
cfg.InitCliConfig so ProfilesFromArg (and other global flags) are respected;
update references to configAndStacksInfo, loadAuthManagerForList,
cfg.InitCliConfig, and flags.ParseGlobalFlags accordingly.
In `@pkg/auth/manager.go`:
- Around line 279-285: The cleanup loop is removing the target identity's legacy
keyring entry prematurely; change the loop that calls
m.deleteLegacyKeyringEntry(step) so it skips the current identityName (the
target identity used in buildWhoamiInfo/Store) — i.e., if step == identityName
then continue — ensuring the target's legacy alias is not deleted before
Store(identityName, creds, m.realm.Value) completes; keep
m.deleteLegacyCredentialFiles() as-is.
In `@pkg/auth/realm_mismatch.go`:
- Around line 131-133: The delete call in m.credentialStore.Delete
(pkg/auth/realm_mismatch.go) currently logs all failures at debug; change it to
distinguish the expected "already gone" case from real backend errors: detect
the not-found condition using the credential store's sentinel error (e.g.,
keyring.ErrNotFound or a provided IsNotFound/IsNotExist helper) or by matching
the well-known error text, and keep that case at Debug with alias context; for
any other error log at Error (or Warn) level with full error details and alias
so migration cleanup failures are visible and actionable.
- Around line 145-177: The deleteLegacyCredentialFiles method on manager
currently deletes every provider subtree under awsDirNameForMismatch, which can
purge unrelated providers after a single login; change it to only target the
specific provider that just refreshed state by accepting (or using) a provider
identifier instead of iterating all entries — e.g., update
deleteLegacyCredentialFiles to take a providerName (or make it a method on the
provider/credential owner) and only remove files under
filepath.Join(awsDirNameForMismatch, providerName), or alternatively remove this
AWS-specific logic from manager and call a provider-owned cleanup hook from the
provider implementation; also remove direct AWS-specific symbols from the
generic auth manager so it remains cloud-agnostic (use an injected provider
cleanup interface rather than awsDirNameForMismatch inside manager).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2adf2568-2fed-434e-8ac9-8ba5fc819748
📒 Files selected for processing (4)
cmd/auth_list.gopkg/auth/manager.gopkg/auth/realm_mismatch.gopkg/auth/realm_mismatch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/auth/realm_mismatch_test.go
osterman
force-pushed
the
osterman/auth-suggest-profile
branch
from
7a7c49d to
27ecd00
Compare
…g entries - Add profile discovery hint to `atmos auth list` when no providers/identities configured - Use error builder pattern with ErrAuthNotConfigured sentinel for rich error messages - Suggest using --profile flag to load auth from profiles (e.g. profiles/developers/) - Clean up legacy (pre-realm) keyring entries after successful credential caching - Prevent realm mismatch warnings by removing outdated keyring entries - Add comprehensive tests for profile suggestion logic and edge cases Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
After switching to realm-scoped credentials (auth.realm), old credentials stored without a realm triggered a persistent warning on every command. The cleanup now runs in Authenticate() so it executes even when cached credentials allow the provider auth step to be skipped. Removes both legacy keyring entries and file-based credentials at the no-realm path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Pass global flags (--base-path, --config, --config-path) through loadAuthManagerForList so config selection flags are respected - Fix misleading error log in deleteLegacyKeyringEntry to report actual keyring failures instead of "no legacy entry" - Replace hand-rolled realmAwareTestStore with generated MockCredentialStore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…h.go Update deleteLegacyKeyringEntry and deleteLegacyCredentialFiles docstrings to accurately reflect that they run early in the Authenticate flow before authenticateChain, removing legacy entries unconditionally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use unfiltered providers/identities for profile suggestion gate to avoid false positives when filters narrow results to empty - Move legacy credential cleanup to post-success path so fallback credentials remain if authentication fails - Add TODO for refactoring AWS-specific cleanup into provider-owned hook - Add tests for uncovered paths to improve patch coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store implementations already handle not-found internally (returning nil), so any error reaching deleteLegacyKeyringEntry is a real backend failure that deserves Warn-level visibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
osterman
force-pushed
the
osterman/auth-suggest-profile
branch
from
27ecd00 to
c9b919b
Compare
…profile Replace the confusing "unsupported identity kind: " error (with dangling colon) with context-aware messages that explain the actual problem and suggest next steps. With a profile active: "Identity is not configured in the `marketplace` profile." Without a profile: "Identity is not configured. Did you forget to specify a profile?" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lAuthSection (#2187) When running `atmos terraform plan -s <stack> --all`, the realm was lost because CopyGlobalAuthConfig omitted Realm/RealmSource fields and buildGlobalAuthSection omitted realm from the auth map. This caused credentials to be stored under the wrong path, triggering realm mismatch warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
@coderabbitai full review please. |
Contributor
🧩 Analysis chainLet me fetch the issue details directly. 🏁 Script executed: Length of output: 1290
Regarding issue Issue Does PR
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cmd/auth_list.go (1)
407-418:⚠️ Potential issue | 🟠 Major
loadAuthManagerForListstill ignores--profileand other global selectors.The manual flag copy only captures
base-path/config/config-path. It still drops profile selection fields, soatmos auth list --profile <name>can load base config and incorrectly trigger profile suggestions.🐛 Proposed fix
import ( _ "embed" "encoding/json" "fmt" "sort" "strings" "github.com/spf13/cobra" "gopkg.in/yaml.v3" errUtils "github.com/cloudposse/atmos/errors" authList "github.com/cloudposse/atmos/pkg/auth/list" authTypes "github.com/cloudposse/atmos/pkg/auth/types" cfg "github.com/cloudposse/atmos/pkg/config" + "github.com/cloudposse/atmos/pkg/flags" log "github.com/cloudposse/atmos/pkg/logger" "github.com/cloudposse/atmos/pkg/perf" "github.com/cloudposse/atmos/pkg/profile" "github.com/cloudposse/atmos/pkg/schema" ) @@ func loadAuthManagerForList(cmd *cobra.Command) (authTypes.AuthManager, *schema.AtmosConfiguration, error) { defer perf.Track(nil, "cmd.loadAuthManagerForList")() - configAndStacksInfo := schema.ConfigAndStacksInfo{} - if bp, _ := cmd.Flags().GetString("base-path"); bp != "" { - configAndStacksInfo.AtmosBasePath = bp - } - if cfgFiles, _ := cmd.Flags().GetStringSlice("config"); len(cfgFiles) > 0 { - configAndStacksInfo.AtmosConfigFilesFromArg = cfgFiles - } - if cfgDirs, _ := cmd.Flags().GetStringSlice("config-path"); len(cfgDirs) > 0 { - configAndStacksInfo.AtmosConfigDirsFromArg = cfgDirs - } + configAndStacksInfo, err := flags.ParseGlobalFlags(cmd, v) + if err != nil { + return nil, nil, fmt.Errorf("%w: failed to parse global flags: %w", errUtils.ErrInvalidFlag, err) + } atmosConfig, err := cfg.InitCliConfig(configAndStacksInfo, false)Based on learnings,
cfg.InitCliConfigmust be fed byflags.ParseGlobalFlags(cmd, v)because config selection fields (includingProfilesFromArg) are read fromschema.ConfigAndStacksInfo, not directly from Viper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth_list.go` around lines 407 - 418, The loadAuthManagerForList flow builds schema.ConfigAndStacksInfo manually and misses global selectors like --profile, causing cfg.InitCliConfig to use incomplete selection data; update the code path that constructs configAndStacksInfo in loadAuthManagerForList to call flags.ParseGlobalFlags(cmd, v) (or otherwise invoke the same parsing routine) so that schema.ConfigAndStacksInfo (including ProfilesFromArg and other selector fields) is populated before calling cfg.InitCliConfig; reference the functions loadAuthManagerForList, cfg.InitCliConfig, flags.ParseGlobalFlags and the struct schema.ConfigAndStacksInfo when making this change.
🧹 Nitpick comments (1)
pkg/auth/manager_test.go (1)
2155-2236: Consider consolidating the new identity-initialization cases into one table-driven test.These four tests validate the same behavior shape with different inputs; a table-driven version will reduce duplication and keep future scenario additions cheaper.
As per coding guidelines
Use table-driven tests for testing multiple scenarios in Go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/manager_test.go` around lines 2155 - 2236, Consolidate the four TestInitializeIdentities_* cases into a single table-driven test: create a slice of test cases with fields (name, cfgIdentities, stackInfo, wantErr bool, wantErrType errUtils.ErrInvalidIdentityConfig?, wantDetails []string), then loop with t.Run(tc.name, func(t *testing.T){ build a manager with tc.cfgIdentities and tc.stackInfo, call m.initializeIdentities(), assert error presence/type via assert.Error/assert.NoError and assert.ErrorIs, and when expecting errors check cockroachErrors.GetAllDetails(err) contains each string in tc.wantDetails }); reference the existing test names (TestInitializeIdentities_EmptyKindWithProfile, TestInitializeIdentities_EmptyKindWithoutProfile, TestInitializeIdentities_EmptyKindNilStackInfo, TestInitializeIdentities_ValidKindSucceeds) and the manager.initializeIdentities method to locate where to consolidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth_list_test.go`:
- Around line 437-510: Add a regression test that passes --profile to the auth
list command and verifies that specifying a profile prevents
ErrAuthNotConfigured by loading the profile's auth config: create a test (e.g.,
TestExecuteAuthListCommand_WithProfileLoadsAuth) that sets up a temp dir with a
profile containing atmos.yaml auth data, call flags.ParseGlobalFlags(cmd, v) so
cfg.InitCliConfig receives a populated schema.ConfigAndStacksInfo, then run
createTestAuthListCmd(); set cmd.RunE = executeAuthListCommand and execute
cmd.Execute(), asserting no ErrAuthNotConfigured and that auth output reflects
the profile. Ensure the test references executeAuthListCommand,
createTestAuthListCmd, cfg.InitCliConfig, flags.ParseGlobalFlags,
schema.ConfigAndStacksInfo, and errUtils.ErrAuthNotConfigured so reviewers can
locate the relevant code.
In `@pkg/auth/factory/factory_test.go`:
- Around line 280-282: The test currently calls err.Error() inside the branch
that checks tt.errorMsg and can panic if err is nil; update the assertion to
first ensure err is non-nil before calling Error() (for example use
assert.Error(t, err) or check err != nil) and then call assert.Contains(t,
err.Error(), tt.errorMsg); adjust the block around tt.errorMsg, err and
assert.Contains in factory_test.go so the test fails cleanly instead of
panicking.
In `@pkg/auth/realm_mismatch.go`:
- Around line 157-177: The cleanup code silently ignores ReadDir and os.Remove
errors (see providerDirs, awsDir, providerDir, and the os.Remove calls), so
update it to log unexpected failures: when the initial ReadDir error is non-nil,
log the error with context (awsDir) instead of just returning; for each
os.Remove call for files and for providerDir/awsDir, check the returned error
and if it's non-nil and not os.IsNotExist(err) log a warning/error including the
path and err; keep successful-delete Debug logs as-is. Ensure you reference
provider.Name(), filepath.Join(...) results, and include clear context in each
log entry to aid debugging.
---
Duplicate comments:
In `@cmd/auth_list.go`:
- Around line 407-418: The loadAuthManagerForList flow builds
schema.ConfigAndStacksInfo manually and misses global selectors like --profile,
causing cfg.InitCliConfig to use incomplete selection data; update the code path
that constructs configAndStacksInfo in loadAuthManagerForList to call
flags.ParseGlobalFlags(cmd, v) (or otherwise invoke the same parsing routine) so
that schema.ConfigAndStacksInfo (including ProfilesFromArg and other selector
fields) is populated before calling cfg.InitCliConfig; reference the functions
loadAuthManagerForList, cfg.InitCliConfig, flags.ParseGlobalFlags and the struct
schema.ConfigAndStacksInfo when making this change.
---
Nitpick comments:
In `@pkg/auth/manager_test.go`:
- Around line 2155-2236: Consolidate the four TestInitializeIdentities_* cases
into a single table-driven test: create a slice of test cases with fields (name,
cfgIdentities, stackInfo, wantErr bool, wantErrType
errUtils.ErrInvalidIdentityConfig?, wantDetails []string), then loop with
t.Run(tc.name, func(t *testing.T){ build a manager with tc.cfgIdentities and
tc.stackInfo, call m.initializeIdentities(), assert error presence/type via
assert.Error/assert.NoError and assert.ErrorIs, and when expecting errors check
cockroachErrors.GetAllDetails(err) contains each string in tc.wantDetails });
reference the existing test names
(TestInitializeIdentities_EmptyKindWithProfile,
TestInitializeIdentities_EmptyKindWithoutProfile,
TestInitializeIdentities_EmptyKindNilStackInfo,
TestInitializeIdentities_ValidKindSucceeds) and the manager.initializeIdentities
method to locate where to consolidate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ae9b85e-e39e-459d-b267-e04c56dcce7b
📒 Files selected for processing (24)
cmd/auth_list.gocmd/auth_list_test.gointernal/exec/utils_auth.gointernal/exec/utils_auth_test.gopkg/auth/config_helpers.gopkg/auth/config_helpers_test.gopkg/auth/factory/factory.gopkg/auth/factory/factory_test.gopkg/auth/manager.gopkg/auth/manager_test.gopkg/auth/manager_whoami.gopkg/auth/realm_mismatch.gopkg/auth/realm_mismatch_test.gotests/snapshots/TestCLICommands_atmos_describe_component_mock_-s_dev_(stack-names_example).stdout.goldentests/snapshots/TestCLICommands_atmos_describe_component_mock_-s_production_(stack-names_example).stdout.goldentests/snapshots/TestCLICommands_atmos_describe_component_vpc_-s_dev_(native-terraform_example).stdout.goldentests/snapshots/TestCLICommands_atmos_describe_component_vpc_-s_my-legacy-prod-stack.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_component_vpc_-s_no-name-prod.stdout.goldentests/snapshots/TestCLICommands_atmos_describe_component_vpc_-s_production_(native-terraform_example).stdout.goldentests/snapshots/TestCLICommands_describe_component_from_nested_dir_discovers_atmos.yaml_in_parent.stdout.goldentests/snapshots/TestCLICommands_describe_component_with_component_name_(backward_compatibility).stdout.goldentests/snapshots/TestCLICommands_describe_component_with_current_directory_(.).stdout.goldentests/snapshots/TestCLICommands_describe_component_with_relative_path.stdout.goldentests/snapshots/TestCLICommands_describe_component_with_stack_flag.stdout.golden
- Use flags.ParseGlobalFlags in loadAuthManagerForList so --profile and other global selectors are respected instead of hand-rolled flag extraction - Add --profile regression test confirming profile auth config suppresses ErrAuthNotConfigured - Only include realm in buildGlobalAuthSection when explicitly configured (env/config), excluding auto-computed config-path hashes that are machine-specific and break CI snapshots - Guard err.Error() in factory_test.go to prevent panic on nil error - Log unexpected file cleanup failures in deleteLegacyCredentialFiles instead of silently ignoring them - Consolidate identity initialization tests into table-driven format - Restore golden snapshots to exclude path-dependent realm hashes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
@coderabbitai please check if this PR fixes the issues reported in #2187 |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 48800
Here's the precise breakdown: Root Cause (Fixed ✅) The issue was in // Before this PR: Realm and RealmSource were silently dropped
config := &schema.AuthConfig{
Realm: globalAuth.Realm, // ← NEW
RealmSource: globalAuth.RealmSource, // ← NEW
Logs: globalAuth.Logs,
...
}
Migration Cleanup (Bonus ✅)
Summary
My previous comment that "root cause is not addressed" was incorrect — I apologize for the confusion. Issue 🧠 Learnings used |
aknysh
approved these changes
Mar 16, 2026
|
These changes were released in v1.210.0-test.28. |
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
2 participants
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.
what
CopyGlobalAuthConfigandbuildGlobalAuthSectionso thatExecuteTerraform(both single-component and--allpaths) uses the correct realm for credential storage. This is the root cause fix for Auth realm not considered when using --all option #2187.CopyGlobalAuthConfiginpkg/auth/config_helpers.gowas missingRealmandRealmSourcefields — when--alliterates components, each call toCopyGlobalAuthConfigproduced an auth config with an empty realmbuildGlobalAuthSectionininternal/exec/utils_auth.gowas not includingrealmin the auth map — the deep-merge path lost the realm valueatmos auth listwith no providers/identities configured, Atmos now checks if profiles exist and suggests using the--profileflag to load auth configuration from profile-specific atmos.yaml filesErrAuthNotConfiguredsentinel error for rich, helpful error messageskindis empty (not configured in active profile)why
When running
atmos terraform plan -s <stack> --all, credentials were stored without the auth realm in the path, causing realm mismatch warnings. Single-component runs (atmos terraform plan <component> -s <stack>) worked correctly because they used a different code path that preserved the realm. The two-line root cause fix ensures realm propagation through both paths.Users working with profile-based configurations (like infra-live) need better guidance when auth config is not in the base atmos.yaml. Suggesting available profiles reduces friction and helps users discover the correct workflow.
The keyring cleanup prevents accumulated migration warnings when users upgrade to realm-aware credential management and transition between different credential storage locations.
testing
Realm/RealmSourceare copied inCopyGlobalAuthConfig(including deep-copy mutation safety)realmis included/excluded inbuildGlobalAuthSectionbased on whether it's setgetMergedAuthConfigWithFetcher(the--allcode path)describe componentoutputreferences
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests