From c5c58eb653d839dcc897194ac55b1178573b0d6d Mon Sep 17 00:00:00 2001
From: Tim Hockin <thockin@google.com>
Date: Thu, 15 Dec 2022 09:33:47 -0800
Subject: [PATCH] StatefulSet validation needs to allow old names

A recent commit changed name validation from DNS Subdomain to DNS Label.
The assumption was that a subdomain-named SS could never work and the
only reasonable thing to do would be to delete it.  But if there is a
finalizer, the delete is not possible because we would reject the update
because the old name (subdomain) did not pass the new validation.

This commit does not re-validate the ObjectMeta on update.  Probably
every resource should follow this pattern, but mostly it's a non-issue
becauase the above change (name validation) is not something we do -
this case was excpetional.
---
 pkg/apis/apps/validation/validation.go      | 12 ++++++---
 pkg/apis/apps/validation/validation_test.go | 27 ++++++++++++++++++++-
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/pkg/apis/apps/validation/validation.go b/pkg/apis/apps/validation/validation.go
index db2e25ba5ef..027ac8923b1 100644
--- a/pkg/apis/apps/validation/validation.go
+++ b/pkg/apis/apps/validation/validation.go
@@ -173,10 +173,14 @@ func ValidateStatefulSet(statefulSet *apps.StatefulSet, opts apivalidation.PodVa
 
 // ValidateStatefulSetUpdate tests if required fields in the StatefulSet are set.
 func ValidateStatefulSetUpdate(statefulSet, oldStatefulSet *apps.StatefulSet, opts apivalidation.PodValidationOptions) field.ErrorList {
-	// first, validate that the new statefulset is valid
-	allErrs := ValidateStatefulSet(statefulSet, opts)
-
-	allErrs = append(allErrs, apivalidation.ValidateObjectMetaUpdate(&statefulSet.ObjectMeta, &oldStatefulSet.ObjectMeta, field.NewPath("metadata"))...)
+	// First, validate that the new statefulset is valid.  Don't call
+	// ValidateStatefulSet() because we don't want to revalidate the name on
+	// update.  This is important here because we used to allow DNS subdomain
+	// for name, but that can't actually create pods.  The only reasonable
+	// thing to do it delete such an instance, but if there is a finalizer, it
+	// would need to pass update validation.  Name can't change anyway.
+	allErrs := apivalidation.ValidateObjectMetaUpdate(&statefulSet.ObjectMeta, &oldStatefulSet.ObjectMeta, field.NewPath("metadata"))
+	allErrs = append(allErrs, ValidateStatefulSetSpec(&statefulSet.Spec, field.NewPath("spec"), opts)...)
 
 	// statefulset updates aren't super common and general updates are likely to be touching spec, so we'll do this
 	// deep copy right away.  This avoids mutating our inputs
diff --git a/pkg/apis/apps/validation/validation_test.go b/pkg/apis/apps/validation/validation_test.go
index eb5bf33168b..9a3b3181a21 100644
--- a/pkg/apis/apps/validation/validation_test.go
+++ b/pkg/apis/apps/validation/validation_test.go
@@ -1190,6 +1190,27 @@ func TestValidateStatefulSetUpdate(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "update existing instance with now-invalid name",
+			old: apps.StatefulSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "abc.123.example", Namespace: metav1.NamespaceDefault, Finalizers: []string{"final"}},
+				Spec: apps.StatefulSetSpec{
+					PodManagementPolicy: apps.OrderedReadyPodManagement,
+					Selector:            &metav1.LabelSelector{MatchLabels: validLabels},
+					Template:            validPodTemplate.Template,
+					UpdateStrategy:      apps.StatefulSetUpdateStrategy{Type: apps.RollingUpdateStatefulSetStrategyType},
+				},
+			},
+			update: apps.StatefulSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "abc.123.example", Namespace: metav1.NamespaceDefault, Finalizers: []string{}},
+				Spec: apps.StatefulSetSpec{
+					PodManagementPolicy: apps.OrderedReadyPodManagement,
+					Selector:            &metav1.LabelSelector{MatchLabels: validLabels},
+					Template:            validPodTemplate.Template,
+					UpdateStrategy:      apps.StatefulSetUpdateStrategy{Type: apps.RollingUpdateStatefulSetStrategyType},
+				},
+			},
+		},
 	}
 
 	errorCases := []testCase{
@@ -1417,7 +1438,11 @@ func TestValidateStatefulSetUpdate(t *testing.T) {
 		},
 	}
 
-	cmpOpts := []cmp.Option{cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail"), cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() })}
+	cmpOpts := []cmp.Option{
+		cmpopts.IgnoreFields(field.Error{}, "BadValue", "Detail"),
+		cmpopts.SortSlices(func(a, b *field.Error) bool { return a.Error() < b.Error() }),
+		cmpopts.EquateEmpty(),
+	}
 	for _, testCase := range append(successCases, errorCases...) {
 		name := testCase.name
 		var testTitle string