Reject ratelimit configs that are missing required values.

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.

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")

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,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 +72,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 +89,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 +110,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 +144,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 +165,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 {

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_{