atmos icon indicating copy to clipboard operation
atmos copied to clipboard

Fix: Enable auto-import of provisioned identities from profiles

Open osterman opened this issue 2 months ago β€’ 6 comments

what

  • Add injectProvisionedIdentitiesPostLoad() function that runs after profiles are loaded, ensuring auth.providers (which may be defined in profiles) is available before checking for provisioned identity cache files
  • Add processFileImportsIfPresent() helper to enable import: directives in profile files (previously silently ignored)
  • Comprehensive test coverage for both provisioned identity injection and profile imports

why

Provisioned identities were not auto-importing when auth.providers was defined in a profile because the provider check happened during early config loading, before profiles were merged. Additionally, import: directives inside profile files were completely ignored. These changes fix both issues, allowing proper auto-discovery of provisioned identities from cache files.

references

Fixes provisioned identity auto-import for profiles with auto_provision_identities: true configured.

Summary by CodeRabbit

  • New Features

    • UI/CLI now preserves and displays original-case identity names (display names) in lists and outputs.
    • Imports are merged before their main files; main file still takes precedence.
  • Bug Fixes

    • User-defined identities reliably override auto-provisioned ones by reapplying user config precedence.
    • Provision/import errors are non-fatal to avoid load failures.
  • Tests

    • Extensive new tests for provisioning, imports, case-preservation, precedence, and edge cases.
  • Chores

    • Increased trace-level logging for import/provision actions.

✏️ Tip: You can customize this high-level summary in your review settings.

osterman avatar Dec 12 '25 21:12 osterman

Dependency Review

βœ… No vulnerabilities or license issues found.

Scanned Files

None

github-actions[bot] avatar Dec 12 '25 21:12 github-actions[bot]

πŸ“ Walkthrough

Walkthrough

This PR implements provisioned identity injection and case-preservation in the Atmos config loading pipeline. It adds logic to load provisioned identities from cache files after profiles are processed, re-applies user config for precedence, handles per-file imports during directory loading, and preserves original-case identity names via a new IdentityCaseMap. A GetIdentityDisplayName method is added to the AuthManager interface and threaded through auth formatters.

Changes

Cohort / File(s) Summary
Config Loading Core
pkg/config/load.go
Modified execution flow to inject provisioned identities post-profile load, re-apply user config for precedence, and process per-file imports before merging main config. Increased trace-level logging and extended case-preservation to include provisioned identity data.
Provisioned Identity Case Preservation
pkg/config/provisioned_identity_case.go
New file implementing discovery and preservation of original-case provisioned identity names from per-provider XDG cache files. Provides helper functions to read YAML, extract identities, and populate IdentityCaseMap.
Config Loading Tests
pkg/config/load_test.go
Added extensive test suite covering provisioned identity injection scenarios, user config precedence override, case preservation across varied edge cases (no providers, multiple providers, cache issues), and config import/merge logic.
Profile Tests
pkg/config/profiles_test.go
Added tests for import directives within profile files and the processFileImportsIfPresent helper.
AuthManager Interface & Core
pkg/auth/types/interfaces.go, pkg/auth/types/mock_interfaces.go, pkg/auth/manager.go
Added public GetIdentityDisplayName method to AuthManager interface and manager implementation. Returns display name preserving original case from IdentityCaseMap or falls back to input key. Updated mock.
Auth Manager Tests
pkg/auth/manager_case_test.go, pkg/auth/providers/aws/saml_test.go
Added test cases for provisioned identity case sensitivity and identity display name retrieval. Extended test stubs.
Auth Formatter Updates
pkg/auth/list/formatter_table.go, pkg/auth/list/formatter_tree.go
Updated to use GetIdentityDisplayName for displaying identity names in table and tree formats, preserving original case.
Auth Test Stubs
cmd/auth_console_test.go, pkg/auth/hooks_test.go
Added GetIdentityDisplayName method to mock AuthManager implementations.
Terraform Output Utils
internal/exec/terraform_output_utils.go
Added GetIdentityDisplayName method to authContextWrapper.
Golden Test Snapshot
tests/snapshots/TestCLICommands_Valid_Log_Level_in_Config_File.stderr.golden
Updated diagnostic logging output to reflect new provisioned identity injection flow, config precedence re-application, and expanded config discovery checks.

Sequence Diagram

sequenceDiagram
    participant User as User
    participant CLI as CLI
    participant ConfigLoader as Config Loader
    participant ProfileMgr as Profile Manager
    participant IdentityCache as Identity Cache
    participant AuthMgr as Auth Manager
    participant Formatter as Identity Formatter

    User->>CLI: Load config
    CLI->>ConfigLoader: LoadConfig()
    
    ConfigLoader->>ProfileMgr: Load profiles
    ProfileMgr-->>ConfigLoader: Merged profiles + user config
    
    ConfigLoader->>IdentityCache: injectProvisionedIdentitiesPostLoad()
    Note over IdentityCache: Read per-provider cache files<br/>from XDG directory
    IdentityCache->>ConfigLoader: Populate IdentityCaseMap
    
    ConfigLoader->>ConfigLoader: reapplyUserConfigForPrecedence()
    Note over ConfigLoader: Re-merge to ensure<br/>user config takes precedence
    
    ConfigLoader->>ConfigLoader: preserveProvisionedIdentityCase()
    Note over ConfigLoader: Extract and preserve<br/>original-case identity names
    
    ConfigLoader-->>CLI: Final AtmosConfiguration
    
    CLI->>AuthMgr: ResolveIdentity(lowercase_name)
    CLI->>AuthMgr: GetIdentityDisplayName(lowercase_name)
    AuthMgr->>AuthMgr: Lookup in IdentityCaseMap
    AuthMgr-->>CLI: Display name (original case)
    
    CLI->>Formatter: Format identity list
    Formatter->>AuthMgr: GetIdentityDisplayName()
    AuthMgr-->>Formatter: Original-case names
    Formatter-->>User: Display with preserved case

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Config loading flow logic: Review new provisioned identity injection, precedence re-application, and import processing order changes in load.go to ensure proper merge semantics and side-effect handling.
  • Identity case preservation: Verify XDG cache file discovery, YAML parsing, and error handling in provisioned_identity_case.go; ensure user config takes precedence over provisioned mappings.
  • Interface changes: Confirm GetIdentityDisplayName implementation across all AuthManager implementations (manager, mocks, stubs) and verify IdentityCaseMap threading through config/auth lifecycle.
  • Test coverage: Scan test additions for correctness of edge cases (nil maps, missing files, multiple providers) and integration points between config loading and identity display.
  • Golden snapshot changes: Verify new diagnostic logging reflects actual flow and is appropriate for trace-level verbosity.

Possibly related PRs

  • #1684: Adds GetIdentityDisplayName and IdentityCaseMap handling to auth identity case/display-name surface.
  • #1805: Touches config/auth loading surface with profile and identity loading concerns.
  • #1742: Adds and propagates GetIdentityDisplayName behavior across auth types and formatters.

Suggested reviewers

  • aknysh
  • milldr

Pre-merge checks and finishing touches

βœ… Passed checks (3 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title directly and specifically describes the main change: enabling auto-import of provisioned identities from profiles, which aligns with the PR objectives.
Docstring Coverage βœ… Passed Docstring coverage is 94.44% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • [ ] πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch osterman/fix-identity-import

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.

❀️ Share

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

coderabbitai[bot] avatar Dec 12 '25 21:12 coderabbitai[bot]

πŸ“ Walkthrough

Walkthrough

The changes extend the config loading pipeline with post-load provisioned identity injection, user config precedence reapplication, and import processing hooks. Functions enable provisioned identities to load after profiles, ensure user config overrides provisioned data, and process import directives within individual config files with non-fatal error handling.

Changes

Cohort / File(s) Summary
Post-load provisioning and precedence logic
pkg/config/load.go
Introduces injectProvisionedIdentitiesPostLoad to load provisioned identity files after profile application; reapplyUserConfigForPrecedence to re-merge user config after provisioned identities; reapplyProfilesToEnforceConfigPrecedence to re-apply profiles; processFileImportsIfPresent to parse and apply import directives within individual files; updates loadAtmosConfigsFromDirectory and mergeDefaultImports to invoke import processing for each loaded file. All new steps use non-fatal, debug-level logging for failures.
Load.go test coverage
pkg/config/load_test.go
Adds TestInjectProvisionedIdentitiesPostLoad (provider skipping, cache presence, identity loading from provisioned-identities.yaml, multiple provider handling) and TestInjectProvisionedIdentitiesPostLoad_ErrorHandling (invalid cache handling); adds TestUserConfigOverridesProvisionedIdentities to verify user config merging and precedence re-application override semantics.
Profile import tests
pkg/config/profiles_test.go
Introduces TestProfileWithImports (import directives in profile files, value merging, override semantics) and TestProcessFileImportsIfPresent (import processing helper validation, missing import handling). Both construct temporary directories, write test files, and assert configuration outcomes.

Sequence Diagram

sequenceDiagram
    participant Client
    participant LoadConfig
    participant ProfileLoader
    participant ProvisionedLoader
    participant UserConfig
    participant ImportProcessor

    Client->>LoadConfig: LoadConfig()
    LoadConfig->>ProfileLoader: Load & apply profiles
    ProfileLoader-->>LoadConfig: Profiles applied
    
    LoadConfig->>ProvisionedLoader: injectProvisionedIdentitiesPostLoad()
    ProvisionedLoader->>ProvisionedLoader: Read provisioned-identities.yaml
    ProvisionedLoader-->>LoadConfig: Identities injected (non-fatal)
    
    LoadConfig->>ProfileLoader: reapplyProfilesToEnforceConfigPrecedence()
    ProfileLoader-->>LoadConfig: Profiles re-applied
    
    LoadConfig->>UserConfig: reapplyUserConfigForPrecedence()
    UserConfig->>UserConfig: Re-merge main config file
    UserConfig-->>LoadConfig: User config precedence restored
    
    LoadConfig->>ImportProcessor: processFileImportsIfPresent()
    ImportProcessor->>ImportProcessor: Parse & apply imports
    ImportProcessor-->>LoadConfig: Imports processed (non-fatal)
    
    LoadConfig-->>Client: Final merged config

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Precedence semantics: Verify that the order of operations (profiles β†’ provisioned identities β†’ user config re-application β†’ imports) maintains correct override hierarchy across all scenarios
  • load.go integration points: Review how new functions hook into loadAtmosConfigsFromDirectory, mergeDefaultImports, and the main LoadConfig flow to ensure no race conditions or missing cases
  • Error handling coverage: Confirm all non-fatal error paths (debug logging) are appropriate and don't mask critical failures; validate test coverage for error scenarios in load_test.go
  • Import processing logic: Assess processFileImportsIfPresent correctness when called at multiple points in the loading pipeline

Pre-merge checks and finishing touches

βœ… Passed checks (3 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title accurately captures the main change: enabling auto-import of provisioned identities from profiles, which is central to the PR's objectives.
Docstring Coverage βœ… Passed Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • [ ] πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch osterman/fix-identity-import

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.

❀️ Share

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

coderabbitai[bot] avatar Dec 12 '25 21:12 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 70.50360% with 41 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 73.48%. Comparing base (638d1b8) to head (42fbc93). :warning: Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/config/load.go 56.94% 20 Missing and 11 partials :warning:
pkg/config/provisioned_identity_case.go 89.79% 3 Missing and 2 partials :warning:
internal/exec/terraform_output_utils.go 0.00% 3 Missing :warning:
pkg/auth/list/formatter_table.go 50.00% 1 Missing and 1 partial :warning:

:x: Your patch check has failed because the patch coverage (70.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
+ Coverage   73.33%   73.48%   +0.15%     
==========================================
  Files         564      566       +2     
  Lines       54336    54732     +396     
==========================================
+ Hits        39847    40221     +374     
  Misses      11625    11625              
- Partials     2864     2886      +22     
Flag Coverage Ξ”
unittests 73.48% <70.50%> (+0.15%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Ξ”
pkg/auth/list/formatter_tree.go 86.61% <100.00%> (+0.14%) :arrow_up:
pkg/auth/manager.go 85.76% <100.00%> (+0.14%) :arrow_up:
pkg/auth/list/formatter_table.go 93.10% <50.00%> (-1.27%) :arrow_down:
internal/exec/terraform_output_utils.go 68.04% <0.00%> (-0.54%) :arrow_down:
pkg/config/provisioned_identity_case.go 89.79% <89.79%> (ΓΈ)
pkg/config/load.go 75.72% <56.94%> (-2.55%) :arrow_down:

... and 17 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Dec 13 '25 00:12 codecov[bot]

[!WARNING]

This PR exceeds the recommended limit of 1,000 lines.

Large PRs are difficult to review and may be rejected due to their size.

Please verify that this PR does not address multiple issues. Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

mergify[bot] avatar Dec 13 '25 03:12 mergify[bot]

πŸ’₯ This pull request now has conflicts. Could you fix it @osterman? πŸ™

mergify[bot] avatar Dec 14 '25 02:12 mergify[bot]

πŸ’₯ This pull request now has conflicts. Could you fix it @osterman? πŸ™

mergify[bot] avatar Dec 16 '25 20:12 mergify[bot]