fix: Truncate too long Deployment name in RS name (#132560)

* fix: Truncate too long Deployment name in RS name

* fix: lint & adjust unit tests

* fix: use const for "-" & unit tests

* Add test case for very long hash

* Explicitly define expected deployment name portion

pkg/controller/deployment/sync.go

@@ -27,6 +27,7 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/klog/v2"
 	"k8s.io/kubernetes/pkg/controller"
@@ -35,6 +36,11 @@ import (
 	labelsutil "k8s.io/kubernetes/pkg/util/labels"
 )
 
+const (
+	// replicaSetNameSeparator is the character used to separate deployment name from hash
+	replicaSetNameSeparator = "-"
+)
+
 // syncStatusOnly only updates Deployments Status and doesn't take any mutating actions.
 func (dc *DeploymentController) syncStatusOnly(ctx context.Context, d *apps.Deployment, rsList []*apps.ReplicaSet) error {
 	newRS, oldRSs, err := dc.getAllReplicaSetsAndSyncRevision(ctx, d, rsList, false)
@@ -198,7 +204,7 @@ func (dc *DeploymentController) getNewReplicaSet(ctx context.Context, d *apps.De
 	newRS := apps.ReplicaSet{
 		ObjectMeta: metav1.ObjectMeta{
 			// Make the name deterministic, to ensure idempotence
-			Name:            d.Name + "-" + podTemplateSpecHash,
+			Name:            generateReplicaSetName(d.Name, podTemplateSpecHash),
 			Namespace:       d.Namespace,
 			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(d, controllerKind)},
 			Labels:          newRSTemplate.Labels,
@@ -549,3 +555,15 @@ func (dc *DeploymentController) isScalingEvent(ctx context.Context, d *apps.Depl
 	}
 	return false, nil
 }
+
+// generateReplicaSetName generates a ReplicaSet name by adding a pod template spec hash
+// to a Deployment name, optionally truncating it to ensure the ReplicaSet name is within the limit.
+func generateReplicaSetName(deploymentName, podTemplateSpecHash string) string {
+	maxDeploymentNameLength := validation.DNS1123SubdomainMaxLength - len(replicaSetNameSeparator) - len(podTemplateSpecHash)
+
+	if len(deploymentName) > maxDeploymentNameLength && maxDeploymentNameLength > 0 {
+		return deploymentName[:maxDeploymentNameLength] + replicaSetNameSeparator + podTemplateSpecHash
+	}
+
+	return deploymentName + replicaSetNameSeparator + podTemplateSpecHash
+}

pkg/controller/deployment/sync_test.go

@@ -18,6 +18,7 @@ package deployment
 
 import (
 	"math"
+	"strings"
 	"testing"
 	"time"
 
@@ -25,6 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	testclient "k8s.io/client-go/testing"
@@ -592,3 +594,134 @@ func TestDeploymentController_cleanupDeploymentOrder(t *testing.T) {
 		}
 	}
 }
+
+func TestDeploymentController_generateReplicaSetName(t *testing.T) {
+	tests := []struct {
+		name                  string
+		deploymentName        string
+		wantDeploymentPortion string
+	}{
+		{
+			name:                  "short name",
+			deploymentName:        "my-deployment",
+			wantDeploymentPortion: "my-deployment",
+		},
+		{
+			name:                  "very long name truncated",
+			deploymentName:        strings.Repeat("a", 250),
+			wantDeploymentPortion: strings.Repeat("a", 242),
+		},
+		{
+			name:                  "very long name not truncated",
+			deploymentName:        strings.Repeat("a", 242),
+			wantDeploymentPortion: strings.Repeat("a", 242),
+		},
+	}
+
+	for _, test := range tests {
+		_, ctx := ktesting.NewTestContext(t)
+
+		fake := &fake.Clientset{}
+		informers := informers.NewSharedInformerFactory(fake, controller.NoResyncPeriodFunc())
+		controller, err := NewDeploymentController(ctx, informers.Apps().V1().Deployments(), informers.Apps().V1().ReplicaSets(), informers.Core().V1().Pods(), fake)
+		if err != nil {
+			t.Fatalf("error creating Deployment controller: %v", err)
+		}
+
+		controller.eventRecorder = &record.FakeRecorder{}
+		controller.dListerSynced = alwaysReady
+		controller.rsListerSynced = alwaysReady
+		controller.podListerSynced = alwaysReady
+
+		stopCh := make(chan struct{})
+		defer close(stopCh)
+		informers.Start(stopCh)
+
+		d := newDeployment(test.deploymentName, 1, nil, nil, nil, map[string]string{"foo": "bar"})
+
+		if _, err := controller.getNewReplicaSet(ctx, d, []*apps.ReplicaSet{}, []*apps.ReplicaSet{}, true); err != nil {
+			t.Errorf("failed to create new ReplicaSet: %v", err)
+			return
+		}
+
+		rsName := ""
+		for _, action := range fake.Actions() {
+			if createAction, ok := action.(testclient.CreateAction); ok {
+				if createdRS, ok := createAction.GetObject().(*apps.ReplicaSet); ok {
+					if createdRS.Name != "" {
+						rsName = createdRS.Name
+						break
+					}
+				}
+			}
+		}
+
+		if len(rsName) > validation.DNS1123SubdomainMaxLength {
+			t.Errorf("ReplicaSet name length %d, want <= %d", len(rsName), validation.DNS1123SubdomainMaxLength)
+		}
+
+		parts := strings.Split(rsName, "-")
+		if len(parts) < 2 {
+			t.Errorf("ReplicaSet name should contain at least one hyphen separator")
+		}
+
+		deploymentPortion := strings.Join(parts[:len(parts)-1], "-")
+		if len(test.deploymentName) <= 242 {
+			if len(deploymentPortion) != len(test.deploymentName) {
+				t.Errorf("Deployment name portion should be %d chars, got %d", len(test.deploymentName), len(deploymentPortion))
+			}
+		} else {
+			if len(deploymentPortion) != 242 {
+				t.Errorf("Truncated deployment name should be 242 chars, got %d", len(deploymentPortion))
+			}
+		}
+
+		if deploymentPortion != test.wantDeploymentPortion {
+			t.Errorf("Deployment name portion mismatch: got %q, want %q", deploymentPortion, test.wantDeploymentPortion)
+		}
+	}
+}
+
+func TestGenerateReplicaSetName(t *testing.T) {
+	tests := []struct {
+		name           string
+		deploymentName string
+		hash           string
+		want           string
+	}{
+		{
+			name:           "short name",
+			deploymentName: "my-deployment",
+			hash:           "abcde12345",
+			want:           "my-deployment-abcde12345",
+		},
+		{
+			name:           "maximum length without truncating",
+			deploymentName: strings.Repeat("a", 242),
+			hash:           "abcde12345",
+			want:           strings.Repeat("a", 242) + "-abcde12345",
+		},
+		{
+			name:           "very long deployment name is truncated",
+			deploymentName: strings.Repeat("b", 250),
+			hash:           "abcde12345",
+			want:           strings.Repeat("b", 242) + "-abcde12345",
+		},
+		{
+			name:           "very long hash is not truncated",
+			deploymentName: strings.Repeat("d", 252),
+			hash:           strings.Repeat("h", 252),
+			want:           strings.Repeat("d", 252) + "-" + strings.Repeat("h", 252),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := generateReplicaSetName(test.deploymentName, test.hash)
+
+			if got != test.want {
+				t.Errorf("generateReplicaSetName(%q, %q) = %q, want %q", test.deploymentName, test.hash, got, test.want)
+			}
+		})
+	}
+}