From 2930f87d8874183b7e102d671a9238fc305a19b5 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Tue, 30 Apr 2024 16:20:19 +0100 Subject: [PATCH 1/8] Add request header filter support for gRPC --- internal/mode/static/state/graph/grpcroute.go | 63 +++- .../mode/static/state/graph/grpcroute_test.go | 56 ++- internal/mode/static/state/graph/httproute.go | 160 --------- .../mode/static/state/graph/httproute_test.go | 322 ----------------- .../mode/static/state/graph/route_common.go | 159 +++++++++ .../static/state/graph/route_common_test.go | 323 ++++++++++++++++++ .../overview/gateway-api-compatibility.md | 5 +- 7 files changed, 591 insertions(+), 497 deletions(-) diff --git a/internal/mode/static/state/graph/grpcroute.go b/internal/mode/static/state/graph/grpcroute.go index bffb929a9b..7488dac5fa 100644 --- a/internal/mode/static/state/graph/grpcroute.go +++ b/internal/mode/static/state/graph/grpcroute.go @@ -73,26 +73,24 @@ func processGRPCRouteRules( validator validation.HTTPFieldsValidator, ) (rules []RouteRule, atLeastOneValid bool, allRulesErrs field.ErrorList) { rules = make([]RouteRule, len(specRules)) - validFilters := true for i, rule := range specRules { rulePath := field.NewPath("spec").Child("rules").Index(i) var allErrs field.ErrorList - var matchesErrs field.ErrorList + var filtersErrs field.ErrorList + for j, match := range rule.Matches { matchPath := rulePath.Child("matches").Index(j) matchesErrs = append(matchesErrs, validateGRPCMatch(validator, match, matchPath)...) } if len(rule.Filters) > 0 { - filterPath := rulePath.Child("filters") - allErrs = append( - allErrs, - field.NotSupported(filterPath, rule.Filters, []string{"gRPC filters are not yet supported"}), - ) - validFilters = false + for j, filter := range rule.Filters { + filterPath := rulePath.Child("filters").Index(j) + filtersErrs = append(filtersErrs, validateGRPCFilter(validator, filter, filterPath)...) + } } backendRefs := make([]RouteBackendRef, 0, len(rule.BackendRefs)) @@ -114,17 +112,20 @@ func processGRPCRouteRules( } allErrs = append(allErrs, matchesErrs...) + allErrs = append(allErrs, filtersErrs...) allRulesErrs = append(allRulesErrs, allErrs...) if len(allErrs) == 0 { atLeastOneValid = true } + validFilters := len(filtersErrs) == 0 + rules[i] = RouteRule{ ValidMatches: len(matchesErrs) == 0, ValidFilters: validFilters, Matches: convertGRPCMatches(rule.Matches), - Filters: nil, + Filters: convertGRPCFilters(rule.Filters, validFilters), RouteBackendRefs: backendRefs, } } @@ -234,3 +235,47 @@ func validateGRPCMethodMatch( } return allErrs } + +func validateGRPCFilter( + validator validation.HTTPFieldsValidator, + filter v1alpha2.GRPCRouteFilter, + filterPath *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + switch filter.Type { + case v1alpha2.GRPCRouteFilterRequestHeaderModifier: + return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath) + default: + valErr := field.NotSupported( + filterPath.Child("type"), + filter.Type, + []string{ + string(v1alpha2.GRPCRouteFilterRequestHeaderModifier), + }, + ) + allErrs = append(allErrs, valErr) + return allErrs + } +} + +func convertGRPCFilters(filters []v1alpha2.GRPCRouteFilter, validFilters bool) []v1.HTTPRouteFilter { + // validation has already been done, don't process the filters if they are invalid + if !validFilters || len(filters) == 0 { + return nil + } + httpFilters := make([]v1.HTTPRouteFilter, 0, len(filters)) + for _, filter := range filters { + switch filter.Type { + case v1alpha2.GRPCRouteFilterRequestHeaderModifier: + httpRequestHeaderFilter := v1.HTTPRouteFilter{ + Type: v1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: filter.RequestHeaderModifier, + } + httpFilters = append(httpFilters, httpRequestHeaderFilter) + default: + continue + } + } + return httpFilters +} diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 574cbc6e60..9f843b96e5 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -223,7 +223,7 @@ func TestBuildGRPCRoute(t *testing.T) { grInvalidFilterRule.Filters = []v1alpha2.GRPCRouteFilter{ { - Type: "RequestHeaderModifier", + Type: "RequestMirror", }, } @@ -234,6 +234,24 @@ func TestBuildGRPCRoute(t *testing.T) { []v1alpha2.GRPCRouteRule{grInvalidFilterRule}, ) + grValidFilterRule := createGRPCMethodMatch("myService", "myMethod", "Exact") + + grValidFilterRule.Filters = []v1alpha2.GRPCRouteFilter{ + { + Type: "RequestHeaderModifier", + RequestHeaderModifier: &v1.HTTPHeaderFilter{ + Remove: []string{"header"}, + }, + }, + } + + grValidFilter := createGRPCRoute( + "gr", + gatewayNsName.Name, + "example.com", + []v1alpha2.GRPCRouteRule{grValidFilterRule}, + ) + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { v := &validationfakes.FakeHTTPFieldsValidator{} v.ValidateMethodInMatchReturns(true, nil) @@ -310,6 +328,36 @@ func TestBuildGRPCRoute(t *testing.T) { }, name: "valid rule with empty match", }, + { + validator: createAllValidValidator(), + gr: grValidFilter, + expected: &L7Route{ + RouteType: RouteTypeGRPC, + Source: grValidFilter, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: gatewayNsName, + SectionName: grValidFilter.Spec.ParentRefs[0].SectionName, + }, + }, + Valid: true, + Attachable: true, + Spec: L7RouteSpec{ + Hostnames: grValidFilter.Spec.Hostnames, + Rules: []RouteRule{ + { + ValidMatches: true, + ValidFilters: true, + Matches: convertGRPCMatches(grValidFilter.Spec.Rules[0].Matches), + RouteBackendRefs: []RouteBackendRef{}, + Filters: convertGRPCFilters(grValidFilter.Spec.Rules[0].Filters, true), + }, + }, + }, + }, + name: "valid rule with filter", + }, { validator: createAllValidValidator(), gr: grInvalidMatchesEmptyMethodFields, @@ -522,10 +570,8 @@ func TestBuildGRPCRoute(t *testing.T) { }, Conditions: []conditions.Condition{ staticConds.NewRouteUnsupportedValue( - `All rules are invalid: spec.rules[0].filters: Unsupported value: []v1alpha2.GRPCRouteFilter{v1alpha2.` + - `GRPCRouteFilter{Type:"RequestHeaderModifier", RequestHeaderModifier:(*v1.HTTPHeaderFilter)(nil), ` + - `ResponseHeaderModifier:(*v1.HTTPHeaderFilter)(nil), RequestMirror:(*v1.HTTPRequestMirrorFilter)(nil), ` + - `ExtensionRef:(*v1.LocalObjectReference)(nil)}}: supported values: "gRPC filters are not yet supported"`, + `All rules are invalid: spec.rules[0].filters[0].type: ` + + `Unsupported value: "RequestMirror": supported values: "RequestHeaderModifier"`, ), }, Spec: L7RouteSpec{ diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index bd5aaefdce..d75ebd714b 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -2,7 +2,6 @@ package graph import ( "fmt" - "strings" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" @@ -366,162 +365,3 @@ func validateFilterRewrite( return allErrs } - -func validateFilterHeaderModifier( - validator validation.HTTPFieldsValidator, - headerModifier *v1.HTTPHeaderFilter, - filterPath *field.Path, -) field.ErrorList { - if headerModifier == nil { - return field.ErrorList{field.Required(filterPath, "cannot be nil")} - } - - return validateFilterHeaderModifierFields(validator, headerModifier, filterPath) -} - -func validateFilterHeaderModifierFields( - validator validation.HTTPFieldsValidator, - headerModifier *v1.HTTPHeaderFilter, - headerModifierPath *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - // Ensure that the header names are case-insensitive unique - allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( - headerModifier.Add, - headerModifierPath.Child(add))..., - ) - allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( - headerModifier.Set, - headerModifierPath.Child(set))..., - ) - allErrs = append(allErrs, validateRequestHeaderStringCaseInsensitiveUnique( - headerModifier.Remove, - headerModifierPath.Child(remove))..., - ) - - for _, h := range headerModifier.Add { - if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { - valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) - allErrs = append(allErrs, valErr) - } - if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { - valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - for _, h := range headerModifier.Set { - if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { - valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) - allErrs = append(allErrs, valErr) - } - if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { - valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - for _, h := range headerModifier.Remove { - if err := validator.ValidateFilterHeaderName(h); err != nil { - valErr := field.Invalid(headerModifierPath.Child(remove), h, err.Error()) - allErrs = append(allErrs, valErr) - } - } - - return allErrs -} - -func validateFilterResponseHeaderModifier( - validator validation.HTTPFieldsValidator, - responseHeaderModifier *v1.HTTPHeaderFilter, - filterPath *field.Path, -) field.ErrorList { - if errList := validateFilterHeaderModifier(validator, responseHeaderModifier, filterPath); errList != nil { - return errList - } - var allErrs field.ErrorList - - allErrs = append(allErrs, validateResponseHeaders( - responseHeaderModifier.Add, - filterPath.Child(add))..., - ) - - allErrs = append(allErrs, validateResponseHeaders( - responseHeaderModifier.Set, - filterPath.Child(set))..., - ) - - var removeHeaders []v1.HTTPHeader - for _, h := range responseHeaderModifier.Remove { - removeHeaders = append(removeHeaders, v1.HTTPHeader{Name: v1.HTTPHeaderName(h)}) - } - - allErrs = append(allErrs, validateResponseHeaders( - removeHeaders, - filterPath.Child(remove))..., - ) - - return allErrs -} - -func validateResponseHeaders( - headers []v1.HTTPHeader, - path *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - disallowedResponseHeaderSet := map[string]struct{}{ - "server": {}, - "date": {}, - "x-pad": {}, - "content-type": {}, - "content-length": {}, - "connection": {}, - } - invalidPrefix := "x-accel" - - for _, h := range headers { - valErr := field.Invalid(path, h, "header name is not allowed") - name := strings.ToLower(string(h.Name)) - if _, exists := disallowedResponseHeaderSet[name]; exists || strings.HasPrefix(name, strings.ToLower(invalidPrefix)) { - allErrs = append(allErrs, valErr) - } - } - - return allErrs -} - -func validateRequestHeadersCaseInsensitiveUnique( - headers []v1.HTTPHeader, - path *field.Path, -) field.ErrorList { - var allErrs field.ErrorList - - seen := make(map[string]struct{}) - - for _, h := range headers { - name := strings.ToLower(string(h.Name)) - if _, exists := seen[name]; exists { - valErr := field.Invalid(path, h, "header name is not unique") - allErrs = append(allErrs, valErr) - } - seen[name] = struct{}{} - } - - return allErrs -} - -func validateRequestHeaderStringCaseInsensitiveUnique(headers []string, path *field.Path) field.ErrorList { - var allErrs field.ErrorList - - seen := make(map[string]struct{}) - - for _, h := range headers { - name := strings.ToLower(h) - if _, exists := seen[name]; exists { - valErr := field.Invalid(path, h, "header name is not unique") - allErrs = append(allErrs, valErr) - } - seen[name] = struct{}{} - } - - return allErrs -} diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index ede2abcbd7..b3c1731514 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -1189,325 +1189,3 @@ func TestValidateFilterRewrite(t *testing.T) { }) } } - -func TestValidateFilterRequestHeaderModifier(t *testing.T) { - createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { - v := &validationfakes.FakeHTTPFieldsValidator{} - return v - } - - tests := []struct { - filter gatewayv1.HTTPRouteFilter - validator *validationfakes.FakeHTTPFieldsValidator - name string - expectErrCount int - }{ - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, - Remove: []string{"Cache-Control"}, - }, - }, - expectErrCount: 0, - name: "valid request header modifier filter", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: nil, - }, - expectErrCount: 1, - name: "nil request header modifier filter", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "$var_name", Value: "gzip"}, - }, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid add", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Remove: []string{"$var-name"}, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid remove", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "yhu$"}, - }, - }, - }, - expectErrCount: 1, - name: "request header modifier filter with invalid header value", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Host", Value: "my_host"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "}90yh&$", Value: "gzip$"}, - {Name: "}67yh&$", Value: "compress$"}, - }, - Remove: []string{"Cache-Control$}"}, - }, - }, - expectErrCount: 7, - name: "request header modifier filter all fields invalid", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, - RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - {Name: "mYbespokeHEader", Value: "duplicate"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - {Name: "accept-encodING", Value: "gzip"}, - }, - Remove: []string{"Cache-Control", "cache-control"}, - }, - }, - expectErrCount: 3, - name: "request header modifier filter not unique names", - }, - } - - filterPath := field.NewPath("test") - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - allErrs := validateFilterHeaderModifier( - test.validator, test.filter.RequestHeaderModifier, filterPath, - ) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - }) - } -} - -func TestValidateFilterResponseHeaderModifier(t *testing.T) { - createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { - v := &validationfakes.FakeHTTPFieldsValidator{} - return v - } - - tests := []struct { - filter gatewayv1.HTTPRouteFilter - validator *validationfakes.FakeHTTPFieldsValidator - name string - expectErrCount int - }{ - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - }, - Remove: []string{"Cache-Control"}, - }, - }, - expectErrCount: 0, - name: "valid response header modifier filter", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: nil, - }, - expectErrCount: 1, - name: "nil response header modifier filter", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "$var_name", Value: "gzip"}, - }, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid add", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Remove: []string{"$var-name"}, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid remove", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "yhu$"}, - }, - }, - }, - expectErrCount: 1, - name: "response header modifier filter with invalid header value", - }, - { - validator: func() *validationfakes.FakeHTTPFieldsValidator { - v := createAllValidValidator() - v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) - v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) - return v - }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Host", Value: "my_host"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "}90yh&$", Value: "gzip$"}, - {Name: "}67yh&$", Value: "compress$"}, - }, - Remove: []string{"Cache-Control$}"}, - }, - }, - expectErrCount: 7, - name: "response header modifier filter all fields invalid", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "MyBespokeHeader", Value: "my-value"}, - {Name: "mYbespokeHEader", Value: "duplicate"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Accept-Encoding", Value: "gzip"}, - {Name: "accept-encodING", Value: "gzip"}, - }, - Remove: []string{"Cache-Control", "cache-control"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter not unique names", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "Content-Length", Value: "163"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "Content-Type", Value: "text/plain"}, - }, - Remove: []string{"X-Pad"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter with disallowed header name", - }, - { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, - ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ - Set: []gatewayv1.HTTPHeader{ - {Name: "X-Accel-Redirect", Value: "/protected/iso.img"}, - }, - Add: []gatewayv1.HTTPHeader{ - {Name: "X-Accel-Limit-Rate", Value: "1024"}, - }, - Remove: []string{"X-Accel-Charset"}, - }, - }, - expectErrCount: 3, - name: "response header modifier filter with disallowed header name prefix", - }, - } - - filterPath := field.NewPath("test") - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - allErrs := validateFilterResponseHeaderModifier( - test.validator, test.filter.ResponseHeaderModifier, filterPath, - ) - g.Expect(allErrs).To(HaveLen(test.expectErrCount)) - }) - } -} diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index dbfe8a541e..914cbdfeb7 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -573,3 +573,162 @@ func validateHeaderMatch( return allErrs } + +func validateFilterHeaderModifier( + validator validation.HTTPFieldsValidator, + headerModifier *v1.HTTPHeaderFilter, + filterPath *field.Path, +) field.ErrorList { + if headerModifier == nil { + return field.ErrorList{field.Required(filterPath, "cannot be nil")} + } + + return validateFilterHeaderModifierFields(validator, headerModifier, filterPath) +} + +func validateFilterHeaderModifierFields( + validator validation.HTTPFieldsValidator, + headerModifier *v1.HTTPHeaderFilter, + headerModifierPath *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + // Ensure that the header names are case-insensitive unique + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Add, + headerModifierPath.Child(add))..., + ) + allErrs = append(allErrs, validateRequestHeadersCaseInsensitiveUnique( + headerModifier.Set, + headerModifierPath.Child(set))..., + ) + allErrs = append(allErrs, validateRequestHeaderStringCaseInsensitiveUnique( + headerModifier.Remove, + headerModifierPath.Child(remove))..., + ) + + for _, h := range headerModifier.Add { + if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { + valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) + allErrs = append(allErrs, valErr) + } + if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { + valErr := field.Invalid(headerModifierPath.Child(add), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + for _, h := range headerModifier.Set { + if err := validator.ValidateFilterHeaderName(string(h.Name)); err != nil { + valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) + allErrs = append(allErrs, valErr) + } + if err := validator.ValidateFilterHeaderValue(h.Value); err != nil { + valErr := field.Invalid(headerModifierPath.Child(set), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + for _, h := range headerModifier.Remove { + if err := validator.ValidateFilterHeaderName(h); err != nil { + valErr := field.Invalid(headerModifierPath.Child(remove), h, err.Error()) + allErrs = append(allErrs, valErr) + } + } + + return allErrs +} + +func validateFilterResponseHeaderModifier( + validator validation.HTTPFieldsValidator, + responseHeaderModifier *v1.HTTPHeaderFilter, + filterPath *field.Path, +) field.ErrorList { + if errList := validateFilterHeaderModifier(validator, responseHeaderModifier, filterPath); errList != nil { + return errList + } + var allErrs field.ErrorList + + allErrs = append(allErrs, validateResponseHeaders( + responseHeaderModifier.Add, + filterPath.Child(add))..., + ) + + allErrs = append(allErrs, validateResponseHeaders( + responseHeaderModifier.Set, + filterPath.Child(set))..., + ) + + var removeHeaders []v1.HTTPHeader + for _, h := range responseHeaderModifier.Remove { + removeHeaders = append(removeHeaders, v1.HTTPHeader{Name: v1.HTTPHeaderName(h)}) + } + + allErrs = append(allErrs, validateResponseHeaders( + removeHeaders, + filterPath.Child(remove))..., + ) + + return allErrs +} + +func validateResponseHeaders( + headers []v1.HTTPHeader, + path *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + disallowedResponseHeaderSet := map[string]struct{}{ + "server": {}, + "date": {}, + "x-pad": {}, + "content-type": {}, + "content-length": {}, + "connection": {}, + } + invalidPrefix := "x-accel" + + for _, h := range headers { + valErr := field.Invalid(path, h, "header name is not allowed") + name := strings.ToLower(string(h.Name)) + if _, exists := disallowedResponseHeaderSet[name]; exists || strings.HasPrefix(name, strings.ToLower(invalidPrefix)) { + allErrs = append(allErrs, valErr) + } + } + + return allErrs +} + +func validateRequestHeadersCaseInsensitiveUnique( + headers []v1.HTTPHeader, + path *field.Path, +) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(string(h.Name)) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} + +func validateRequestHeaderStringCaseInsensitiveUnique(headers []string, path *field.Path) field.ErrorList { + var allErrs field.ErrorList + + seen := make(map[string]struct{}) + + for _, h := range headers { + name := strings.ToLower(h) + if _, exists := seen[name]; exists { + valErr := field.Invalid(path, h, "header name is not unique") + allErrs = append(allErrs, valErr) + } + seen[name] = struct{}{} + } + + return allErrs +} diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 36a7031783..85d5802b2e 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -16,6 +16,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" ) func TestBuildSectionNameRefs(t *testing.T) { @@ -1242,3 +1243,325 @@ func TestValidateHostnames(t *testing.T) { }) } } + +func TestValidateFilterRequestHeaderModifier(t *testing.T) { + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + return v + } + + tests := []struct { + filter gatewayv1.HTTPRouteFilter + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectErrCount int + }{ + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + }, + Remove: []string{"Cache-Control"}, + }, + }, + expectErrCount: 0, + name: "valid request header modifier filter", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: nil, + }, + expectErrCount: 1, + name: "nil request header modifier filter", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "$var_name", Value: "gzip"}, + }, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid add", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"$var-name"}, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid remove", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "yhu$"}, + }, + }, + }, + expectErrCount: 1, + name: "request header modifier filter with invalid header value", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Host", Value: "my_host"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "}90yh&$", Value: "gzip$"}, + {Name: "}67yh&$", Value: "compress$"}, + }, + Remove: []string{"Cache-Control$}"}, + }, + }, + expectErrCount: 7, + name: "request header modifier filter all fields invalid", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + {Name: "mYbespokeHEader", Value: "duplicate"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + {Name: "accept-encodING", Value: "gzip"}, + }, + Remove: []string{"Cache-Control", "cache-control"}, + }, + }, + expectErrCount: 3, + name: "request header modifier filter not unique names", + }, + } + + filterPath := field.NewPath("test") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + allErrs := validateFilterHeaderModifier( + test.validator, test.filter.RequestHeaderModifier, filterPath, + ) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + }) + } +} + +func TestValidateFilterResponseHeaderModifier(t *testing.T) { + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { + v := &validationfakes.FakeHTTPFieldsValidator{} + return v + } + + tests := []struct { + filter gatewayv1.HTTPRouteFilter + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectErrCount int + }{ + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + }, + Remove: []string{"Cache-Control"}, + }, + }, + expectErrCount: 0, + name: "valid response header modifier filter", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: nil, + }, + expectErrCount: 1, + name: "nil response header modifier filter", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "$var_name", Value: "gzip"}, + }, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid add", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Remove: []string{"$var-name"}, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid remove", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "yhu$"}, + }, + }, + }, + expectErrCount: 1, + name: "response header modifier filter with invalid header value", + }, + { + validator: func() *validationfakes.FakeHTTPFieldsValidator { + v := createAllValidValidator() + v.ValidateFilterHeaderValueReturns(errors.New("Invalid header value")) + v.ValidateFilterHeaderNameReturns(errors.New("Invalid header")) + return v + }(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Host", Value: "my_host"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "}90yh&$", Value: "gzip$"}, + {Name: "}67yh&$", Value: "compress$"}, + }, + Remove: []string{"Cache-Control$}"}, + }, + }, + expectErrCount: 7, + name: "response header modifier filter all fields invalid", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "MyBespokeHeader", Value: "my-value"}, + {Name: "mYbespokeHEader", Value: "duplicate"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Accept-Encoding", Value: "gzip"}, + {Name: "accept-encodING", Value: "gzip"}, + }, + Remove: []string{"Cache-Control", "cache-control"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter not unique names", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "Content-Length", Value: "163"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "Content-Type", Value: "text/plain"}, + }, + Remove: []string{"X-Pad"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter with disallowed header name", + }, + { + validator: createAllValidValidator(), + filter: gatewayv1.HTTPRouteFilter{ + Type: gatewayv1.HTTPRouteFilterResponseHeaderModifier, + ResponseHeaderModifier: &gatewayv1.HTTPHeaderFilter{ + Set: []gatewayv1.HTTPHeader{ + {Name: "X-Accel-Redirect", Value: "/protected/iso.img"}, + }, + Add: []gatewayv1.HTTPHeader{ + {Name: "X-Accel-Limit-Rate", Value: "1024"}, + }, + Remove: []string{"X-Accel-Charset"}, + }, + }, + expectErrCount: 3, + name: "response header modifier filter with disallowed header name prefix", + }, + } + + filterPath := field.NewPath("test") + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + allErrs := validateFilterResponseHeaderModifier( + test.validator, test.filter.ResponseHeaderModifier, filterPath, + ) + g.Expect(allErrs).To(HaveLen(test.expectErrCount)) + }) + } +} diff --git a/site/content/overview/gateway-api-compatibility.md b/site/content/overview/gateway-api-compatibility.md index cdb8896d18..dff2e713b9 100644 --- a/site/content/overview/gateway-api-compatibility.md +++ b/site/content/overview/gateway-api-compatibility.md @@ -199,7 +199,10 @@ See the [static-mode]({{< relref "/reference/cli-help.md#static-mode">}}) comman - `matches` - `method`: Partially supported. Only `Exact` type with both `method.service` and `method.method` specified. - `headers`: Partially supported. Only `Exact` type. - - `filters`: Not supported + - `filters` + - `type`: Supported. + - `requestHeaderModifier`: Supported. If multiple filters are configured, NGINX Gateway Fabric will choose the first and ignore the rest. + - `responseHeaderModifier`, `requestMirror`, `extensionRef`: Not supported. - `backendRefs`: Partially supported. Backend ref `filters` are not supported. - `status` - `parents` From 1b63994904468e831c1a15adafbe8f5cb7f6a3b5 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 1 May 2024 17:51:27 +0100 Subject: [PATCH 2/8] Review feedback --- internal/mode/static/state/graph/grpcroute.go | 14 +- .../mode/static/state/graph/grpcroute_test.go | 2 +- internal/mode/static/state/graph/httproute.go | 10 +- .../mode/static/state/graph/httproute_test.go | 197 +++++++----------- 4 files changed, 88 insertions(+), 135 deletions(-) diff --git a/internal/mode/static/state/graph/grpcroute.go b/internal/mode/static/state/graph/grpcroute.go index 7488dac5fa..72df4e8e54 100644 --- a/internal/mode/static/state/graph/grpcroute.go +++ b/internal/mode/static/state/graph/grpcroute.go @@ -121,11 +121,16 @@ func processGRPCRouteRules( validFilters := len(filtersErrs) == 0 + var convertedFilters []v1.HTTPRouteFilter + if validFilters { + convertedFilters = convertGRPCFilters(rule.Filters) + } + rules[i] = RouteRule{ ValidMatches: len(matchesErrs) == 0, ValidFilters: validFilters, Matches: convertGRPCMatches(rule.Matches), - Filters: convertGRPCFilters(rule.Filters, validFilters), + Filters: convertedFilters, RouteBackendRefs: backendRefs, } } @@ -259,9 +264,10 @@ func validateGRPCFilter( } } -func convertGRPCFilters(filters []v1alpha2.GRPCRouteFilter, validFilters bool) []v1.HTTPRouteFilter { - // validation has already been done, don't process the filters if they are invalid - if !validFilters || len(filters) == 0 { +// convertGRPCFilters converts GRPCRouteFilters (a subset of HTTPRouteFilter) to HTTPRouteFilters +// so we can reuse the logic from HTTPRoute filter validation and processing +func convertGRPCFilters(filters []v1alpha2.GRPCRouteFilter) []v1.HTTPRouteFilter { + if len(filters) == 0 { return nil } httpFilters := make([]v1.HTTPRouteFilter, 0, len(filters)) diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 9f843b96e5..8e113835e5 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -351,7 +351,7 @@ func TestBuildGRPCRoute(t *testing.T) { ValidFilters: true, Matches: convertGRPCMatches(grValidFilter.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, - Filters: convertGRPCFilters(grValidFilter.Spec.Rules[0].Filters, true), + Filters: convertGRPCFilters(grValidFilter.Spec.Rules[0].Filters), }, }, }, diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index d75ebd714b..cac85228c6 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -248,9 +248,9 @@ func validateFilter( switch filter.Type { case v1.HTTPRouteFilterRequestRedirect: - return validateFilterRedirect(validator, filter, filterPath) + return validateFilterRedirect(validator, filter.RequestRedirect, filterPath) case v1.HTTPRouteFilterURLRewrite: - return validateFilterRewrite(validator, filter, filterPath) + return validateFilterRewrite(validator, filter.URLRewrite, filterPath) case v1.HTTPRouteFilterRequestHeaderModifier: return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath.Child(string(filter.Type))) case v1.HTTPRouteFilterResponseHeaderModifier: @@ -275,12 +275,11 @@ func validateFilter( func validateFilterRedirect( validator validation.HTTPFieldsValidator, - filter v1.HTTPRouteFilter, + redirect *v1.HTTPRequestRedirectFilter, filterPath *field.Path, ) field.ErrorList { var allErrs field.ErrorList - redirect := filter.RequestRedirect redirectPath := filterPath.Child("requestRedirect") if redirect == nil { @@ -325,12 +324,11 @@ func validateFilterRedirect( func validateFilterRewrite( validator validation.HTTPFieldsValidator, - filter v1.HTTPRouteFilter, + rewrite *v1.HTTPURLRewriteFilter, filterPath *field.Path, ) field.ErrorList { var allErrs field.ErrorList - rewrite := filter.URLRewrite rewritePath := filterPath.Child("urlRewrite") if rewrite == nil { diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index b3c1731514..f6cb898097 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -904,42 +904,33 @@ func TestValidateFilterRedirect(t *testing.T) { } tests := []struct { - filter gatewayv1.HTTPRouteFilter - validator *validationfakes.FakeHTTPFieldsValidator - name string - expectErrCount int + requestRedirect *gatewayv1.HTTPRequestRedirectFilter + validator *validationfakes.FakeHTTPFieldsValidator + name string + expectErrCount int }{ { - validator: &validationfakes.FakeHTTPFieldsValidator{}, - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: nil, - }, - name: "nil filter", - expectErrCount: 1, + validator: &validationfakes.FakeHTTPFieldsValidator{}, + requestRedirect: nil, + name: "nil filter", + expectErrCount: 1, }, { validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Scheme: helpers.GetPointer("http"), - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]("example.com"), - Port: helpers.GetPointer[gatewayv1.PortNumber](80), - StatusCode: helpers.GetPointer(301), - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Scheme: helpers.GetPointer("http"), + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]("example.com"), + Port: helpers.GetPointer[gatewayv1.PortNumber](80), + StatusCode: helpers.GetPointer(301), }, expectErrCount: 0, name: "valid redirect filter", }, { - validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, - }, - expectErrCount: 0, - name: "valid redirect filter with no fields set", + validator: createAllValidValidator(), + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{}, + expectErrCount: 0, + name: "valid redirect filter with no fields set", }, { validator: func() *validationfakes.FakeHTTPFieldsValidator { @@ -947,11 +938,8 @@ func TestValidateFilterRedirect(t *testing.T) { validator.ValidateRedirectSchemeReturns(false, []string{"valid-scheme"}) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Scheme: helpers.GetPointer("http"), // any value is invalid by the validator - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Scheme: helpers.GetPointer("http"), // any value is invalid by the validator }, expectErrCount: 1, name: "redirect filter with invalid scheme", @@ -962,13 +950,10 @@ func TestValidateFilterRedirect(t *testing.T) { validator.ValidateHostnameReturns(errors.New("invalid hostname")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( - "example.com", - ), // any value is invalid by the validator - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator }, expectErrCount: 1, name: "redirect filter with invalid hostname", @@ -979,22 +964,16 @@ func TestValidateFilterRedirect(t *testing.T) { validator.ValidateRedirectPortReturns(errors.New("invalid port")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Port: helpers.GetPointer[gatewayv1.PortNumber](80), // any value is invalid by the validator - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Port: helpers.GetPointer[gatewayv1.PortNumber](80), // any value is invalid by the validator }, expectErrCount: 1, name: "redirect filter with invalid port", }, { validator: createAllValidValidator(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Path: &gatewayv1.HTTPPathModifier{}, - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Path: &gatewayv1.HTTPPathModifier{}, }, expectErrCount: 1, name: "redirect filter with unsupported path modifier", @@ -1005,11 +984,8 @@ func TestValidateFilterRedirect(t *testing.T) { validator.ValidateRedirectStatusCodeReturns(false, []string{"200"}) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - StatusCode: helpers.GetPointer(301), // any value is invalid by the validator - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + StatusCode: helpers.GetPointer(301), // any value is invalid by the validator }, expectErrCount: 1, name: "redirect filter with invalid status code", @@ -1021,16 +997,13 @@ func TestValidateFilterRedirect(t *testing.T) { validator.ValidateRedirectPortReturns(errors.New("invalid port")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterRequestRedirect, - RequestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( - "example.com", - ), // any value is invalid by the validator - Port: helpers.GetPointer[gatewayv1.PortNumber]( - 80, - ), // any value is invalid by the validator - }, + requestRedirect: &gatewayv1.HTTPRequestRedirectFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator + Port: helpers.GetPointer[gatewayv1.PortNumber]( + 80, + ), // any value is invalid by the validator }, expectErrCount: 2, name: "redirect filter with multiple errors", @@ -1043,7 +1016,7 @@ func TestValidateFilterRedirect(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - allErrs := validateFilterRedirect(test.validator, test.filter, filterPath) + allErrs := validateFilterRedirect(test.validator, test.requestRedirect, filterPath) g.Expect(allErrs).To(HaveLen(test.expectErrCount)) }) } @@ -1051,41 +1024,32 @@ func TestValidateFilterRedirect(t *testing.T) { func TestValidateFilterRewrite(t *testing.T) { tests := []struct { - filter gatewayv1.HTTPRouteFilter + URLRewrite *gatewayv1.HTTPURLRewriteFilter validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int }{ { - validator: &validationfakes.FakeHTTPFieldsValidator{}, - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: nil, - }, + validator: &validationfakes.FakeHTTPFieldsValidator{}, + URLRewrite: nil, name: "nil filter", expectErrCount: 1, }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]("example.com"), - Path: &gatewayv1.HTTPPathModifier{ - Type: gatewayv1.FullPathHTTPPathModifier, - ReplaceFullPath: helpers.GetPointer("/path"), - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]("example.com"), + Path: &gatewayv1.HTTPPathModifier{ + Type: gatewayv1.FullPathHTTPPathModifier, + ReplaceFullPath: helpers.GetPointer("/path"), }, }, expectErrCount: 0, name: "valid rewrite filter", }, { - validator: &validationfakes.FakeHTTPFieldsValidator{}, - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, - }, + validator: &validationfakes.FakeHTTPFieldsValidator{}, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, expectErrCount: 0, name: "valid rewrite filter with no fields set", }, @@ -1095,25 +1059,19 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateHostnameReturns(errors.New("invalid hostname")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( - "example.com", - ), // any value is invalid by the validator - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator }, expectErrCount: 1, name: "rewrite filter with invalid hostname", }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Path: &gatewayv1.HTTPPathModifier{ - Type: "bad-type", - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Path: &gatewayv1.HTTPPathModifier{ + Type: "bad-type", }, }, expectErrCount: 1, @@ -1125,14 +1083,11 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path value")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Path: &gatewayv1.HTTPPathModifier{ - Type: gatewayv1.FullPathHTTPPathModifier, - ReplaceFullPath: helpers.GetPointer("/path"), - }, // any value is invalid by the validator - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Path: &gatewayv1.HTTPPathModifier{ + Type: gatewayv1.FullPathHTTPPathModifier, + ReplaceFullPath: helpers.GetPointer("/path"), + }, // any value is invalid by the validator }, expectErrCount: 1, name: "rewrite filter with invalid full path", @@ -1143,14 +1098,11 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Path: &gatewayv1.HTTPPathModifier{ - Type: gatewayv1.PrefixMatchHTTPPathModifier, - ReplacePrefixMatch: helpers.GetPointer("/path"), - }, // any value is invalid by the validator - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Path: &gatewayv1.HTTPPathModifier{ + Type: gatewayv1.PrefixMatchHTTPPathModifier, + ReplacePrefixMatch: helpers.GetPointer("/path"), + }, // any value is invalid by the validator }, expectErrCount: 1, name: "rewrite filter with invalid prefix path", @@ -1162,17 +1114,14 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path")) return validator }(), - filter: gatewayv1.HTTPRouteFilter{ - Type: gatewayv1.HTTPRouteFilterURLRewrite, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ - Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( - "example.com", - ), // any value is invalid by the validator - Path: &gatewayv1.HTTPPathModifier{ - Type: gatewayv1.PrefixMatchHTTPPathModifier, - ReplacePrefixMatch: helpers.GetPointer("/path"), - }, // any value is invalid by the validator - }, + URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator + Path: &gatewayv1.HTTPPathModifier{ + Type: gatewayv1.PrefixMatchHTTPPathModifier, + ReplacePrefixMatch: helpers.GetPointer("/path"), + }, // any value is invalid by the validator }, expectErrCount: 2, name: "rewrite filter with multiple errors", @@ -1184,7 +1133,7 @@ func TestValidateFilterRewrite(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - allErrs := validateFilterRewrite(test.validator, test.filter, filterPath) + allErrs := validateFilterRewrite(test.validator, test.URLRewrite, filterPath) g.Expect(allErrs).To(HaveLen(test.expectErrCount)) }) } From 0a88443131866d1b207bfd650af0acec0a672818 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 1 May 2024 19:39:42 +0100 Subject: [PATCH 3/8] Change to lowercase urlRewrite in tests --- .../mode/static/state/graph/httproute_test.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index f6cb898097..60af11914a 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -1024,20 +1024,20 @@ func TestValidateFilterRedirect(t *testing.T) { func TestValidateFilterRewrite(t *testing.T) { tests := []struct { - URLRewrite *gatewayv1.HTTPURLRewriteFilter + urlRewrite *gatewayv1.HTTPURLRewriteFilter validator *validationfakes.FakeHTTPFieldsValidator name string expectErrCount int }{ { validator: &validationfakes.FakeHTTPFieldsValidator{}, - URLRewrite: nil, + urlRewrite: nil, name: "nil filter", expectErrCount: 1, }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]("example.com"), Path: &gatewayv1.HTTPPathModifier{ Type: gatewayv1.FullPathHTTPPathModifier, @@ -1049,7 +1049,7 @@ func TestValidateFilterRewrite(t *testing.T) { }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{}, + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{}, expectErrCount: 0, name: "valid rewrite filter with no fields set", }, @@ -1059,7 +1059,7 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateHostnameReturns(errors.New("invalid hostname")) return validator }(), - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( "example.com", ), // any value is invalid by the validator @@ -1069,7 +1069,7 @@ func TestValidateFilterRewrite(t *testing.T) { }, { validator: &validationfakes.FakeHTTPFieldsValidator{}, - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Path: &gatewayv1.HTTPPathModifier{ Type: "bad-type", }, @@ -1083,7 +1083,7 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path value")) return validator }(), - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Path: &gatewayv1.HTTPPathModifier{ Type: gatewayv1.FullPathHTTPPathModifier, ReplaceFullPath: helpers.GetPointer("/path"), @@ -1098,7 +1098,7 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path")) return validator }(), - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Path: &gatewayv1.HTTPPathModifier{ Type: gatewayv1.PrefixMatchHTTPPathModifier, ReplacePrefixMatch: helpers.GetPointer("/path"), @@ -1114,7 +1114,7 @@ func TestValidateFilterRewrite(t *testing.T) { validator.ValidateRewritePathReturns(errors.New("invalid path")) return validator }(), - URLRewrite: &gatewayv1.HTTPURLRewriteFilter{ + urlRewrite: &gatewayv1.HTTPURLRewriteFilter{ Hostname: helpers.GetPointer[gatewayv1.PreciseHostname]( "example.com", ), // any value is invalid by the validator @@ -1133,7 +1133,7 @@ func TestValidateFilterRewrite(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - allErrs := validateFilterRewrite(test.validator, test.URLRewrite, filterPath) + allErrs := validateFilterRewrite(test.validator, test.urlRewrite, filterPath) g.Expect(allErrs).To(HaveLen(test.expectErrCount)) }) } From c5d9acc00fb5c394796555b637de4c37b6718da7 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Thu, 2 May 2024 13:33:46 +0100 Subject: [PATCH 4/8] Review feedback --- internal/mode/static/state/graph/grpcroute.go | 8 +- .../mode/static/state/graph/grpcroute_test.go | 82 ++++++++++++++++++- 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/internal/mode/static/state/graph/grpcroute.go b/internal/mode/static/state/graph/grpcroute.go index 72df4e8e54..477b594a23 100644 --- a/internal/mode/static/state/graph/grpcroute.go +++ b/internal/mode/static/state/graph/grpcroute.go @@ -86,11 +86,9 @@ func processGRPCRouteRules( matchesErrs = append(matchesErrs, validateGRPCMatch(validator, match, matchPath)...) } - if len(rule.Filters) > 0 { - for j, filter := range rule.Filters { - filterPath := rulePath.Child("filters").Index(j) - filtersErrs = append(filtersErrs, validateGRPCFilter(validator, filter, filterPath)...) - } + for j, filter := range rule.Filters { + filterPath := rulePath.Child("filters").Index(j) + filtersErrs = append(filtersErrs, validateGRPCFilter(validator, filter, filterPath)...) } backendRefs := make([]RouteBackendRef, 0, len(rule.BackendRefs)) diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 8e113835e5..3de0ea8931 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -252,6 +252,15 @@ func TestBuildGRPCRoute(t *testing.T) { []v1alpha2.GRPCRouteRule{grValidFilterRule}, ) + convertedFilters := []v1.HTTPRouteFilter{ + { + Type: v1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: &v1.HTTPHeaderFilter{ + Remove: []string{"header"}, + }, + }, + } + createAllValidValidator := func() *validationfakes.FakeHTTPFieldsValidator { v := &validationfakes.FakeHTTPFieldsValidator{} v.ValidateMethodInMatchReturns(true, nil) @@ -351,7 +360,7 @@ func TestBuildGRPCRoute(t *testing.T) { ValidFilters: true, Matches: convertGRPCMatches(grValidFilter.Spec.Rules[0].Matches), RouteBackendRefs: []RouteBackendRef{}, - Filters: convertGRPCFilters(grValidFilter.Spec.Rules[0].Filters), + Filters: convertedFilters, }, }, }, @@ -630,3 +639,74 @@ func TestBuildGRPCRoute(t *testing.T) { }) } } + +func TestConvertGRPCMatches(t *testing.T) { + methodMatch := createGRPCMethodMatch("myService", "myMethod", "Exact").Matches + + headersMatch := createGRPCHeadersMatch("Exact", "MyHeader", "SomeValue").Matches + + expectedHTTPMatches := []v1.HTTPRouteMatch{ + { + Path: &v1.HTTPPathMatch{ + Type: helpers.GetPointer(v1.PathMatchExact), + Value: helpers.GetPointer("/myService/myMethod"), + }, + Headers: []v1.HTTPHeaderMatch{}, + }, + } + + expectedHeadersMatches := []v1.HTTPRouteMatch{ + { + Path: &v1.HTTPPathMatch{ + Type: helpers.GetPointer(v1.PathMatchPathPrefix), + Value: helpers.GetPointer("/"), + }, + Headers: []v1.HTTPHeaderMatch{ + { + Value: "SomeValue", + Name: v1.HTTPHeaderName("MyHeader"), + }, + }, + }, + } + + expectedEmptyMatches := []v1.HTTPRouteMatch{ + { + Path: &v1.HTTPPathMatch{ + Type: helpers.GetPointer(v1.PathMatchPathPrefix), + Value: helpers.GetPointer("/"), + }, + }, + } + + tests := []struct { + name string + methodMatches []v1alpha2.GRPCRouteMatch + expected []v1.HTTPRouteMatch + }{ + { + name: "exact match", + methodMatches: methodMatch, + expected: expectedHTTPMatches, + }, + { + name: "headers matches", + methodMatches: headersMatch, + expected: expectedHeadersMatches, + }, + { + name: "empty matches", + methodMatches: []v1alpha2.GRPCRouteMatch{}, + expected: expectedEmptyMatches, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + httpMatches := convertGRPCMatches(test.methodMatches) + g.Expect(helpers.Diff(test.expected, httpMatches)).To(BeEmpty()) + }) + } +} From a65d8b9c8b505606f390a5f08725e410c0a92e14 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Thu, 2 May 2024 13:34:24 +0100 Subject: [PATCH 5/8] Add base gRPC headers --- internal/mode/static/nginx/config/servers.go | 25 ++++- .../mode/static/nginx/config/servers_test.go | 105 ++++++++++++------ 2 files changed, 93 insertions(+), 37 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 9e3fe06177..c4ce9ce4f2 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -30,15 +30,25 @@ var baseHeaders = []http.Header{ Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, - { +} + +// httpBaseHeaders contains the constant headers set in each HTTP server block +var httpBaseHeaders = append(baseHeaders, + http.Header{ Name: "Upgrade", Value: "$http_upgrade", }, - { + http.Header{ Name: "Connection", Value: "$connection_upgrade", - }, -} + }) + +// grpcBaseHeaders contains the constant headers set in each gRPC server block +var grpcBaseHeaders = append(baseHeaders, + http.Header{ + Name: "Authority", + Value: "$gw_api_compliant_host", + }) func executeServers(conf dataplane.Configuration) []executeResult { servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers) @@ -558,8 +568,11 @@ func createMatchLocation(path string) http.Location { func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.Header { var headers []http.Header if !grpc { - headers = make([]http.Header, len(baseHeaders)) - copy(headers, baseHeaders) + headers = make([]http.Header, len(httpBaseHeaders)) + copy(headers, httpBaseHeaders) + } else { + headers = make([]http.Header, len(grpcBaseHeaders)) + copy(headers, grpcBaseHeaders) } if filters != nil && filters.RequestURLRewrite != nil && filters.RequestURLRewrite.Hostname != nil { diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 5f039a7a86..04fd0298ca 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -649,19 +649,19 @@ func TestCreateServers(t *testing.T) { { Path: "@rule0-route0", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "@rule0-route1", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "@rule0-route2", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { @@ -671,7 +671,7 @@ func TestCreateServers(t *testing.T) { { Path: "@rule1-route0", ProxyPass: "http://$test__route1_rule1$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { @@ -681,19 +681,19 @@ func TestCreateServers(t *testing.T) { { Path: "/path-only/", ProxyPass: "http://invalid-backend-ref$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "= /path-only", ProxyPass: "http://invalid-backend-ref$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "/backend-tls-policy/", ProxyPass: "https://test_btp_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ProxySSLVerify: &http.ProxySSLVerify{ Name: "test-btp.example.com", TrustedCertificate: "/etc/nginx/secrets/test-btp.crt", @@ -702,7 +702,7 @@ func TestCreateServers(t *testing.T) { { Path: "= /backend-tls-policy", ProxyPass: "https://test_btp_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ProxySSLVerify: &http.ProxySSLVerify{ Name: "test-btp.example.com", TrustedCertificate: "/etc/nginx/secrets/test-btp.crt", @@ -809,13 +809,13 @@ func TestCreateServers(t *testing.T) { { Path: "= /exact", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "@rule12-route0", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { @@ -898,7 +898,7 @@ func TestCreateServers(t *testing.T) { Path: "= /grpc/method", ProxyPass: "grpc://test_foo_80", GRPC: true, - ProxySetHeaders: nil, + ProxySetHeaders: grpcBaseHeaders, }, { Path: "= /grpc-with-backend-tls-policy/method", @@ -908,7 +908,7 @@ func TestCreateServers(t *testing.T) { TrustedCertificate: "/etc/nginx/secrets/test-btp.crt", }, GRPC: true, - ProxySetHeaders: nil, + ProxySetHeaders: grpcBaseHeaders, }, } } @@ -1028,13 +1028,13 @@ func TestCreateServersConflicts(t *testing.T) { { Path: "/coffee/", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "= /coffee", ProxyPass: "http://test_bar_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, createDefaultRootLocation(), @@ -1068,13 +1068,13 @@ func TestCreateServersConflicts(t *testing.T) { { Path: "= /coffee", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "/coffee/", ProxyPass: "http://test_bar_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, createDefaultRootLocation(), @@ -1118,13 +1118,13 @@ func TestCreateServersConflicts(t *testing.T) { { Path: "/coffee/", ProxyPass: "http://test_bar_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "= /coffee", ProxyPass: "http://test_baz_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, createDefaultRootLocation(), @@ -1243,13 +1243,13 @@ func TestCreateLocationsRootPath(t *testing.T) { { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { @@ -1268,17 +1268,18 @@ func TestCreateLocationsRootPath(t *testing.T) { { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, }, { - Path: "/grpc", - ProxyPass: "grpc://test_foo_80", - GRPC: true, + Path: "/grpc", + ProxyPass: "grpc://test_foo_80", + GRPC: true, + ProxySetHeaders: grpcBaseHeaders, }, { Path: "/", @@ -1295,19 +1296,19 @@ func TestCreateLocationsRootPath(t *testing.T) { { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, { Path: "/", ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: baseHeaders, + ProxySetHeaders: httpBaseHeaders, ResponseHeaders: http.ResponseHeaders{}, }, }, @@ -2027,9 +2028,51 @@ func TestGenerateProxySetHeaders(t *testing.T) { }, }, { - msg: "grpc", - expectedHeaders: nil, - GRPC: true, + msg: "header filter with gRPC", + GRPC: true, + filters: &dataplane.HTTPFilters{ + RequestHeaderModifiers: &dataplane.HTTPHeaderFilter{ + Add: []dataplane.HTTPHeader{ + { + Name: "Authorization", + Value: "my-auth", + }, + }, + Set: []dataplane.HTTPHeader{ + { + Name: "Accept-Encoding", + Value: "gzip", + }, + }, + Remove: []string{"my-header"}, + }, + }, + expectedHeaders: []http.Header{ + { + Name: "Authorization", + Value: "${authorization_header_var}my-auth", + }, + { + Name: "Accept-Encoding", + Value: "gzip", + }, + { + Name: "my-header", + Value: "", + }, + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { + Name: "Authority", + Value: "$gw_api_compliant_host", + }, + }, }, } From 5e19f8647a5c01643d5e4ab94712617476f093c6 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Thu, 2 May 2024 19:04:14 +0100 Subject: [PATCH 6/8] Remove append from baseHeaders --- internal/mode/static/nginx/config/servers.go | 30 ++++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index c4ce9ce4f2..b67b98af76 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -20,8 +20,8 @@ const ( rootPath = "/" ) -// baseHeaders contains the constant headers set in each server block -var baseHeaders = []http.Header{ +// httpBaseHeaders contains the constant headers set in each HTTP server block +var httpBaseHeaders = []http.Header{ { Name: "Host", Value: "$gw_api_compliant_host", @@ -30,25 +30,31 @@ var baseHeaders = []http.Header{ Name: "X-Forwarded-For", Value: "$proxy_add_x_forwarded_for", }, -} - -// httpBaseHeaders contains the constant headers set in each HTTP server block -var httpBaseHeaders = append(baseHeaders, - http.Header{ + { Name: "Upgrade", Value: "$http_upgrade", }, - http.Header{ + { Name: "Connection", Value: "$connection_upgrade", - }) + }, +} // grpcBaseHeaders contains the constant headers set in each gRPC server block -var grpcBaseHeaders = append(baseHeaders, - http.Header{ +var grpcBaseHeaders = []http.Header{ + { + Name: "Host", + Value: "$gw_api_compliant_host", + }, + { + Name: "X-Forwarded-For", + Value: "$proxy_add_x_forwarded_for", + }, + { Name: "Authority", Value: "$gw_api_compliant_host", - }) + }, +} func executeServers(conf dataplane.Configuration) []executeResult { servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers) From 04b4bdb5f4e9257b80d4e99560b41cd39cd1d999 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 15 May 2024 09:33:40 +0100 Subject: [PATCH 7/8] Add missing test coverage --- .../mode/static/state/graph/grpcroute_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/internal/mode/static/state/graph/grpcroute_test.go b/internal/mode/static/state/graph/grpcroute_test.go index 3de0ea8931..2983127d67 100644 --- a/internal/mode/static/state/graph/grpcroute_test.go +++ b/internal/mode/static/state/graph/grpcroute_test.go @@ -710,3 +710,29 @@ func TestConvertGRPCMatches(t *testing.T) { }) } } + +func TestConvertGRPCFilters(t *testing.T) { + grFilters := []v1alpha2.GRPCRouteFilter{ + { + Type: "RequestHeaderModifier", + RequestHeaderModifier: &v1.HTTPHeaderFilter{ + Remove: []string{"header"}, + }, + }, + { + Type: "RequestMirror", + }, + } + + expectedHTTPFilters := []v1.HTTPRouteFilter{ + { + Type: v1.HTTPRouteFilterRequestHeaderModifier, + RequestHeaderModifier: grFilters[0].RequestHeaderModifier, + }, + } + + g := NewWithT(t) + + httpFilters := convertGRPCFilters(grFilters) + g.Expect(helpers.Diff(expectedHTTPFilters, httpFilters)).To(BeEmpty()) +} From 44f565a131e6d4120a6762f4d9f2712c9229bc90 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 15 May 2024 09:48:29 +0100 Subject: [PATCH 8/8] Update validateGRPCFilter filter path --- internal/mode/static/state/graph/grpcroute.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/state/graph/grpcroute.go b/internal/mode/static/state/graph/grpcroute.go index 477b594a23..d50001422f 100644 --- a/internal/mode/static/state/graph/grpcroute.go +++ b/internal/mode/static/state/graph/grpcroute.go @@ -248,7 +248,7 @@ func validateGRPCFilter( switch filter.Type { case v1alpha2.GRPCRouteFilterRequestHeaderModifier: - return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath) + return validateFilterHeaderModifier(validator, filter.RequestHeaderModifier, filterPath.Child(string(filter.Type))) default: valErr := field.NotSupported( filterPath.Child("type"),