Skip to content

Commit bb3c8da

Browse files
ryanhristovskiryanhristovski
authored
Fix shared=true when no clientSelector, (#6072)
* Fix shared=true when no clientSelector, cleanup filter logic, fix rl descriptor logic Signed-off-by: Ryan Hristovski <[email protected]> * testdata update Signed-off-by: Ryan Hristovski <[email protected]> * Linting, remove unused funcs Signed-off-by: Ryan Hristovski <[email protected]> * fix e2e Signed-off-by: Ryan Hristovski <[email protected]>
1 parent 8f538e7 commit bb3c8da

File tree

6 files changed

+166
-142
lines changed

6 files changed

+166
-142
lines changed

‎internal/xds/translator/ratelimit.go‎

Lines changed: 60 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"bytes"
1010
"fmt"
1111
"net/url"
12+
"sort"
1213
"strconv"
1314
"strings"
1415

@@ -24,6 +25,7 @@ import (
2425
"google.golang.org/protobuf/types/known/durationpb"
2526
"google.golang.org/protobuf/types/known/wrapperspb"
2627
goyaml "gopkg.in/yaml.v3" // nolint: depguard
28+
"k8s.io/apimachinery/pkg/util/sets"
2729

2830
egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
2931
"github.com/envoyproxy/gateway/internal/ir"
@@ -72,50 +74,33 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3.
7274
if irListener == nil || irListener.Routes == nil {
7375
return nil
7476
}
75-
76-
filters := []*hcmv3.HttpFilter{}
77-
created := make(map[string]bool)
78-
77+
domains := sets.New[string]()
7978
for _, route := range irListener.Routes {
8079
if !isValidGlobalRateLimit(route) {
8180
continue
8281
}
83-
84-
hasShared, hasNonShared := false, false
85-
var sharedRuleName string
86-
8782
for _, rule := range route.Traffic.RateLimit.Global.Rules {
8883
if isRuleShared(rule) {
89-
hasShared = true
90-
sharedRuleName = stripRuleIndexSuffix(rule.Name)
91-
} else {
92-
hasNonShared = true
84+
domains.Insert(stripRuleIndexSuffix(rule.Name))
9385
}
9486
}
87+
}
88+
domains.Insert(irListener.Name)
9589

96-
if hasShared {
97-
sharedDomain := sharedRuleName
98-
if !created[sharedDomain] {
99-
filterName := fmt.Sprintf("%s/%s", egv1a1.EnvoyFilterRateLimit.String(), sharedRuleName)
100-
if filter := createRateLimitFilter(t, irListener, sharedDomain, filterName); filter != nil {
101-
filters = append(filters, filter)
102-
}
103-
created[sharedDomain] = true
104-
}
105-
}
90+
// Deterministic order otherwise tests break
91+
domainList := sets.List(domains)
92+
sort.Strings(domainList)
10693

107-
if hasNonShared {
108-
nonSharedDomain := irListener.Name
109-
if !created[nonSharedDomain] {
110-
filterName := egv1a1.EnvoyFilterRateLimit.String()
111-
if filter := createRateLimitFilter(t, irListener, nonSharedDomain, filterName); filter != nil {
112-
filters = append(filters, filter)
113-
}
114-
created[nonSharedDomain] = true
115-
}
94+
var filters []*hcmv3.HttpFilter
95+
for _, domain := range domainList {
96+
filterName := egv1a1.EnvoyFilterRateLimit.String()
97+
if domain != irListener.Name {
98+
filterName += "/" + domain
99+
}
100+
if filter := createRateLimitFilter(t, irListener, domain, filterName); filter != nil {
101+
filters = append(filters, filter)
116102
}
117103
}
118-
119104
return filters
120105
}
121106

@@ -438,13 +423,15 @@ func BuildRateLimitServiceConfig(irListeners []*ir.HTTPListener) []*rlsconfv3.Ra
438423
continue
439424
}
440425

441-
// Process shared rules - add to traffic policy domain
442-
sharedDomain := getDomainSharedName(route)
443-
addRateLimitDescriptor(route, descriptors, sharedDomain, domainDesc, true)
444-
445-
// Process non-shared rules - add to listener-specific domain
446-
listenerDomain := irListener.Name
447-
addRateLimitDescriptor(route, descriptors, listenerDomain, domainDesc, false)
426+
// For each rule, add to the correct domain only
427+
for rIdx, rule := range route.Traffic.RateLimit.Global.Rules {
428+
descriptor := descriptors[rIdx]
429+
domain := irListener.Name
430+
if isRuleShared(rule) {
431+
domain = stripRuleIndexSuffix(rule.Name)
432+
}
433+
addRateLimitDescriptor(route, rule, descriptor, domain, domainDesc)
434+
}
448435
}
449436
}
450437

@@ -488,7 +475,7 @@ func descriptorsEqual(a, b *rlsconfv3.RateLimitDescriptor) bool {
488475
return true
489476
}
490477

491-
// addRateLimitDescriptor adds rate limit descriptors to the domain descriptor map.
478+
// addRateLimitDescriptor adds rate limit descriptors from a single rule to the domain descriptor map.
492479
// Handles both shared and route-specific rate limits.
493480
//
494481
// An example of route descriptor looks like this:
@@ -501,54 +488,48 @@ func descriptorsEqual(a, b *rlsconfv3.RateLimitDescriptor) bool {
501488
// - ...
502489
func addRateLimitDescriptor(
503490
route *ir.HTTPRoute,
504-
serviceDescriptors []*rlsconfv3.RateLimitDescriptor,
491+
rule *ir.RateLimitRule,
492+
descriptor *rlsconfv3.RateLimitDescriptor,
505493
domain string,
506494
domainDescriptors map[string][]*rlsconfv3.RateLimitDescriptor,
507-
includeShared bool,
508495
) {
509-
if !isValidGlobalRateLimit(route) || len(serviceDescriptors) == 0 {
496+
if !isValidGlobalRateLimit(route) || descriptor == nil {
510497
return
511498
}
512499

513-
for i, rule := range route.Traffic.RateLimit.Global.Rules {
514-
if i >= len(serviceDescriptors) || (includeShared != isRuleShared(rule)) {
515-
continue
516-
}
517-
518-
var descriptorKey string
519-
if isRuleShared(rule) {
520-
descriptorKey = rule.Name
521-
} else {
522-
descriptorKey = getRouteDescriptor(route.Name)
523-
}
500+
var descriptorKey string
501+
if isRuleShared(rule) {
502+
descriptorKey = rule.Name
503+
} else {
504+
descriptorKey = getRouteDescriptor(route.Name)
505+
}
524506

525-
// Find or create descriptor in domainDescriptors[domain]
526-
var descriptorRule *rlsconfv3.RateLimitDescriptor
527-
found := false
528-
for _, d := range domainDescriptors[domain] {
529-
if d.Key == descriptorKey {
530-
descriptorRule = d
531-
found = true
532-
break
533-
}
534-
}
535-
if !found {
536-
descriptorRule = &rlsconfv3.RateLimitDescriptor{Key: descriptorKey, Value: descriptorKey}
537-
domainDescriptors[domain] = append(domainDescriptors[domain], descriptorRule)
507+
// Find or create descriptor in domainDescriptors[domain]
508+
var descriptorRule *rlsconfv3.RateLimitDescriptor
509+
found := false
510+
for _, d := range domainDescriptors[domain] {
511+
if d.Key == descriptorKey {
512+
descriptorRule = d
513+
found = true
514+
break
538515
}
516+
}
517+
if !found {
518+
descriptorRule = &rlsconfv3.RateLimitDescriptor{Key: descriptorKey, Value: descriptorKey}
519+
domainDescriptors[domain] = append(domainDescriptors[domain], descriptorRule)
520+
}
539521

540-
// Ensure no duplicate descriptors
541-
alreadyExists := false
542-
for _, existing := range descriptorRule.Descriptors {
543-
if descriptorsEqual(existing, serviceDescriptors[i]) {
544-
alreadyExists = true
545-
break
546-
}
547-
}
548-
if !alreadyExists {
549-
descriptorRule.Descriptors = append(descriptorRule.Descriptors, serviceDescriptors[i])
522+
// Ensure no duplicate descriptors
523+
alreadyExists := false
524+
for _, existing := range descriptorRule.Descriptors {
525+
if descriptorsEqual(existing, descriptor) {
526+
alreadyExists = true
527+
break
550528
}
551529
}
530+
if !alreadyExists {
531+
descriptorRule.Descriptors = append(descriptorRule.Descriptors, descriptor)
532+
}
552533
}
553534

554535
// isSharedRateLimit checks if a route has at least one shared rate limit rule.
@@ -579,16 +560,6 @@ func isRuleShared(rule *ir.RateLimitRule) bool {
579560
return rule != nil && rule.Shared != nil && *rule.Shared
580561
}
581562

582-
// Helper function to check if a specific rule in a route is shared
583-
func isRuleAtIndexShared(route *ir.HTTPRoute, ruleIndex int) bool {
584-
if route == nil || route.Traffic == nil || route.Traffic.RateLimit == nil ||
585-
route.Traffic.RateLimit.Global == nil || len(route.Traffic.RateLimit.Global.Rules) <= ruleIndex || ruleIndex < 0 {
586-
return false
587-
}
588-
589-
return isRuleShared(route.Traffic.RateLimit.Global.Rules[ruleIndex])
590-
}
591-
592563
// Helper function to map a global rule index to a domain-specific rule index
593564
// This ensures that both shared and non-shared rules have indices starting from 0 in their own domains.
594565
func getDomainRuleIndex(rules []*ir.RateLimitRule, globalRuleIdx int, ruleIsShared bool) int {
@@ -710,19 +681,8 @@ func buildRateLimitServiceDescriptors(route *ir.HTTPRoute) []*rlsconfv3.RateLimi
710681
// 3) No Match (apply to all traffic)
711682
if !rule.IsMatchSet() {
712683
pbDesc := new(rlsconfv3.RateLimitDescriptor)
713-
714-
// Determine if we should use the shared rate limit key (rule-based) or a generic route key
715-
if isRuleAtIndexShared(route, rIdx) {
716-
// For shared rate limits, use rule name
717-
pbDesc.Key = rule.Name
718-
pbDesc.Value = rule.Name
719-
} else {
720-
// Use generic key for non-shared rate limits, with prefix for uniqueness
721-
descriptorKey := getRouteRuleDescriptor(domainRuleIdx, -1)
722-
pbDesc.Key = descriptorKey
723-
pbDesc.Value = pbDesc.Key
724-
}
725-
684+
pbDesc.Key = getRouteRuleDescriptor(domainRuleIdx, -1)
685+
pbDesc.Value = getRouteRuleDescriptor(domainRuleIdx, -1)
726686
head = pbDesc
727687
cur = head
728688
}
@@ -735,11 +695,6 @@ func buildRateLimitServiceDescriptors(route *ir.HTTPRoute) []*rlsconfv3.RateLimi
735695
return pbDescriptors
736696
}
737697

738-
// getDomainSharedName returns the shared domain (stripped policy name), stripRuleIndexSuffix is used to remove the rule index suffix.
739-
func getDomainSharedName(route *ir.HTTPRoute) string {
740-
return stripRuleIndexSuffix(route.Traffic.RateLimit.Global.Rules[0].Name)
741-
}
742-
743698
func getRouteRuleDescriptor(ruleIndex, matchIndex int) string {
744699
return "rule-" + strconv.Itoa(ruleIndex) + "-match-" + strconv.Itoa(matchIndex)
745700
}

‎internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,30 +14,30 @@
1414
initialStreamWindowSize: 65536
1515
maxConcurrentStreams: 100
1616
httpFilters:
17-
- name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
17+
- name: envoy.filters.http.ratelimit
1818
typedConfig:
1919
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
20-
domain: test-namespace/test-policy-1
20+
domain: first-listener
2121
enableXRatelimitHeaders: DRAFT_VERSION_03
2222
rateLimitService:
2323
grpcService:
2424
envoyGrpc:
2525
clusterName: ratelimit_cluster
2626
transportApiVersion: V3
27-
- name: envoy.filters.http.ratelimit/test-namespace/test-policy-2
27+
- name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
2828
typedConfig:
2929
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
30-
domain: test-namespace/test-policy-2
30+
domain: test-namespace/test-policy-1
3131
enableXRatelimitHeaders: DRAFT_VERSION_03
3232
rateLimitService:
3333
grpcService:
3434
envoyGrpc:
3535
clusterName: ratelimit_cluster
3636
transportApiVersion: V3
37-
- name: envoy.filters.http.ratelimit
37+
- name: envoy.filters.http.ratelimit/test-namespace/test-policy-2
3838
typedConfig:
3939
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
40-
domain: first-listener
40+
domain: test-namespace/test-policy-2
4141
enableXRatelimitHeaders: DRAFT_VERSION_03
4242
rateLimitService:
4343
grpcService:

‎internal/xds/translator/testdata/out/xds-ir/ratelimit-multi-global-shared.listeners.yaml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,16 @@
1414
initialStreamWindowSize: 65536
1515
maxConcurrentStreams: 100
1616
httpFilters:
17+
- name: envoy.filters.http.ratelimit
18+
typedConfig:
19+
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
20+
domain: first-listener
21+
enableXRatelimitHeaders: DRAFT_VERSION_03
22+
rateLimitService:
23+
grpcService:
24+
envoyGrpc:
25+
clusterName: ratelimit_cluster
26+
transportApiVersion: V3
1727
- name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
1828
typedConfig:
1929
'@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit

‎test/e2e/e2e_test.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,15 @@ func TestE2E(t *testing.T) {
6161
)
6262
}
6363

64+
// TODO: make these tests work in GatewayNamespaceMode
65+
if tests.IsGatewayNamespaceMode() {
66+
skipTests = append(skipTests,
67+
tests.HTTPWasmTest.ShortName,
68+
tests.OCIWasmTest.ShortName,
69+
tests.ZoneAwareRoutingTest.ShortName,
70+
)
71+
}
72+
6473
cSuite, err := suite.NewConformanceTestSuite(suite.ConformanceOptions{
6574
Client: c,
6675
RestConfig: cfg,

‎test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,10 @@ spec:
9393
type: Global
9494
global:
9595
rules:
96+
- limit:
97+
requests: 21
98+
unit: Hour
99+
shared: true
96100
- clientSelectors:
97101
- headers:
98102
- name: x-user-id

0 commit comments

Comments
 (0)