fix(credentials): rank an EKS ownership record above ambient AWS variables - #6472
Conversation
…ables RecordedAWSResolver resolved a cluster's lifecycle credentials base-first, falling back to the ownership record's captured alias only where the base resolved nothing. In the web UI the base is credentials.Manager, whose Value falls through to the canonical process environment whenever the secure store holds nothing — so an exported AWS_ACCESS_KEY_ID outranked the alias the create actually ran through. The precedence was written for the secure store, where a stored value is name-independent operator intent and rightly wins. Manager could not express the difference between that and its own ambient fall-through, so the rule applied to both halves. Introduce ExplicitResolver, the optional half of Resolver that reports only a deliberately-configured value, and implement it on Manager over the secure store alone. RecordedAWSResolver now resolves explicit intent first, the ownership record second, and the ambient environment last. A base that declares no explicit channel is treated as ambient in full, so a resolver that forgets to distinguish its halves loses to the record rather than silently overriding it. This also settles a name/value disagreement: EnvVar reported the record's alias while Value could come from the canonical variable, and the frozen resolution carries EnvVar onward to scrub child process environments. Fixes #6427
The type comment still described values as base-first and the composition as strictly additive, which the precedence change makes false. State the actual order and name the direction the change moves a mismatch in: a refused mutation becomes a working one, never the reverse.
✅MegaLinter analysis: Success✅ Linters with no issuesactionlint, bash-exec, git_diff, hadolint, jscpd, jsonlint, lychee, markdown-table-formatter, markdownlint, prettier, prettier, shellcheck, shfmt, stylelint, syft, trivy-sbom, trufflehog, v8r, v8r, yamllint Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts
|
ExplicitResolver ranks a base above the ownership record only for values it holds deliberately, and *Manager — the sole implementer — returns only the secure store from ExplicitValue. A Settings entry selects which variable to read, not its contents, so its value arrives from the ambient environment and ranks behind the record. Two doc claims said the opposite.
@coderabbitai review |
|
✅ Action performedReview finished.
|
Readiness record for
|
📝 WalkthroughWalkthroughThe credential system adds an explicit-value resolution contract. Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
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}
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/svc/credentials/credentials.go`:
- Around line 217-224: Update the credential resolution flow around Value and
EnvVar to capture the recorded variable name once and use that check for both
operations: after ExplicitValue lookup, delegate to base when no captured name
exists, while keeping the nil-base path safe. Preserve the existing behavior for
recorded mappings, add a regression test covering a canonical variable plus a
Settings-selected base alias with no recorded mapping, and revise the nearby
documentation to state that Settings selects ambient input rather than
deliberate credential intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55087499-5bea-4981-9a01-dcfe1695274b
📒 Files selected for processing (4)
pkg/svc/credentials/credentials.gopkg/svc/credentials/manager.gopkg/svc/credentials/recorded_aws_resolver_ambient_test.gopkg/svc/credentials/recorded_aws_resolver_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Use Go 1.26.1 or newer, matching the version declared ingo.mod.
All user-supplied file path arguments in CLI commands must be canonicalized withfsutil.EvalCanonicalPathbefore use; create parent directories first for new output paths.
Usefsutil.ReadFileSafefor constrained file reads instead of reimplementing path-containment checks.
Do not manually register MCP or Copilot tool handlers; runnable Cobra commands are exposed through automatic generation inpkg/toolgen.
Use a typedexperimentalfield inksail.yamlfor configuration-gated behavior that is not an entire command; regenerate the schema and CRD.
Graduate validated experimental features by deleting the singleGuardcall; do not retain unnecessary experimental scaffolding.
Run formatting and linting withgolangci-lint run --fixandgolangci-lint run --timeout 5m; validate withgo buildandgo test ./....
Files:
pkg/svc/credentials/recorded_aws_resolver_ambient_test.gopkg/svc/credentials/manager.gopkg/svc/credentials/recorded_aws_resolver_test.gopkg/svc/credentials/credentials.go
**/*.{go,yaml,yml,md,mdx,ts,tsx,json}
📄 CodeRabbit inference engine (AGENTS.md)
Generated files must not be hand-edited; run
make generateas the canonical regeneration command.
Files:
pkg/svc/credentials/recorded_aws_resolver_ambient_test.gopkg/svc/credentials/manager.gopkg/svc/credentials/recorded_aws_resolver_test.gopkg/svc/credentials/credentials.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Add regression tests for confident bug fixes and run flaky-test candidates repeatedly with
go test -run <T> -count=10 ./....
Files:
pkg/svc/credentials/recorded_aws_resolver_ambient_test.gopkg/svc/credentials/recorded_aws_resolver_test.go
🧠 Learnings (1)
📚 Learning: 2026-08-02T19:26:41.922Z
Learnt from: devantler
Repo: devantler-tech/ksail PR: 6434
File: pkg/cli/clusterapi/eks_create_identity_test.go:0-0
Timestamp: 2026-08-02T19:26:41.922Z
Learning: In Go tests using Testify v1.11.1, do not flag require.NoError(t, err) inside an Eventually condition solely because the condition may run in another goroutine: require calls t.Errorf before FailNow, and t.Errorf marks the outer test as failed. However, prefer decomposing Eventually conditions so service errors, missing resources, and state or phase mismatches are reported separately for clearer diagnostics.
Applied to files:
pkg/svc/credentials/recorded_aws_resolver_ambient_test.gopkg/svc/credentials/recorded_aws_resolver_test.go
🔇 Additional comments (4)
pkg/svc/credentials/credentials.go (1)
192-206: LGTM!Also applies to: 225-237, 249-251, 261-267, 272-277
pkg/svc/credentials/manager.go (1)
140-150: LGTM!pkg/svc/credentials/recorded_aws_resolver_test.go (1)
29-32: LGTM!pkg/svc/credentials/recorded_aws_resolver_ambient_test.go (1)
1-11: LGTM!Also applies to: 13-39, 41-58, 60-76, 78-88, 90-101, 103-119, 121-142
…e value AWSOptionsResolver.EnvVar resolves an absent mapping to the canonical default, so consulting the record for an UNRECORDED key fabricated an opinion it does not hold -- the plain ambient value -- and that outranked the base own Settings-selected alias, which had won before this PR. EnvVar had the mirror defect: it reported the canonical name for a value the base resolved through its alias, so the frozen resolution would scrub the wrong variable. Both now check the captured name directly and delegate to base when there is none. Regression test covers a canonical variable plus a base alias with no recorded mapping; the existing unrecorded-key arm cannot see it, because its stub configures no alias so both paths read the same variable.
@codex review |
Review ResultNo actionable findings. The updated resolver correctly:
No code changes were necessary, so no commit or follow-up pull request was created. Validation
|
Readiness record for
|
| case | EnvVar() |
Value() |
|---|---|---|
| A — key is in the ownership record | RECORDED_ACCESS |
RECORDED-VALUE |
| B — key not recorded, base has an alias | SETTINGS_ALIAS |
SETTINGS-VALUE |
| C — key not recorded, nil base | AWS_ACCESS_KEY_ID |
AMBIENT-CANONICAL |
Both halves are printed deliberately: the original defect was EnvVar() and Value() naming
different variables, so asserting on the value alone would not have seen it. They agree in all three
cases.
Counterfactual against the parent commit, which is what makes case B evidence rather than
assertion — same program, credentials.go reverted to 83834e53:
BEFORE B) EnvVar=AWS_ACCESS_KEY_ID Value="AMBIENT-CANONICAL" <- the regression
AFTER B) EnvVar=SETTINGS_ALIAS Value="SETTINGS-VALUE" <- fixed
A and C are unchanged by the fix, so the change is localised to the unrecorded-key path it claims to
address. Promoting.
1 participant
Why
An EKS cluster created through the web UI records the AWS variable names its create actually ran through, so later delete/start/stop can resolve the same identity. That record was being ignored whenever the operator also had a plain
AWS_ACCESS_KEY_IDexported — the ambient value won instead.The practical effect is a dead end: an operator whose shell points at a different account can no longer operate a cluster through the UI, even though the correct credentials are available under the recorded name. Ownership verification then refuses the mutation, so nothing acts on the wrong account — but the cluster becomes stuck rather than manageable.
What
Credentials the operator set deliberately (secure store / Settings) still win, exactly as before. What changed is that a resolver's ambient fall-through no longer outranks the cluster's own ownership record — the record now sits between the two.
The safe direction is the default: a credential source that does not distinguish "deliberately configured" from "found in the environment" is treated as ambient, so it loses to the record rather than silently overriding it.
Fixes #6427