From c33de9f2044b0d72b4bcbec3d081133c7a9d0848 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Thu, 24 Aug 2017 10:27:46 -0700 Subject: [PATCH 1/2] unify the validation rules on initializer name --- .../validation/validation.go | 22 ++--------- .../validation/validation_test.go | 4 +- .../pkg/api/validation/objectmeta.go | 4 +- .../apimachinery/pkg/util/validation/BUILD | 2 + .../pkg/util/validation/validation.go | 17 +++++++++ .../pkg/util/validation/validation_test.go | 37 +++++++++++++++++++ 6 files changed, 62 insertions(+), 24 deletions(-) diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index 4c478f457e0..39a5c4ab81f 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/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 @@ -186,15 +178,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))...) diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index a864ac3a866..a8f74808006 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/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", @@ -274,7 +274,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", diff --git a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go b/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go index 5bef9495c60..3c32a937ac2 100644 --- a/staging/src/k8s.io/apimachinery/pkg/api/validation/objectmeta.go +++ b/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 diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/BUILD b/staging/src/k8s.io/apimachinery/pkg/util/validation/BUILD index cd174f80082..252ee530862 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/BUILD +++ b/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( diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go index 1159a02573c..13f3bc80788 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/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 diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go index 061be1a6e65..2ce8437d400 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation_test.go +++ b/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) { @@ -450,3 +452,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) + } + } +} From 85ee09e4c901e9fcf725bb4797ea2b3c278ee96c Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Mon, 28 Aug 2017 15:04:09 -0700 Subject: [PATCH 2/2] update initializer names to valid ones in tests --- .../pkg/registry/generic/registry/store_test.go | 16 +++++++++------- test/e2e/apimachinery/initializers.go | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index cbe510eed53..759fdd8e6f0 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -57,6 +57,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) @@ -394,7 +396,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"}, @@ -422,7 +424,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) } @@ -497,7 +499,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"}, @@ -519,7 +521,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 @@ -849,7 +851,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"}, } @@ -993,7 +995,7 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { 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"}, } @@ -1608,7 +1610,7 @@ func TestStoreDeleteCollection(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "baz", Initializers: &metav1.Initializers{ - Pending: []metav1.Initializer{{Name: "Test"}}, + Pending: []metav1.Initializer{{Name: validInitializerName}}, }, }, } diff --git a/test/e2e/apimachinery/initializers.go b/test/e2e/apimachinery/initializers.go index 39f02528562..b2179eaf813 100644 --- a/test/e2e/apimachinery/initializers.go +++ b/test/e2e/apimachinery/initializers.go @@ -290,7 +290,7 @@ var _ = SIGDescribe("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 }