From a58e11a4783244e52a08645fef981531fda1dc80 Mon Sep 17 00:00:00 2001 From: Rob Scott Date: Wed, 20 Apr 2022 17:54:26 -0700 Subject: [PATCH] Refactoring path modifier to use union discriminator --- apis/v1alpha2/httproute_types.go | 29 +++--- apis/v1alpha2/validation/httproute.go | 29 ++++++ apis/v1alpha2/validation/httproute_test.go | 84 ++++++++++++++++- apis/v1alpha2/zz_generated.deepcopy.go | 16 +++- .../gateway.networking.k8s.io_httproutes.yaml | 92 ++++++++++--------- .../experimental/v1alpha2/http-redirect.yaml | 18 ++++ .../experimental/v1alpha2/http-rewrite.yaml | 18 ++++ 7 files changed, 229 insertions(+), 57 deletions(-) create mode 100644 examples/experimental/v1alpha2/http-redirect.yaml create mode 100644 examples/experimental/v1alpha2/http-rewrite.yaml diff --git a/apis/v1alpha2/httproute_types.go b/apis/v1alpha2/httproute_types.go index 5edaa18e46..3b77317558 100644 --- a/apis/v1alpha2/httproute_types.go +++ b/apis/v1alpha2/httproute_types.go @@ -684,13 +684,13 @@ type HTTPRequestHeaderFilter struct { Remove []string `json:"remove,omitempty"` } -// HTTPPathModifierType defines the type of path redirect. +// HTTPPathModifierType defines the type of path redirect or rewrite. type HTTPPathModifierType string const ( - // This type of modifier indicates that the complete path will be replaced - // by the path redirect value. - AbsoluteHTTPPathModifier HTTPPathModifierType = "Absolute" + // This type of modifier indicates that the full path will be replaced + // by the specified value. + FullPathHTTPPathModifier HTTPPathModifierType = "ReplaceFullPath" // This type of modifier indicates that any prefix path matches will be // replaced by the substitution value. For example, a path with a prefix @@ -705,17 +705,24 @@ type HTTPPathModifier struct { // Type defines the type of path modifier. // // - // +kubebuilder:validation:Enum=Absolute;ReplacePrefixMatch + // +kubebuilder:validation:Enum=ReplaceFullPath;ReplacePrefixMatch Type HTTPPathModifierType `json:"type"` - // Substitution defines the HTTP path value to substitute. An empty value - // ("") indicates that the portion of the path to be changed should be - // removed from the resulting path. For example, a request to "/foo/bar" - // with a prefix match of "/foo" would be modified to "/bar". + // ReplaceFullPath specifies the value with which to replace the full path + // of a request during a rewrite or redirect. // // // +kubebuilder:validation:MaxLength=1024 - Substitution string `json:"substitution"` + // +optional + ReplaceFullPath *string `json:"replaceFullPath,omitempty"` + + // ReplacePrefixMatch specifies the value with which to replace the prefix + // match of a request during a rewrite or redirect. + // + // + // +kubebuilder:validation:MaxLength=1024 + // +optional + ReplacePrefixMatch *string `json:"replacePrefixMatch,omitempty"` } // HTTPRequestRedirect defines a filter that redirects a request. This filter @@ -783,7 +790,7 @@ type HTTPURLRewriteFilter struct { // // // +optional - Hostname *Hostname `json:"hostname,omitempty"` + Hostname *PreciseHostname `json:"hostname,omitempty"` // Path defines a path rewrite. // diff --git a/apis/v1alpha2/validation/httproute.go b/apis/v1alpha2/validation/httproute.go index 67101ba9ec..e21da19e25 100644 --- a/apis/v1alpha2/validation/httproute.go +++ b/apis/v1alpha2/validation/httproute.go @@ -96,6 +96,12 @@ func validateHTTPRouteFilters(filters []gatewayv1a2.HTTPRouteFilter, path *field for i, filter := range filters { counts[filter.Type]++ + if filter.RequestRedirect != nil && filter.RequestRedirect.Path != nil { + errs = append(errs, validateHTTPPathModifier(*filter.RequestRedirect.Path, path.Index(i).Child("requestRedirect", "path"))...) + } + if filter.URLRewrite != nil && filter.URLRewrite.Path != nil { + errs = append(errs, validateHTTPPathModifier(*filter.URLRewrite.Path, path.Index(i).Child("urlRewrite", "path"))...) + } errs = append(errs, validateHTTPRouteFilterTypeMatchesValue(filter, path.Index(i))...) } // custom filters don't have any validation @@ -103,6 +109,10 @@ func validateHTTPRouteFilters(filters []gatewayv1a2.HTTPRouteFilter, path *field delete(counts, key) } + if counts[gatewayv1a2.HTTPRouteFilterRequestRedirect] > 0 && counts[gatewayv1a2.HTTPRouteFilterURLRewrite] > 0 { + errs = append(errs, field.Invalid(path.Child("filters"), gatewayv1a2.HTTPRouteFilterRequestRedirect, "Redirect and Rewrite filters cannot be defined in the same list of filters")) + } + for filterType, count := range counts { if count > 1 { errs = append(errs, field.Invalid(path.Child("filters"), filterType, "cannot be used multiple times in the same rule")) @@ -185,3 +195,22 @@ func validateHTTPRouteFilterTypeMatchesValue(filter gatewayv1a2.HTTPRouteFilter, } return errs } + +// validateHTTPPathModifier validates that only the expected fields are set in a +// path modifier. +func validateHTTPPathModifier(modifier gatewayv1a2.HTTPPathModifier, path *field.Path) field.ErrorList { + var errs field.ErrorList + if modifier.ReplaceFullPath != nil && modifier.Type != gatewayv1a2.FullPathHTTPPathModifier { + errs = append(errs, field.Invalid(path, modifier.ReplaceFullPath, "must be nil if the HTTPRouteFilter.Type is not ReplaceFullPath")) + } + if modifier.ReplaceFullPath == nil && modifier.Type == gatewayv1a2.FullPathHTTPPathModifier { + errs = append(errs, field.Invalid(path, modifier.ReplaceFullPath, "must not be nil if the HTTPRouteFilter.Type is ReplaceFullPath")) + } + if modifier.ReplacePrefixMatch != nil && modifier.Type != gatewayv1a2.PrefixMatchHTTPPathModifier { + errs = append(errs, field.Invalid(path, modifier.ReplaceFullPath, "must be nil if the HTTPRouteFilter.Type is not ReplacePrefixMatch")) + } + if modifier.ReplacePrefixMatch == nil && modifier.Type == gatewayv1a2.PrefixMatchHTTPPathModifier { + errs = append(errs, field.Invalid(path, modifier.ReplacePrefixMatch, "must not be nil if the HTTPRouteFilter.Type is ReplacePrefixMatch")) + } + return errs +} diff --git a/apis/v1alpha2/validation/httproute_test.go b/apis/v1alpha2/validation/httproute_test.go index 770fba1872..abf4c35335 100644 --- a/apis/v1alpha2/validation/httproute_test.go +++ b/apis/v1alpha2/validation/httproute_test.go @@ -293,6 +293,88 @@ func TestValidateHTTPRoute(t *testing.T) { }, }, }, + }, { + name: "valid redirect path modifier", + errCount: 0, + rules: []gatewayv1a2.HTTPRouteRule{ + { + Filters: []gatewayv1a2.HTTPRouteFilter{ + { + Type: gatewayv1a2.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1a2.HTTPRequestRedirectFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.FullPathHTTPPathModifier, + ReplaceFullPath: utilpointer.String("foo"), + }, + }, + }, + }, + }, + }, + }, { + name: "redirect path modifier with type mismatch", + errCount: 2, + rules: []gatewayv1a2.HTTPRouteRule{{ + Filters: []gatewayv1a2.HTTPRouteFilter{{ + Type: gatewayv1a2.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1a2.HTTPRequestRedirectFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.PrefixMatchHTTPPathModifier, + ReplaceFullPath: utilpointer.String("foo"), + }, + }, + }}, + }}, + }, { + name: "valid rewrite path modifier", + errCount: 0, + rules: []gatewayv1a2.HTTPRouteRule{{ + Filters: []gatewayv1a2.HTTPRouteFilter{{ + Type: gatewayv1a2.HTTPRouteFilterURLRewrite, + URLRewrite: &gatewayv1a2.HTTPURLRewriteFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.PrefixMatchHTTPPathModifier, + ReplacePrefixMatch: utilpointer.String("foo"), + }, + }, + }}, + }}, + }, { + name: "redirect path modifier with type mismatch", + errCount: 2, + rules: []gatewayv1a2.HTTPRouteRule{{ + Filters: []gatewayv1a2.HTTPRouteFilter{{ + Type: gatewayv1a2.HTTPRouteFilterURLRewrite, + URLRewrite: &gatewayv1a2.HTTPURLRewriteFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.FullPathHTTPPathModifier, + ReplacePrefixMatch: utilpointer.String("foo"), + }, + }, + }}, + }}, + }, { + name: "rewrite and redirect filters combined (invalid)", + errCount: 1, + rules: []gatewayv1a2.HTTPRouteRule{{ + Filters: []gatewayv1a2.HTTPRouteFilter{{ + Type: gatewayv1a2.HTTPRouteFilterURLRewrite, + URLRewrite: &gatewayv1a2.HTTPURLRewriteFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.PrefixMatchHTTPPathModifier, + ReplacePrefixMatch: utilpointer.String("foo"), + }, + }, + }, { + Type: gatewayv1a2.HTTPRouteFilterRequestRedirect, + RequestRedirect: &gatewayv1a2.HTTPRequestRedirectFilter{ + Path: &gatewayv1a2.HTTPPathModifier{ + Type: gatewayv1a2.PrefixMatchHTTPPathModifier, + ReplacePrefixMatch: utilpointer.String("foo"), + }, + }, + }}, + }}, }} for _, tc := range tests { @@ -654,7 +736,7 @@ func TestValidateHTTPRouteTypeMatchesField(t *testing.T) { routeFilter: gatewayv1a2.HTTPRouteFilter{ Type: gatewayv1a2.HTTPRouteFilterURLRewrite, URLRewrite: &gatewayv1a2.HTTPURLRewriteFilter{ - Hostname: new(gatewayv1a2.Hostname), + Hostname: new(gatewayv1a2.PreciseHostname), Path: &gatewayv1a2.HTTPPathModifier{}, }, }, diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index a09e3e31ed..ef6778fe38 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -554,6 +554,16 @@ func (in *HTTPPathMatch) DeepCopy() *HTTPPathMatch { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HTTPPathModifier) DeepCopyInto(out *HTTPPathModifier) { *out = *in + if in.ReplaceFullPath != nil { + in, out := &in.ReplaceFullPath, &out.ReplaceFullPath + *out = new(string) + **out = **in + } + if in.ReplacePrefixMatch != nil { + in, out := &in.ReplacePrefixMatch, &out.ReplacePrefixMatch + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HTTPPathModifier. @@ -648,7 +658,7 @@ func (in *HTTPRequestRedirectFilter) DeepCopyInto(out *HTTPRequestRedirectFilter if in.Path != nil { in, out := &in.Path, &out.Path *out = new(HTTPPathModifier) - **out = **in + (*in).DeepCopyInto(*out) } if in.Port != nil { in, out := &in.Port, &out.Port @@ -895,13 +905,13 @@ func (in *HTTPURLRewriteFilter) DeepCopyInto(out *HTTPURLRewriteFilter) { *out = *in if in.Hostname != nil { in, out := &in.Hostname, &out.Hostname - *out = new(Hostname) + *out = new(PreciseHostname) **out = **in } if in.Path != nil { in, out := &in.Path, &out.Path *out = new(HTTPPathModifier) - **out = **in + (*in).DeepCopyInto(*out) } } diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index de674392ab..0740087c55 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -488,26 +488,28 @@ spec: path is used as-is. \n Support: Extended \n " properties: - substitution: - description: "Substitution defines the HTTP - path value to substitute. An empty value - (\"\") indicates that the portion of the - path to be changed should be removed from - the resulting path. For example, a request - to \"/foo/bar\" with a prefix match of - \"/foo\" would be modified to \"/bar\". - \n " + replaceFullPath: + description: "ReplaceFullPath specifies + the value with which to replace the full + path of a request during a rewrite or + redirect. \n " + maxLength: 1024 + type: string + replacePrefixMatch: + description: "ReplacePrefixMatch specifies + the value with which to replace the prefix + match of a request during a rewrite or + redirect. \n " maxLength: 1024 type: string type: description: "Type defines the type of path modifier. \n " enum: - - Absolute + - ReplaceFullPath - ReplacePrefixMatch type: string required: - - substitution - type type: object port: @@ -581,32 +583,34 @@ spec: \n Support: Extended \n " maxLength: 253 minLength: 1 - pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string path: description: "Path defines a path rewrite. \n Support: Extended \n " properties: - substitution: - description: "Substitution defines the HTTP - path value to substitute. An empty value - (\"\") indicates that the portion of the - path to be changed should be removed from - the resulting path. For example, a request - to \"/foo/bar\" with a prefix match of - \"/foo\" would be modified to \"/bar\". - \n " + replaceFullPath: + description: "ReplaceFullPath specifies + the value with which to replace the full + path of a request during a rewrite or + redirect. \n " + maxLength: 1024 + type: string + replacePrefixMatch: + description: "ReplacePrefixMatch specifies + the value with which to replace the prefix + match of a request during a rewrite or + redirect. \n " maxLength: 1024 type: string type: description: "Type defines the type of path modifier. \n " enum: - - Absolute + - ReplaceFullPath - ReplacePrefixMatch type: string required: - - substitution - type type: object type: object @@ -931,13 +935,16 @@ spec: When empty, the request path is used as-is. \n Support: Extended \n " properties: - substitution: - description: "Substitution defines the HTTP path - value to substitute. An empty value (\"\") indicates - that the portion of the path to be changed should - be removed from the resulting path. For example, - a request to \"/foo/bar\" with a prefix match - of \"/foo\" would be modified to \"/bar\". \n + replaceFullPath: + description: "ReplaceFullPath specifies the value + with which to replace the full path of a request + during a rewrite or redirect. \n " + maxLength: 1024 + type: string + replacePrefixMatch: + description: "ReplacePrefixMatch specifies the + value with which to replace the prefix match + of a request during a rewrite or redirect. \n " maxLength: 1024 type: string @@ -945,11 +952,10 @@ spec: description: "Type defines the type of path modifier. \n " enum: - - Absolute + - ReplaceFullPath - ReplacePrefixMatch type: string required: - - substitution - type type: object port: @@ -1021,19 +1027,22 @@ spec: \n Support: Extended \n " maxLength: 253 minLength: 1 - pattern: ^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string path: description: "Path defines a path rewrite. \n Support: Extended \n " properties: - substitution: - description: "Substitution defines the HTTP path - value to substitute. An empty value (\"\") indicates - that the portion of the path to be changed should - be removed from the resulting path. For example, - a request to \"/foo/bar\" with a prefix match - of \"/foo\" would be modified to \"/bar\". \n + replaceFullPath: + description: "ReplaceFullPath specifies the value + with which to replace the full path of a request + during a rewrite or redirect. \n " + maxLength: 1024 + type: string + replacePrefixMatch: + description: "ReplacePrefixMatch specifies the + value with which to replace the prefix match + of a request during a rewrite or redirect. \n " maxLength: 1024 type: string @@ -1041,11 +1050,10 @@ spec: description: "Type defines the type of path modifier. \n " enum: - - Absolute + - ReplaceFullPath - ReplacePrefixMatch type: string required: - - substitution - type type: object type: object diff --git a/examples/experimental/v1alpha2/http-redirect.yaml b/examples/experimental/v1alpha2/http-redirect.yaml new file mode 100644 index 0000000000..87db55e6fc --- /dev/null +++ b/examples/experimental/v1alpha2/http-redirect.yaml @@ -0,0 +1,18 @@ +apiVersion: gateway.networking.k8s.io/v1alpha2 +kind: HTTPRoute +metadata: + name: http-filter-1 + namespace: gateway-api-example-ns1 +spec: + parentRefs: + - name: my-filter-gateway + sectionName: http + hostnames: + - my-filter.example.com + rules: + - filters: + - type: RequestRedirect + requestRedirect: + path: + type: ReplaceFullPath + replaceFullPath: /foo diff --git a/examples/experimental/v1alpha2/http-rewrite.yaml b/examples/experimental/v1alpha2/http-rewrite.yaml new file mode 100644 index 0000000000..fa61b8064e --- /dev/null +++ b/examples/experimental/v1alpha2/http-rewrite.yaml @@ -0,0 +1,18 @@ +apiVersion: gateway.networking.k8s.io/v1alpha2 +kind: HTTPRoute +metadata: + name: http-filter-1 + namespace: gateway-api-example-ns1 +spec: + parentRefs: + - name: my-filter-gateway + sectionName: http + hostnames: + - my-filter.example.com + rules: + - filters: + - type: URLRewrite + urlRewrite: + path: + type: ReplaceFullPath + replaceFullPath: /foo