From 2563c03d347ae2e7c3ea2c32732952b7014d2cf4 Mon Sep 17 00:00:00 2001 From: Ethan Date: Fri, 11 Aug 2023 15:38:22 -0400 Subject: [PATCH 1/4] plugin updates and test --- changelog/v1.16.0-beta1/rl-nil-checks.yaml | 5 + projects/gloo/pkg/plugins/ratelimit/plugin.go | 8 +- .../gloo/pkg/plugins/ratelimit/raw_util.go | 68 +++++++++++--- .../pkg/plugins/ratelimit/raw_util_test.go | 93 +++++++++++++++++-- 4 files changed, 151 insertions(+), 23 deletions(-) create mode 100644 changelog/v1.16.0-beta1/rl-nil-checks.yaml diff --git a/changelog/v1.16.0-beta1/rl-nil-checks.yaml b/changelog/v1.16.0-beta1/rl-nil-checks.yaml new file mode 100644 index 00000000000..364c43b8478 --- /dev/null +++ b/changelog/v1.16.0-beta1/rl-nil-checks.yaml @@ -0,0 +1,5 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/gloo/issues/8573 + resolvesIssue: false + description: Check for empty values in fields that are required by Envoy for RateLimitActions. diff --git a/projects/gloo/pkg/plugins/ratelimit/plugin.go b/projects/gloo/pkg/plugins/ratelimit/plugin.go index 329bd833fac..c5c3fbed47f 100644 --- a/projects/gloo/pkg/plugins/ratelimit/plugin.go +++ b/projects/gloo/pkg/plugins/ratelimit/plugin.go @@ -75,7 +75,9 @@ func (p *plugin) ProcessVirtualHost( if newRateLimits := in.GetOptions().GetRatelimit().GetRateLimits(); len(newRateLimits) > 0 { serverSettings := p.getServerSettingsForListener(params.HttpListener) rateLimitStage := GetRateLimitStageForServerSettings(serverSettings) - out.RateLimits = toEnvoyRateLimits(params.Ctx, newRateLimits, rateLimitStage) + var err error + out.RateLimits, err = toEnvoyRateLimits(params.Ctx, newRateLimits, rateLimitStage) + return err } return nil } @@ -85,8 +87,10 @@ func (p *plugin) ProcessRoute(params plugins.RouteParams, in *v1.Route, out *env if ra := out.GetRoute(); ra != nil { serverSettings := p.getServerSettingsForListener(params.HttpListener) rateLimitStage := GetRateLimitStageForServerSettings(serverSettings) - ra.RateLimits = toEnvoyRateLimits(params.Ctx, rateLimits.GetRateLimits(), rateLimitStage) + var err error + ra.RateLimits, err = toEnvoyRateLimits(params.Ctx, rateLimits.GetRateLimits(), rateLimitStage) ra.IncludeVhRateLimits = &wrappers.BoolValue{Value: rateLimits.GetIncludeVhRateLimits()} + return err } else { // TODO(yuval-k): maybe return nil here instead and just log a warning? return fmt.Errorf("cannot apply rate limits without a route action") diff --git a/projects/gloo/pkg/plugins/ratelimit/raw_util.go b/projects/gloo/pkg/plugins/ratelimit/raw_util.go index 1b5df3855e3..601d479d4bd 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util.go @@ -2,6 +2,8 @@ package ratelimit import ( "context" + "github.com/hashicorp/go-multierror" + "github.com/rotisserie/eris" envoy_type_metadata_v3 "github.com/envoyproxy/go-control-plane/envoy/type/metadata/v3" @@ -16,30 +18,44 @@ import ( // to treat those descriptors differently (for the set-style rate-limit API) const SetDescriptorValue = "solo.setDescriptor.uniqueValue" +// TODO: this error and handling around it can be removed if we eventually generate CRDs with required values from the ratelimit proto. +func missingFieldError(field string) error { + return eris.Errorf("Missing required field in ratelimit action %s", field) +} + func toEnvoyRateLimits( ctx context.Context, actions []*solo_rl.RateLimitActions, stage uint32, -) []*envoy_config_route_v3.RateLimit { +) ([]*envoy_config_route_v3.RateLimit, error) { var ret []*envoy_config_route_v3.RateLimit + var allErrors error for _, action := range actions { if len(action.GetActions()) != 0 { rl := &envoy_config_route_v3.RateLimit{ Stage: &wrappers.UInt32Value{Value: stage}, } - rl.Actions = ConvertActions(ctx, action.GetActions()) + var err error + rl.Actions, err = ConvertActions(ctx, action.GetActions()) + if err != nil { + allErrors = multierror.Append(allErrors, err) + } ret = append(ret, rl) } } - return ret + return ret, allErrors } -func ConvertActions(ctx context.Context, actions []*solo_rl.Action) []*envoy_config_route_v3.RateLimit_Action { +func ConvertActions(ctx context.Context, actions []*solo_rl.Action) ([]*envoy_config_route_v3.RateLimit_Action, error) { var retActions []*envoy_config_route_v3.RateLimit_Action var isSetStyle bool + var allErrors error for _, action := range actions { - convertedAction := convertAction(ctx, action) + convertedAction, err := convertAction(ctx, action) + if err != nil { + allErrors = multierror.Append(allErrors, err) + } if convertedAction.GetGenericKey().GetDescriptorValue() == SetDescriptorValue { isSetStyle = true } @@ -55,10 +71,10 @@ func ConvertActions(ctx context.Context, actions []*solo_rl.Action) []*envoy_con } } - return retActions + return retActions, allErrors } -func convertAction(ctx context.Context, action *solo_rl.Action) *envoy_config_route_v3.RateLimit_Action { +func convertAction(ctx context.Context, action *solo_rl.Action) (*envoy_config_route_v3.RateLimit_Action, error) { var retAction envoy_config_route_v3.RateLimit_Action switch specificAction := action.GetActionSpecifier().(type) { @@ -72,10 +88,18 @@ func convertAction(ctx context.Context, action *solo_rl.Action) *envoy_config_ro } case *solo_rl.Action_RequestHeaders_: + headerName := specificAction.RequestHeaders.GetHeaderName() + if headerName == "" { + return nil, missingFieldError("HeaderName") + } + descriptorKey := specificAction.RequestHeaders.GetDescriptorKey() + if descriptorKey == "" { + return nil, missingFieldError("DescriptorKey") + } retAction.ActionSpecifier = &envoy_config_route_v3.RateLimit_Action_RequestHeaders_{ RequestHeaders: &envoy_config_route_v3.RateLimit_Action_RequestHeaders{ - HeaderName: specificAction.RequestHeaders.GetHeaderName(), - DescriptorKey: specificAction.RequestHeaders.GetDescriptorKey(), + HeaderName: headerName, + DescriptorKey: descriptorKey, }, } @@ -85,17 +109,25 @@ func convertAction(ctx context.Context, action *solo_rl.Action) *envoy_config_ro } case *solo_rl.Action_GenericKey_: + descriptorValue := specificAction.GenericKey.GetDescriptorValue() + if descriptorValue == "" { + return nil, missingFieldError("DescriptorValue") + } retAction.ActionSpecifier = &envoy_config_route_v3.RateLimit_Action_GenericKey_{ GenericKey: &envoy_config_route_v3.RateLimit_Action_GenericKey{ - DescriptorValue: specificAction.GenericKey.GetDescriptorValue(), + DescriptorValue: descriptorValue, }, } case *solo_rl.Action_HeaderValueMatch_: + descriptorValue := specificAction.HeaderValueMatch.GetDescriptorValue() + if descriptorValue == "" { + return nil, missingFieldError("DescriptorValue") + } retAction.ActionSpecifier = &envoy_config_route_v3.RateLimit_Action_HeaderValueMatch_{ HeaderValueMatch: &envoy_config_route_v3.RateLimit_Action_HeaderValueMatch{ ExpectMatch: specificAction.HeaderValueMatch.GetExpectMatch(), - DescriptorValue: specificAction.HeaderValueMatch.GetDescriptorValue(), + DescriptorValue: descriptorValue, Headers: convertHeaders(ctx, specificAction.HeaderValueMatch.GetHeaders()), }, } @@ -111,11 +143,19 @@ func convertAction(ctx context.Context, action *solo_rl.Action) *envoy_config_ro }) } + descriptorKey := specificAction.Metadata.GetDescriptorKey() + if descriptorKey == "" { + return nil, missingFieldError("DescriptorKey") + } + metadataKey := specificAction.Metadata.GetMetadataKey().GetKey() + if metadataKey == "" { + return nil, missingFieldError("MetadataKey") + } retAction.ActionSpecifier = &envoy_config_route_v3.RateLimit_Action_Metadata{ Metadata: &envoy_config_route_v3.RateLimit_Action_MetaData{ - DescriptorKey: specificAction.Metadata.GetDescriptorKey(), + DescriptorKey: descriptorKey, MetadataKey: &envoy_type_metadata_v3.MetadataKey{ - Key: specificAction.Metadata.GetMetadataKey().GetKey(), + Key: metadataKey, Path: envoyPathSegments, }, DefaultValue: specificAction.Metadata.GetDefaultValue(), @@ -124,7 +164,7 @@ func convertAction(ctx context.Context, action *solo_rl.Action) *envoy_config_ro } } - return &retAction + return &retAction, nil } func convertHeaders(ctx context.Context, headers []*solo_rl.Action_HeaderValueMatch_HeaderMatcher) []*envoy_config_route_v3.HeaderMatcher { diff --git a/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go b/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go index 88994cb45a7..899979ed14b 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go @@ -57,8 +57,8 @@ var _ = Describe("RawUtil", func() { DescribeTable( "should convert protos to the same thing till we properly vendor them", func(actions []*gloorl.Action) { - out := ConvertActions(nil, actions) - + out, err := ConvertActions(nil, actions) + Expect(err).NotTo(HaveOccurred()) Expect(len(actions)).To(Equal(len(out))) for i := range actions { golangjson := golangjsonpb.Marshaler{} @@ -168,7 +168,86 @@ var _ = Describe("RawUtil", func() { }, ), ) - + DescribeTable("Errors on missing required fields", func(actions []*gloorl.Action) { + _, err := ConvertActions(nil, actions) + Expect(err).To(MatchError(ContainSubstring("Missing required field in ratelimit action"))) + }, + Entry("Missing descriptorValue in genericKey", + []*gloorl.Action{{ + ActionSpecifier: &gloorl.Action_GenericKey_{ + GenericKey: &gloorl.Action_GenericKey{}, + }, + }}, + ), + Entry("Missing headername in request headers", + []*gloorl.Action{{ + ActionSpecifier: &gloorl.Action_RequestHeaders_{ + RequestHeaders: &gloorl.Action_RequestHeaders{ + DescriptorKey: "key", + }, + }, + }}, + ), + Entry("Missing descriptor key in request headers", + []*gloorl.Action{{ + ActionSpecifier: &gloorl.Action_RequestHeaders_{ + RequestHeaders: &gloorl.Action_RequestHeaders{ + HeaderName: "header", + }, + }, + }}, + ), + Entry("Missing descriptorValue in headerValueMatch", + []*gloorl.Action{ + { + ActionSpecifier: &gloorl.Action_HeaderValueMatch_{ + HeaderValueMatch: &gloorl.Action_HeaderValueMatch{ + ExpectMatch: &wrappers.BoolValue{Value: true}, + Headers: hm, + }, + }, + }}, + ), + Entry("Missing DescriptorKey in ActionMetadata", + []*gloorl.Action{{ + ActionSpecifier: &gloorl.Action_Metadata{ + Metadata: &gloorl.Action_MetaData{ + MetadataKey: &gloorl.Action_MetaData_MetadataKey{ + Key: "io.solo.some.filter", + Path: []*gloorl.Action_MetaData_MetadataKey_PathSegment{ + { + Segment: &gloorl.Action_MetaData_MetadataKey_PathSegment_Key{ + Key: "foo", + }, + }, + }, + }, + DefaultValue: "nothing", + Source: gloorl.Action_MetaData_ROUTE_ENTRY, + }, + }, + }}, + ), + Entry("Missing metadataKey_key in ActionMetadata", + []*gloorl.Action{{ + ActionSpecifier: &gloorl.Action_Metadata{ + Metadata: &gloorl.Action_MetaData{ + DescriptorKey: "some-key", + MetadataKey: &gloorl.Action_MetaData_MetadataKey{ + Path: []*gloorl.Action_MetaData_MetadataKey_PathSegment{ + { + Segment: &gloorl.Action_MetaData_MetadataKey_PathSegment_Key{ + Key: "foo", + }, + }, + }, + }, + DefaultValue: "nothing", + Source: gloorl.Action_MetaData_ROUTE_ENTRY, + }, + }, + }}), + ) // Needs to be separate because the yaml is no longer compatible Context("special cases - not a straight conversion", func() { @@ -192,8 +271,8 @@ var _ = Describe("RawUtil", func() { }, } - out := ConvertActions(nil, actions) - + out, err := ConvertActions(nil, actions) + Expect(err).NotTo(HaveOccurred()) expected := []*envoy_config_route_v3.RateLimit_Action{ { ActionSpecifier: &envoy_config_route_v3.RateLimit_Action_HeaderValueMatch_{ @@ -253,8 +332,8 @@ var _ = Describe("RawUtil", func() { }, } - out := ConvertActions(nil, actions) - + out, err := ConvertActions(nil, actions) + Expect(err).NotTo(HaveOccurred()) expected := []*envoy_config_route_v3.RateLimit_Action{ { ActionSpecifier: &envoy_config_route_v3.RateLimit_Action_GenericKey_{ From 1b5b2f966a27d07c0d5e992d05c4ab86c269bf14 Mon Sep 17 00:00:00 2001 From: Ethan Date: Fri, 11 Aug 2023 16:11:17 -0400 Subject: [PATCH 2/4] gofmt --- projects/gloo/pkg/plugins/ratelimit/raw_util.go | 1 + 1 file changed, 1 insertion(+) diff --git a/projects/gloo/pkg/plugins/ratelimit/raw_util.go b/projects/gloo/pkg/plugins/ratelimit/raw_util.go index 601d479d4bd..6f4fac2c7b4 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util.go @@ -2,6 +2,7 @@ package ratelimit import ( "context" + "github.com/hashicorp/go-multierror" "github.com/rotisserie/eris" From 306d8bb4ceda92dbf73cd6989433cdbef7560a0f Mon Sep 17 00:00:00 2001 From: Ethan Date: Mon, 14 Aug 2023 15:18:41 -0400 Subject: [PATCH 3/4] review comments, do not append invalid action to returned list --- projects/gloo/pkg/plugins/ratelimit/raw_util.go | 3 +++ projects/gloo/pkg/plugins/ratelimit/raw_util_test.go | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/projects/gloo/pkg/plugins/ratelimit/raw_util.go b/projects/gloo/pkg/plugins/ratelimit/raw_util.go index 6f4fac2c7b4..c46f4a3c03a 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util.go @@ -20,6 +20,7 @@ import ( const SetDescriptorValue = "solo.setDescriptor.uniqueValue" // TODO: this error and handling around it can be removed if we eventually generate CRDs with required values from the ratelimit proto. +// Issue: https://github.com/solo-io/gloo/issues/8580 func missingFieldError(field string) error { return eris.Errorf("Missing required field in ratelimit action %s", field) } @@ -47,6 +48,7 @@ func toEnvoyRateLimits( return ret, allErrors } +// ConvertActions generates Envoy RateLimit_Actions from the solo-apis API. It checks that all required fields are set. func ConvertActions(ctx context.Context, actions []*solo_rl.Action) ([]*envoy_config_route_v3.RateLimit_Action, error) { var retActions []*envoy_config_route_v3.RateLimit_Action @@ -56,6 +58,7 @@ func ConvertActions(ctx context.Context, actions []*solo_rl.Action) ([]*envoy_co convertedAction, err := convertAction(ctx, action) if err != nil { allErrors = multierror.Append(allErrors, err) + continue } if convertedAction.GetGenericKey().GetDescriptorValue() == SetDescriptorValue { isSetStyle = true diff --git a/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go b/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go index 899979ed14b..28466d8634e 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util_test.go @@ -169,7 +169,8 @@ var _ = Describe("RawUtil", func() { ), ) DescribeTable("Errors on missing required fields", func(actions []*gloorl.Action) { - _, err := ConvertActions(nil, actions) + envoyActions, err := ConvertActions(nil, actions) + Expect(envoyActions).To(HaveLen(0)) Expect(err).To(MatchError(ContainSubstring("Missing required field in ratelimit action"))) }, Entry("Missing descriptorValue in genericKey", From ace47d0ed3bd5ad027cb33d633142775533c4964 Mon Sep 17 00:00:00 2001 From: Ethan Date: Mon, 14 Aug 2023 15:51:59 -0400 Subject: [PATCH 4/4] link to envoy docs in changelog --- changelog/v1.16.0-beta1/rl-nil-checks.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/changelog/v1.16.0-beta1/rl-nil-checks.yaml b/changelog/v1.16.0-beta1/rl-nil-checks.yaml index 364c43b8478..9e09bb221bd 100644 --- a/changelog/v1.16.0-beta1/rl-nil-checks.yaml +++ b/changelog/v1.16.0-beta1/rl-nil-checks.yaml @@ -2,4 +2,6 @@ changelog: - type: FIX issueLink: https://github.com/solo-io/gloo/issues/8573 resolvesIssue: false - description: Check for empty values in fields that are required by Envoy for RateLimitActions. + description: > + Check for empty values in fields that are required by Envoy for RateLimitActions. Previously invalid config would be passed to Envoy. + API reference: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-ratelimit-action.