Skip to content

fix: remove empty alternatives from CRD validation patterns - #9462

Open
semx wants to merge 1 commit into
aws:mainfrom
semx:fix/empty-alternative-in-crd-patterns
Open

fix: remove empty alternatives from CRD validation patterns#9462
semx wants to merge 1 commit into
aws:mainfrom
semx:fix/empty-alternative-in-crd-patterns

Conversation

@semx

@semx semx commented Jul 30, 2026

Copy link
Copy Markdown

Fixes #N/A

Description

The evictionHard / evictionSoft validation pattern contains a doubled pipe:

^((\d{1,2}(\.\d{1,2})?|100(\.0{1,2})?)%||(\+|-)?(([0-9]+(\.[0-9]*)?)|...
                                        ^^

%|| makes the empty string a valid alternative, so memory.available: "" passes admission. It then reaches resource.MustParse() with nothing to parse and panics the nodeclass reconciler.

The same typo is in the volumeSize pattern — ([1-9]||[1-5][0-7]|58)Ti and ([1-9]||[1-5][0-9]|6[0-3]|64)T accept a bare "Ti" / "T". There Quantity deserialization rejects the value afterwards so nothing crashes, and only the schema is wrong; fixed alongside since it is the same mistake.

The pattern is generated by hack/validation/kubelet.sh, so the fix is applied there and to the two generated copies.

How was this change tested?

On a live EKS 1.36 cluster running Karpenter 1.14.0 (public.ecr.aws/karpenter/controller:1.14.0).

Patching a working EC2NodeClass is accepted:

$ kubectl patch ec2nodeclass default --type=merge \
    -p '{"spec":{"kubelet":{"evictionHard":{"memory.available":""}}}}'
ec2nodeclass.karpenter.k8s.aws/default patched

The controller then panics in a loop — 49 panics in the first minute:

{"level":"ERROR","message":"Observed a panic","controller":"nodeclass",
 "EC2NodeClass":{"name":"default"},
 "panic":"cannot parse '': quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'"}
k8s.io/apimachinery/pkg/api/resource.MustParse(...)          quantity.go:142
  instancetype.computeEvictionSignal(...)                    types.go:607
  instancetype.evictionThreshold(...)                        types.go:566
  instancetype.NewInstanceType(...)                          types.go:153
  instancetype.(*DefaultResolver).Resolve(...)               types.go:104
  instancetype.(*DefaultProvider).List(...)                  instancetype.go:166
  nodeclass.(*Validation).getPrioritizedInstanceTypes(...)   validation.go:437
  nodeclass.(*Validation).Reconcile(...)                     validation.go:189

Pending pods are never provisioned and no NodeClaim is created, while the EC2NodeClass keeps reporting healthy:

$ kubectl get pods -l app=eviction-probe
NAME                              READY   STATUS    AGE
eviction-probe-6c75bc97d8-285fx   0/1     Pending   91s

$ kubectl get nodeclaims
No resources found

$ kubectl get ec2nodeclass default -o jsonpath='{range .status.conditions[*]}{.type}={.status} {end}'
AMIsReady=True ... ValidationSucceeded=True Ready=True

So the failure is quiet — green status, no provisioning, and the only symptom is in the controller log. Removing the empty value stops the panics immediately (0 in the following 15s), which confirms the cause. The cluster was torn down afterwards.

Also verified against a real API server with the existing CEL suite (envtest): the added case fails on main with "Expected failure, but got no error" and passes with this change, 269/269 green. The corrected pattern still accepts every valid form (10%, 100%, 0%, 500Mi, 1Gi, 1e9, 1.5e+9, +5Gi, -5Gi, .5, 5G, 100m) and still rejects the invalid ones.

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The evictionHard/evictionSoft pattern reads (...)%||(...), where the
doubled pipe makes the empty string a valid alternative. An empty value
therefore passes admission and reaches resource.MustParse() unparsed,
which panics the nodeclass reconciler and stops provisioning while the
EC2NodeClass still reports Ready.

The volumeSize pattern has the same typo in ([1-9]||...)Ti and ([1-9]||...)T,
where a bare "Ti" or "T" passes the pattern; Quantity deserialization
rejects it afterwards, so only the schema is wrong there.
@semx
semx requested a review from a team as a code owner July 30, 2026 09:25
@semx
semx requested a review from joshuakguo July 30, 2026 09:25
@semx

semx commented Aug 6, 2026

Copy link
Copy Markdown
Author

Hi — friendly ping on this one. I re-checked against current main (HEAD 6e13b3e, #9326) and the patch still applies cleanly; the empty-alternative typos are all still present, e.g. the %|| at pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml:397 (evictionHard) and :412 (evictionSoft), plus ([1-9]||…)Ti in the volumeSize pattern at :223. The evictionHard case is the one with real impact: the empty alternative lets "" pass admission, and it then reaches resource.MustParse("") via computeEvictionSignal at pkg/providers/instancetype/types.go:762, which panics during instance-type resolution while the EC2NodeClass still reports Ready. The volumeSize change is schema-only (a bare Ti/T is rejected later by Quantity parsing), as called out in the commit message. Would a maintainer be willing to take a look, or drop an /ok-to-test when convenient? Thanks very much for your time — no rush at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant