From 3e47a3ef48528a6403dcf3d1a998e4c4e2f556a7 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 1 May 2024 17:51:27 +0100 Subject: [PATCH] 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 5a53cfa259..ef32da599a 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -242,9 +242,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) default: @@ -264,12 +264,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 { @@ -314,12 +313,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 8c963c3abf..6872416205 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -896,42 +896,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 { @@ -939,11 +930,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", @@ -954,13 +942,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", @@ -971,22 +956,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", @@ -997,11 +976,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", @@ -1013,16 +989,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", @@ -1035,7 +1008,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)) }) } @@ -1043,41 +1016,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", }, @@ -1087,25 +1051,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, @@ -1117,14 +1075,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", @@ -1135,14 +1090,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", @@ -1154,17 +1106,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", @@ -1176,7 +1125,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)) }) }