Merge pull request #51283 from caesarxuchao/fix-initializer-validate

Automatic merge from submit-queue (batch tested with PRs 51583, 51283, 51374, 51690, 51716)

Unify initializer name validation

Unify the validation rules on initializer names. Fix https://github.com/kubernetes/kubernetes/issues/51843.

```release-note
Action required: validation rule on metadata.initializers.pending[x].name is tightened. The initializer name needs to contain at least three segments separated by dots. If you create objects with pending initializers, (i.e., not relying on apiserver adding pending initializers according to initializerconfiguration), you need to update the initializer name in existing objects and in configuration files to comply to the new validation rule.
```

pkg/apis/admissionregistration/validation/validation.go

@@ -22,7 +22,7 @@ import (
 
 	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
 	"k8s.io/apimachinery/pkg/util/sets"
-	validationutil "k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/apis/admissionregistration"
 )
@@ -38,15 +38,7 @@ func ValidateInitializerConfiguration(ic *admissionregistration.InitializerConfi
 func validateInitializer(initializer *admissionregistration.Initializer, fldPath *field.Path) field.ErrorList {
 	var allErrors field.ErrorList
 	// initlializer.Name must be fully qualified
-	if len(initializer.Name) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("name"), ""))
-	}
-	if errs := validationutil.IsDNS1123Subdomain(initializer.Name); len(errs) > 0 {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), initializer.Name, strings.Join(errs, ",")))
-	}
-	if len(strings.Split(initializer.Name, ".")) < 3 {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), initializer.Name, "should be a domain with at least two dots"))
-	}
+	allErrors = append(allErrors, validation.IsFullyQualifiedName(fldPath.Child("name"), initializer.Name)...)
 
 	for i, rule := range initializer.Rules {
 		notAllowSubresources := false
@@ -182,15 +174,7 @@ func ValidateExternalAdmissionHookConfiguration(e *admissionregistration.Externa
 func validateExternalAdmissionHook(hook *admissionregistration.ExternalAdmissionHook, fldPath *field.Path) field.ErrorList {
 	var allErrors field.ErrorList
 	// hook.Name must be fully qualified
-	if len(hook.Name) == 0 {
-		allErrors = append(allErrors, field.Required(fldPath.Child("name"), ""))
-	}
-	if errs := validationutil.IsDNS1123Subdomain(hook.Name); len(errs) > 0 {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), hook.Name, strings.Join(errs, ",")))
-	}
-	if len(strings.Split(hook.Name, ".")) < 3 {
-		allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), hook.Name, "should be a domain with at least two dots"))
-	}
+	allErrors = append(allErrors, validation.IsFullyQualifiedName(fldPath.Child("name"), hook.Name)...)
 
 	for i, rule := range hook.Rules {
 		allErrors = append(allErrors, validateRuleWithOperations(&rule, fldPath.Child("rules").Index(i))...)

pkg/apis/admissionregistration/validation/validation_test.go

@@ -62,7 +62,7 @@ func TestValidateInitializerConfiguration(t *testing.T) {
 						Name: "",
 					},
 				}),
-			expectedError: `initializers[1].name: Invalid value: "k8s.io": should be a domain with at least two dots, initializers[2].name: Required value`,
+			expectedError: `initializers[1].name: Invalid value: "k8s.io": should be a domain with at least three segments separated by dots, initializers[2].name: Required value`,
 		},
 		{
 			name: "APIGroups must not be empty or nil",
@@ -260,7 +260,7 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) {
 						Name: "",
 					},
 				}),
-			expectedError: `externalAdmissionHooks[1].name: Invalid value: "k8s.io": should be a domain with at least two dots, externalAdmissionHooks[2].name: Required value`,
+			expectedError: `externalAdmissionHooks[1].name: Invalid value: "k8s.io": should be a domain with at least three segments separated by dots, externalAdmissionHooks[2].name: Required value`,
 		},
 		{
 			name: "Operations must not be empty or nil",

staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go

@@ -195,9 +195,7 @@ func ValidateInitializers(initializers *metav1.Initializers, fldPath *field.Path
 		return allErrs
 	}
 	for i, initializer := range initializers.Pending {
-		for _, msg := range validation.IsQualifiedName(initializer.Name) {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("pending").Index(i), initializer.Name, msg))
-		}
+		allErrs = append(allErrs, validation.IsFullyQualifiedName(fldPath.Child("pending").Index(i).Child("name"), initializer.Name)...)
 	}
 	allErrs = append(allErrs, validateInitializersResult(initializers.Result, fldPath.Child("result"))...)
 	return allErrs

staging/src/k8s.io/apimachinery/pkg/util/validation/BUILD

@@ -10,11 +10,13 @@ go_test(
     name = "go_default_test",
     srcs = ["validation_test.go"],
     library = ":go_default_library",
+    deps = ["//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library"],
 )
 
 go_library(
     name = "go_default_library",
     srcs = ["validation.go"],
+    deps = ["//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library"],
 )
 
 filegroup(

staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go

@@ -22,6 +22,8 @@ import (
 	"net"
 	"regexp"
 	"strings"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 const qnameCharFmt string = "[A-Za-z0-9]"
@@ -67,6 +69,21 @@ func IsQualifiedName(value string) []string {
 	return errs
 }
 
+// IsFullyQualifiedName checks if the name is fully qualified.
+func IsFullyQualifiedName(fldPath *field.Path, name string) field.ErrorList {
+	var allErrors field.ErrorList
+	if len(name) == 0 {
+		return append(allErrors, field.Required(fldPath, ""))
+	}
+	if errs := IsDNS1123Subdomain(name); len(errs) > 0 {
+		return append(allErrors, field.Invalid(fldPath, name, strings.Join(errs, ",")))
+	}
+	if len(strings.Split(name, ".")) < 3 {
+		return append(allErrors, field.Invalid(fldPath, name, "should be a domain with at least three segments separated by dots"))
+	}
+	return allErrors
+}
+
 const labelValueFmt string = "(" + qualifiedNameFmt + ")?"
 const labelValueErrMsg string = "a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"
 const LabelValueMaxLength int = 63

staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go

@@ -19,6 +19,8 @@ package validation
 import (
 	"strings"
 	"testing"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 func TestIsDNS1123Label(t *testing.T) {
@@ -474,3 +476,38 @@ func TestIsWildcardDNS1123Subdomain(t *testing.T) {
 		}
 	}
 }
+
+func TestIsFullyQualifiedName(t *testing.T) {
+	tests := []struct {
+		name       string
+		targetName string
+		err        string
+	}{
+		{
+			name:       "name needs to be fully qualified, i.e., contains at least 2 dots",
+			targetName: "k8s.io",
+			err:        "should be a domain with at least three segments separated by dots",
+		},
+		{
+			name:       "name cannot be empty",
+			targetName: "",
+			err:        "Required value",
+		},
+		{
+			name:       "name must conform to RFC 1123",
+			targetName: "A.B.C",
+			err:        "a DNS-1123 subdomain must consist of lower case alphanumeric characters",
+		},
+	}
+	for _, tc := range tests {
+		err := IsFullyQualifiedName(field.NewPath(""), tc.targetName).ToAggregate()
+		switch {
+		case tc.err == "" && err != nil:
+			t.Errorf("%q: unexpected error: %v", tc.name, err)
+		case tc.err != "" && err == nil:
+			t.Errorf("%q: unexpected no error, expected %s", tc.name, tc.err)
+		case tc.err != "" && err != nil && !strings.Contains(err.Error(), tc.err):
+			t.Errorf("%q: expected %s, got %v", tc.name, tc.err, err)
+		}
+	}
+}

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go

@@ -60,6 +60,8 @@ import (
 var scheme = runtime.NewScheme()
 var codecs = serializer.NewCodecFactory(scheme)
 
+const validInitializerName = "test.k8s.io"
+
 func init() {
 	metav1.AddToGroupVersion(scheme, metav1.SchemeGroupVersion)
 	example.AddToScheme(scheme)
@@ -399,7 +401,7 @@ func TestStoreCreateInitialized(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "foo", Namespace: "test",
 			Initializers: &metav1.Initializers{
-				Pending: []metav1.Initializer{{Name: "Test"}},
+				Pending: []metav1.Initializer{{Name: validInitializerName}},
 			},
 		},
 		Spec: example.PodSpec{NodeName: "machine"},
@@ -427,7 +429,7 @@ func TestStoreCreateInitialized(t *testing.T) {
 		defer w.Stop()
 		event := <-w.ResultChan()
 		pod := event.Object.(*example.Pod)
-		if event.Type != watch.Added || !hasInitializers(pod, "Test") {
+		if event.Type != watch.Added || !hasInitializers(pod, validInitializerName) {
 			t.Fatalf("unexpected event: %s %#v", event.Type, event.Object)
 		}
 
@@ -504,7 +506,7 @@ func TestStoreCreateInitializedFailed(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "foo", Namespace: "test",
 			Initializers: &metav1.Initializers{
-				Pending: []metav1.Initializer{{Name: "Test"}},
+				Pending: []metav1.Initializer{{Name: validInitializerName}},
 			},
 		},
 		Spec: example.PodSpec{NodeName: "machine"},
@@ -526,7 +528,7 @@ func TestStoreCreateInitializedFailed(t *testing.T) {
 		}
 		event := <-w.ResultChan()
 		pod := event.Object.(*example.Pod)
-		if event.Type != watch.Added || !hasInitializers(pod, "Test") {
+		if event.Type != watch.Added || !hasInitializers(pod, validInitializerName) {
 			t.Fatalf("unexpected event: %s %#v", event.Type, event.Object)
 		}
 		pod.Initializers.Pending = nil
@@ -856,7 +858,7 @@ func TestStoreDelete(t *testing.T) {
 
 func TestStoreDeleteUninitialized(t *testing.T) {
 	podA := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: "Testing"}}}},
+		ObjectMeta: metav1.ObjectMeta{Name: "foo", Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: validInitializerName}}}},
 		Spec:       example.PodSpec{NodeName: "machine"},
 	}
 
@@ -1002,7 +1004,7 @@ func TestFailedInitializationStoreUpdate(t *testing.T) {
 
 	initialGeneration := int64(1)
 	podInitializing := &example.Pod{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: "Test"}}}, Generation: initialGeneration},
+		ObjectMeta: metav1.ObjectMeta{Name: "foo", Initializers: &metav1.Initializers{Pending: []metav1.Initializer{{Name: validInitializerName}}}, Generation: initialGeneration},
 		Spec:       example.PodSpec{NodeName: "machine"},
 	}
 
@@ -1617,7 +1619,7 @@ func TestStoreDeleteCollection(t *testing.T) {
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "baz",
 			Initializers: &metav1.Initializers{
-				Pending: []metav1.Initializer{{Name: "Test"}},
+				Pending: []metav1.Initializer{{Name: validInitializerName}},
 			},
 		},
 	}

test/e2e/apimachinery/initializers.go

@@ -291,7 +291,7 @@ var _ = SIGDescribe("Initializers [Feature:Initializers]", func() {
 func newUninitializedPod(podName string) *v1.Pod {
 	pod := newInitPod(podName)
 	pod.Initializers = &metav1.Initializers{
-		Pending: []metav1.Initializer{{Name: "Test"}},
+		Pending: []metav1.Initializer{{Name: "test.k8s.io"}},
 	}
 	return pod
 }