fix: manage Karpenter CRDs in lockstep with the controller

[amdove]

Summary

Manage Karpenter's CRDs in lockstep with the controller, so a karpenter_version bump can never run against stale CRDs.

The controller chart bundles its CRDs in crds/, which Helm installs once and never upgrades. So a karpenter_version bump moves the controller but leaves the CRDs frozen — and a controller >= 1.9 emitting a Gte/Lte requirement (normalized from a NodePool Gt/Lt floor) against a pre-1.9 CRD gets its NodeClaim rejected, so no nodes provision.

What changed

• Install the dedicated karpenter-crd chart as a second helm.cattle.io/v1 HelmChart, pinned to the same karpenter_version as the controller. Its CRDs are templated, so Helm upgrades them on every bump (Karpenter's recommended approach). dependsOn orders it ahead of the controller chart and the NodePool/EC2NodeClass CRs.
• Stamp the chart's CRDs with helm.sh/resource-policy: keep so a helm uninstall (the HelmChart CR being deleted/renamed, or the code reverted) never cascade-deletes the CRDs and every NodePool/NodeClaim/EC2NodeClass — restoring the delete-safety the controller chart's bundled crds/ had.

Why adoption is a one-time manual step

Existing clusters already have the Karpenter CRDs — the controller chart's crds/ created them — without Helm ownership metadata. A plain karpenter-crd install fails with invalid ownership metadata. Helm's fix is --take-ownership, but PTD's helm job image is intentionally pinned to klipper-helm:v0.9.5-build20250306 (Helm 3.16.4, predating that flag; newer Helm-4 builds regressed install/upgrade detection). So adoption can't be automated in the chart.

Instead it's a one-time manual stamp per existing cluster (label + annotate the CRDs with Helm ownership metadata), after which the karpenter-crd install adopts them cleanly. Greenfield clusters skip it. Full procedure: docs/infrastructure/karpenter-crds.md.

⚠️ Rollout: once merged, the helm step installs karpenter-crd on every cluster; on an un-migrated existing cluster that install fails until the stamp is applied — non-destructive (running Karpenter unaffected, chart just doesn't reconcile). Migrate each existing cluster before/with its next helm-step apply. If skipped, ptd ensure still reports success while the karpenter-crd install job fails silently (invalid ownership metadata) and retries — nothing breaks beyond the already-stale CRDs; apply the stamp and re-run to recover.

Testing

just check-go, just cli, just test-lib pass.

Validated end-to-end on a staging cluster in the stale-CRD state (Karpenter controller 1.12.1, pre-1.9 CRDs): applied the one-time ownership stamp, installed the karpenter-crd chart, and confirmed it adopted the existing CRDs (now owned by the release, no invalid ownership metadata error), upgraded their schema (the operator enum gained Gte/Lte), and left the existing NodePools/EC2NodeClasses and the running controller untouched. A new Workbench session then triggered Karpenter to provision a fresh node and scheduled onto it successfully, confirming live provisioning against the adopted/upgraded CRDs. Per-cluster migration is still required before broad rollout.

Future

When the fleet's helm job image moves to a fixed Helm-4 klipper-helm build (>= v0.11.1-build20260615) + helm-controller >= v0.16.14, this can switch to spec.takeOwnership: true and drop the manual step. Tracked separately.

🤖 Generated with Claude Code

[amdove]

@claude review please

[amdove]

@claude review please

[claude]

Claude finished @amdove's task in 1m 47s —— View job

---

Code Review

• Read changed files
• Review lib/steps/helm_aws.go changes
• Review docs/infrastructure/karpenter-crds.md
• Post feedback

---

The approach is sound and follows the recommended Karpenter pattern. The implementation is minimal and consistent with the existing codebase. A few things worth calling out:

lib/steps/helm_aws.go

dependsOn ordering semantics (acknowledged, but worth surfacing)

The comment at line 1450–1454 correctly notes that dependsOn(crdChart) only orders HelmChart CR creation, not completion of the underlying helm job. On a greenfield cluster, Pulumi may submit the NodePool/EC2NodeClass CRs before the CRD helm job has actually installed the CRDs into the cluster — causing the Kubernetes provider to fail applying them. On existing clusters (CRDs already installed) this race doesn't materialize. Since this is acknowledged and the retry behavior mitigates it, it's fine as-is, but consider a brief note in the doc's greenfield section so operators know a re-apply may be needed on first deploy.

NodePool/EC2NodeClass depend on crdChart, not on the controller chart

// line 1606
}, k8sOpt, pulumi.DependsOn([]pulumi.Resource{crdChart}),

The NodePools and EC2NodeClasses only depend on the CRD chart, not the controller chart. This was the case before (no explicit dep at all), so it's not a regression — the controller reconciles them asynchronously, and the API server only needs the CRDs to accept the objects. Calling it out for visibility rather than as a bug.

withAlias on the new CRD chart

withAlias("kubernetes:helm.cattle.io/v1:HelmChart", crdChartResourceName)

This is consistent with every other NewCustomResource call in the function, and harmless for a net-new resource. Fine.

spec fields look correct

The CRD chart spec (chart, targetNamespace, version) matches the pattern of the controller chart and correctly omits valuesContent — karpenter-crd has no configurable values. No jobImage is set, consistent with the controller chart and other HelmCharts in this file.

docs/infrastructure/karpenter-crds.md

The doc is clear and actionable. Two small nits:

1. "covers installs that lack nodeoverlays" (line 63) — the term nodeoverlays is unexplained. If that's an internal config concept, a one-phrase gloss would help an operator who hasn't seen it before.

2. Step 3 dry-run command — ptd ensure <cluster> --only-steps helm --dry-run will show all helm changes, not just karpenter-crd. If the cluster has other pending helm changes, the diff will be noisy. Worth a note like "the diff will include all pending helm changes, not just the new CRD chart."

Overall

The problem is real and the fix is correct. The migration procedure is thorough (confirm → stamp → apply → verify → rollback path all covered). The future path to spec.takeOwnership: true is well-scoped. Good to merge once the staging validation mentioned in the PR description is complete.

[claude]

Claude finished @amdove's task in 56s —— View job

---

Code Review

• Read changed files and review guidelines
• Review lib/steps/helm_aws.go
• Review docs/infrastructure/karpenter-crds.md

---

The implementation is correct and minimal. One substantive issue and two small doc nits:

lib/steps/helm_aws.go — controller chart depends on CRD chart, but NodePools/EC2NodeClasses don't depend on the controller

NodePools and EC2NodeClasses depend on crdChart (correct — the API server needs the CRDs to accept the objects), but not on the controller chart. That matches the pre-existing behavior (no dep at all), and the controller reconciles CRs asynchronously. Fine as-is; noting it for reviewer awareness rather than flagging a bug.

lib/steps/helm_aws.go — CRD chart spec omits jobImage

The controller chart also omits jobImage, so this is consistent. Calling it out only because all other HelmChart CRs in the file follow the same pattern and a reviewer might wonder. Fine.

lib/steps/helm_aws.go — crdChart resource name and k8s name are decoupled

The Pulumi resource name is compoundName + "-karpenter-crd-helm-release" and the Kubernetes metadata.name is the hard-coded "karpenter-crd". This is the same pattern as the controller chart (chartResourceName / "karpenter") and is intentional. Fine.

docs/infrastructure/karpenter-crds.md — two nits

Nit 1 (line 63): nodeoverlays is used without explanation. A brief gloss like "nodeoverlays (a Karpenter CRD added in v1.7.1)" already appears on line 64 — just move the parenthetical before the conditional, or fold the aside into the preceding sentence. As written, a reader hits the term before the definition.

Nit 2 (step 3, line 78–79): ptd ensure <cluster> --only-steps helm --dry-run diffs all pending helm changes, not just karpenter-crd. If the cluster has other pending changes the output is noisy. Worth a note: "the diff shows all helm changes, not only the new CRD chart."

Overall

The approach is correct and the adoption constraint is well-documented. The dependsOn ordering semantics are honestly described in the inline comment. Good to merge.