Fix Karpenter IAM policy size limit (CASCL-1645) - #3315
Conversation
Co-authored-by: clamoriniere <cedric.lamoriniere@datadoghq.com>
|
I can only run on private repositories. |
![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: 9f77c2b76c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| KarpenterControllerPolicy2: | ||
| Type: AWS::IAM::ManagedPolicy | ||
| Properties: | ||
| ManagedPolicyName: !Sub "KarpenterControllerPolicy2-${ClusterName}" |
There was a problem hiding this comment.
On updates of an existing installation, createCloudFormationStacks applies KarpenterCfn and waits for it before applying the mode-specific dd stack, so moving these statements into a policy that is only attached later means the live controller role keeps just KarpenterControllerPolicy after this stack update. If the later dd stack update is slow or fails, Karpenter loses the EC2/Describe/SQS/instance-profile permissions moved here and can no longer provision or process interruptions until a rerun succeeds; stage the new attachment before shrinking the original policy, or make the transition atomic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I can only run on private repositories.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 1e7e7e1 | Docs | Datadog PR Page | Give us feedback! |
|
This is related to aws/karpenter-provider-aws#7874 and aws/karpenter-provider-aws#8690 |
2 participants
What does this PR do?
Splits the Karpenter controller's IAM managed policy (
KarpenterControllerPolicyincmd/kubectl-datadog/autoscaling/cluster/apply/assets/karpenter.yaml) into two managed policies —KarpenterControllerPolicyand a newKarpenterControllerPolicy2— and attaches both to the controller role indd-karpenter.yamlanddd-karpenter-fargate.yaml.Motivation
Customers with long EKS cluster names hit
ServiceLimitExceeded: Cannot exceed quota for PolicySize: 6144when thekubectl datadog autoscaling cluster install/updateCloudFormation stack tries to createKarpenterControllerPolicy(CASCL-1645). The${ClusterName}parameter (plus ARNs that embed it) is interpolated ~17 times across the single policy document, so the rendered, whitespace-stripped policy already sits at ~90% of IAM's 6,144-character managed-policy limit before substitution — pushing cluster names as short as ~27-35 characters over the hard, non-raisable IAM quota.Splitting the policy's 17 statements across two managed policies keeps each one well under the limit. I verified this against AWS EKS's actual maximum cluster-name length (100 characters, per the
CreateClusterAPI'sNamefield docs) and, as an extra margin, against the Kubernetes generic resource-name maximum (253 characters):I also checked IAM's separate 128-character managed-policy name limit (distinct from the document-size limit above): both
KarpenterControllerPolicy-${ClusterName}andKarpenterControllerPolicy2-${ClusterName}stay under it at the true 100-char EKS maximum, though with only 1-2 characters of headroom.TestKarpenterControllerPolicyNameLengthguards this so a future rename to something longer doesn't silently reintroduce a similar failure via the policy name instead of its document.A follow-up ticket (CASCL-1646) tracks a separate, tighter constraint discovered while verifying against the true 100-char EKS max:
KarpenterNodeRole-${ClusterName}is an IAM role name (64-char limit), which now becomes the new effective ceiling (~46 chars) after this fix. That ticket calls out explicitly that its fix must remain backward compatible withupdate/uninstallon stacks created before it, since it would need to change resource naming, not just policy content.Changes
cmd/kubectl-datadog/autoscaling/cluster/apply/assets/karpenter.yaml: split the 17-statementKarpenterControllerPolicyPolicyDocumentintoKarpenterControllerPolicy(8 statements, unchanged name) and a newKarpenterControllerPolicy2(9 statements), balanced by how many${ClusterName}-driven tokens each statement carries (not just statement count) so both halves have comparable headroom.cmd/kubectl-datadog/autoscaling/cluster/apply/assets/dd-karpenter.yamlanddd-karpenter-fargate.yaml: add a secondManagedPolicyArnsentry forKarpenterControllerPolicy2-${ClusterName}alongside the existing one.cmd/kubectl-datadog/autoscaling/cluster/apply/run.go: updated a comment mentioning the (now two) managed policies.test/e2e/tests/autoscaling_suite/eks_test.go: updated the comment explaining why the E2E stack name is kept short, now referencing the two tickets and the remainingKarpenterNodeRolenaming constraint.cmd/kubectl-datadog/autoscaling/cluster/apply/karpenter_cfn_test.go(new): regression tests that:TestKarpenterControllerPolicySize: render both embeddedPolicyDocuments with the longest realistic cluster name (100 chars, the real EKS max) and AWS partition/region strings, and assert each stays under IAM's 6,144-char document-size limit.TestKarpenterControllerPolicyNameLength: assert both managed policy names (KarpenterControllerPolicy-${ClusterName}andKarpenterControllerPolicy2-${ClusterName}) stay under IAM's separate 128-char name-length limit at the same 100-char cluster-name maximum.TestKarpenterControllerPolicyReferencedByBothStacks: assert bothdd-karpenter.yaml/dd-karpenter-fargate.yamlattach both policy ARNs (guarding the additive, backward-compatible nature of the change).Backward compatibility (update/uninstall of pre-existing stacks)
Verified that stacks created before this change remain fully manageable:
KarpenterControllerPolicy's logical ID andManagedPolicyName(KarpenterControllerPolicy-${ClusterName}) are unchanged, so CloudFormation performs an in-placePolicyDocumentupdate (no replacement).KarpenterControllerPolicy2is a genuinely new resource added to the same, already-existing CFN stack — CloudFormation creates it as part of the same stack update. On the mode-specific stacks,ManagedPolicyArnsgains a second entry on the existingRole/KarpenterIRSARoleresource, which CloudFormation updates in place (AttachRolePolicy) without replacing the role.apply.Runalways fully updates the mode-independentkarpenter.yamlstack (and waits for completion) before updating the mode-specific stack that references the new policy ARN, so the new policy always exists by the time it's referenced.aws.DeleteStack(cmd/kubectl-datadog/autoscaling/cluster/common/aws/cloudformation.go) deletes a stack purely by name via the CloudFormation API — it has no dependency on which resources exist inside the stack, so it deletes all resources present (including the newKarpenterControllerPolicy2, if it exists) with zero code changes required.uninstall.gostill deletes the mode-specific stack (which detaches the extra managed policy) before the mode-independent stack (which deletes it), preserving correct teardown ordering. Anuninstallagainst a stack that never hadupdateapplied to it still works too, since deletion never reads the embedded YAML templates.run.go,update.go, oruninstall.goneeded to change to support this — the change is confined to the embedded CloudFormation template assets, which is why no logical-ID renames were made.Describe your test plan
go build ./cmd/kubectl-datadog/...— succeeds.go test ./cmd/kubectl-datadog/autoscaling/...— all packages pass, including the three new tests inkarpenter_cfn_test.godescribed above.go vet ./cmd/kubectl-datadog/autoscaling/...— clean.gofmt -lon all touched Go files — clean.GOWORK=off go build ./tests/autoscaling_suite/...andgo vet ./tests/autoscaling_suite/...in thetest/e2emodule — succeed after the comment update.yaml.safe_loadwith a permissive!Sub/intrinsic-tag constructor), confirming bothKarpenterControllerPolicyandKarpenterControllerPolicy2show up as top-level resources.us-east-1,ap-southeast-1,us-gov-west-1/aws-us-gov, andcn-north-1/aws-cn) before writing the equivalent Go regression tests.kubectl datadog autoscaling clusterrun against a real AWS account was performed (would require live AWS/EKS access not available in this environment); the Go-level tests plus the CFN update/delete semantics reasoning above are the available verification for the update/uninstall backward-compatibility claim.Additional Notes
Filed CASCL-1646 to track the deeper
KarpenterNodeRole/IRSA role/SQS queue naming-length constraints as separate follow-up work, since fixing those requires a naming-scheme change that is not automatically backward compatible the way this policy split is.Minimum Agent Versions
Not applicable — this only changes the
kubectl-datadogplugin's Karpenter CloudFormation assets, not the Datadog Agent or Cluster Agent.Checklist
bug,enhancement,refactoring,documentation,tooling, and/ordependenciesqa/skip-qalabelPR by Bits - View session in Datadog
Comment @DataDog to request changes