Merge pull request #91034 from liggitt/ingress-validation

Ingress validation

pkg/apis/networking/validation/validation.go

@@ -19,7 +19,6 @@ package validation
 import (
 	"fmt"
 	"net"
-	"regexp"
 	"strings"
 
 	apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
@@ -198,8 +197,6 @@ var ValidateIngressName = apimachineryvalidation.NameIsDNSSubdomain
 
 // IngressValidationOptions cover beta to GA transitions for HTTP PathType
 type IngressValidationOptions struct {
-	requireRegexPath     bool
-	allowResourceBackend bool
 }
 
 // ValidateIngress validates Ingresses on create and update.
@@ -213,12 +210,7 @@ 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{
-		// TODO(robscott): Remove regex validation for 1.19.
-		requireRegexPath: true,
-		// TODO(cmluciano): Allow resource backend for 1.19.
-		allowResourceBackend: false,
-	}
+	opts = IngressValidationOptions{}
 	allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...)
 	annotationVal, annotationIsSet := ingress.Annotations[annotationIngressClass]
 	if annotationIsSet && ingress.Spec.IngressClassName != nil {
@@ -232,13 +224,7 @@ 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{
-		// TODO(robscott): Remove regex validation for 1.19.
-		// Only require regex path validation for this Ingress if the previous
-		// version of the Ingress also passed that validation.
-		requireRegexPath:     allPathsPassRegexValidation(oldIngress),
-		allowResourceBackend: resourceBackendPresent(oldIngress),
-	}
+	opts = IngressValidationOptions{}
 
 	allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...)
 	return allErrs
@@ -377,14 +363,6 @@ func validateHTTPIngressPath(path *networking.HTTPIngressPath, fldPath *field.Pa
 		allErrs = append(allErrs, field.NotSupported(fldPath.Child("pathType"), *path.PathType, supportedPathTypes.List()))
 	}
 
-	// TODO(robscott): Remove regex validation for 1.19.
-	if opts.requireRegexPath {
-		_, err := regexp.CompilePOSIX(path.Path)
-		if err != nil {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("path"), path.Path, "must be a valid regex"))
-		}
-	}
-
 	allErrs = append(allErrs, validateIngressBackend(&path.Backend, fldPath.Child("backend"), opts)...)
 	return allErrs
 }
@@ -399,8 +377,6 @@ func validateIngressBackend(backend *networking.IngressBackend, fldPath *field.P
 	switch {
 	case hasResourceBackend && hasServiceBackend:
 		return append(allErrs, field.Invalid(fldPath, "", "cannot set both resource and service backends"))
-	case hasResourceBackend && !opts.allowResourceBackend:
-		return append(allErrs, field.Forbidden(fldPath.Child("resource"), "not supported; only service backends are supported in this version"))
 	case hasResourceBackend:
 		allErrs = append(allErrs, validateIngressTypedLocalObjectReference(backend.Resource, fldPath.Child("resource"))...)
 	default:
@@ -484,39 +460,3 @@ func validateIngressTypedLocalObjectReference(params *api.TypedLocalObjectRefere
 
 	return allErrs
 }
-
-// allPathsPassRegexValidation returns true if the Ingress has paths that all
-// match the Ingress path validation with requireRegexPath enabled.
-func allPathsPassRegexValidation(ingress *networking.Ingress) bool {
-	for _, rule := range ingress.Spec.Rules {
-		if rule.HTTP == nil {
-			continue
-		}
-		for _, path := range rule.HTTP.Paths {
-			if len(path.Path) == 0 {
-				continue
-			}
-			if _, err := regexp.CompilePOSIX(path.Path); err != nil {
-				return false
-			}
-		}
-	}
-	return true
-}
-
-func resourceBackendPresent(ingress *networking.Ingress) bool {
-	if ingress.Spec.Backend != nil && ingress.Spec.Backend.Resource != nil {
-		return true
-	}
-	for _, rule := range ingress.Spec.Rules {
-		if rule.HTTP == nil {
-			continue
-		}
-		for _, path := range rule.HTTP.Paths {
-			if path.Backend.Resource != nil {
-				return true
-			}
-		}
-	}
-	return false
-}

pkg/apis/networking/validation/validation_test.go

@@ -1122,7 +1122,7 @@ func TestValidateIngress(t *testing.T) {
 			if gv == nil {
 				gv = &networkingv1.SchemeGroupVersion
 			}
-			errs := validateIngress(ingress, IngressValidationOptions{requireRegexPath: true}, *gv)
+			errs := validateIngress(ingress, IngressValidationOptions{}, *gv)
 			if len(testCase.expectErrsOnFields) != len(errs) {
 				t.Fatalf("Expected %d errors, got %d errors: %v", len(testCase.expectErrsOnFields), len(errs), errs)
 			}
@@ -1138,10 +1138,9 @@ func TestValidateIngress(t *testing.T) {
 func TestValidateIngressRuleValue(t *testing.T) {
 	fldPath := field.NewPath("testing.http.paths[0].path")
 	testCases := map[string]struct {
-		pathType         networking.PathType
-		path             string
-		requireRegexPath bool
-		expectedErrs     field.ErrorList
+		pathType     networking.PathType
+		path         string
+		expectedErrs field.ErrorList
 	}{
 		"implementation specific: no leading slash": {
 			pathType:     networking.PathTypeImplementationSpecific,
@@ -1242,7 +1241,7 @@ func TestValidateIngressRuleValue(t *testing.T) {
 				},
 			}
 
-			errs := validateIngressRuleValue(irv, field.NewPath("testing"), IngressValidationOptions{requireRegexPath: true})
+			errs := validateIngressRuleValue(irv, field.NewPath("testing"), IngressValidationOptions{})
 
 			if len(errs) != len(testCase.expectedErrs) {
 				t.Fatalf("Expected %d errors, got %d (%+v)", len(testCase.expectedErrs), len(errs), errs)
@@ -1335,16 +1334,16 @@ func TestValidateIngressCreate(t *testing.T) {
 					},
 				}}
 			},
-			expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec.rules[0].http.paths[0].path"), "/([a-z0-9]*)[", "must be a valid regex")},
+			expectedErrs: field.ErrorList{},
 		},
-		"Spec.Backend.Resource field not allowed on create": {
+		"Spec.Backend.Resource field allowed on create": {
 			tweakIngress: func(ingress *networking.Ingress) {
 				ingress.Spec.Backend = &networking.IngressBackend{
 					Resource: resourceBackend}
 			},
-			expectedErrs: field.ErrorList{field.Forbidden(field.NewPath("spec.backend.resource"), "not supported; only service backends are supported in this version")},
+			expectedErrs: field.ErrorList{},
 		},
-		"Paths.Backend.Resource field not allowed on create": {
+		"Paths.Backend.Resource field allowed on create": {
 			tweakIngress: func(ingress *networking.Ingress) {
 				ingress.Spec.Rules = []networking.IngressRule{{
 					Host: "foo.bar.com",
@@ -1360,7 +1359,7 @@ func TestValidateIngressCreate(t *testing.T) {
 					},
 				}}
 			},
-			expectedErrs: field.ErrorList{field.Forbidden(field.NewPath("spec.rules[0].http.paths[0].backend.resource"), "not supported; only service backends are supported in this version")},
+			expectedErrs: field.ErrorList{},
 		},
 	}
 
@@ -1484,7 +1483,7 @@ func TestValidateIngressUpdate(t *testing.T) {
 					},
 				}}
 			},
-			expectedErrs: field.ErrorList{field.Invalid(field.NewPath("spec.rules[0].http.paths[0].path"), "/bar[", "must be a valid regex")},
+			expectedErrs: field.ErrorList{},
 		},
 		"invalid regex path -> valid regex path": {
 			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
@@ -1544,13 +1543,13 @@ func TestValidateIngressUpdate(t *testing.T) {
 			},
 			expectedErrs: field.ErrorList{},
 		},
-		"new Backend.Resource not allowed on update": {
+		"new Backend.Resource allowed on update": {
 			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
 				oldIngress.Spec.Backend = &defaultBackend
 				newIngress.Spec.Backend = &networking.IngressBackend{
 					Resource: resourceBackend}
 			},
-			expectedErrs: field.ErrorList{field.Forbidden(field.NewPath("spec.backend.resource"), "not supported; only service backends are supported in this version")},
+			expectedErrs: field.ErrorList{},
 		},
 		"old Backend.Resource allowed on update": {
 			tweakIngresses: func(newIngress, oldIngress *networking.Ingress) {
@@ -1687,7 +1686,7 @@ func TestValidateIngressUpdate(t *testing.T) {
 					},
 				}}
 			},
-			expectedErrs: field.ErrorList{field.Forbidden(field.NewPath("spec.rules[0].http.paths[0].backend.resource"), "not supported; only service backends are supported in this version")},
+			expectedErrs: field.ErrorList{},
 		},
 	}
 
@@ -1956,7 +1955,7 @@ func TestValidateIngressTLS(t *testing.T) {
 	errorCases[badWildcardTLSErr] = badWildcardTLS
 
 	for k, v := range errorCases {
-		errs := validateIngress(&v, IngressValidationOptions{requireRegexPath: true}, networkingv1beta1.SchemeGroupVersion)
+		errs := validateIngress(&v, IngressValidationOptions{}, networkingv1beta1.SchemeGroupVersion)
 		if len(errs) == 0 {
 			t.Errorf("expected failure for %q", k)
 		} else {
@@ -1980,7 +1979,7 @@ func TestValidateIngressTLS(t *testing.T) {
 	}
 	validCases[fmt.Sprintf("spec.tls[0].hosts: Valid value: '%v'", wildHost)] = goodWildcardTLS
 	for k, v := range validCases {
-		errs := validateIngress(&v, IngressValidationOptions{requireRegexPath: true}, networkingv1beta1.SchemeGroupVersion)
+		errs := validateIngress(&v, IngressValidationOptions{}, networkingv1beta1.SchemeGroupVersion)
 		if len(errs) != 0 {
 			t.Errorf("expected success for %q", k)
 		}
@@ -2078,151 +2077,3 @@ func TestValidateIngressStatusUpdate(t *testing.T) {
 		}
 	}
 }
-
-func TestValidateResourceBackendPresent(t *testing.T) {
-	implementationPathType := networking.PathTypeImplementationSpecific
-	defaultBackend := networking.IngressBackend{
-		ServiceName: "default-backend",
-		ServicePort: intstr.FromInt(80),
-	}
-	resourceBackend := &api.TypedLocalObjectReference{
-		APIGroup: utilpointer.StringPtr("example.com"),
-		Kind:     "foo",
-		Name:     "bar",
-	}
-	baseIngress := networking.Ingress{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "foo",
-			Namespace: metav1.NamespaceDefault,
-		},
-		Spec: networking.IngressSpec{
-			Backend: &networking.IngressBackend{
-				ServiceName: "default-backend",
-				ServicePort: intstr.FromInt(80),
-			},
-			Rules: []networking.IngressRule{
-				{
-					Host: "foo.bar.com",
-					IngressRuleValue: networking.IngressRuleValue{
-						HTTP: &networking.HTTPIngressRuleValue{
-							Paths: []networking.HTTPIngressPath{
-								{
-									Path:     "/foo",
-									PathType: &implementationPathType,
-									Backend:  defaultBackend,
-								},
-							},
-						},
-					},
-				},
-			},
-		},
-		Status: networking.IngressStatus{
-			LoadBalancer: api.LoadBalancerStatus{
-				Ingress: []api.LoadBalancerIngress{
-					{IP: "127.0.0.1"},
-				},
-			},
-		},
-	}
-	testCases := map[string]struct {
-		groupVersion          *schema.GroupVersion
-		tweakIngress          func(ing *networking.Ingress)
-		expectResourceBackend bool
-	}{
-		"nil spec.Backend and no paths": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Path = ""
-			},
-			expectResourceBackend: false,
-		},
-		"nil spec.Backend.Resource and no paths": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths = []networking.HTTPIngressPath{}
-			},
-			expectResourceBackend: false,
-		},
-		"non-nil spec.Backend.Resource and no paths": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = &networking.IngressBackend{
-					Resource: resourceBackend,
-				}
-				ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].Path = ""
-			},
-			expectResourceBackend: true,
-		},
-		"nil spec.Backend, one rule with nil HTTP ": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue.HTTP = nil
-			},
-			expectResourceBackend: false,
-		},
-		"nil spec.Backend, one rule with non-nil HTTP, no paths": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue = networking.IngressRuleValue{
-					HTTP: &networking.HTTPIngressRuleValue{
-						Paths: []networking.HTTPIngressPath{},
-					},
-				}
-			},
-			expectResourceBackend: false,
-		},
-		"nil spec.Backend, one rule with non-nil HTTP, one path with nil Backend.Resource": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue = networking.IngressRuleValue{
-					HTTP: &networking.HTTPIngressRuleValue{
-						Paths: []networking.HTTPIngressPath{
-							{
-								Path:     "/foo",
-								PathType: &implementationPathType,
-								Backend: networking.IngressBackend{
-									Resource: nil,
-								},
-							},
-						},
-					},
-				}
-			},
-			expectResourceBackend: false,
-		},
-		"nil spec.Backend, one rule with non-nil HTTP, one path with non-nil Backend.Resource": {
-			tweakIngress: func(ing *networking.Ingress) {
-				ing.Spec.Backend = nil
-				ing.Spec.Rules[0].IngressRuleValue = networking.IngressRuleValue{
-					HTTP: &networking.HTTPIngressRuleValue{
-						Paths: []networking.HTTPIngressPath{
-							{
-								Path:     "/foo",
-								PathType: &implementationPathType,
-								Backend: networking.IngressBackend{
-									Resource: resourceBackend,
-								},
-							},
-						},
-					},
-				}
-			},
-			expectResourceBackend: true,
-		},
-	}
-
-	for name, testCase := range testCases {
-		t.Run(name, func(t *testing.T) {
-			ingress := baseIngress.DeepCopy()
-			testCase.tweakIngress(ingress)
-			gv := testCase.groupVersion
-			if gv == nil {
-				gv = &networkingv1.SchemeGroupVersion
-			}
-			isBackendAllowed := resourceBackendPresent(ingress)
-			if isBackendAllowed != testCase.expectResourceBackend {
-				t.Errorf("Expected resourceBackendPresent to return: %v, got: %v", testCase.expectResourceBackend, isBackendAllowed)
-			}
-		})
-	}
-}

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

@@ -137,7 +137,7 @@ func TestCreate(t *testing.T) {
 	noBackendAndRules.Spec.Rules = []networking.IngressRule{}
 	badPath := validIngress()
 	badPath.Spec.Rules = toIngressRules(map[string]IngressRuleValues{
-		"foo.bar.com": {"/invalid[": "svc"}})
+		"foo.bar.com": {"invalid-no-leading-slash": "svc"}})
 	test.TestCreate(
 		// valid
 		ingress,
@@ -176,7 +176,7 @@ func TestUpdate(t *testing.T) {
 		func(obj runtime.Object) runtime.Object {
 			object := obj.(*networking.Ingress)
 			object.Spec.Rules = toIngressRules(map[string]IngressRuleValues{
-				"foo.bar.com": {"/invalid[": "svc"}})
+				"foo.bar.com": {"invalid-no-leading-slash": "svc"}})
 			return object
 		},
 	)