Bound and hash the drain PDB's node label

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._-]`)

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