Merge pull request #93929 from liggitt/ingress-secret

Validate ingress TLS secretName in v1

pkg/apis/networking/validation/validation.go

@@ -199,6 +199,8 @@ var ValidateIngressName = apimachineryvalidation.NameIsDNSSubdomain
 
 // IngressValidationOptions cover beta to GA transitions for HTTP PathType
 type IngressValidationOptions struct {
+	// AllowInvalidSecretName indicates whether spec.tls[*].secretName values that are not valid Secret names should be allowed
+	AllowInvalidSecretName bool
 }
 
 // ValidateIngress validates Ingresses on create and update.
@@ -212,7 +214,9 @@ func validateIngress(ingress *networking.Ingress, opts IngressValidationOptions,
 func ValidateIngressCreate(ingress *networking.Ingress, requestGV schema.GroupVersion) field.ErrorList {
 	allErrs := field.ErrorList{}
 	var opts IngressValidationOptions
-	opts = IngressValidationOptions{}
+	opts = IngressValidationOptions{
+		AllowInvalidSecretName: allowInvalidSecretName(requestGV, nil),
+	}
 	allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...)
 	annotationVal, annotationIsSet := ingress.Annotations[annotationIngressClass]
 	if annotationIsSet && ingress.Spec.IngressClassName != nil {
@@ -226,26 +230,34 @@ func ValidateIngressCreate(ingress *networking.Ingress, requestGV schema.GroupVe
 func ValidateIngressUpdate(ingress, oldIngress *networking.Ingress, requestGV schema.GroupVersion) field.ErrorList {
 	allErrs := apivalidation.ValidateObjectMetaUpdate(&ingress.ObjectMeta, &oldIngress.ObjectMeta, field.NewPath("metadata"))
 	var opts IngressValidationOptions
-	opts = IngressValidationOptions{}
+	opts = IngressValidationOptions{
+		AllowInvalidSecretName: allowInvalidSecretName(requestGV, oldIngress),
+	}
 
 	allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...)
 	return allErrs
 }
 
-func validateIngressTLS(spec *networking.IngressSpec, fldPath *field.Path) field.ErrorList {
+func validateIngressTLS(spec *networking.IngressSpec, fldPath *field.Path, opts IngressValidationOptions) field.ErrorList {
 	allErrs := field.ErrorList{}
 	// TODO: Perform a more thorough validation of spec.TLS.Hosts that takes
 	// the wildcard spec from RFC 6125 into account.
-	for _, itls := range spec.TLS {
+	for tlsIndex, itls := range spec.TLS {
 		for i, host := range itls.Hosts {
 			if strings.Contains(host, "*") {
 				for _, msg := range validation.IsWildcardDNS1123Subdomain(host) {
-					allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("hosts"), host, msg))
+					allErrs = append(allErrs, field.Invalid(fldPath.Index(tlsIndex).Child("hosts").Index(i), host, msg))
 				}
 				continue
 			}
 			for _, msg := range validation.IsDNS1123Subdomain(host) {
-				allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("hosts"), host, msg))
+				allErrs = append(allErrs, field.Invalid(fldPath.Index(tlsIndex).Child("hosts").Index(i), host, msg))
+			}
+		}
+
+		if !opts.AllowInvalidSecretName {
+			for _, msg := range validateTLSSecretName(itls.SecretName) {
+				allErrs = append(allErrs, field.Invalid(fldPath.Index(tlsIndex).Child("secretName"), itls.SecretName, msg))
 			}
 		}
 	}
@@ -278,7 +290,7 @@ func ValidateIngressSpec(spec *networking.IngressSpec, fldPath *field.Path, opts
 		allErrs = append(allErrs, validateIngressRules(spec.Rules, fldPath.Child("rules"), opts, requestGV)...)
 	}
 	if len(spec.TLS) > 0 {
-		allErrs = append(allErrs, validateIngressTLS(spec, fldPath.Child("tls"))...)
+		allErrs = append(allErrs, validateIngressTLS(spec, fldPath.Child("tls"), opts)...)
 	}
 	if spec.IngressClassName != nil {
 		for _, msg := range ValidateIngressClassName(*spec.IngressClassName, false) {
@@ -523,3 +535,26 @@ func validateIngressTypedLocalObjectReference(params *api.TypedLocalObjectRefere
 
 	return allErrs
 }
+
+func allowInvalidSecretName(gv schema.GroupVersion, oldIngress *networking.Ingress) bool {
+	if gv == networkingv1beta1.SchemeGroupVersion || gv == extensionsv1beta1.SchemeGroupVersion {
+		// backwards compatibility with released API versions that allowed invalid names
+		return true
+	}
+	if oldIngress != nil {
+		for _, tls := range oldIngress.Spec.TLS {
+			if len(validateTLSSecretName(tls.SecretName)) > 0 {
+				// backwards compatibility with existing persisted object
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func validateTLSSecretName(name string) []string {
+	if len(name) == 0 {
+		return nil
+	}
+	return apivalidation.ValidateSecretName(name, false)
+}

pkg/apis/networking/validation/validation_test.go

@@ -1379,6 +1379,31 @@ func TestValidateIngressCreate(t *testing.T) {
 			},
 			expectedErrs: field.ErrorList{},
 		},
+		"v1: valid secret": {
+			groupVersion: &networkingv1.SchemeGroupVersion,
+			tweakIngress: func(ingress *networking.Ingress) {
+				ingress.Spec.TLS = []networking.IngressTLS{{SecretName: "valid"}}
+			},
+		},
+		"v1: invalid secret": {
+			groupVersion: &networkingv1.SchemeGroupVersion,
+			tweakIngress: func(ingress *networking.Ingress) {
+				ingress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name"}}
+			},
+			expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec").Child("tls").Index(0).Child("secretName"), "invalid name", `a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`)},
+		},
+		"v1beta1: valid secret": {
+			groupVersion: &networkingv1beta1.SchemeGroupVersion,
+			tweakIngress: func(ingress *networking.Ingress) {
+				ingress.Spec.TLS = []networking.IngressTLS{{SecretName: "valid"}}
+			},
+		},
+		"v1beta1: invalid secret": {
+			groupVersion: &networkingv1beta1.SchemeGroupVersion,
+			tweakIngress: func(ingress *networking.Ingress) {
+				ingress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 1"}}
+			},
+		},
 	}
 
 	for name, testCase := range testCases {
@@ -1431,6 +1456,7 @@ func TestValidateIngressUpdate(t *testing.T) {
 	}
 
 	testCases := map[string]struct {
+		gv             schema.GroupVersion
 		tweakIngresses func(newIngress, oldIngress *networking.Ingress)
 		expectedErrs   field.ErrorList
 	}{
@@ -1714,6 +1740,35 @@ func TestValidateIngressUpdate(t *testing.T) {
 			},
 			expectedErrs: field.ErrorList{},
 		},
+		"v1: change valid secret -> invalid secret": {
+			gv: networkingv1.SchemeGroupVersion,
+			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
+				oldIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "valid"}}
+				newIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name"}}
+			},
+			expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec").Child("tls").Index(0).Child("secretName"), "invalid name", `a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`)},
+		},
+		"v1: change invalid secret -> invalid secret": {
+			gv: networkingv1.SchemeGroupVersion,
+			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
+				oldIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 1"}}
+				newIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 2"}}
+			},
+		},
+		"v1beta1: change valid secret -> invalid secret": {
+			gv: networkingv1beta1.SchemeGroupVersion,
+			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
+				oldIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "valid"}}
+				newIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name"}}
+			},
+		},
+		"v1beta1: change invalid secret -> invalid secret": {
+			gv: networkingv1beta1.SchemeGroupVersion,
+			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
+				oldIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 1"}}
+				newIngress.Spec.TLS = []networking.IngressTLS{{SecretName: "invalid name 2"}}
+			},
+		},
 	}
 
 	for name, testCase := range testCases {
@@ -1722,7 +1777,11 @@ func TestValidateIngressUpdate(t *testing.T) {
 			oldIngress := baseIngress.DeepCopy()
 			testCase.tweakIngresses(newIngress, oldIngress)
 
-			errs := ValidateIngressUpdate(newIngress, oldIngress, networkingv1beta1.SchemeGroupVersion)
+			gv := testCase.gv
+			if gv.Empty() {
+				gv = networkingv1beta1.SchemeGroupVersion
+			}
+			errs := ValidateIngressUpdate(newIngress, oldIngress, gv)
 
 			if len(errs) != len(testCase.expectedErrs) {
 				t.Fatalf("Expected %d errors, got %d (%+v)", len(testCase.expectedErrs), len(errs), errs)
@@ -1978,7 +2037,7 @@ func TestValidateIngressTLS(t *testing.T) {
 			Hosts: []string{wildcardHost},
 		},
 	}
-	badWildcardTLSErr := fmt.Sprintf("spec.tls[0].hosts: Invalid value: '%v'", wildcardHost)
+	badWildcardTLSErr := fmt.Sprintf("spec.tls[0].hosts[0]: Invalid value: '%v'", wildcardHost)
 	errorCases[badWildcardTLSErr] = badWildcardTLS
 
 	for k, v := range errorCases {

pkg/registry/networking/ingress/storage/storage_test.go

@@ -60,7 +60,7 @@ var (
 	defaultPathType     = networking.PathTypeImplementationSpecific
 	defaultPathMap      = map[string]string{defaultPath: defaultBackendName}
 	defaultTLS          = []networking.IngressTLS{
-		{Hosts: []string{"foo.bar.com", "*.bar.com"}, SecretName: "fooSecret"},
+		{Hosts: []string{"foo.bar.com", "*.bar.com"}, SecretName: "foosecret"},
 	}
 	serviceBackend = &networking.IngressServiceBackend{
 		Name: "defaultbackend",
@@ -173,7 +173,7 @@ func TestUpdate(t *testing.T) {
 			})
 			object.Spec.TLS = append(object.Spec.TLS, networking.IngressTLS{
 				Hosts:      []string{"*.google.com"},
-				SecretName: "googleSecret",
+				SecretName: "googlesecret",
 			})
 			return object
 		},