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) + } + }) +}