From e93ebc23b4a4b4511b75a1ca29fc53364537ba5a Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Wed, 9 Sep 2026 18:11:51 +0200 Subject: [PATCH] fix(operator): bound and hash the drain PDB's node label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensurePDB built the per-node PodDisruptionBudget's selector from the raw Node name, while labelStoragePod labeled the pods that selector has to match through sanitizeLabelValue. A Node name may be 253 characters and a label value stops at 63, so the two diverged the moment a name needed shortening, and three things went wrong at once: - The selector value was illegal for a name over 63 bytes, so the API server rejected the PDB and drain protection was never established. - Even had it been accepted, it selected a value no pod carried, so the budget protected nothing and eviction proceeded unblocked. - sanitizeLabelValue truncated without a digest, so two names sharing a 63-character prefix produced one label value and one PDB name. Draining the second node then retargeted the first node's budget. sanitizeLabelValue now returns a name that is already a legal label value unchanged, and otherwise cuts it to leave room for a digest of the whole original and joins the two. That bounds the result, keeps a readable prefix, keeps distinct names distinct, and cannot end on a character a label value may not end on. It is deterministic, so the same node yields the same value on every reconcile. The PDB selector now uses it, so the selector and the pod label are derived the same way. Because an already-legal value passes through untouched, every node name in use on a cluster today keeps the label value it has, and a drain in flight across the upgrade is unaffected. Verified: TestDrainPDBLabelIsBoundedAndMatchesPod is red on the unchanged tree in all three cases — the selector value is "not a legal label value", the selector "protects nothing", and two names "both derive the label value" — and green after. The operator suite (15 packages) passes, including the pre-existing TestSanitizeLabelValue and TestEnsurePDB cases, which pin the unchanged short-name behavior. make -C operator lint reports 0 issues. Co-Authored-By: Claude Opus 5 (1M context) --- .../controller/nodedrain_controller.go | 61 +++++++++++++++++-- .../nodedrain_controller_unit_test.go | 61 +++++++++++++++++++ 2 files changed, 117 insertions(+), 5 deletions(-) diff --git a/operator/internal/controller/nodedrain_controller.go b/operator/internal/controller/nodedrain_controller.go index aef5ca062..28edfc5c0 100644 --- a/operator/internal/controller/nodedrain_controller.go +++ b/operator/internal/controller/nodedrain_controller.go @@ -18,10 +18,14 @@ package controller import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "net/http" + "regexp" "slices" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -31,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/util/retry" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -894,7 +899,10 @@ func (r *NodeDrainCoordinatorReconciler) ensurePDB( MaxUnavailable: &maxUnavailableVal, Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - drainNodeLabelKey: nodeName, + // Must be the value labelStoragePod writes, not the raw node + // name: they diverge as soon as the name needs shortening, and + // a selector that misses every pod protects nothing. + drainNodeLabelKey: sanitizeLabelValue(nodeName), }, }, }, @@ -1530,10 +1538,53 @@ func isClusterRebalancing( return info.Rebalancing, nil } -// sanitizeLabelValue truncates to 63 chars (Kubernetes label value limit). +// maxLabelValueLen is Kubernetes' limit on a label value. +const maxLabelValueLen = 63 + +// labelValueHashLen is how many hex characters of the digest a shortened value +// carries. Eight is enough that two node names on one cluster colliding is not +// a practical concern, and short enough to leave the readable prefix useful. +const labelValueHashLen = 8 + +// sanitizeLabelValue derives a legal label value from a Kubernetes Node name. +// +// A Node name may be 253 characters and a label value stops at 63, so a long +// name cannot be used as-is: the API server rejects the object. Truncating +// alone is not enough either. It can leave a trailing '-' or '.', which a label +// value may not end on, and it maps every name sharing the same 63-character +// prefix onto one value — for the drain PDB that means two nodes collapsing +// onto a single budget, so draining the second retargets the first's +// protection. +// +// A name that is already a legal label value is returned unchanged, so values +// on existing clusters keep the form they have today. Anything else is cut to +// leave room for a digest of the whole original name and joined to it, which +// bounds the result, keeps it readable, and keeps distinct names distinct. +// Deterministic: the same name yields the same value on every reconcile. func sanitizeLabelValue(name string) string { - if len(name) > 63 { - return name[:63] + if len(name) <= maxLabelValueLen && len(utilvalidation.IsValidLabelValue(name)) == 0 { + return name } - return name + + sum := sha256.Sum256([]byte(name)) + suffix := hex.EncodeToString(sum[:])[:labelValueHashLen] + + keep := maxLabelValueLen - len(suffix) - 1 + prefix := name + if len(prefix) > keep { + prefix = prefix[:keep] + } + // A label value must start and end alphanumeric, and the cut may have landed + // on a separator. Trimming here rather than after joining keeps the digest + // intact. + prefix = strings.Trim(prefix, "-._") + prefix = unsafeForLabelValue.ReplaceAllString(prefix, "-") + prefix = strings.Trim(prefix, "-._") + if prefix == "" { + return suffix + } + return prefix + "-" + suffix } + +// unsafeForLabelValue matches every character a label value may not carry. +var unsafeForLabelValue = regexp.MustCompile(`[^A-Za-z0-9._-]`) diff --git a/operator/internal/controller/nodedrain_controller_unit_test.go b/operator/internal/controller/nodedrain_controller_unit_test.go index edfbb4140..5b0e738ba 100644 --- a/operator/internal/controller/nodedrain_controller_unit_test.go +++ b/operator/internal/controller/nodedrain_controller_unit_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -15,6 +16,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + utilvalidation "k8s.io/apimachinery/pkg/util/validation" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -1313,3 +1315,62 @@ func newNodeDrainTestReconciler(t *testing.T, objects ...client.Object) *NodeDra Scheme: scheme, } } + +// Regression: 2026-09-09-drain-pdb-label-unbounded — ensurePDB built the PDB's +// selector from the raw node name while labelStoragePod labeled the pods it must +// select through sanitizeLabelValue. A Kubernetes Node name may be 253 +// characters, a label value stops at 63, so for a long name the selector value +// was illegal and the PDB was rejected outright; sanitizeLabelValue's own +// truncation could also leave a trailing '-' or '.', which a label value may not +// end on, and two names sharing a 63-character prefix collapsed onto one PDB, so +// draining the second node retargeted the first node's protection. +func TestDrainPDBLabelIsBoundedAndMatchesPod(t *testing.T) { + // A realistic long name: a cloud provider's node names reach this length. + longNode := "ip-10-0-4-118.eu-central-1.compute.internal." + strings.Repeat("sub.", 5) + "example.com" + if len(longNode) <= 63 { + t.Fatalf("test premise: node name is only %d characters", len(longNode)) + } + + t.Run("the PDB selector value is a legal label value", func(t *testing.T) { + r := newNodeDrainTestReconciler(t) + if err := r.ensurePDB(context.Background(), "default", longNode, 0); err != nil { + t.Fatalf("ensurePDB returned error for a %d-character node name: %v", len(longNode), err) + } + + var pdbList policyv1.PodDisruptionBudgetList + if err := r.List(context.Background(), &pdbList); err != nil { + t.Fatalf("list PDBs: %v", err) + } + if len(pdbList.Items) != 1 { + t.Fatalf("expected exactly one PDB, got %d", len(pdbList.Items)) + } + value := pdbList.Items[0].Spec.Selector.MatchLabels[drainNodeLabelKey] + if errs := utilvalidation.IsValidLabelValue(value); len(errs) > 0 { + t.Fatalf("PDB selector value %q is not a legal label value: %v", value, errs) + } + }) + + t.Run("the PDB selector matches the label the pod is given", func(t *testing.T) { + r := newNodeDrainTestReconciler(t) + if err := r.ensurePDB(context.Background(), "default", longNode, 0); err != nil { + t.Fatalf("ensurePDB returned error: %v", err) + } + + var pdbList policyv1.PodDisruptionBudgetList + if err := r.List(context.Background(), &pdbList); err != nil { + t.Fatalf("list PDBs: %v", err) + } + selector := pdbList.Items[0].Spec.Selector.MatchLabels[drainNodeLabelKey] + onPod := sanitizeLabelValue(longNode) + if selector != onPod { + t.Fatalf("PDB selects %q but the pod is labeled %q, so the PDB protects nothing", selector, onPod) + } + }) + + t.Run("two node names sharing a 63-character prefix do not collide", func(t *testing.T) { + prefix := strings.Repeat("a", 70) + if got, other := sanitizeLabelValue(prefix+"-one"), sanitizeLabelValue(prefix+"-two"); got == other { + t.Fatalf("distinct node names both derive the label value %q", got) + } + }) +}