Skip to content

Commit

Permalink
1.13 backport to reject ratelimit configs that are missing required v… (
Browse files Browse the repository at this point in the history 
#8584)

Also fix GHA to push to solo-apis on release.
  • Loading branch information
@elcasteel
elcasteel authored Aug 15, 2023
1 parent f249eca commit d0e1d5f
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 24 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/push-solo-apis-branch.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions changelog/v1.13.25/rl-nil-checks.yaml
View file
Original file line number Diff line number Diff line change
@@ -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.
8 changes: 6 additions & 2 deletions projects/gloo/pkg/plugins/ratelimit/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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")
Expand Down
74 changes: 60 additions & 14 deletions projects/gloo/pkg/plugins/ratelimit/raw_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
}
Expand All @@ -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) {
Expand All @@ -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,
},
}

Expand All @@ -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()),
},
}
Expand All @@ -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 {
Expand Down
94 changes: 87 additions & 7 deletions projects/gloo/pkg/plugins/ratelimit/raw_util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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() {

Expand All @@ -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_{
Expand Down Expand Up @@ -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_{
Expand Down

0 comments on commit d0e1d5f

Please sign in to comment.