From d0e1d5f3b3a88beef56c3ff53d9245644a30ec54 Mon Sep 17 00:00:00 2001 From: Ethan Casteel Date: Tue, 15 Aug 2023 16:12:18 -0400 Subject: [PATCH] =?UTF-8?q?1.13=20backport=20to=20reject=20ratelimit=20con?= =?UTF-8?q?figs=20that=20are=20missing=20required=20v=E2=80=A6=20(#8584)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also fix GHA to push to solo-apis on release. --- .github/workflows/push-solo-apis-branch.yaml | 2 +- changelog/v1.13.25/rl-nil-checks.yaml | 9 ++ projects/gloo/pkg/plugins/ratelimit/plugin.go | 8 +- .../gloo/pkg/plugins/ratelimit/raw_util.go | 74 ++++++++++++--- .../pkg/plugins/ratelimit/raw_util_test.go | 94 +++++++++++++++++-- 5 files changed, 163 insertions(+), 24 deletions(-) create mode 100644 changelog/v1.13.25/rl-nil-checks.yaml diff --git a/.github/workflows/push-solo-apis-branch.yaml b/.github/workflows/push-solo-apis-branch.yaml index e99733452eb..c13a41881bd 100644 --- a/.github/workflows/push-solo-apis-branch.yaml +++ b/.github/workflows/push-solo-apis-branch.yaml @@ -83,7 +83,7 @@ jobs: path: gloo ref: ${{ env.RELEASE_TAG_NAME }} - name: Prep Go Runner - uses: ./.github/workflows/composite-actions/prep-go-runner + uses: ./gloo/.github/workflows/composite-actions/prep-go-runner with: working-directory: gloo - name: Install Protoc diff --git a/changelog/v1.13.25/rl-nil-checks.yaml b/changelog/v1.13.25/rl-nil-checks.yaml new file mode 100644 index 00000000000..9a4d2fa884a --- /dev/null +++ b/changelog/v1.13.25/rl-nil-checks.yaml @@ -0,0 +1,9 @@ +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. 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. + - type: NON_USER_FACING + description: Provide correct path for sub-actions in the action to create solo-apis PRs. 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..0ab559b2357 100644 --- a/projects/gloo/pkg/plugins/ratelimit/raw_util.go +++ b/projects/gloo/pkg/plugins/ratelimit/raw_util.go @@ -3,6 +3,9 @@ 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" envoy_config_route_v3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" @@ -16,30 +19,47 @@ 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. +// 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) +} + 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 { +// 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 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) + continue + } if convertedAction.GetGenericKey().GetDescriptorValue() == SetDescriptorValue { isSetStyle = true } @@ -55,10 +75,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 +92,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 +113,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,20 +147,30 @@ 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(), Source: envoy_config_route_v3.RateLimit_Action_MetaData_Source(specificAction.Metadata.GetSource()), }, } + default: + return nil, eris.Errorf("Unknown action type") } - 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 bdd8b9de8fb..b1cace7ef63 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,87 @@ var _ = Describe("RawUtil", func() { }, ), ) - + DescribeTable("Errors on missing required fields", func(actions []*gloorl.Action) { + 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", + []*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 +272,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 +333,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_{