From 8f7b8105a1162ccdcb48e05de959e7b3a416bfa9 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 12 May 2020 13:58:35 -0400 Subject: [PATCH 1/2] Remove ingress regex path requirement --- pkg/apis/networking/validation/validation.go | 35 ------------------- .../networking/validation/validation_test.go | 19 +++++----- .../ingress/storage/storage_test.go | 4 +-- 3 files changed, 11 insertions(+), 47 deletions(-) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 3b2ebeeb8b8..086c29cb61a 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/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,7 +197,6 @@ var ValidateIngressName = apimachineryvalidation.NameIsDNSSubdomain // IngressValidationOptions cover beta to GA transitions for HTTP PathType type IngressValidationOptions struct { - requireRegexPath bool allowResourceBackend bool } @@ -214,8 +212,6 @@ func ValidateIngressCreate(ingress *networking.Ingress, requestGV schema.GroupVe 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, } @@ -233,10 +229,6 @@ func ValidateIngressUpdate(ingress, oldIngress *networking.Ingress, requestGV sc 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), } @@ -377,14 +369,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 } @@ -485,25 +469,6 @@ 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 diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 615588e954a..39187e37714 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/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,7 +1334,7 @@ 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": { tweakIngress: func(ingress *networking.Ingress) { @@ -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) { @@ -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) } diff --git a/pkg/registry/networking/ingress/storage/storage_test.go b/pkg/registry/networking/ingress/storage/storage_test.go index 612cc81b9e0..dd93bbac715 100644 --- a/pkg/registry/networking/ingress/storage/storage_test.go +++ b/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 }, ) From 1758d176899c3cef671eb4a3bb77df852280eb95 Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 12 May 2020 15:38:19 -0400 Subject: [PATCH 2/2] Allow resource backends in Ingress --- pkg/apis/networking/validation/validation.go | 29 +--- .../networking/validation/validation_test.go | 162 +----------------- 2 files changed, 9 insertions(+), 182 deletions(-) diff --git a/pkg/apis/networking/validation/validation.go b/pkg/apis/networking/validation/validation.go index 086c29cb61a..26ee85614cd 100644 --- a/pkg/apis/networking/validation/validation.go +++ b/pkg/apis/networking/validation/validation.go @@ -197,7 +197,6 @@ var ValidateIngressName = apimachineryvalidation.NameIsDNSSubdomain // IngressValidationOptions cover beta to GA transitions for HTTP PathType type IngressValidationOptions struct { - allowResourceBackend bool } // ValidateIngress validates Ingresses on create and update. @@ -211,10 +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(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 { @@ -228,9 +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{ - allowResourceBackend: resourceBackendPresent(oldIngress), - } + opts = IngressValidationOptions{} allErrs = append(allErrs, validateIngress(ingress, opts, requestGV)...) return allErrs @@ -383,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: @@ -468,20 +460,3 @@ func validateIngressTypedLocalObjectReference(params *api.TypedLocalObjectRefere return allErrs } - -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 -} diff --git a/pkg/apis/networking/validation/validation_test.go b/pkg/apis/networking/validation/validation_test.go index 39187e37714..1b45d618b4e 100644 --- a/pkg/apis/networking/validation/validation_test.go +++ b/pkg/apis/networking/validation/validation_test.go @@ -1336,14 +1336,14 @@ func TestValidateIngressCreate(t *testing.T) { }, 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", @@ -1359,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{}, }, } @@ -1543,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) { @@ -1686,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{}, }, } @@ -2077,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) - } - }) - } -}