Validate AWS account consistency before Karpenter operations#2892
Conversation
Prevent accidental cross-account resource deployment by verifying that the current AWS credentials and the target EKS cluster belong to the same AWS account. This check runs in both `install` and `uninstall` commands right after clients are built. In the install path, a mismatch is a hard error. In the uninstall path, a confirmed mismatch is a hard error but unreachable clusters (e.g. already deleted) produce a warning so cleanup can still proceed. Also extract a shared GetAWSAccountID helper to deduplicate the STS GetCallerIdentity boilerplate that was repeated in install and uninstall. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@codex review |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (14.63%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2892 +/- ##
==========================================
+ Coverage 40.06% 40.10% +0.04%
==========================================
Files 319 320 +1
Lines 28039 28810 +771
==========================================
+ Hits 11233 11555 +322
- Misses 15983 16426 +443
- Partials 823 829 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 7996a21 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
![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: 29130a60da
ℹ️ 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".
The previous approach used DescribeCluster with the same AWS credentials being validated, which gives a false positive when both accounts have a cluster with the same name (e.g. "prod"). Now extract the account ID directly from the kubeconfig context when it is an EKS ARN (arn:aws:eks:region:account:cluster/name). This source is independent of the AWS credentials and cannot be fooled by same-named clusters. Falls back to DescribeCluster only when the kubeconfig context is not an ARN (eksctl format, plain name). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Extract shared helper to avoid duplicating kubeconfig loading and context resolution between GetClusterNameFromKubeconfig and GetAccountIDFromKubeconfig. Also rename ctx variable to kubeCtx to avoid shadowing context.Context convention, and remove inline comments that repeated the function's docstring. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…nsistency Make getAccountIDFromKubeconfig private and call it from inside ValidateAWSAccountConsistency instead of requiring callers to pre-compute and pass the kubeconfig account ID. This simplifies the call sites and encapsulates the two-strategy validation logic (kubeconfig ARN vs DescribeCluster fallback). Also inline the trivial validateAccountIDs helper and remove its now-unnecessary tests. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Make getAccountIDFromKubeconfig return (string, error) so that real failures (kubeconfig load error, missing context) are visible as warnings in ValidateAWSAccountConsistency, while non-ARN cluster references (plain names, eksctl format) remain a silent fallback to DescribeCluster. Also remove the unused context.Context parameter from GetClusterNameFromKubeconfig in both the clients and guess packages — neither implementation uses it. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e987c3d05
ℹ️ 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".
| if clusterAccountID == "" { | ||
| cluster, err := cli.EKS.DescribeCluster(ctx, &eks.DescribeClusterInput{ | ||
| Name: awssdk.String(clusterName), | ||
| }) |
There was a problem hiding this comment.
When getAccountIDFromKubeconfig returns empty, this code falls back to DescribeCluster using the same AWS credentials being validated, so a same-named cluster in the credential account will still make the check pass while Kubernetes operations target a different account via kubeconfig. Fresh evidence in this commit is the new tests that explicitly keep non-ARN contexts (plain cluster name, eksctl format) on the empty-account path, which guarantees this fallback remains active for those real-world kubeconfigs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Contesting this finding.
The DescribeCluster fallback for non-ARN kubeconfigs (eksctl FQDN, plain cluster names) is a documented known limitation, not an oversight. The alternatives are:
- Fail closed on non-ARN kubeconfigs — would break legitimate eksctl and plain-name users, which is a hostile UX regression.
- Current approach — prefer the kubeconfig ARN when available (this IS independent of AWS credentials and cannot be fooled); fall back to DescribeCluster with documented risk when the kubeconfig doesn't carry an ARN.
The kubeconfig-ARN path, when available, provides the genuine guard your original P1 was about. For kubeconfigs that don't carry an ARN, there is no independent source of truth for the cluster's account that we can reach from the CLI, so accepting DescribeCluster with its caveat is the pragmatic tradeoff.
The caveat is documented in the function body at clients.go:203-204.
| if err = clients.ValidateAWSAccountConsistency(ctx, cli, clusterName, o.ConfigFlags); err != nil { | ||
| var mismatch *clients.AccountMismatchError | ||
| if errors.As(err, &mismatch) { | ||
| return err | ||
| } |
There was a problem hiding this comment.
In uninstall, only AccountMismatchError is returned; every other validation failure is downgraded to a warning and execution continues. That means failures like STS.GetCallerIdentity errors (for example missing sts:GetCallerIdentity permission or transient STS failures) silently disable the account-consistency guard and allow cleanup actions to proceed against whatever AWS account is configured, which contradicts the intended “warn only when cluster lookup is unavailable” behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7996a21.
Introduced ClusterLookupUnavailableError that wraps EKS.DescribeCluster failures only when the underlying error is a ResourceNotFoundException (cluster actually deleted). In the uninstall path, only this typed error triggers warn-and-proceed; every other failure is now a hard error:
- STS.GetCallerIdentity failures (missing permission, transient)
- Other DescribeCluster errors (AccessDenied, throttling, wrong region, transient API errors)
- Malformed responses (nil cluster, nil ARN, unparseable ARN)
- AccountMismatchError
This matches the existing pattern used in guess/ekspodidentityagent.go.
The previous code returned (nil error) inside an `if err \!= nil` block, which triggered the `nilerr` linter even though the swallow was intentional (non-ARN cluster refs are a legitimate fallback). Replace with an explicit `arn.IsARN` pre-check so ARN parse failures now genuinely propagate, and the non-ARN fallback path no longer touches an error at all. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Introduce ClusterLookupUnavailableError to wrap EKS.DescribeCluster failures — but only when the underlying error is a ResourceNotFoundException (cluster actually deleted). In the uninstall path, only this typed error triggers warn-and-proceed; every other failure from ValidateAWSAccountConsistency is a hard error: - STS.GetCallerIdentity failures (missing permission, transient) - Other DescribeCluster errors (AccessDenied, throttling, wrong region) - Malformed responses (nil cluster, nil ARN, unparseable ARN) - AccountMismatchError Previously, any non-mismatch error was silently downgraded to a warning, which disabled the account-consistency guard for reasons unrelated to "cluster is gone". Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7996a21d3f
ℹ️ 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".
6 participants
What does this PR do?
Adds a guard to verify that the AWS credentials being used belong to the same AWS account as the target EKS cluster before any
kubectl datadog autoscaling clusteroperations (install/uninstall). This prevents accidentally creating CloudFormation stacks in one AWS account while deploying Helm charts to an EKS cluster in another.Motivation
The install and uninstall commands create both AWS resources (CloudFormation stacks, IAM roles) and Kubernetes resources (Helm chart, Karpenter CRDs). The AWS clients are configured from the default AWS credential chain, while the Kubernetes clients come from kubeconfig. Without validation, a misconfigured environment could silently target different AWS accounts for each.
Additional Notes
GetAWSAccountIDhelper to deduplicate theSTS.GetCallerIdentityboilerplate that was repeated in install and uninstall.AccountMismatchErrortyped error so the uninstall flow can distinguish "confirmed mismatch" from "cannot verify".Minimum Agent Versions
N/A — this is a
kubectl-datadogplugin change, not an agent change.Describe your test plan
validateAccountIDspure logic covering: matching accounts, mismatched accounts, GovCloud/China partitions, and invalid ARN format.go build ./cmd/kubectl-datadog/...passes.go test ./cmd/kubectl-datadog/autoscaling/cluster/common/clients/...passes.go vet ./cmd/kubectl-datadog/...passes.Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabel🤖 Generated with Claude Code