From 1c1e0d7a95a87f6007ce2956d0efc6b949255166 Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Mon, 10 Oct 2022 17:52:47 -0700 Subject: [PATCH 1/4] Add actions limit to HTTPHeaderFilter Add documentation for HTTPHeaderFilter establishing a single action per header limit, with explanation and examples showing the expected CSV format for multiple-value headers. --- apis/v1beta1/httproute_types.go | 13 +++++--- .../gateway.networking.k8s.io_grpcroutes.yaml | 8 ++--- .../gateway.networking.k8s.io_httproutes.yaml | 32 +++++++++---------- .../gateway.networking.k8s.io_httproutes.yaml | 16 +++++----- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index 78767ccd2c..15dfe0b220 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -720,8 +720,12 @@ 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 MUST NOT specify multiple actions of the same or different type for +// any one header name. 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 +760,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/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml index af9ed2f226..a240dac27c 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_grpcroutes.yaml @@ -326,9 +326,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -678,8 +678,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. diff --git a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml index ae998d0e4b..32dac9e31d 100644 --- a/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/experimental/gateway.networking.k8s.io_httproutes.yaml @@ -307,9 +307,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -604,9 +604,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -947,8 +947,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. @@ -1223,8 +1223,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. @@ -2141,9 +2141,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -2438,9 +2438,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -2781,8 +2781,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. @@ -3057,8 +3057,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. diff --git a/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml b/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml index 755dfdcf95..c6a136f2d5 100644 --- a/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml +++ b/config/crd/standard/gateway.networking.k8s.io_httproutes.yaml @@ -281,9 +281,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -698,8 +698,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. @@ -1632,9 +1632,9 @@ spec: appends to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - \ - name: \"my-header\" value: \"bar\" + \ - name: \"my-header\" value: \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC @@ -2049,8 +2049,8 @@ spec: to any existing values associated with the header name. \n Input: GET /foo HTTP/1.1 my-header: foo \n Config: add: - name: \"my-header\" value: - \"bar\" \n Output: GET /foo HTTP/1.1 my-header: - foo my-header: bar" + \"bar,baz\" \n Output: GET /foo HTTP/1.1 my-header: + foo,bar,baz" items: description: HTTPHeader represents an HTTP Header name and value as defined by RFC 7230. From f7343c5124bd75f2e46b66808c128e9f54682323 Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Wed, 2 Nov 2022 14:43:57 -0700 Subject: [PATCH 2/4] Add webhook limitation on header actions Add an HTTPRoute webhook validation rule for RequestHeaderModifier and ResponseHeaderModifier filters. The rule rejects filters that contain multiple actions for the same header name. --- apis/v1beta1/validation/httproute.go | 42 ++++++++++++++++++ apis/v1beta1/validation/httproute_test.go | 52 +++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go index bb50f91d8b..79c0d8b66f 100644 --- 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[string(action.Name)]; ok { + if needsErr { + errs = append(errs, field.Invalid(path.Child("add"), filter.Add[i], "cannot specify multiple actions for header")) + } + singleAction[string(action.Name)] = false + } else { + singleAction[string(action.Name)] = true + } + } + for i, action := range filter.Set { + if needsErr, ok := singleAction[string(action.Name)]; ok { + if needsErr { + errs = append(errs, field.Invalid(path.Child("set"), filter.Set[i], "cannot specify multiple actions for header")) + } + singleAction[string(action.Name)] = false + } else { + singleAction[string(action.Name)] = true + } + } + for i, action := range filter.Remove { + if needsErr, ok := singleAction[action]; ok { + if needsErr { + errs = append(errs, field.Invalid(path.Child("remove"), filter.Remove[i], "cannot specify multiple actions for header")) + } + singleAction[action] = false + } else { + singleAction[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 index e12d44a28b..a4b1a6e93f 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -445,6 +445,58 @@ 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: "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", + }}, + }, + }}, + }}, }} for _, tc := range tests { From 6bef1d556b9ad09d0fc433342f62f1b4e351d48b Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Thu, 3 Nov 2022 16:14:24 -0700 Subject: [PATCH 3/4] Handle case-inconsistent duplicate header filters Ignore case when checking to see if a header has multiple actions defined. Add tests for case insensitivity, valid header filters, and multiple of the same action for the same header. --- apis/v1beta1/validation/httproute.go | 18 ++--- apis/v1beta1/validation/httproute_test.go | 94 +++++++++++++++++++++++ 2 files changed, 103 insertions(+), 9 deletions(-) diff --git a/apis/v1beta1/validation/httproute.go b/apis/v1beta1/validation/httproute.go index 79c0d8b66f..0647b33640 100644 --- a/apis/v1beta1/validation/httproute.go +++ b/apis/v1beta1/validation/httproute.go @@ -286,33 +286,33 @@ func validateHTTPHeaderModifier(filter gatewayv1b1.HTTPHeaderFilter, path *field var errs field.ErrorList singleAction := make(map[string]bool) for i, action := range filter.Add { - if needsErr, ok := singleAction[string(action.Name)]; ok { + 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[string(action.Name)] = false + singleAction[strings.ToLower(string(action.Name))] = false } else { - singleAction[string(action.Name)] = true + singleAction[strings.ToLower(string(action.Name))] = true } } for i, action := range filter.Set { - if needsErr, ok := singleAction[string(action.Name)]; ok { + 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[string(action.Name)] = false + singleAction[strings.ToLower(string(action.Name))] = false } else { - singleAction[string(action.Name)] = true + singleAction[strings.ToLower(string(action.Name))] = true } } for i, action := range filter.Remove { - if needsErr, ok := singleAction[action]; ok { + 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[action] = false + singleAction[strings.ToLower(action)] = false } else { - singleAction[action] = true + singleAction[strings.ToLower(action)] = true } } return errs diff --git a/apis/v1beta1/validation/httproute_test.go b/apis/v1beta1/validation/httproute_test.go index a4b1a6e93f..ed19c439a7 100644 --- a/apis/v1beta1/validation/httproute_test.go +++ b/apis/v1beta1/validation/httproute_test.go @@ -475,6 +475,82 @@ func TestValidateHTTPRoute(t *testing.T) { 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", + }, }, }, }}, @@ -497,6 +573,24 @@ func TestValidateHTTPRoute(t *testing.T) { }, }}, }}, + }, { + 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 { From c96074baa0a2e600d25b38269a2f3f938e611aa9 Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Tue, 8 Nov 2022 15:20:29 -0800 Subject: [PATCH 4/4] Reword header filter limitations documentation --- apis/v1beta1/httproute_types.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apis/v1beta1/httproute_types.go b/apis/v1beta1/httproute_types.go index 15dfe0b220..f31622fc79 100644 --- a/apis/v1beta1/httproute_types.go +++ b/apis/v1beta1/httproute_types.go @@ -722,9 +722,10 @@ type HTTPHeader struct { // 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 MUST NOT specify multiple actions of the same or different type for -// any one header name. Configuration to set or add multiple values for a -// header MUST use RFC 7230 header value formatting, separating each value with +// 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)