fix(eks): honor custom AWS credential mappings - #6089
Conversation
✅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
|
Code Coverage OverviewLanguages: Go Go / code-coverage/goThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
@coderabbitai review Fallback exact-head review requested after the Codex lane produced no comment, review, thread, or explicit failure for 10m09s. CodeRabbit’s previously stated cooldown has elapsed; all CI remains green at e41b526, including 27/27 Docker system tests. |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAWS credential aliases defined in cluster configuration are resolved into explicit credential values and immutable snapshots capturing which environment variables were used as sources. These resolved credentials are then isolated into child process environments that contain only canonical AWS variables, with competing identity-provider variables removed when custom sources are configured. The EKS SDK client, eksctl client, AWS provider, and EKS provisioner all support explicit credential configuration with fail-closed validation: they reject incomplete static keypairs and enforce credential availability when explicitly required. Factory and discovery code resolve credentials once and build aligned option sets for all dependent clients. The cluster lifecycle now retains resolved AWS options and passes them through cluster info status lookup into eksctl and provider credential configuration. Throughout the flow, credential values are extracted from configured environment sources, validated, propagated via client options, and redacted from captured stderr without mutating the parent process environment. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 4
🤖 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/lifecycle/simple.go`:
- Around line 175-190: Refactor resolveFromConfig and its callers to return or
populate a single named result struct containing clusterName, provider,
kubeconfigPath, omniOpts, kubernetesOpts, awsOpts, and awsRegion instead of
seven positional pointer parameters. Update the call sites around the
resolveFromConfig invocation and downstream field access to use the struct
fields, preserving existing configuration resolution behavior.
In `@pkg/client/eksctl/client.go`:
- Around line 292-295: Update the environment-variable switch in the stderr
redaction logic to exclude AWS_PROFILE from uniqueValues; retain only
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_SESSION_TOKEN so
strings.ReplaceAll does not redact common profile names from unrelated
diagnostics.
In `@pkg/svc/credentials/credentials.go`:
- Around line 177-227: Refactor ResolveAWS so each credential’s source name is
evaluated with its corresponding canonical environment name in a single paired
structure, rather than matching sourceEnvVars and canonicalNames by shared array
index. Update hasCustomCredentialEnv detection to iterate those pairs while
preserving the existing non-empty, non-canonical check and AWSResolution values.
In `@pkg/svc/provisioner/cluster/factory_eks.go`:
- Around line 56-71: Extract the shared AWS credential setup into a helper in
pkg/svc/credentials that returns the eksctl client and provider options while
preserving all RequireCredentialValues wiring. In
pkg/svc/provisioner/cluster/factory_eks.go lines 56-71, update
resolveEKSCredentialOptions to use the helper and keep the provisioner-specific
option construction local; in pkg/svc/clusterdiscovery/cloud.go lines 88-102,
replace the duplicated auth, eksctlOptions, and providerOptions construction
with the same helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b898fba3-1544-4d76-bcf4-51ebd0366cfc
📒 Files selected for processing (22)
pkg/cli/cmd/cluster/info.gopkg/cli/cmd/cluster/info_test.gopkg/cli/lifecycle/awsregion_test.gopkg/cli/lifecycle/simple.gopkg/client/eks/client.gopkg/client/eks/client_test.gopkg/client/eks/errors.gopkg/client/eksctl/client.gopkg/client/eksctl/client_test.gopkg/client/eksctl/errors.gopkg/svc/clusterdiscovery/aws_environment_test.gopkg/svc/clusterdiscovery/cloud.gopkg/svc/credentials/aws_environment_test.gopkg/svc/credentials/credentials.gopkg/svc/credentials/credentials_test.gopkg/svc/credentials/environment_name_test.gopkg/svc/provider/aws/provider.gopkg/svc/provisioner/cluster/eks/connector.gopkg/svc/provisioner/cluster/eks/connector_test.gopkg/svc/provisioner/cluster/eks/provisioner.gopkg/svc/provisioner/cluster/factory_eks.gopkg/svc/provisioner/cluster/factory_eks_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pkg/cli/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/cli/**/*.go: Canonicalize all user-supplied file path arguments in CLI commands withfsutil.EvalCanonicalPathbefore using them.
For output paths that may not already exist, create the parent directory withos.MkdirAll(filepath.Dir(outputPath), <mode>)before callingfsutil.EvalCanonicalPath.
Usefsutil.ReadFileSafefor constrained reads instead of reimplementing path-containment checks.
Files:
pkg/cli/lifecycle/awsregion_test.gopkg/cli/cmd/cluster/info.gopkg/cli/cmd/cluster/info_test.gopkg/cli/lifecycle/simple.go
🧠 Learnings (1)
📚 Learning: 2026-06-27T23:31:11.055Z
Learnt from: devantler
Repo: devantler-tech/ksail PR: 5489
File: pkg/svc/provider/hetzner/factory.go:54-57
Timestamp: 2026-06-27T23:31:11.055Z
Learning: In KSail’s settings connection-test flow, `pkg/svc/credentials/manager.go` `Manager.Overlay()` resolves stored credentials (including secure-storage values) and exports them into process environment variables before provider validation runs. Therefore, when reviewing provider factory/helpers (e.g., `NewProviderFromOptions()` in `pkg/svc/provider/*/factory.go`) that read tokens via `os.Getenv(tokenEnvVar)`, ensure they rely on these manager-overlayed env vars (not only values the user manually exported in the shell) and that provider construction/validation happens after `Manager.Overlay()` has populated the environment for the connection-test path.
Applied to files:
pkg/svc/provider/aws/provider.go
🔇 Additional comments (22)
pkg/svc/clusterdiscovery/aws_environment_test.go (1)
1-94: LGTM!pkg/svc/provider/aws/provider.go (1)
29-90: LGTM!Also applies to: 290-306
pkg/cli/lifecycle/awsregion_test.go (1)
1-108: LGTM!pkg/cli/cmd/cluster/info_test.go (1)
29-67: LGTM!Also applies to: 222-288
pkg/svc/provisioner/cluster/factory_eks.go (1)
14-26: 🎯 Functional CorrectnessNo change needed
DefaultFactory.Createrejects a nilclusterbefore this branch, socluster.Spec.Provider.AWSis safe here.> Likely an incorrect or invalid review comment.pkg/svc/provisioner/cluster/eks/provisioner.go (1)
8-8: LGTM!Also applies to: 42-46, 62-79, 100-108
pkg/svc/provisioner/cluster/eks/connector.go (1)
119-124: LGTM!pkg/svc/provisioner/cluster/eks/connector_test.go (1)
11-11: LGTM!Also applies to: 177-193
pkg/svc/provisioner/cluster/factory_eks_test.go (1)
34-62: LGTM!Also applies to: 75-75, 90-100
pkg/cli/lifecycle/simple.go (1)
122-124: LGTM!Also applies to: 217-217
pkg/cli/cmd/cluster/info.go (1)
20-20: LGTM!Also applies to: 82-87, 216-217, 227-227, 341-358
pkg/svc/credentials/aws_environment_test.go (1)
1-361: LGTM!pkg/svc/credentials/credentials.go (2)
13-14: LGTM!Also applies to: 131-176, 237-287, 399-401
288-347: 🔒 Security & PrivacyVerify container-credential exemption is safe without downstream enforcement guarantees.
strippedEnvironmentNamesstripsAWS_WEB_IDENTITY_TOKEN_FILE/AWS_ROLE_ARN/etc. when custom sources are detected, but intentionally leavesAWS_CONTAINER_CREDENTIALS_RELATIVE_URI/_FULL_URI/AUTHORIZATION_TOKEN(_FILE)untouched (confirmed by the sibling test ataws_environment_test.golines 132-177). Per the AWS SDK default credential chain, static/profile credentials are checked before container credentials, but if a configured custom profile/access-key resolves to an empty value, the chain can still fall through to any ECS/Pod-Identity container credentials left in the child environment. This file has no gate against that — it relies entirely on theRequireCredentialValues()option that callers conditionally append (seepkg/cli/cmd/cluster/info.go,pkg/svc/clusterdiscovery/cloud.go,pkg/svc/provisioner/cluster/factory_eks.go) actually failing the operation before eksctl/the EKS SDK client ever runs with such an environment.Since
pkg/client/eksctlandpkg/client/eks(whereRequireCredentialValuesis implemented) aren't in this review batch, please confirm that path truly errors out rather than silently falling back to ambient container credentials when a custom alias is unset.pkg/svc/credentials/environment_name_test.go (1)
1-15: LGTM!pkg/svc/credentials/credentials_test.go (1)
68-68: LGTM!pkg/client/eks/client.go (1)
60-65: LGTM!Also applies to: 87-196
pkg/client/eks/errors.go (1)
9-18: LGTM!pkg/client/eks/client_test.go (1)
180-297: LGTM!pkg/client/eksctl/client.go (1)
28-70: LGTM!Also applies to: 91-95, 121-155, 182-277, 315-325
pkg/client/eksctl/errors.go (1)
16-33: LGTM!pkg/client/eksctl/client_test.go (1)
29-86: LGTM!Also applies to: 116-279
![ksail-bot[bot]](https://avatars.githubusercontent.com/in/2880167?s=60&v=4){kind=small}
|
🤖 Sent by Codex Daily AI Engineer. @coderabbitai review |
|
✅ Action performedReview finished.
|
|
🤖 Sent by Codex Daily AI Engineer. CodeRabbit acknowledged the exact-head request but produced no review artifact after more than ten minutes, so this is the documented fallback lane. @codex review |
|
🤖 Sent by Codex Daily AI Engineer. @coderabbitai run pre-merge checks |
✅ Actions performedRunning Pre-merge checks. Results will be posted in the summary comment once complete. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
1 participant
Why
KSail resolved configured AWS credential variable names but did not project them into the environment used by
eksctlor the SDK-backed EKS paths. A custom profile or static credential mapping could therefore be ignored while a stale ambient AWS identity won.What changed
eksctlpath: provisioning, status/info, and discovery.eksctlstderr longest-first and keep environment slices isolated across concurrent calls.Scope
Standalone
ksail cluster delete/start/stop --provider AWSstill lacks EKS routing independently of credential propagation; #6087 tracks that separate child of #4328.Validation
go test -p=4 ./...go test -race ./pkg/svc/credentials ./pkg/client/eksctlgolangci-lintgate: 0 issuesgo buildplus CLI--helpsmoke testFixes #6078
Part of #4328
Follow-up #6087