Repository navigation
feat: add required field to identities for multi-account Terraform components - #2180
Merged
Merged
Conversation
…nents via auth.needs Add `auth.needs` field to component auth config, enabling components to declare which identities they require. All listed identities are authenticated before Terraform runs, with their profiles written to the shared credentials file. The first identity becomes primary (sets AWS_PROFILE). This fixes CI failures when using multiple AWS provider aliases (e.g., hub-spoke networking) with OIDC, where only one profile was previously written to the credentials file. Changes: - Add Needs field to AuthConfig schema with comprehensive documentation - Update resolveTargetIdentityName() to prioritize auth.needs first entry as primary - Add authenticateAdditionalIdentities() for non-primary identity authentication with non-fatal error handling - Call authenticateAdditionalIdentities() from authenticateAndWriteEnv() after primary auth succeeds - Add 7 test cases covering needs list resolution, CLI override, fallback, success auth, skipping primary, non-fatal errors, and empty needs Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
March 12, 2026 15:51
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Add the `auth` definition to the Atmos stack manifest JSON schema, including the new `needs` field, along with `realm`, `providers`, `identities`, and `integrations`. Reference it from `terraform_component_manifest` so IDE autocompletion and schema validation recognize the component-level auth config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
|
Warning Release Documentation RequiredThis PR is labeled
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds "required" identities support: schema and docs for component-level auth, CLI/hook logic to authenticate a primary identity plus additional required identities non-fatally, and tests covering schema validation and authentication behavior. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Hook
participant AuthMgr as AuthManager
participant Primary as Primary Identity
participant Secondary as Required Identity
participant Creds as Shared Credentials / Env
CLI->>AuthMgr: ResolveTargetIdentityName(--identity or default)
AuthMgr->>Primary: Authenticate(primary)
Primary-->>AuthMgr: Credentials
AuthMgr->>Creds: Set primary env (AWS_PROFILE / env vars)
CLI->>AuthMgr: AuthenticateAdditionalIdentities()
loop for each identity in AuthManager.GetIdentities() marked Required and != primary
AuthMgr->>Secondary: Authenticate(identity)
alt Success
Secondary-->>AuthMgr: Credentials
AuthMgr->>Creds: Write secondary profile to shared credentials
else Failure
Secondary-->>AuthMgr: Error
AuthMgr->>AuthMgr: Log warning (non-fatal)
end
end
Creds-->>CLI: All profiles available for Terraform
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Update all three copies of the manifest schema to include auth.needs: - pkg/datafetcher/schema (source, already had auth definition) - website/static/schemas (published, add needs/realm/integrations to component_auth) - tests/fixtures/schemas (test fixture, same as website) Add schema validation tests: - TestManifestSchema_AuthDefinitionExists: verify auth definition in embedded schema - TestManifestSchema_AuthNeedsField: verify needs is array of strings - TestManifestSchema_ValidAuthConfig: validate realistic auth configs against schema - TestAuthConfig_Needs: struct-level tests for Needs field - TestAuthConfig_NeedsWithMapstructure: verify Needs works with full AuthConfig Also changed component_auth additionalProperties from false to true to allow fields like logs, keyring, and realm that exist in the Go struct. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Release Documentation RequiredThis PR is labeled
|
Add announcement blog post for the auth.needs feature enabling concurrent identity authentication for multi-account Terraform components. Update the roadmap with a shipped milestone and add a dedicated documentation page at /cli/configuration/auth/needs with configuration examples and behavior reference. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2180 +/- ##
==========================================
+ Coverage 77.17% 77.22% +0.04%
==========================================
Files 1014 1014
Lines 95875 95883 +8
==========================================
+ Hits 73994 74046 +52
+ Misses 17700 17657 -43
+ Partials 4181 4180 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (3)
pkg/schema/schema_auth_test.go (1)
175-195: Test name slightly misleading.
TestAuthConfig_NeedsWithMapstructureconstructs the struct directly rather than usingmapstructure.Decode. The test is valid for verifying field assignment, but consider renaming toTestAuthConfig_NeedsWithFullConfigfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/schema_auth_test.go` around lines 175 - 195, Rename the test function TestAuthConfig_NeedsWithMapstructure to a clearer name like TestAuthConfig_NeedsWithFullConfig since the test constructs the AuthConfig struct directly (no mapstructure.Decode); update the function declaration and any references to the test symbol so the test suite continues to run and the name accurately reflects that it verifies a fully populated AuthConfig (reference symbol: TestAuthConfig_NeedsWithMapstructure → TestAuthConfig_NeedsWithFullConfig).pkg/datafetcher/schema_auth_validation_test.go (1)
35-47: Unchecked type assertions could cause test panics.Several type assertions (lines 35, 39, 47) don't use the comma-ok idiom. If the schema structure changes, tests will panic rather than fail gracefully with a helpful message.
Consider using
require.IsTypeor the comma-ok pattern for safer assertions.🔧 Example fix for safer type assertions
- oneOf := tfManifest["oneOf"].([]interface{}) + oneOfRaw, ok := tfManifest["oneOf"].([]interface{}) + require.True(t, ok, "terraform_component_manifest should have oneOf as array") + oneOf := oneOfRaw // Find the object variant (not the !include string variant). var objectVariant map[string]interface{} for _, v := range oneOf { - variant := v.(map[string]interface{}) + variant, ok := v.(map[string]interface{}) + require.True(t, ok, "oneOf entry should be a map") if variant["type"] == "object" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/datafetcher/schema_auth_validation_test.go` around lines 35 - 47, The test uses unchecked type assertions on tfManifest["oneOf"], each element in oneOf, and objectVariant["properties"] which can cause panics; update the code around tfManifest, oneOf and objectVariant to use safe checks (comma-ok) or require.IsType/require.IsNotNil before casting: assert tfManifest["oneOf"] is a []interface{}, iterate with type-checked assertions for each element to ensure it's a map[string]interface{} when checking variant["type"], confirm objectVariant is not nil before accessing objectVariant["properties"], and ensure properties is a map[string]interface{} via a safe cast so failures produce test errors instead of panics.pkg/auth/hooks.go (1)
91-116: Resolution logic is correct with clear precedence.CLI flag → auth.needs[0] → default identity. The implementation matches the documented behavior.
One observation:
decodeAuthConfigFromStackis called multiple times during the flow (TerraformPreHook, resolveTargetIdentityName, authenticateAdditionalIdentities). Consider passing the decoded config through to avoid redundant decoding, though the current approach works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/hooks.go` around lines 91 - 116, resolveTargetIdentityName currently calls decodeAuthConfigFromStack again causing redundant decoding; change its signature to accept a pre-decoded authConfig (type returned by decodeAuthConfigFromStack) or a pointer to it and update callers (TerraformPreHook, authenticateAdditionalIdentities) to decode once and pass the result through, then use authConfig.Needs[0] inside resolveTargetIdentityName instead of calling decodeAuthConfigFromStack; ensure nil/empty checks remain and preserve existing error handling around GetDefaultIdentity in resolveTargetIdentityName.
🤖 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/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 313-315: The manifest's inline "auth" schema is too
permissive—replace it with the shared, structured auth definitions used by the
other manifest copies so "providers" and "identities" validate consistently;
specifically, update the "auth" entry to reference the common auth definitions
(use the same definitions/auth, definitions/provider, and definitions/identity
shapes used elsewhere) rather than allowing free-form objects, and apply the
same change to the other inline blocks noted (around 1301-1343) so all copies
share the identical auth schema.
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 1573-1597: The component auth section currently routes
"identities" to the existing "#/definitions/auth_identities", whose key pattern
forbids "/" and thus rejects slash-delimited names like "core/network" and
"spoke/production"; update the schema so component auth accepts these names by
widening the key pattern to allow "/" (e.g., modify the regex in
"auth_identities" or create a new definition for component-level identities and
reference it here) and apply the same pattern change to the fixture copy so the
examples validate.
---
Nitpick comments:
In `@pkg/auth/hooks.go`:
- Around line 91-116: resolveTargetIdentityName currently calls
decodeAuthConfigFromStack again causing redundant decoding; change its signature
to accept a pre-decoded authConfig (type returned by decodeAuthConfigFromStack)
or a pointer to it and update callers (TerraformPreHook,
authenticateAdditionalIdentities) to decode once and pass the result through,
then use authConfig.Needs[0] inside resolveTargetIdentityName instead of calling
decodeAuthConfigFromStack; ensure nil/empty checks remain and preserve existing
error handling around GetDefaultIdentity in resolveTargetIdentityName.
In `@pkg/datafetcher/schema_auth_validation_test.go`:
- Around line 35-47: The test uses unchecked type assertions on
tfManifest["oneOf"], each element in oneOf, and objectVariant["properties"]
which can cause panics; update the code around tfManifest, oneOf and
objectVariant to use safe checks (comma-ok) or require.IsType/require.IsNotNil
before casting: assert tfManifest["oneOf"] is a []interface{}, iterate with
type-checked assertions for each element to ensure it's a map[string]interface{}
when checking variant["type"], confirm objectVariant is not nil before accessing
objectVariant["properties"], and ensure properties is a map[string]interface{}
via a safe cast so failures produce test errors instead of panics.
In `@pkg/schema/schema_auth_test.go`:
- Around line 175-195: Rename the test function
TestAuthConfig_NeedsWithMapstructure to a clearer name like
TestAuthConfig_NeedsWithFullConfig since the test constructs the AuthConfig
struct directly (no mapstructure.Decode); update the function declaration and
any references to the test symbol so the test suite continues to run and the
name accurately reflects that it verifies a fully populated AuthConfig
(reference symbol: TestAuthConfig_NeedsWithMapstructure →
TestAuthConfig_NeedsWithFullConfig).
🪄 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: 7b16bfd9-f4c9-4cc9-9c01-f109d2030b4e
📒 Files selected for processing (14)
pkg/auth/hooks.gopkg/auth/hooks_needs_test.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema_auth_validation_test.gopkg/schema/schema_auth.gopkg/schema/schema_auth_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/blog/2026-03-12-auth-needs-concurrent-identities.mdxwebsite/docs/cli/configuration/auth/identities.mdxwebsite/docs/cli/configuration/auth/keyring.mdxwebsite/docs/cli/configuration/auth/logs.mdxwebsite/docs/cli/configuration/auth/needs.mdxwebsite/src/data/roadmap.jswebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
… slash-delimited identity names The embedded manifest schema had a loose auth definition with unstructured providers/identities, while the website and fixture schemas used rich structured definitions. This aligns all three copies and widens the identity/provider key pattern to accept slash-delimited names like core/network. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/datafetcher/schema_auth_validation_test.go (1)
35-49: Consider defensive type assertions for clearer failure messages.The type assertions on lines 35, 39, and 47 could panic with opaque messages if the schema structure changes unexpectedly. While panics still fail tests, using the two-value assertion form produces more actionable output.
That said, this is a minor nit for test code where panics are acceptable.
♻️ Optional: Defensive assertions
- oneOf := tfManifest["oneOf"].([]interface{}) + oneOfRaw, ok := tfManifest["oneOf"] + require.True(t, ok, "terraform_component_manifest should have oneOf") + oneOf, ok := oneOfRaw.([]interface{}) + require.True(t, ok, "oneOf should be an array")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/datafetcher/schema_auth_validation_test.go` around lines 35 - 49, The test uses single-value type assertions that can panic (oneOf := tfManifest["oneOf"].([]interface{}), variant := v.(map[string]interface{}), props := objectVariant["properties"].(map[string]interface{})); change these to the two-value form and assert/require the ok value before use: e.g., get oneOfRaw, ok := tfManifest["oneOf"]; require.True(t, ok) and oneOf, ok := oneOfRaw.([]interface{}); require.True(t, ok), and likewise for variant and props so objectVariant and props are only accessed after confirming their types in the code paths around oneOf, objectVariant and props.pkg/datafetcher/schema/atmos/manifest/1.0.json (1)
375-411: Consider addingauthproperty tohelmfile_component_manifestandpacker_component_manifestfor consistency.The website and fixture schemas include
authin these manifests, but the embedded schema doesn't. This gap looks intentional given the PR's Terraform focus, but aligning them would keep things consistent across all three schema sources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/datafetcher/schema/atmos/manifest/1.0.json` around lines 375 - 411, Add an "auth" property to the "helmfile_component_manifest" and "packer_component_manifest" object schemas: inside each manifest's "properties" object add an "auth" entry with "$ref": "#/definitions/auth" so the embedded schema matches the website and fixture schemas; keep "additionalProperties": false and do not add "auth" to "required" unless required by other schemas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/datafetcher/schema_auth_validation_test.go`:
- Around line 35-49: The test uses single-value type assertions that can panic
(oneOf := tfManifest["oneOf"].([]interface{}), variant :=
v.(map[string]interface{}), props :=
objectVariant["properties"].(map[string]interface{})); change these to the
two-value form and assert/require the ok value before use: e.g., get oneOfRaw,
ok := tfManifest["oneOf"]; require.True(t, ok) and oneOf, ok :=
oneOfRaw.([]interface{}); require.True(t, ok), and likewise for variant and
props so objectVariant and props are only accessed after confirming their types
in the code paths around oneOf, objectVariant and props.
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 375-411: Add an "auth" property to the
"helmfile_component_manifest" and "packer_component_manifest" object schemas:
inside each manifest's "properties" object add an "auth" entry with "$ref":
"#/definitions/auth" so the embedded schema matches the website and fixture
schemas; keep "additionalProperties": false and do not add "auth" to "required"
unless required by other schemas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdfcc570-0d56-4c4d-9c51-244c9f97803c
📒 Files selected for processing (4)
pkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema_auth_validation_test.gotests/fixtures/schemas/atmos/atmos-manifest/1.0/atmos-manifest.jsonwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
Previously, the first entry in auth.needs became the primary identity, requiring users to re-list the default. Now the default identity is always primary, and needs only lists additional identities. If no default exists, the first needs entry becomes the primary as a fallback. Precedence: --identity CLI flag > default identity > needs[0] > error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/auth/hooks.go (1)
129-139:⚠️ Potential issue | 🟠 MajorSnapshot the primary env before secondary auth runs.
AuthenticatemutatesstackInfo.ComponentEnvSectionduring post-auth. With this new ordering,PrepareShellEnvironmentnow starts from the last extra identity's env instead of the primary one. CapturebaseEnvListbeforeauthenticateAdditionalIdentities, or restore the primary env after the loop.Patch sketch
log.Debug("Authentication successful", "identity", whoami.Identity, "expiration", whoami.Expiration) + // Convert ComponentEnvSection to env list for PrepareShellEnvironment. + // This includes any component-specific env vars already set in the stack config. + baseEnvList := componentEnvSectionToList(stackInfo.ComponentEnvSection) + // Authenticate additional needed identities so their profiles exist in the shared credentials file. // This is required for Terraform components that use multiple AWS provider aliases. authenticateAdditionalIdentities(ctx, authManager, identityName, stackInfo) - - // Convert ComponentEnvSection to env list for PrepareShellEnvironment. - // This includes any component-specific env vars already set in the stack config. - baseEnvList := componentEnvSectionToList(stackInfo.ComponentEnvSection)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/hooks.go` around lines 129 - 139, The code builds baseEnvList from stackInfo.ComponentEnvSection after calling authenticateAdditionalIdentities, but authenticateAdditionalIdentities mutates stackInfo.ComponentEnvSection so PrepareShellEnvironment ends up using a secondary identity's env; to fix, snapshot the primary env before calling authenticateAdditionalIdentities by moving the call to componentEnvSectionToList(stackInfo.ComponentEnvSection) above authenticateAdditionalIdentities (or reassign baseEnvList from a saved copy after the loop), ensuring authManager.PrepareShellEnvironment(ctx, identityName, baseEnvList) receives the original primary env; update references to authenticateAdditionalIdentities, componentEnvSectionToList, stackInfo.ComponentEnvSection, and authManager.PrepareShellEnvironment accordingly.
🤖 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/hooks.go`:
- Around line 166-183: The loop in authenticateAdditionalIdentities calls
authManager.Authenticate for each identity which causes Azure's WriteCredentials
to truncate credentials.json so only the last identity remains; fix by changing
the Azure file manager's credential write logic (the method invoked by
SetupFiles()/WriteCredentials()) to read the existing credentials.json, merge in
the new identity's credentials keyed by identity/client-id (preserve existing
entries), and then write the merged JSON atomically, so per-identity calls from
authenticateAdditionalIdentities and authManager.Authenticate no longer
overwrite previous Azure identities.
- Around line 99-113: GetDefaultIdentity(false) is currently treating any
non-nil error as equivalent to "no default" and falls back to
authConfig.Needs[0]; change this to discriminate sentinel errors using
errors.Is(err, errUtils.ErrNoDefaultIdentity) so only ErrNoDefaultIdentity
triggers the fallback path, and return/bubble up any other errors (e.g.,
ErrIdentitySelectionRequiresTTY, ErrUserAborted) immediately from the function;
locate the call to authManager.GetDefaultIdentity(false) and replace the simple
nil-check with an errors.Is check, keeping the existing
decodeAuthConfigFromStack and authConfig.Needs fallback only for the
ErrNoDefaultIdentity case.
---
Outside diff comments:
In `@pkg/auth/hooks.go`:
- Around line 129-139: The code builds baseEnvList from
stackInfo.ComponentEnvSection after calling authenticateAdditionalIdentities,
but authenticateAdditionalIdentities mutates stackInfo.ComponentEnvSection so
PrepareShellEnvironment ends up using a secondary identity's env; to fix,
snapshot the primary env before calling authenticateAdditionalIdentities by
moving the call to componentEnvSectionToList(stackInfo.ComponentEnvSection)
above authenticateAdditionalIdentities (or reassign baseEnvList from a saved
copy after the loop), ensuring authManager.PrepareShellEnvironment(ctx,
identityName, baseEnvList) receives the original primary env; update references
to authenticateAdditionalIdentities, componentEnvSectionToList,
stackInfo.ComponentEnvSection, and authManager.PrepareShellEnvironment
accordingly.
🪄 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: 695ad31e-466a-4c30-89c4-ae88eb3261c8
📒 Files selected for processing (6)
pkg/auth/hooks.gopkg/auth/hooks_needs_test.gopkg/schema/schema_auth.gowebsite/blog/2026-03-12-auth-needs-concurrent-identities.mdxwebsite/docs/cli/configuration/auth/needs.mdxwebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/auth/hooks_needs_test.go
- website/src/data/roadmap.js
- website/blog/2026-03-12-auth-needs-concurrent-identities.mdx
- pkg/schema/schema_auth.go
GetDefaultIdentity can return multiple error types beyond ErrNoDefaultIdentity (e.g., ErrUserAborted, ErrIdentitySelectionRequiresTTY). Previously all errors fell through to the auth.needs fallback, masking real failures. Now only ErrNoDefaultIdentity triggers the fallback; other errors are returned immediately. Also properly surfaces decodeAuthConfigFromStack errors instead of silently ignoring them, and documents the Azure credential overwrite limitation for multi-identity scenarios. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/auth/hooks.go (1)
182-185: Consider logging decode failures for debugging.If
decodeAuthConfigFromStackfails here, the function silently returns. While non-fatal behavior is intentional, a trace-level log would help operators troubleshoot configuration issues without breaking the flow.🔧 Optional: Add trace log on decode failure
func authenticateAdditionalIdentities(ctx context.Context, authManager types.AuthManager, primaryIdentity string, stackInfo *schema.ConfigAndStacksInfo, ) { authConfig, err := decodeAuthConfigFromStack(stackInfo) - if err != nil || len(authConfig.Needs) == 0 { + if err != nil { + log.Trace("Failed to decode auth config for additional identities", "error", err) + return + } + if len(authConfig.Needs) == 0 { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/hooks.go` around lines 182 - 185, The function currently returns silently when decodeAuthConfigFromStack fails; add a trace-level log when err != nil to record the decode error and relevant context (e.g., the stack info or identifier) without changing control flow. Locate the call to decodeAuthConfigFromStack and, before the early return, emit a trace/debug log including err and enough stackInfo to diagnose the failure; keep behavior unchanged when authConfig is present but authConfig.Needs is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/auth/hooks.go`:
- Around line 182-185: The function currently returns silently when
decodeAuthConfigFromStack fails; add a trace-level log when err != nil to record
the decode error and relevant context (e.g., the stack info or identifier)
without changing control flow. Locate the call to decodeAuthConfigFromStack and,
before the early return, emit a trace/debug log including err and enough
stackInfo to diagnose the failure; keep behavior unchanged when authConfig is
present but authConfig.Needs is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7550b68-b592-4a7e-ad9a-980ab0b682f6
📒 Files selected for processing (1)
pkg/auth/hooks.go
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Erik Osterman (Cloud Posse) (osterman)
changed the title
Move the multi-identity declaration from a centralized `auth.needs` string array on AuthConfig to a `required: true` boolean on each Identity. Required identities are automatically authenticated without prompting. The `required` field is orthogonal to `default`: default sets the primary identity, required means auto-authenticate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflict in roadmap.js progress value, keeping 92% from this branch which includes auth.needs milestones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/auth/hooks.go (1)
159-180:⚠️ Potential issue | 🟠 Major
required: trueis still broken for Azure multi-identity flows.This isn't just future work—the new loop now calls
Authenticate()for multiple identities, while Azure credentials are still keyed by provider name, so the last required Azure identity wins and earlier ones get overwritten. The TODO is useful, but the feature stays provider-dependent until you either gate this path for Azure or fix Azure's per-identity/merge strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/hooks.go` around lines 159 - 180, The loop in authenticateAdditionalIdentities incorrectly calls authManager.Authenticate for Azure identities which are keyed by provider name and thus overwrite earlier required identities; to fix, detect Azure identities (e.g., inspect identity.Provider / identity.Type or a method on types.AuthManager) and skip authenticating additional Azure identities in authenticateAdditionalIdentities (log a clear warning about the provider limitation), or alternatively implement a per-identity Azure storage/merge strategy inside the Azure-specific auth implementation; update authenticateAdditionalIdentities to only call authManager.Authenticate for non-Azure providers (and add the warning) unless you also change the Azure auth backend to support per-identity keys.
🧹 Nitpick comments (4)
pkg/schema/schema_auth_test.go (1)
128-183: These tests mostly just restate struct literals.
TestIdentity_Required,TestAuthConfig_WithRequiredIdentities, and the newRequiredassertion inTestIdentity_Structureonly read back bools assigned in the same test. A YAML/mapstructure/JSON round-trip would give you real regression coverage for the new field instead of compile-time tautologies.Based on learnings: Test Quality (MANDATORY): Test behavior, not implementation. Avoid tautological tests.
Also applies to: 216-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/schema/schema_auth_test.go` around lines 128 - 183, The tests TestIdentity_Required, TestAuthConfig_WithRequiredIdentities and the added Required assertion in TestIdentity_Structure are tautological because they only read back boolean literals; change them to perform a serialization/deserialization round-trip (e.g., marshal to YAML/JSON or build a map[string]interface{} and use mapstructure) and then unmarshal into Identity/AuthConfig to assert the Required and Default fields are preserved after parsing; update TestIdentity_Required to create input bytes (or map) with required:true/false/missing cases and assert the parsed Identity.Required matches expectations, and update TestAuthConfig_WithRequiredIdentities to parse a sample YAML/JSON auth config and assert identities' Required/Default values post-unmarshal so the tests cover mapping/decoding behavior rather than literal assignment.pkg/auth/hooks_required_test.go (1)
30-68: These resolve-target tests don't hit a new path.
resolveTargetIdentityNamenever inspectsGetIdentities(), so the required-identity setup here is dead data and the assertions mostly duplicateTestResolveTargetIdentityNameinpkg/auth/hooks_test.go. I'd keep this file focused onauthenticateAdditionalIdentitiesand fold/remove the duplicate precedence checks.Based on learnings: Test Quality (MANDATORY): Test behavior, not implementation. Avoid tautological tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/auth/hooks_required_test.go` around lines 30 - 68, The tests in hooks_required_test.go are exercising resolveTargetIdentityName but pass required identities that are unused because resolveTargetIdentityName never calls GetIdentities; remove or fold the duplicate precedence tests (those using mgr.identities with Required:true) and instead add/keep tests that directly exercise authenticateAdditionalIdentities and any behavior that reads GetIdentities; specifically, delete or merge tests referencing resolveTargetIdentityName that duplicate pkg/auth/hooks_test.go, and replace them with tests that call authenticateAdditionalIdentities (or update existing tests to assert behavior that depends on mgr.GetIdentities), ensuring you reference resolveTargetIdentityName only where it actually inspects input and use authenticateAdditionalIdentities for required-identity logic.pkg/datafetcher/schema_auth_validation_test.go (2)
100-165: Consider adding a negative test case.The
expectErrfield is present but unused (all cases arefalse). Adding a case with invalid config (e.g., identity missingkind) would strengthen the test suite.💡 Example negative test case
{ name: "identity missing required kind field", manifest: map[string]interface{}{ "components": map[string]interface{}{ "terraform": map[string]interface{}{ "bad": map[string]interface{}{ "auth": map[string]interface{}{ "identities": map[string]interface{}{ "missing-kind": map[string]interface{}{ "default": true, // "kind" intentionally omitted }, }, }, }, }, }, }, expectErr: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/datafetcher/schema_auth_validation_test.go` around lines 100 - 165, Add a negative test case to the tests slice that sets expectErr: true and supplies a manifest with an identity missing the required "kind" (e.g., under "components" -> "terraform" -> "bad" -> "auth" -> "identities" include an entry with "default": true but no "kind"), and ensure the test loop that iterates over tests reads the expectErr flag and asserts an error is returned when expectErr is true (reference the tests slice, manifest maps, and the expectErr field in the test runner).
35-49: Type assertions without safety checks may cause cryptic panics.Lines 35, 39, 47 use direct type assertions (
.([]interface{}),.(map[string]interface{})) that will panic if the schema structure differs from expectations. Consider using the two-value form for clearer failure messages.♻️ Safer type assertion example
- oneOf := tfManifest["oneOf"].([]interface{}) + oneOf, ok := tfManifest["oneOf"].([]interface{}) + require.True(t, ok, "terraform_component_manifest should have oneOf array") // Find the object variant (not the !include string variant). var objectVariant map[string]interface{} for _, v := range oneOf { - variant := v.(map[string]interface{}) + variant, ok := v.(map[string]interface{}) + if !ok { + continue + } if variant["type"] == "object" { objectVariant = variant break } } require.NotNil(t, objectVariant, "terraform_component_manifest should have an object variant") - props := objectVariant["properties"].(map[string]interface{}) + props, ok := objectVariant["properties"].(map[string]interface{}) + require.True(t, ok, "object variant should have properties")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/datafetcher/schema_auth_validation_test.go` around lines 35 - 49, The test uses unchecked type assertions on tfManifest["oneOf"], loop variable variant, and objectVariant["properties"] which can panic; change them to the two-value "ok" form and fail the test with clear messages instead of panicking: safely extract oneOf using v, ok := tfManifest["oneOf"].([]interface{}) and require.True/require.NotNil on ok, inside the loop assert variant, ok := v.(map[string]interface{}) and skip or fail if not ok, and read props with props, ok := objectVariant["properties"].(map[string]interface{}) followed by require.True(t, ok, ...) so failures produce readable test errors referencing oneOf, variant, objectVariant and props rather than panics.
🤖 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/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1301-1331: The ComponentAuthConfig Go struct is missing the schema
fields "realm" and "integrations", so add a Realm string `Realm string
`json:"realm,omitempty"` to represent the credential isolation realm and add an
Integrations field (e.g., `Integrations map[string]interface{}
`json:"integrations,omitempty"` or `map[string]any`) to hold client-only
credential materializations; ensure both fields are exported, have the correct
json tags, and live alongside the existing Providers and Identities fields so
unmarshalling preserves these values.
In `@website/blog/2026-03-12-auth-required-concurrent-identities.mdx`:
- Around line 26-39: The sample aws/assume-role identities (auth.identities ->
core-network, plat-prod, plat-staging) are incomplete: update each
aws/assume-role entry to either include the required fields "via" and
"principal.assume_role" with realistic example values, or clearly mark the YAML
block as a partial override/snippet and link to the full required schema; ensure
the examples use the exact keys shown (kind: aws/assume-role, default, required,
via, principal.assume_role) so copy-paste consumers get valid config.
In `@website/docs/cli/configuration/auth/required.mdx`:
- Around line 28-42: The examples under the atmos.yaml auth.identities block
(identities: core-network, plat-prod, plat-staging with kind: aws/assume-role)
are missing required fields (via and principal.assume_role) per the
aws/assume-role schema; either update those identity examples to include valid
via and principal.assume_role entries (showing a complete aws/assume-role
identity) or clearly label the code blocks as "partial overrides" and add a
short note that these fragments only override default/required flags and rely on
pre-defined global identities for the full assume-role configuration; apply the
same change to the second snippet referenced (the other block covering
plat-prod/plat-staging) so docs are accurate and consistent with the identities
spec.
---
Duplicate comments:
In `@pkg/auth/hooks.go`:
- Around line 159-180: The loop in authenticateAdditionalIdentities incorrectly
calls authManager.Authenticate for Azure identities which are keyed by provider
name and thus overwrite earlier required identities; to fix, detect Azure
identities (e.g., inspect identity.Provider / identity.Type or a method on
types.AuthManager) and skip authenticating additional Azure identities in
authenticateAdditionalIdentities (log a clear warning about the provider
limitation), or alternatively implement a per-identity Azure storage/merge
strategy inside the Azure-specific auth implementation; update
authenticateAdditionalIdentities to only call authManager.Authenticate for
non-Azure providers (and add the warning) unless you also change the Azure auth
backend to support per-identity keys.
---
Nitpick comments:
In `@pkg/auth/hooks_required_test.go`:
- Around line 30-68: The tests in hooks_required_test.go are exercising
resolveTargetIdentityName but pass required identities that are unused because
resolveTargetIdentityName never calls GetIdentities; remove or fold the
duplicate precedence tests (those using mgr.identities with Required:true) and
instead add/keep tests that directly exercise authenticateAdditionalIdentities
and any behavior that reads GetIdentities; specifically, delete or merge tests
referencing resolveTargetIdentityName that duplicate pkg/auth/hooks_test.go, and
replace them with tests that call authenticateAdditionalIdentities (or update
existing tests to assert behavior that depends on mgr.GetIdentities), ensuring
you reference resolveTargetIdentityName only where it actually inspects input
and use authenticateAdditionalIdentities for required-identity logic.
In `@pkg/datafetcher/schema_auth_validation_test.go`:
- Around line 100-165: Add a negative test case to the tests slice that sets
expectErr: true and supplies a manifest with an identity missing the required
"kind" (e.g., under "components" -> "terraform" -> "bad" -> "auth" ->
"identities" include an entry with "default": true but no "kind"), and ensure
the test loop that iterates over tests reads the expectErr flag and asserts an
error is returned when expectErr is true (reference the tests slice, manifest
maps, and the expectErr field in the test runner).
- Around line 35-49: The test uses unchecked type assertions on
tfManifest["oneOf"], loop variable variant, and objectVariant["properties"]
which can panic; change them to the two-value "ok" form and fail the test with
clear messages instead of panicking: safely extract oneOf using v, ok :=
tfManifest["oneOf"].([]interface{}) and require.True/require.NotNil on ok,
inside the loop assert variant, ok := v.(map[string]interface{}) and skip or
fail if not ok, and read props with props, ok :=
objectVariant["properties"].(map[string]interface{}) followed by require.True(t,
ok, ...) so failures produce readable test errors referencing oneOf, variant,
objectVariant and props rather than panics.
In `@pkg/schema/schema_auth_test.go`:
- Around line 128-183: The tests TestIdentity_Required,
TestAuthConfig_WithRequiredIdentities and the added Required assertion in
TestIdentity_Structure are tautological because they only read back boolean
literals; change them to perform a serialization/deserialization round-trip
(e.g., marshal to YAML/JSON or build a map[string]interface{} and use
mapstructure) and then unmarshal into Identity/AuthConfig to assert the Required
and Default fields are preserved after parsing; update TestIdentity_Required to
create input bytes (or map) with required:true/false/missing cases and assert
the parsed Identity.Required matches expectations, and update
TestAuthConfig_WithRequiredIdentities to parse a sample YAML/JSON auth config
and assert identities' Required/Default values post-unmarshal so the tests cover
mapping/decoding behavior rather than literal assignment.
🪄 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: d849cfde-e23f-45a9-ac2f-ccf90b70bdac
📒 Files selected for processing (11)
pkg/auth/hooks.gopkg/auth/hooks_required_test.gopkg/auth/hooks_test.gopkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema_auth_validation_test.gopkg/schema/schema_auth.gopkg/schema/schema_auth_test.gowebsite/blog/2026-03-12-auth-required-concurrent-identities.mdxwebsite/docs/cli/configuration/auth/identities.mdxwebsite/docs/cli/configuration/auth/required.mdxwebsite/src/data/roadmap.js
Add Realm and Integrations fields to ComponentAuthConfig struct to match the JSON schema, preventing silent data loss during unmarshalling. Mark aws/assume-role examples in blog and docs as partial overrides with notes linking to full identity configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
Resolve roadmap.js conflicts: keep both 'Required identities' (this branch) and 'Isolated browser sessions' (main) milestones. Recalculate auth progress to 89% (24 shipped / 27 total). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ative case - Convert bare type assertions to two-value form with require.True for clear failure messages instead of cryptic panics - Add negative test case: identity with string "required" field rejected by JSON schema (validates the expectErr path) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Mar 24, 2026
|
These changes were released in v1.211.1-test.0. |
This branch was successfully deployed
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
required: trueboolean field to identity config for automatic authentication without promptingrequiredanddefaultare orthogonal:default: truesets the primary identity,required: truemeans auto-authenticaterequired: true— all are authenticated before Terraform runs--identityCLI flag >default: trueidentity > errorcomponent_auth,auth_providers,auth_identity, etc.)patternPropertiesregex to allow slash-delimited identity names (e.g.,core/network)why
When Terraform components use multiple AWS provider aliases for multi-account patterns (hub-spoke networking, cross-account peering), each provider assumes a different IAM role. In CI with OIDC, only the primary identity's profile existed in the shared credentials file, causing "failed to get shared config profile" errors for additional provider aliases.
Marking identities as
required: trueensures they are automatically authenticated — no prompting, no selection. This is cleaner than the previousauth.needsarray approach because the declaration lives on the identity itself rather than in a disconnected list.example
references
auth.needsarray to per-identityrequiredbooleanSummary by CodeRabbit
New Features
Configuration
Documentation
Tests