Repository navigation
fix(auth): normalize --identity=false to disable authentication - #2412
Merged
Merged
Conversation
`--identity=false` (and `=0`, `=no`, `=off`, case-insensitive) was reaching auth hooks as the literal string "false" instead of the disabled sentinel, so authentication was not skipped. The env-var path (`ATMOS_IDENTITY=false`) already normalized correctly; the flag path regressed when `internal/exec/cli_utils.go::parseIdentityFlag` was extracted in #2225 and the new helper omitted the call to `cfg.NormalizeIdentityValue`. Apply normalization to both value branches (`--identity=value` and `--identity value`) so every `ProcessCommandLineArgs` consumer (terraform / helmfile / packer / list / describe / workflow / …) receives the disabled sentinel and short-circuits auth. Add parser-level and end-to-end unit tests covering all boolean-false spellings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
May 15, 2026 20:12
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR normalizes --identity inputs (both --identity=value and --identity value) to detect false-like values as the disabled sentinel, then threads an AuthDisabled boolean through command parsing, describe/stacks processors, Terraform state resolution, YAML terraform-state handling, and list instances execution; tests added/updated across unit and integration suites. ChangesAuthDisabled propagation and identity normalization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2412 +/- ##
==========================================
+ Coverage 78.06% 78.10% +0.04%
==========================================
Files 1110 1110
Lines 104673 104784 +111
==========================================
+ Hits 81713 81842 +129
+ Misses 18425 18399 -26
- Partials 4535 4543 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Erik Osterman (Cloud Posse) (osterman)
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 15, 2026 21:00
53c2231
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/describe_stacks_component_processor_auth_test.go (1)
60-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun go-fumpt on this file before merge.
CI is currently failing pre-commit because this test file is not in go-fumpt format.
As per coding guidelines: “Follow standard Go coding style: use
gofmtandgoimportsto format code, prefer short descriptive variable names, use kebab-case for command-line flags, and snake_case for environment variables.”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exec/describe_stacks_component_processor_auth_test.go` around lines 60 - 61, This test file is not formatted with gofumpt; run gofumpt (or gofumpt -w) on internal/exec/describe_stacks_component_processor_auth_test.go to reformat it so it passes pre-commit CI (the failing line is around the test declaration using t.Parallel()); ensure imports and spacing follow gofumpt rules and re-run tests/commit.
🧹 Nitpick comments (1)
internal/exec/terraform_state_utils.go (1)
98-110: 💤 Low valueRedundant fallback assignment.
Line 108 re-assigns
resolvedAuthMgr = parentAuthMgr, but it was already set toparentAuthMgron line 98. The assignment in the error branch is unnecessary.Suggested simplification
resolvedAuthMgr := parentAuthMgr if !authDisabled { var err error resolvedAuthMgr, err = resolveAuthManagerForNestedComponent(atmosConfig, component, stack, parentAuthMgr) if err != nil { log.Debug("Auth does not exist for nested component, using parent AuthManager", "component", component, "stack", stack, "error", err, ) - resolvedAuthMgr = parentAuthMgr } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exec/terraform_state_utils.go` around lines 98 - 110, The code redundantly reassigns resolvedAuthMgr to parentAuthMgr in the error branch even though resolvedAuthMgr is already initialized to parentAuthMgr; update the error handling inside the if !authDisabled block to simply log the error and leave resolvedAuthMgr untouched (remove the duplicate assignment), keeping the call to resolveAuthManagerForNestedComponent and variables atmosConfig, component, stack, authDisabled, parentAuthMgr and resolvedAuthMgr intact so the fallback behavior remains implicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/exec/describe_stacks_component_processor_auth_test.go`:
- Around line 60-61: This test file is not formatted with gofumpt; run gofumpt
(or gofumpt -w) on
internal/exec/describe_stacks_component_processor_auth_test.go to reformat it so
it passes pre-commit CI (the failing line is around the test declaration using
t.Parallel()); ensure imports and spacing follow gofumpt rules and re-run
tests/commit.
---
Nitpick comments:
In `@internal/exec/terraform_state_utils.go`:
- Around line 98-110: The code redundantly reassigns resolvedAuthMgr to
parentAuthMgr in the error branch even though resolvedAuthMgr is already
initialized to parentAuthMgr; update the error handling inside the if
!authDisabled block to simply log the error and leave resolvedAuthMgr untouched
(remove the duplicate assignment), keeping the call to
resolveAuthManagerForNestedComponent and variables atmosConfig, component,
stack, authDisabled, parentAuthMgr and resolvedAuthMgr intact so the fallback
behavior remains implicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9c861b7-70ca-44ef-a8a2-377dfaff8d2e
📒 Files selected for processing (12)
cmd/list/instances.gocmd/list/utils.gocmd/list/utils_test.gointernal/exec/describe_component.gointernal/exec/describe_stacks.gointernal/exec/describe_stacks_component_processor.gointernal/exec/describe_stacks_component_processor_auth_test.gointernal/exec/stacks_processor.gointernal/exec/terraform_state_utils.gointernal/exec/yaml_func_terraform_state.gopkg/list/list_instances.gopkg/schema/schema.go
Erik Osterman (Cloud Posse) (osterman)
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 15, 2026 21:22
85d9f1e
…-false-flag # Conflicts: # internal/exec/describe_stacks_component_processor.go
Coverage report showed 30 missing lines on the PR; 26 of them were in the
auth-disabled wiring added for `--identity=false`. Pure pass-through and
test-seam code that wasn't reachable from existing test fixtures.
* internal/exec/stacks_processor_test.go:
TestDefaultStacksProcessor_ExecuteDescribeStacksWithAuthDisabled — table
drives authDisabled=true and authDisabled=false through the new
pass-through method, mirroring the existing
TestDefaultStacksProcessor_ExecuteDescribeStacks empty-fixture pattern.
Lifts the function from 0% to 100% (15 missing lines recovered).
* pkg/list/list_instances_authdisabled_test.go (new):
Hand-written fakes for the unexported authDisabledStacksProcessor
interface (the gomock MockStacksProcessor only models the public
interface; the auth-disabled method is an optional capability
type-asserted at runtime).
- TestExecuteDescribeStacksForInstances_AuthDisabledDispatchesToAuthDisabledMethod
pins authDisabled=true + capable processor → auth-disabled method.
- TestExecuteDescribeStacksForInstances_AuthDisabledFalseUsesRegularPath
pins authDisabled=false → regular ExecuteDescribeStacks even when
the optional interface is implemented.
- TestExecuteDescribeStacksForInstances_FallsBackWhenInterfaceNotImplemented
safety-net: authDisabled=true with a non-capable processor must
fall back to ExecuteDescribeStacks rather than panic on a failed
type assertion.
- TestProcessInstancesWithDepsAuthDisabled_PropagatesAuthDisabledFlag
end-to-end through the helper with a realistic stacks map.
- TestProcessInstancesWithAuthDisabled_ConstructsDefaultProcessor
smoke test for the production wrapper.
Lifts processInstancesWithDepsAuthDisabled, executeDescribeStacksForInstances,
and processInstancesWithAuthDisabled from <80% to 100% each.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
dismissed stale reviews from coderabbitai[bot] and atmos-pro[bot]
via
May 16, 2026 00:36
adb1a68
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/list/list_instances_authdisabled_test.go`:
- Around line 219-233: The test currently constructs an empty
schema.AtmosConfiguration and calls processInstancesWithAuthDisabled, which lets
path resolution rely on ambient defaults and can make assert.Empty flaky; fix by
making the config deterministic: create a schema.AtmosConfiguration with its
stacks path(s) explicitly set to an isolated non-existent or temporary directory
(or an empty temp dir) before calling processInstancesWithAuthDisabled so
ExecuteDescribeStacksWithAuthDisabled cannot find any stacks; this ensures
instances is always empty and the test is stable.
🪄 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: d1d50437-0560-4529-b41b-62e7631d4545
📒 Files selected for processing (4)
internal/exec/describe_stacks_component_processor.gointernal/exec/describe_stacks_component_processor_auth_test.gointernal/exec/stacks_processor_test.gopkg/list/list_instances_authdisabled_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/exec/describe_stacks_component_processor_auth_test.go
- internal/exec/describe_stacks_component_processor.go
…pDir CodeRabbit flagged the smoke test as ambient-state-dependent: with an empty AtmosConfiguration the underlying FindStacksMap resolves CWD-relative empty BasePath/Stacks.BasePath, so the assertion that the collector returns zero instances depends on whatever happens to exist in the test runner's CWD. The fix matches the existing convention in TestDefaultStacksProcessor_ExecuteDescribeStacks (internal/exec/stacks_processor_test.go:14-27) and the CLAUDE.md guidance to anchor file-discovery tests to an absolute path: set BasePath to t.TempDir() and give the relative Stacks/Components paths explicit values. The temp dir is empty, so the empty-instances assertion is now guaranteed regardless of CWD or filesystem state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 16, 2026
Contributor
|
These changes were released in v1.218.1-rc.1. |
5 tasks done
Andriy Knysh (aknysh)
added a commit
that referenced
this pull request
May 22, 2026
…#2471) * fix(auth): honor --identity=false in describe affected and dependents `--identity=false` (and aliases `off`/`0`/`no`) normalized correctly at the parser layer in 1.219 (PR #2412), but the disabled signal was only wired through `list instances`. In `describe affected`, the top-level AuthManager correctly became nil, but a nil AuthManager was indistinguishable from "no identity specified" downstream — so the per-component auth resolver still ran whenever `processTemplates` was true (its default), reintroducing the AssumeRoleWithWebIdentity call the user tried to disable. Thread an `AuthDisabled bool` from the cmd layer through `executeDescribeAffectedWith*`, `executeDescribeAffected`, `addDependentsToAffected`, and `ExecuteDescribeDependents`, routing inner stack resolution through `ExecuteDescribeStacksWithAuthDisabled` so `processor.authDisabled=true` short-circuits the per-component resolver. Also propagated through `terraform_affected.go`, `terraform_affected_graph.go`, `pkg/list/list_affected.go`, `pkg/ai/tools/atmos/describe_affected.go`, and `atlantis_generate_repo_config.go` call sites. Extracted `pkg/list/list_affected.go::executeAffectedLogic` into three per-mode helpers to stay under the 60-line function-length limit. Tests: - cmd/describe_affected_test.go::TestDescribeAffectedSetsAuthDisabled verifies `false`/`off`/`0`/`no` env values set `AuthDisabled=true` and `AuthManager=nil`. - internal/exec/describe_affected_authdisabled_test.go verifies `Execute()` forwards `AuthDisabled` to all three helper paths and to `addDependentsToAffected`. - describe_stacks_component_processor_auth_test.go adds the exact `(processTemplates=true, processYamlFunctions=false, authDisabled=true)` regression case from the infra-live CI failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style: apply CI go-fumpt formatting Address pre-commit hook failures from PR #2471 CI run. The newer gofumpt in CI flagged formatting in four files I touched in the previous commit: - pkg/list/list_affected.go — split `})` from inline closure into `},` + `)` on separate lines. - internal/exec/describe_affected_utils.go — split the long log.Warn message into its own line in two greenfield-handling branches. - internal/exec/atlantis_generate_repo_config.go — split trailing args in two errors.Errorf calls onto their own lines. - internal/exec/describe_affected_utils_2.go — remove redundant parens from `&((*slice)[i])` → `&(*slice)[i]` in four loop bodies. No behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(auth): propagate AuthDisabled through DescribeDependentsExecProps Address CodeRabbit review on PR #2471. The previous commit added `AuthDisabled` to `DescribeDependentsArgs` but missed adding it to `DescribeDependentsExecProps`, which is the type that `describe dependents` constructs from CLI flags and passes to `describeDependentsExec.Execute`. The executor then built a `DescribeDependentsArgs` without the flag, so the inner `ExecuteDescribeStacksWithAuthDisabled` always received `authDisabled=false` — re-introducing the per-component auth attempt for `atmos describe dependents --identity=false`. - internal/exec/describe_dependents.go: add `AuthDisabled` to `DescribeDependentsExecProps` and forward it into `DescribeDependentsArgs` inside `describeDependentsExec.Execute`. - cmd/describe_dependents.go: set `describe.AuthDisabled` from the normalized identity name, mirroring the wiring in `cmd/describe_affected.go`. Tests: - cmd/describe_dependents_test.go::TestDescribeDependentsSetsAuthDisabled — table covers `false`/`off`/`0`/`no` env spellings. - internal/exec/describe_dependents_authdisabled_test.go — TestDescribeDependentsExec_Execute_ForwardsAuthDisabled pins the executor-side prop → arg propagation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
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
false,0,no,off, case-insensitive) passed via--identity=<value>and--identity <value>to the disabled sentinel (cfg.IdentityFlagDisabledValue), so the auth pre-hook andCreateAndAuthenticateManager*short-circuit instead of trying to authenticate with the literal name"false".internal/exec/cli_utils.go::parseIdentityFlag— the single arg-walker that feedsinfo.Identity/configAndStacksInfo.Identityfor everyProcessCommandLineArgsconsumer (terraform, helmfile, packer, list, describe, workflow, vendor, pro, validate, atlantis, docs, generate).TestParseIdentityFlagfor=false/=False/=FALSE/=0/=no/=offplus space-separated form, and end-to-end cases inTestProcessArgsAndFlags_IdentityFlag{,Helmfile,Packer}assertinginfo.Identity == cfg.IdentityFlagDisabledValue.why
ATMOS_IDENTITY=falsewas fixed in fix(auth): normalize ATMOS_IDENTITY=false (issue #1931) #1935 by normalizing the env-var fallback atcli_utils.go:199, but the env fallback only runs whenIdentity == "". When--identity=falseis passed on the CLI,parseIdentityFlagpopulated the literal"false", the env-fallback branch was skipped, and the literal flowed through topkg/auth/hooks.go::isAuthenticationDisabled(which only matches__DISABLED__).parseIdentityFlaginto a new helper without porting the normalization from the env path, silently breaking the documented--identity=falsecontract. Reported by users foratmos terraform *andatmos list instances.ProcessCommandLineArgsis fixed in one place, and matches the behavior already implemented for the StandardParser path (pkg/flags/global_registry.go) and thecmd/identity_helpers.go/cmd/list/utils.go/cmd/list/affected.goper-command identity reads.references
Summary by CodeRabbit
Bug Fixes
New Features
Tests