Merge pull request #1497 from rainest/doc/header-filter-single

Add and test single filter action per header restriction
kubernetes-sigs · Nov 9, 2022 · fa2063e · fa2063e
2 parents c99f925 + c96074b
commit fa2063e
Show file tree

Hide file tree

Showing 6 changed files with 225 additions and 33 deletions.
diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go
@@ -720,8 +720,13 @@ type HTTPHeader struct {
 	Value string `json:"value"`
 }
 
-// HTTPHeaderFilter defines a filter that modifies the headers of an HTTP request
-// or response.
+// HTTPHeaderFilter defines a filter that modifies the headers of an HTTP
+// request or response. Only one action for a given header name is permitted.
+// Filters specifying multiple actions of the same or different type for
+// any one header name are invalid and will be rejected by the webhook if
+// installed. Configuration to set or add multiple values for a
+// header must use RFC 7230 header value formatting, separating each value with
+// a comma.
 type HTTPHeaderFilter struct {
 	// Set overwrites the request with the given header (name, value)
 	// before the action.
@@ -756,12 +761,11 @@ type HTTPHeaderFilter struct {
 	// Config:
 	//   add:
 	//   - name: "my-header"
-	//     value: "bar"
+	//     value: "bar,baz"
 	//
 	// Output:
 	//   GET /foo HTTP/1.1
-	//   my-header: foo
-	//   my-header: bar
+	//   my-header: foo,bar,baz
 	//
 	// +optional
 	// +listType=map

diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go
@@ -111,6 +111,12 @@ func validateHTTPRouteFilters(filters []gatewayv1b1.HTTPRouteFilter, matches []g
 		if filter.URLRewrite != nil && filter.URLRewrite.Path != nil {
 			errs = append(errs, validateHTTPPathModifier(*filter.URLRewrite.Path, matches, path.Index(i).Child("urlRewrite", "path"))...)
 		}
+		if filter.RequestHeaderModifier != nil {
+			errs = append(errs, validateHTTPHeaderModifier(*filter.RequestHeaderModifier, path.Index(i).Child("requestHeaderModifier"))...)
+		}
+		if filter.ResponseHeaderModifier != nil {
+			errs = append(errs, validateHTTPHeaderModifier(*filter.ResponseHeaderModifier, path.Index(i).Child("responseHeaderModifier"))...)
+		}
 		errs = append(errs, validateHTTPRouteFilterTypeMatchesValue(filter, path.Index(i))...)
 	}
 	// custom filters don't have any validation
@@ -276,6 +282,42 @@ func validateHTTPPathModifier(modifier gatewayv1b1.HTTPPathModifier, matches []g
 	return errs
 }
 
+func validateHTTPHeaderModifier(filter gatewayv1b1.HTTPHeaderFilter, path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	singleAction := make(map[string]bool)
+	for i, action := range filter.Add {
+		if needsErr, ok := singleAction[strings.ToLower(string(action.Name))]; ok {
+			if needsErr {
+				errs = append(errs, field.Invalid(path.Child("add"), filter.Add[i], "cannot specify multiple actions for header"))
+			}
+			singleAction[strings.ToLower(string(action.Name))] = false
+		} else {
+			singleAction[strings.ToLower(string(action.Name))] = true
+		}
+	}
+	for i, action := range filter.Set {
+		if needsErr, ok := singleAction[strings.ToLower(string(action.Name))]; ok {
+			if needsErr {
+				errs = append(errs, field.Invalid(path.Child("set"), filter.Set[i], "cannot specify multiple actions for header"))
+			}
+			singleAction[strings.ToLower(string(action.Name))] = false
+		} else {
+			singleAction[strings.ToLower(string(action.Name))] = true
+		}
+	}
+	for i, action := range filter.Remove {
+		if needsErr, ok := singleAction[strings.ToLower(action)]; ok {
+			if needsErr {
+				errs = append(errs, field.Invalid(path.Child("remove"), filter.Remove[i], "cannot specify multiple actions for header"))
+			}
+			singleAction[strings.ToLower(action)] = false
+		} else {
+			singleAction[strings.ToLower(action)] = true
+		}
+	}
+	return errs
+}
+
 func hasExactlyOnePrefixMatch(matches []gatewayv1b1.HTTPRouteMatch) bool {
 	if len(matches) != 1 || matches[0].Path == nil {
 		return false

diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go
@@ -445,6 +445,152 @@ func TestValidateHTTPRoute(t *testing.T) {
 				},
 			}},
 		}},
+	}, {
+		name:     "multiple actions for the same request header (invalid)",
+		errCount: 2,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterRequestHeaderModifier,
+				RequestHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "apple",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-vegetable"),
+							Value: "carrot",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-grain"),
+							Value: "rye",
+						},
+					},
+					Set: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "watermelon",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-grain"),
+							Value: "wheat",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-spice"),
+							Value: "coriander",
+						},
+					},
+				},
+			}},
+		}},
+	}, {
+		name:     "multiple actions for the same request header with inconsistent case (invalid)",
+		errCount: 1,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterRequestHeaderModifier,
+				RequestHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "apple",
+						},
+					},
+					Set: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("X-Fruit"),
+							Value: "watermelon",
+						},
+					},
+				},
+			}},
+		}},
+	}, {
+		name:     "multiple of the same action for the same request header (invalid)",
+		errCount: 1,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterRequestHeaderModifier,
+				RequestHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "apple",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "plum",
+						},
+					},
+				},
+			}},
+		}},
+	}, {
+		name:     "multiple actions for different request headers",
+		errCount: 0,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterRequestHeaderModifier,
+				RequestHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-vegetable"),
+							Value: "carrot",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-grain"),
+							Value: "rye",
+						},
+					},
+					Set: []gatewayv1b1.HTTPHeader{
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-fruit"),
+							Value: "watermelon",
+						},
+						{
+							Name:  gatewayv1b1.HTTPHeaderName("x-spice"),
+							Value: "coriander",
+						},
+					},
+				},
+			}},
+		}},
+	}, {
+		name:     "multiple actions for the same response header (invalid)",
+		errCount: 1,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterResponseHeaderModifier,
+				ResponseHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{{
+						Name:  gatewayv1b1.HTTPHeaderName("x-example"),
+						Value: "blueberry",
+					}},
+					Set: []gatewayv1b1.HTTPHeader{{
+						Name:  gatewayv1b1.HTTPHeaderName("x-example"),
+						Value: "turnip",
+					}},
+				},
+			}},
+		}},
+	}, {
+		name:     "multiple actions for different response headers",
+		errCount: 0,
+		rules: []gatewayv1b1.HTTPRouteRule{{
+			Filters: []gatewayv1b1.HTTPRouteFilter{{
+				Type: gatewayv1b1.HTTPRouteFilterResponseHeaderModifier,
+				ResponseHeaderModifier: &gatewayv1b1.HTTPHeaderFilter{
+					Add: []gatewayv1b1.HTTPHeader{{
+						Name:  gatewayv1b1.HTTPHeaderName("x-example"),
+						Value: "blueberry",
+					}},
+					Set: []gatewayv1b1.HTTPHeader{{
+						Name:  gatewayv1b1.HTTPHeaderName("x-different"),
+						Value: "turnip",
+					}},
+				},
+			}},
+		}},
 	}}
 
 	for _, tc := range tests {

diff --git a/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml
diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml