fix: authbridge resolver reads auth context from manager's stackInfo, not caller's#2379
Conversation
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. |
📝 WalkthroughWalkthroughAdds a Go version pin and changes the auth bridge resolver to read post-auth AuthContext from the auth manager’s Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant AuthManager
participant Store
Client->>Resolver: Request auth config (AWS/Azure/GCP)
Resolver->>AuthManager: GetStackInfo()
AuthManager-->>Resolver: managerStackInfo (ConfigAndStacksInfo with AuthContext)
Resolver->>Resolver: nil-check managerStackInfo and cloud AuthContext
Resolver-->>Store: return cloud-specific store.*AuthConfig extracted from managerStackInfo
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: panic: runtime error: invalid memory address or nil pointer dereference goroutine 1 [running]: ... [truncated 5045 characters] ... /cobra.(*Command).execute(0xc00061c908, {0xc000b66c80, 0x8, 0x8}) Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.go-version (1)
1-1: ⚡ Quick winAlign Docker/devcontainer Go images with the pinned toolchain (
1.26.2).
.go-versionpins Go1.26.2, andgo.modappears to match. However, the repo context shows.devcontainer/Dockerfileis currently usinggolang:1.26.0, which can lead to dev/CI drift (and confusing “works on my machine” failures). Please update any Docker/devcontainer/toolchain image tags that still reference1.26.0to1.26.2to keep environments consistent.Verify in the PR branch (or subsequent PR) that all Go toolchain pins (e.g.,
.devcontainer/Dockerfile, any builder stages, and CI images) are aligned to1.26.2and not1.26.0by searching for1.26.0/golang:1.26in the repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.go-version at line 1, Update any Docker/devcontainer/CI image tags that still reference the older Go toolchain so they match the pinned .go-version value 1.26.2: search the repo for occurrences of "golang:1.26.0", "1.26.0" and "golang:1.26" and replace them with "golang:1.26.2" (for example update the image tag in .devcontainer/Dockerfile and any builder/CI images), then rebuild/test the devcontainer/CI to verify consistency with go.mod/.go-version.
🤖 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/store/authbridge/resolver_test.go`:
- Around line 131-133: Test hardcodes Unix-style paths for CredentialsFile and
ConfigFile; update the test in resolver_test.go to build expected path values
using filepath.Join instead of literal "/tmp/aws-creds" and "/tmp/aws-config"
(and the other occurrences around lines 152-153). Import "path/filepath" in the
test, replace the hardcoded strings assigned to CredentialsFile and ConfigFile
and any corresponding assertions with filepath.Join(...) calls so the test is
portable across platforms.
---
Nitpick comments:
In @.go-version:
- Line 1: Update any Docker/devcontainer/CI image tags that still reference the
older Go toolchain so they match the pinned .go-version value 1.26.2: search the
repo for occurrences of "golang:1.26.0", "1.26.0" and "golang:1.26" and replace
them with "golang:1.26.2" (for example update the image tag in
.devcontainer/Dockerfile and any builder/CI images), then rebuild/test the
devcontainer/CI to verify consistency with go.mod/.go-version.
🪄 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: bfdb3e35-e84d-4c22-99ab-693c00050f11
📒 Files selected for processing (3)
.go-versionpkg/store/authbridge/resolver.gopkg/store/authbridge/resolver_test.go
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/store/authbridge/resolver_test.go (1)
122-125: ⚡ Quick winStrengthen pointer-mismatch regression with conflicting caller context.
resolverStackInfois empty today; seeding it with conflicting AWS values would better prove resolver output always comes from manager-owned stack info, not caller-owned stack info.Suggested hardening for the regression test.
- resolverStackInfo := &schema.ConfigAndStacksInfo{} + resolverStackInfo := &schema.ConfigAndStacksInfo{ + AuthContext: &schema.AuthContext{ + AWS: &schema.AWSAuthContext{ + CredentialsFile: filepath.Join("tmp", "caller-creds"), + ConfigFile: filepath.Join("tmp", "caller-config"), + Profile: "caller-profile", + Region: "us-east-2", + }, + }, + } @@ assert.Equal(t, expectedCredsFile, authConfig.CredentialsFile) + assert.Equal(t, expectedConfigFile, authConfig.ConfigFile) assert.Equal(t, "dev-admin", authConfig.Profile) + assert.Equal(t, "us-west-2", authConfig.Region) - // Confirm the resolver's own stackInfo was never populated (proving the fix reads from the manager). - assert.Nil(t, resolverStackInfo.AuthContext) + // Confirm caller-owned stackInfo was not used/mutated. + assert.Equal(t, "caller-profile", resolverStackInfo.AuthContext.AWS.Profile)Based on learnings: "Applies to **/*_test.go : For aliasing/isolation tests, verify BOTH directions: after a merge, mutate the result and confirm the original inputs are unchanged (result→src isolation); also mutate a source map before the merge and confirm the result is unaffected (src→result isolation)".
Also applies to: 152-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/authbridge/resolver_test.go` around lines 122 - 125, The test currently creates an empty resolverStackInfo (type *schema.ConfigAndStacksInfo) which doesn't validate pointer-aliasing; seed resolverStackInfo with conflicting AWS values (e.g., different AccountID/Region/Partition fields) before calling NewResolver or the auth manager functions and then assert the resolved stacks come from the manager-owned info rather than this caller-provided struct. After resolution, mutate the returned result and verify resolverStackInfo remains unchanged, and conversely mutate resolverStackInfo prior to merge and verify the final resolved result is unaffected—this ensures both result→src and src→result isolation for functions like NewResolver and any methods on the auth manager that populate stack info.
🤖 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/store/authbridge/resolver_test.go`:
- Around line 122-125: The test currently creates an empty resolverStackInfo
(type *schema.ConfigAndStacksInfo) which doesn't validate pointer-aliasing; seed
resolverStackInfo with conflicting AWS values (e.g., different
AccountID/Region/Partition fields) before calling NewResolver or the auth
manager functions and then assert the resolved stacks come from the
manager-owned info rather than this caller-provided struct. After resolution,
mutate the returned result and verify resolverStackInfo remains unchanged, and
conversely mutate resolverStackInfo prior to merge and verify the final resolved
result is unaffected—this ensures both result→src and src→result isolation for
functions like NewResolver and any methods on the auth manager that populate
stack info.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc75313b-6be9-400f-9cba-7c6fd2af100f
📒 Files selected for processing (2)
.devcontainer/Dockerfilepkg/store/authbridge/resolver_test.go
✅ Files skipped from review due to trivial changes (1)
- .devcontainer/Dockerfile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2379 +/- ##
=======================================
Coverage 77.95% 77.96%
=======================================
Files 1090 1090
Lines 103075 103078 +3
=======================================
+ Hits 80352 80361 +9
+ Misses 18316 18311 -5
+ Partials 4407 4406 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
These changes were released in v1.217.0-rc.3. |
3 participants
what
instead of the caller's stackInfo
why
the terraform executor to authbridge.NewResolver
stackInfo.AuthContext.AWS, never the caller's info
manager's pointer, populated by auth)
available"
references
Summary by CodeRabbit
Bug Fixes
Chores
Tests