-
-
Notifications
You must be signed in to change notification settings - Fork 153
Fix Atmos Auth for atmos describe and atmos list commands
#1827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
500d66b
fix auth
aknysh 910b7e1
fix auth
aknysh 444a862
fix auth
aknysh 46a307e
fix auth
aknysh b69917c
fix auth
aknysh 9406338
fix auth
aknysh 767e771
fix auth
aknysh 5de975d
fix auth
aknysh aeae533
[autofix.ci] apply automated fixes
autofix-ci[bot] 2637b07
Add tests to improve code coverage for auth changes
aknysh 07f856c
Remove dead code for identity flag check in list instances
aknysh 7013c32
Add --identity flag and ATMOS_IDENTITY env var support to atmos list …
aknysh 97fc054
Add --identity flag documentation to all atmos list commands
aknysh d09f6b3
Add --identity support to all list commands using ExecuteDescribeStacks
aknysh 4ad1ad0
Add unit tests to improve code coverage
aknysh eea99ca
address comments
aknysh ad9c0a8
add tests
aknysh ea70340
address comments
aknysh 43daaa5
address comments
aknysh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,210 @@ | ||
| # Stack-Level Default Auth Identity Not Recognized | ||
|
|
||
| ## Issue Summary | ||
|
|
||
| When a default identity is configured only in stack config (e.g., `stacks/orgs/acme/_defaults`), | ||
| the identity is not passed to `describe component` and other commands. Users are prompted to | ||
| select an identity even though one is configured as default in their stack configuration. | ||
|
|
||
| ## Symptoms | ||
|
|
||
| ```bash | ||
| ❯ atmos describe component <component> -s <stack> | ||
| ┃ No default identity configured. Please choose an identity: | ||
| ┃ > identity-1 | ||
| ┃ identity-2 | ||
| ┃ ... | ||
| ``` | ||
|
|
||
| Even when stack config has: | ||
| ```yaml | ||
| # stacks/orgs/acme/_defaults.yaml | ||
| auth: | ||
| identities: | ||
| identity-1: | ||
| default: true | ||
| ``` | ||
|
|
||
| ## Workaround | ||
|
|
||
| Setting the default in the profile config works: | ||
| ```yaml | ||
| # profiles/managers/atmos.yaml | ||
| auth: | ||
| identities: | ||
| identity-1: | ||
| kind: aws/assume-role | ||
| default: true | ||
| via: | ||
| identity: identity-1/permission-set | ||
| principal: | ||
| assume_role: arn:aws:iam::123456789012:role/managers | ||
| ``` | ||
|
|
||
| ## Root Cause Analysis | ||
|
|
||
| ### Different Root Causes for Different Commands | ||
|
|
||
| The issue manifests differently depending on the command type: | ||
|
|
||
| #### For `atmos describe component` - Missing Implementation | ||
|
|
||
| The `atmos describe component` command was **not following the same pattern** as `atmos terraform *` commands. While terraform commands already implemented Component Auth Merge (Approach 1), `describe component` was simply using global auth config directly: | ||
|
|
||
| **Old (broken) code in `cmd/describe_component.go`:** | ||
| ```go | ||
| // Load atmos configuration - processStacks = FALSE | ||
| atmosConfig, err := cfg.InitCliConfig(..., false) | ||
|
|
||
| // Create AuthManager using ONLY atmos.yaml + profile auth config | ||
| // Stack-level defaults were completely ignored! | ||
| authManager, err := CreateAuthManagerFromIdentity(identityName, &atmosConfig.Auth) | ||
| ``` | ||
|
|
||
| The fix was to update `describe component` to follow the same Approach 1 pattern that `terraform` commands already used. | ||
|
|
||
| #### For Multi-Stack Commands - Timing Problem (Chicken-and-Egg) | ||
|
|
||
| For commands like `describe stacks`, `describe affected`, and `list instances` that operate on multiple stacks/components, there's a genuine **chicken-and-egg problem**: | ||
|
|
||
| 1. **`InitCliConfig(processStacks=false)`** only loads: | ||
| - System atmos config | ||
| - User atmos config (`~/.atmos/atmos.yaml`) | ||
| - Project atmos config (`atmos.yaml`) | ||
| - Profile configs (e.g., `profiles/managers/atmos.yaml`) | ||
|
|
||
| 2. **Stack configs are NOT loaded** at this point because: | ||
| - Processing stacks may require authentication (for YAML functions) | ||
| - Authentication requires knowing the identity | ||
| - Identity resolution happens before stack processing | ||
|
|
||
| 3. These commands cannot use Approach 1 (Component Auth Merge) because they don't have a specific component+stack pair to query upfront. | ||
|
|
||
| ### Why Profile Config Works | ||
|
|
||
| Profile configs are loaded during `InitCliConfig` **before** stacks are processed: | ||
| - Profiles are part of the atmos configuration layer | ||
| - They're merged into `atmosConfig.Auth` immediately | ||
| - The `AuthManager` sees them when checking for defaults | ||
|
|
||
| ### Why Stack Config Didn't Work | ||
|
|
||
| For `describe component`: The command simply wasn't merging stack-level auth config. | ||
|
|
||
| For multi-stack commands: Stack configs are only processed when `processStacks=true`, creating a timing issue that required Approach 2 (Stack Scanning) to solve. | ||
|
|
||
| ## Solution | ||
|
|
||
| Two approaches are used depending on whether a specific component+stack pair is available: | ||
|
|
||
| ### Approach 1: Component Auth Merge (for commands with specific component+stack) | ||
|
|
||
| For commands like `describe component` and `terraform *` where both component and stack are known, | ||
| we leverage the **existing stack inheritance and merge functionality**: | ||
|
|
||
| 1. Call `ExecuteDescribeComponent()` with `ProcessTemplates=false, ProcessYamlFunctions=false, AuthManager=nil` | ||
| 2. The component config output includes the merged auth section from stack inheritance | ||
| 3. Merge with global auth using `auth.MergeComponentAuthFromConfig()` | ||
| 4. Create auth manager with the merged config | ||
|
|
||
| This approach is preferred because: | ||
| - It uses the existing stack merge logic (no duplication) | ||
| - The auth section includes all inherited defaults from stack hierarchy | ||
| - Component-level auth overrides are respected | ||
|
|
||
| **Example flow in `terraform.go` and `describe_component.go`:** | ||
| ```go | ||
| // 1. Start with global auth config | ||
| mergedAuthConfig := auth.CopyGlobalAuthConfig(&atmosConfig.Auth) | ||
|
|
||
| // 2. Get component config (includes stack-level auth with default flag) | ||
| componentConfig, err := ExecuteDescribeComponent(&ExecuteDescribeComponentParams{ | ||
| Component: component, | ||
| Stack: stack, | ||
| ProcessTemplates: false, | ||
| ProcessYamlFunctions: false, // Avoid circular dependency | ||
| AuthManager: nil, // No auth manager yet | ||
| }) | ||
|
|
||
| // 3. Merge component auth (including stack defaults) with global auth | ||
| if err == nil { | ||
| mergedAuthConfig, err = auth.MergeComponentAuthFromConfig(...) | ||
| } | ||
|
|
||
| // 4. Create auth manager with fully merged config | ||
| authManager, err := CreateAuthManagerFromIdentity(identityName, mergedAuthConfig) | ||
| ``` | ||
|
|
||
| ### Approach 2: Stack Scanning (for commands without specific component) | ||
|
|
||
| For commands that operate on multiple stacks/components (e.g., `describe stacks`, `describe affected`), | ||
| we perform a lightweight pre-scan of stack configurations to extract auth identity defaults: | ||
|
|
||
| 1. **Stack auth scanner** (`pkg/config/stack_auth_scanner.go`): | ||
| - Scans stack manifest files for `auth.identities.*.default: true` | ||
| - Uses minimal YAML parsing without template/function processing | ||
| - Returns a map of identity names to their default status | ||
|
|
||
| 2. **Merge into auth config** before creating an auth manager: | ||
| - Stack defaults take **precedence** over atmos.yaml defaults | ||
| - Follows Atmos inheritance model (more specific config overrides global) | ||
|
|
||
| ### Precedence Order (lowest to highest priority) | ||
|
|
||
| 1. Atmos config defaults (`atmos.yaml`) | ||
| 2. Stack config defaults (scanned or from component merge) | ||
| 3. CLI flag (`--identity`) / environment variable (`ATMOS_IDENTITY`) | ||
|
|
||
| ### Key Files Changed | ||
|
|
||
| **New Files:** | ||
| - `pkg/config/stack_auth_scanner.go` - Scanner for stack-level auth defaults | ||
| - `pkg/config/stack_auth_scanner_test.go` - Unit tests for scanner | ||
|
|
||
| **Commands Using Component Auth Merge (Approach 1):** | ||
| - `cmd/describe_component.go` - Uses terraform.go pattern with `ExecuteDescribeComponent` + `MergeComponentAuthFromConfig` | ||
| - `internal/exec/terraform.go` - Original implementation of this pattern | ||
|
|
||
| **Commands Using Stack Scanning (Approach 2):** | ||
| - `cmd/describe_stacks.go` - Uses `CreateAuthManagerFromIdentityWithAtmosConfig` | ||
| - `cmd/describe_affected.go` - Uses `CreateAuthManagerFromIdentityWithAtmosConfig` | ||
| - `cmd/describe_dependents.go` - Uses `CreateAuthManagerFromIdentityWithAtmosConfig` | ||
| - `cmd/list/instances.go` - Uses `CreateAndAuthenticateManagerWithAtmosConfig` | ||
| - `internal/exec/workflow_utils.go` - Scans stack defaults when creating AuthManager | ||
|
|
||
| **Updated Internal Execution:** | ||
| - `internal/exec/terraform_nested_auth_helper.go` - Resolves AuthManager for nested component references in YAML functions using Approach 1 | ||
|
|
||
| **Updated Auth Helpers:** | ||
| - `pkg/auth/manager_helpers.go` - New `CreateAndAuthenticateManagerWithAtmosConfig` function | ||
| - `cmd/identity_flag.go` - New `CreateAuthManagerFromIdentityWithAtmosConfig` wrapper | ||
|
|
||
| ## Testing | ||
|
|
||
| ### Unit Tests | ||
| - `pkg/config/stack_auth_scanner_test.go` - Scanner logic tests | ||
| - `pkg/auth/manager_helpers_test.go` - Integration with auth manager | ||
|
|
||
| ### Integration Tests | ||
| - Test fixture in `tests/fixtures/scenarios/stack-auth-defaults/` | ||
| - CLI test verifying stack-level defaults work without prompting | ||
|
|
||
| ## Commands/Features Now Supporting Stack-Level Auth Defaults | ||
|
|
||
| ### CLI Commands Using Component Auth Merge (Approach 1) | ||
| - `atmos describe component` - Has specific component+stack | ||
| - `atmos terraform *` (all terraform subcommands) - Has specific component+stack | ||
|
|
||
| ### CLI Commands Using Stack Scanning (Approach 2) | ||
| - `atmos describe stacks` - Operates on multiple stacks/components | ||
| - `atmos describe affected` - Operates on all affected components | ||
| - `atmos describe dependents` - Operates on multiple stacks | ||
| - `atmos list instances` - Lists all instances across stacks | ||
|
|
||
| ### YAML Functions | ||
| - `!terraform.state` - Inherits AuthManager from parent command context | ||
| - `!terraform.output` - Inherits AuthManager from parent command context | ||
| - For nested component references, uses Approach 1 (Component Auth Merge) via `resolveAuthManagerForNestedComponent()` | ||
|
|
||
| ### Workflows | ||
| - Workflow execution scans for stack-level defaults when no explicit identity is specified (uses Approach 2) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.