fix(clusterapi): verify EKS ownership before local API lifecycle actions - #6434
Conversation
…tions The local web API built its EKS provisioner without an ownership verifier, so VerifyBeforeMutation was a no-op on every delete, start and stop it issued: a destructive action ran with no identity query at all, while the standalone CLI path performs one. Resolve the verifier from the binding written at create time, verify before any provisioner is built, and carry the same frozen credential snapshot and verifier into the factory so the provisioner re-checks at its own mutation boundary. Creates stay unguarded — they have no prior incarnation. Part of #6203
✅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
|
…elds no verifier VerifyBeforeMutation reads a nil verifier as "nothing to check", which is right for creates and non-EKS callers and a silent fail-open on the mutation path: the guard would travel into the provisioner and authorize the action while checking nothing. Refuse it instead, and call the verifier directly so the failure names the cluster. Part of #6203
Validation recordThe defect, stated precisely. Ablation, per arm, each verified to BUILD (a delete-the-code arm that fails to compile runs zero
Arms A and B ran at Two controls, both green in every arm — they are what stops this from being over-tightened: A defect the linter found in my own guard, and it was a fail-open. The first version called A test-harness bug I caught in my own first RED, worth naming because it would have produced a Exercised, not just reasoned about. Ran the real unstubbed No AWS call is reached — the refusal happens on local evidence. The pre-existing Limitation, stated rather than hidden: the AWS-reachable success path (frozen credentials → Scope. |
…wnership guard Every other test in this package injects a fake factory, and a fake satisfies the guard interface because it was written to. Nothing touched the factory the shipped binary builds — so if DefaultFactory ever stopped satisfying it, every real EKS mutation would be refused with ErrUnguardableFactory while this package stayed green, and the failure would surface only to an operator. Also pins the value receiver (guarding one action must not leave its identity on the shared factory), the fail-closed refusal, and the no-guard control. Part of #6203
Arm D at
|
A trade-off this change makes, stated rather than left for someone to discoverSelf-reviewing the diff surfaced one consequence worth putting in front of a reviewer explicitly, An EKS cluster deleted out-of-band can no longer be cleared through the web UI. Why I kept it rather than carving out an exception. The issue is explicit that "delete is What it costs, honestly. The create-refusal message elsewhere in this package promises that Filed as a follow-up rather than widened into this PR: the recovery path for a genuinely-absent EKS If a reviewer thinks the fail-closed default is wrong here, that is a legitimate call to make on this |
@coderabbitai review State at Where the risk is concentrated, if it helps focus the review:
Two limitations are disclosed rather than hidden, in the comments above: the AWS-reachable |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change adds persisted EKS ownership verification for delete, start, and stop operations. It freezes the create-time AWS resolution, captures immutable identity after creation, and reuses recorded credential aliases for later lifecycle checks. Verification and capture use bounded contexts and fail closed. Missing clusters map to Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 2
🤖 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/cli/clusterapi/eks_ownership.go`:
- Around line 54-88: Gate the new fail-closed behavior in
resolveEKSMutationGuard behind a typed experimental configuration setting
exposed through ksail.yaml. When the setting is disabled, preserve the previous
EKS delete/start/stop behavior; when enabled, retain the existing verifier
resolution and validation. Add the setting to the configuration model and
regenerate the corresponding schema and CRD.
- Around line 79-85: Bound the EKS ownership verification flow with an explicit
timeout: create a timeout context around verifier(ctx), FreezeAWS, and EKS
client construction, and ensure that context is used for all three operations.
Preserve the existing verification error wrapping while allowing slow or
unresponsive AWS endpoints to terminate when the timeout expires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b3c8f2e5-b036-47b0-ad6a-30b85491deba
📒 Files selected for processing (7)
pkg/cli/clusterapi/eks_ownership.gopkg/cli/clusterapi/eks_ownership_internal_test.gopkg/cli/clusterapi/eks_ownership_test.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/local_service_test.gopkg/svc/provisioner/cluster/factory.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/cli/clusterapi/local_service_test.gopkg/svc/provisioner/cluster/factory.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.gopkg/cli/clusterapi/eks_ownership_internal_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/eks_ownership_test.go
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: New not-yet-stable commands must be wrapped withexperimental.Guard(cmd), remain disabled by default, and require the global--experimentalflag.
Test experimental commands in both states: enabled with--experimentaland disabled withexperimental.ErrDisabled.
Files:
pkg/cli/clusterapi/local_service_test.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.gopkg/cli/clusterapi/eks_ownership_internal_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/eks_ownership_test.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/cli/clusterapi/local_service_test.gopkg/svc/provisioner/cluster/factory.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.gopkg/cli/clusterapi/eks_ownership_internal_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/eks_ownership_test.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/cli/clusterapi/local_service_test.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership_internal_test.gopkg/cli/clusterapi/eks_ownership_test.go
🔇 Additional comments (11)
pkg/cli/clusterapi/eks_ownership.go (4)
1-51: LGTM!
92-106: LGTM!
108-127: LGTM!Also applies to: 136-169
128-135: 🎯 Functional CorrectnessNo change needed.
ksail cluster eks-bindis registered fromNewRebindEKSOwnershipCmd()and accepts the--nameargument used in the error guidance.pkg/cli/clusterapi/local_service.go (2)
95-98: LGTM!Also applies to: 177-177
830-830: LGTM!Also applies to: 930-930, 975-994, 1003-1016, 1051-1051, 1060-1064
pkg/svc/provisioner/cluster/factory.go (1)
191-208: LGTM!pkg/cli/clusterapi/eks_ownership_internal_test.go (1)
1-109: LGTM!pkg/cli/clusterapi/eks_ownership_test.go (1)
1-313: LGTM!pkg/cli/clusterapi/export_test.go (1)
9-10: LGTM!Also applies to: 119-141
pkg/cli/clusterapi/local_service_test.go (1)
19-20: LGTM!Also applies to: 193-201
The mutation paths run on a context.WithoutCancel background context, so the request that triggered the action can never cancel this work, and the AWS SDK applies no overall per-operation deadline of its own. An unresponsive STS or EKS endpoint therefore left the job pinned in Deleting/Updating with no way to dismiss it — the undismissable-row failure runDelete's idempotency handling already exists to avoid. Bound the whole resolution (config load, caller identity, DescribeCluster). The verifier handed onward is not closed over this context, so the provisioner still re-checks under its own deadline at the mutation boundary. Part of #6203
@coderabbitai review Restarting the ordered loop at the top after round 1's findings, as a findings round requires. The What changed since round 1 (
State at The two disclosed limitations from round 1 are unchanged and deliberate: the AWS-reachable success (Edited to restore two commit SHAs that a shell quoting slip dropped from the "what changed" |
|
I will verify the full ownership-resolution deadline, the mutation-boundary re-verification context, and the fail-closed guard behavior. ✅ Action performedReview finished.
|
|
I will verify that the ownership-resolution deadline covers AWS configuration, caller identity, and cluster identity resolution. I will also verify that the provisioner re-verifies with its own context and that all unsupported or missing guard paths fail closed. ✅ Action performedReview finished.
|
@codex review Advancing to Codex after recording
Restarting at CodeRabbit remains correct if a later round produces findings; this only advances the State at Round 1's outcome, so this is not reviewed from scratch: one finding adopted — the ownership Two limitations are disclosed rather than hidden: the AWS-reachable success path is unexercised (it |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 713caf2db5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…on create Two P1 findings from the Codex round, both real. The verifier handed to the provisioner was unbounded: the deadline ended with resolveEKSMutationGuard, and the provisioner re-checks identity using the context it was given, which is the same uncancellable background context. A hung endpoint could still pin the job even though the first check returned quickly. Wrap it so every invocation carries its own deadline. The previous comment claiming the provisioner applied its own deadline was simply wrong. Nothing recorded the ownership identity of a cluster this backend creates: runCreate persisted only spec.json, and the EKS provisioner just runs eksctl, so eksidentity.Capture was never reached outside the CLI path. The guard therefore blocked the very clusters this API creates, and an operator would have had to run eks-bind by hand after every create. Capture it as part of a successful create, and fail the job loudly if that cannot be done. Part of #6203
Taking over this PR. Tick 894's declared ownership window ended 07:00Z; at 07:05Z the head I verified no live owner before claiming: no process holds the Picking up at: root-cause the failing system test, push, then restart the ordered review loop at CodeRabbit. |
User evaluation at
|
CI is green at Requesting the lane-priority reviewer for a current-head review. One thing worth stating up front, because it changes what a green review means here: a clean review will not make this promotable. The third readiness condition — exercising the change as a user — is not satisfiable from this host. The ownership guard sits behind a discovery gate that needs live AWS credentials and a real cluster, so the refusal path cannot be reached locally. Evidence is in the comment above; the general problem is filed as monorepo#2617. This PR therefore stays a draft on a named blocker rather than being self-promoted. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Current revision and changed files ==='
git rev-parse HEAD
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-status HEAD~1 HEAD
printf '%s\n' '=== Changed Go declarations ==='
for f in \
pkg/cli/clusterapi/eks_ownership.go \
pkg/cli/clusterapi/local_service.go \
pkg/svc/provisioner/cluster/factory.go \
pkg/svc/provisioner/cluster/eks/provisioner.go \
pkg/svc/provider/aws/nodegroup_state.go
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all --type function,method,struct,interface,var --view compact || true
fi
done
printf '%s\n' '=== Ownership-guard references ==='
rg -n -C 4 --glob '*.go' \
'resolveEKSMutationGuard|applyEKSMutationGuard|WithEKSMutationGuard|VerifyBeforeMutation|AWSOwnershipVerifier|EKSOwnership' \
pkg/cli/clusterapi pkg/svc/provisioner/cluster pkg/svc/provider/aws
printf '%s\n' '=== Focused production code ==='
sed -n '1,280p' pkg/cli/clusterapi/eks_ownership.go
sed -n '1,360p' pkg/cli/clusterapi/local_service.go
sed -n '1,260p' pkg/svc/provisioner/cluster/factory.go
sed -n '150,250p' pkg/svc/provisioner/cluster/eks/provisioner.goLength of output: 101945 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Function map ==='
ast-grep outline pkg/cli/clusterapi/local_service.go --items all --type function,method --view signatures
ast-grep outline pkg/svc/provisioner/cluster/factory_eks.go --items all --type function,method --view signatures
ast-grep outline pkg/svc/eksidentity --items all --type function,method --view signatures
printf '%s\n' '=== Lifecycle callers and provisioner construction ==='
sed -n '960,1110p' pkg/cli/clusterapi/local_service.go
rg -n -C 8 --glob '*.go' \
'\brunProvisioner\s*\(|\bnewProvisioner\s*\(|\brunDelete\s*\(|\brunStart\s*\(|\brunStop\s*\(' \
pkg/cli/clusterapi
printf '%s\n' '=== EKS factory and frozen credential consumption ==='
sed -n '1,180p' pkg/svc/provisioner/cluster/factory_eks.go
rg -n -C 5 --glob '*.go' \
'AWSResolution|OptionsForFrozenAWSConfig|FreezeAWS|NewVerifier|type Verifier|VerifyBeforeMutation' \
pkg/svc/credentials pkg/svc/eksidentity pkg/svc/provisioner/cluster
printf '%s\n' '=== New ownership tests ==='
sed -n '1,560p' pkg/cli/clusterapi/eks_ownership_test.go
sed -n '1,180p' pkg/cli/clusterapi/eks_ownership_internal_test.go
printf '%s\n' '=== Full PR diff for production files ==='
git diff "$(git merge-base HEAD origin/main)" HEAD -- \
pkg/cli/clusterapi/eks_ownership.go \
pkg/cli/clusterapi/local_service.go \
pkg/svc/provisioner/cluster/factory.go \
pkg/svc/provisioner/cluster/factory_eks.goLength of output: 110316 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Public lifecycle entry points ==='
sed -n '350,490p' pkg/cli/clusterapi/local_service.go
sed -n '800,870p' pkg/cli/clusterapi/local_service.go
printf '%s\n' '=== Immutable identity verifier ==='
sed -n '1,290p' pkg/svc/eksidentity/identity.go
printf '%s\n' '=== All provisioner lifecycle calls in cluster API ==='
rg -n -C 5 --glob '*.go' \
'\.(Create|Delete|Start|Stop)\(.*name\)|runLifecycleAction\(' \
pkg/cli/clusterapi
printf '%s\n' '=== Guard-related changed-line review ==='
git diff --check "$(git merge-base HEAD origin/main)" HEAD
git diff --unified=20 "$(git merge-base HEAD origin/main)" HEAD -- \
pkg/cli/clusterapi/eks_ownership.go \
pkg/cli/clusterapi/local_service.go \
pkg/svc/provisioner/cluster/factory.go \
pkg/cli/clusterapi/eks_ownership_test.go \
pkg/cli/clusterapi/eks_ownership_internal_test.goLength of output: 74843
I found no actionable findings.
You are interacting with an AI system. |
CodeRabbit chat-routed the Recording State at this head: CI green (55 success / 0 failures / 0 pending), 0 unresolved threads, no conflicts. Context worth having before you review: two earlier rounds on this PR each found that the previous round's fix was half-done — the carried verifier was left unbounded on its second call, and nothing captured ownership identity for clusters the API itself creates, so the guard blocked its own happy path. Both are fixed here. I would rather you look hardest at whether this round has the same shape. One thing that will not change on your verdict: this PR stays a draft. Its third readiness condition — exercising the behaviour as a user — is unreachable from this host, because the ownership guard sits behind a discovery gate that needs live AWS. That is filed as monorepo#2617; a clean review here is still worth having, it just is not sufficient to promote. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 730067860b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
@coderabbitai review Restarting the provider ladder at CodeRabbit for head |
|
I will focus on whether EKS creation, ownership capture, and later mutation verification use the same frozen AWS credential resolution and source AWS options. I will also check that the absent-cluster normalization remains limited to delete cleanup and that identity mismatches fail closed. ✅ Action performedReview finished.
|
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/cli/clusterapi/eks_create_identity_test.go`:
- Around line 102-109: Update the require.Eventually condition around
service.List to store any list error in an enclosing-scope variable and return
false when an error occurs, rather than calling require.NoError inside the
callback. After Eventually completes, call require.NoError(t, listErr) so the
original service.List error is reported directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51ec754b-47db-4d0d-80ac-4b6c01d51ff7
📒 Files selected for processing (4)
pkg/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/eks_ownership.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/local_service.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.go
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: New not-yet-stable commands must be wrapped withexperimental.Guard(cmd), remain disabled by default, and require the global--experimentalflag.
Test experimental commands in both states: enabled with--experimentaland disabled withexperimental.ErrDisabled.
Files:
pkg/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.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/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.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/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/export_test.go
🧠 Learnings (1)
📚 Learning: 2026-08-02T03:51:48.602Z
Learnt from: devantler
Repo: devantler-tech/ksail PR: 6434
File: pkg/cli/clusterapi/eks_ownership.go:100-106
Timestamp: 2026-08-02T03:51:48.602Z
Learning: For EKS lifecycle mutations in the pkg/cli/clusterapi package, which may run with context.WithoutCancel, resolve EKS ownership guards using an explicit injectable timeout covering AWS config loading, caller-identity resolution, and EKS DescribeCluster. This prevents unresponsive AWS endpoints from leaving delete or update jobs stuck. Keep the verifier passed to the provisioner independent of this guard-resolution timeout because it performs its own mutation-boundary check using its call-time context.
Applied to files:
pkg/cli/clusterapi/eks_create_identity_test.gopkg/cli/clusterapi/local_service.gopkg/cli/clusterapi/export_test.gopkg/cli/clusterapi/eks_ownership.go
🔇 Additional comments (9)
pkg/cli/clusterapi/eks_ownership.go (5)
183-225: LGTM!
236-245: LGTM!
267-298: LGTM!
369-375: LGTM!
377-398: LGTM!pkg/cli/clusterapi/local_service.go (1)
95-106: LGTM!Also applies to: 903-946
pkg/cli/clusterapi/export_test.go (1)
121-127: LGTM!Also applies to: 134-169
pkg/cli/clusterapi/eks_create_identity_test.go (2)
14-49: LGTM!
121-189: LGTM!
The condition ran on Eventually's own goroutine while asserting on the outer t, so a List error failed that tick by exiting the goroutine rather than by being reported. EventuallyWithT carries the failure back out through CollectT. Splitting the boolean into named assertions is the part that changes what a failure says: the old form could only ever report "Condition never satisfied", with no indication of whether the cluster was missing or merely not ready yet.
Readiness evidence at
|
@codex review Skipping CodeRabbit at this head on measured evidence rather than by spending a request that cannot CI is green here: 55 success, 14 skipped, 0 failures, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c848c66a2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Capture persists the AWS variable names a create actually resolved through, but the local API lifecycle path built its identity client from the current selection before eksidentity.NewVerifier ever loaded that record. The record could therefore never influence credential resolution: a cluster created with custom spec.provider.aws names failed Delete/Start/Stop with unavailable credentials, or verified against whatever identity the canonical variables named. Load the record first and layer its captured names over the injected resolver. Values stay base-first so a credential the injected resolver already resolves is untouched; the recorded alias is consulted only where it resolves nothing. A missing or legacy record falls back to the injected resolver unchanged, keeping the UseCredentials injection seam intact and leaving NewVerifier's migration error as the authoritative failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai review Restarting the provider ladder at CodeRabbit for a new head. Since the last review at
Both threads are resolved. CI is green at this head (56 success / 14 skipped). |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD~1)"
printf '%s\n' '=== Revision and changed files ==='
git rev-parse HEAD
git diff --name-status "$base" HEAD
printf '%s\n' '=== Relevant declarations ==='
for f in \
pkg/cli/clusterapi/eks_ownership.go \
pkg/cli/clusterapi/local_service.go \
pkg/svc/credentials \
pkg/svc/eksidentity \
pkg/svc/provisioner/cluster/factory.go \
pkg/svc/provisioner/cluster/factory_eks.go
do
if [ -e "$f" ]; then
printf '\n--- %s ---\n' "$f"
ast-grep outline "$f" --items all --type function,method,struct,interface,var --view signatures || true
fi
done
printf '%s\n' '=== Resolver and ownership call paths ==='
rg -n -C 8 --glob '*.go' \
'NewRecordedAWSResolver|RecordedAWSResolver|UseCredentials|ResolveAWS|FreezeAWS|NewVerifier|resolveEKSMutationGuard|defaultEKSGuard|Ownership|ownership' \
pkg/cli/clusterapi pkg/svc/credentials pkg/svc/eksidentity pkg/svc/provisioner/cluster
printf '%s\n' '=== Focused production diffs ==='
git diff --check "$base" HEAD
git diff --unified=45 "$base" HEAD -- \
pkg/cli/clusterapi/eks_ownership.go \
pkg/cli/clusterapi/local_service.go \
pkg/svc/credentials \
pkg/svc/eksidentity \
pkg/svc/provisioner/cluster/factory.go \
pkg/svc/provisioner/cluster/factory_eks.go
printf '%s\n' '=== Test coverage references ==='
rg -n -C 8 --glob '*_test.go' \
'NewRecordedAWSResolver|RecordedAWSResolver|UseCredentials|custom.*AWS|ownership.*record|legacy|migration|defaultEKSGuard|resolveEKSMutationGuard' \
pkg/cli/clusterapi pkg/svc/credentials pkg/svc/eksidentityLength of output: 164396 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Credential resolver implementation ==='
nl -ba pkg/svc/credentials/credentials.go | sed -n '1,290p'
nl -ba pkg/svc/credentials/credentials.go | sed -n '290,560p'
printf '%s\n' '=== Service credential injection and ownership resolver ==='
rg -n -C 12 --glob '*.go' \
'func \(s \*Service\) UseCredentials|UseCredentials\(|eksOwnershipResolver|eksIdentityClient\(' \
pkg/cli/clusterapi
nl -ba pkg/cli/clusterapi/eks_ownership.go | sed -n '300,430p'
printf '%s\n' '=== Ownership state schema and load validation ==='
rg -n -C 12 --glob '*.go' \
'type EKSOwnershipState|EKSOwnershipStateVersion|func LoadEKSOwnershipState|func SaveEKSOwnershipState|AWSOptions' \
pkg/svc/state pkg/svc/eksidentity
printf '%s\n' '=== Guard-to-factory frozen credential path ==='
nl -ba pkg/svc/provisioner/cluster/factory_eks.go | sed -n '80,190p'
rg -n -C 10 --glob '*.go' \
'OptionsForFrozenAWSConfig|RequireCredentialValues|ChildEnvironment|AWSResolution' \
pkg/svc/provisioner/cluster pkg/svc/credentials
printf '%s\n' '=== Focused resolver integration tests ==='
nl -ba pkg/cli/clusterapi/eks_ownership_resolver_internal_test.go | sed -n '1,260p'Length of output: 1583 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Credential resolver implementation ==='
sed -n '1,290p' pkg/svc/credentials/credentials.go
sed -n '290,560p' pkg/svc/credentials/credentials.go
printf '%s\n' '=== Service credential injection and ownership resolver ==='
rg -n -C 12 --glob '*.go' \
'func \(s \*Service\) UseCredentials|UseCredentials\(|eksOwnershipResolver|eksIdentityClient\(' \
pkg/cli/clusterapi
sed -n '300,430p' pkg/cli/clusterapi/eks_ownership.go
printf '%s\n' '=== Ownership state schema and load validation ==='
rg -n -C 12 --glob '*.go' \
'type EKSOwnershipState|EKSOwnershipStateVersion|func LoadEKSOwnershipState|func SaveEKSOwnershipState|AWSOptions' \
pkg/svc/state pkg/svc/eksidentity
printf '%s\n' '=== Guard-to-factory frozen credential path ==='
sed -n '80,190p' pkg/svc/provisioner/cluster/factory_eks.go
rg -n -C 10 --glob '*.go' \
'OptionsForFrozenAWSConfig|RequireCredentialValues|ChildEnvironment|AWSResolution' \
pkg/svc/provisioner/cluster pkg/svc/credentials
printf '%s\n' '=== Focused resolver integration tests ==='
sed -n '1,260p' pkg/cli/clusterapi/eks_ownership_resolver_internal_test.goLength of output: 179901
I found no new actionable findings.
The lifecycle guard freezes this resolved selection. Missing, legacy, and invalid ownership records retain the injected resolver. The focused tests cover alias resolution, region isolation, injected-resolver fallback, and legacy records. The shell sandbox did not run repository tests. ✏️ Learnings added
✅ Action performedReview finished.
|
CodeRabbit did not deliver at this head. The request at 2026-08-02T23:07:51Z was acknowledged at 23:08:44Z (analysis-chain reply) but produced no review object at |
Requesting a Codex review at the current head @codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
User evaluation at
|
| function | coverage (full package) |
|---|---|
resolveEKSMutationGuard |
95.0% |
normalizeEKSVerificationError / applyEKSMutationGuard / captureEKSOwnership / newEKSCreateIdentity |
100% |
defaultEKSCapture |
0.0% |
defaultEKSGuard / eksIdentityClient / eksIdentityClientFor |
0.0% |
Package total 78.0%; the file was reverted after the run (git status clean).
Assessment — not a blocker. The seams are thoroughly tested and the PR closes a real hole. But defaultEKSCapture is the function actually wired in at local_service.go:187, and it is not pure AWS glue: its identity == nil and bound == nil branches are security decisions that return before any AWS call, so they are cheaply testable without credentials. Right now a refusal can be deleted from the production path and the suite stays green — which is the same shape of gap this PR exists to close, one level down.
Not pushing a fix here, deliberately: a push would stale the current-head Codex green, and all three review lanes are degraded right now (CodeRabbit account-rate-limited, Codex queued, Bugbot at its usage limit since 2026-07-21), so re-securing a review could take hours. Filed instead.
This leaves readiness condition 3 unmet — I exercised it and found something, rather than confirming it works — so the PR stays a draft.
defaultEKSCapture and defaultEKSGuard are the closures the production factory wires, and both sat at 0.0% while the seams around them were at 95-100%. Deleting the identity refusal from the shipped capture path left the whole ownership suite green.
The coverage gap found in the last user evaluation is closed —
|
| function | before | after |
|---|---|---|
defaultEKSCapture |
0.0% | 35.3% |
defaultEKSGuard |
0.0% | 30.8% |
| package total | 78.0% | 78.8% |
Stated plainly: eksIdentityClient and eksIdentityClientFor remain at 0.0%, and the uncovered remainder of the two functions above is their AWS-calling tail (FreezeAWS, eksidentity.Capture). Those need real credentials; the refusals — the security-relevant part — do not, and are now pinned.
Lint clean on the new file (golangci-lint reports only the three pre-existing gosec G704 findings in kubeproxy.go / kubewatch.go / plugincatalog.go). The //nolint:paralleltest markers follow this package's existing convention for t.Setenv-based tests, which cannot call t.Parallel().
This push stales the Codex green at 573b9995; re-requesting once CI settles.
@coderabbitai review |
|
I will verify that the new production-path tests reach the intended refusal branches and that the EKS ownership guard remains fail-closed. ✅ Action performedReview finished.
|
Readiness at
|
1 participant
Why
Deleting, starting or stopping an EKS cluster from the local web UI ran no ownership check at
all. The check exists and the standalone command-line path performs it — the web backend simply
never switched it on, so the safeguard was silently inactive on exactly the surface where a click is
cheapest. A delete cannot be undone, so an action aimed at the wrong account or at a replacement
cluster of the same name had nothing standing in its way.
What
Every EKS action that changes something now confirms, before any work starts, that the cluster in
front of it is still the same cluster this machine created — same AWS account, same cluster, same
creation moment. If that cannot be confirmed, the action is refused and says how to recover instead
of proceeding on an unconfirmed target.
Creating a cluster now records that identity as part of the create, so clusters made here can be
operated here. Without it the guard would have blocked its own happy path: every cluster created
through the UI would have failed its first delete, start or stop and needed a manual re-binding step
first. A create that cannot record the identity fails rather than reporting success on a cluster
nobody could later operate.
The confirmed identity is also handed to the layer that actually calls AWS, so the final check
happens as close to the change as possible rather than only up front.
Part of #6203
Fixes #6443