Merge pull request #1124 from robscott/rewrite-cleanup

Refactoring path modifier to use union discriminator

apis/v1alpha2/httproute_types.go

@@ -699,13 +699,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
@@ -717,20 +717,29 @@ const (
 // HTTPPathModifier defines configuration for path modifiers.
 // <gateway:experimental>
 type HTTPPathModifier struct {
-	// Type defines the type of path modifier.
+	// Type defines the type of path modifier. Additional types may be
+	// added in a future release of the API.
 	//
 	// <gateway:experimental>
-	// +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.
 	//
 	// <gateway:experimental>
 	// +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. For example, a request
+	// to "/foo/bar" with a prefix match of "/foo" would be modified to "/bar".
+	//
+	// <gateway:experimental>
+	// +kubebuilder:validation:MaxLength=1024
+	// +optional
+	ReplacePrefixMatch *string `json:"replacePrefixMatch,omitempty"`
 }
 
 // HTTPRequestRedirect defines a filter that redirects a request. This filter
@@ -798,7 +807,7 @@ type HTTPURLRewriteFilter struct {
 	//
 	// <gateway:experimental>
 	// +optional
-	Hostname *Hostname `json:"hostname,omitempty"`
+	Hostname *PreciseHostname `json:"hostname,omitempty"`
 
 	// Path defines a path rewrite.
 	//

apis/v1alpha2/validation/httproute.go

@@ -96,13 +96,23 @@ 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
 	for _, key := range repeatableHTTPRouteFilters {
 		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.ReplacePrefixMatch, "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
+}

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{},
 			},
 		},