Fix shared=true when no clientSelector,

internal/xds/translator/ratelimit.go

@@ -9,6 +9,7 @@ import (
 	"bytes"
 	"fmt"
 	"net/url"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -24,6 +25,7 @@ import (
 	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/wrapperspb"
 	goyaml "gopkg.in/yaml.v3" // nolint: depguard
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 	"github.com/envoyproxy/gateway/internal/ir"
@@ -72,50 +74,33 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3.
 	if irListener == nil || irListener.Routes == nil {
 		return nil
 	}
-
-	filters := []*hcmv3.HttpFilter{}
-	created := make(map[string]bool)
-
+	domains := sets.New[string]()
 	for _, route := range irListener.Routes {
 		if !isValidGlobalRateLimit(route) {
 			continue
 		}
-
-		hasShared, hasNonShared := false, false
-		var sharedRuleName string
-
 		for _, rule := range route.Traffic.RateLimit.Global.Rules {
 			if isRuleShared(rule) {
-				hasShared = true
-				sharedRuleName = stripRuleIndexSuffix(rule.Name)
-			} else {
-				hasNonShared = true
+				domains.Insert(stripRuleIndexSuffix(rule.Name))
 			}
 		}
+	}
+	domains.Insert(irListener.Name)
 
-		if hasShared {
-			sharedDomain := sharedRuleName
-			if !created[sharedDomain] {
-				filterName := fmt.Sprintf("%s/%s", egv1a1.EnvoyFilterRateLimit.String(), sharedRuleName)
-				if filter := createRateLimitFilter(t, irListener, sharedDomain, filterName); filter != nil {
-					filters = append(filters, filter)
-				}
-				created[sharedDomain] = true
-			}
-		}
+	// Deterministic order otherwise tests break
+	domainList := sets.List(domains)
+	sort.Strings(domainList)
 
-		if hasNonShared {
-			nonSharedDomain := irListener.Name
-			if !created[nonSharedDomain] {
-				filterName := egv1a1.EnvoyFilterRateLimit.String()
-				if filter := createRateLimitFilter(t, irListener, nonSharedDomain, filterName); filter != nil {
-					filters = append(filters, filter)
-				}
-				created[nonSharedDomain] = true
-			}
+	var filters []*hcmv3.HttpFilter
+	for _, domain := range domainList {
+		filterName := egv1a1.EnvoyFilterRateLimit.String()
+		if domain != irListener.Name {
+			filterName += "/" + domain
+		}
+		if filter := createRateLimitFilter(t, irListener, domain, filterName); filter != nil {
+			filters = append(filters, filter)
 		}
 	}
-
 	return filters
 }
 
@@ -438,13 +423,15 @@ func BuildRateLimitServiceConfig(irListeners []*ir.HTTPListener) []*rlsconfv3.Ra
 				continue
 			}
 
-			// Process shared rules - add to traffic policy domain
-			sharedDomain := getDomainSharedName(route)
-			addRateLimitDescriptor(route, descriptors, sharedDomain, domainDesc, true)
-
-			// Process non-shared rules - add to listener-specific domain
-			listenerDomain := irListener.Name
-			addRateLimitDescriptor(route, descriptors, listenerDomain, domainDesc, false)
+			// For each rule, add to the correct domain only
+			for rIdx, rule := range route.Traffic.RateLimit.Global.Rules {
+				descriptor := descriptors[rIdx]
+				domain := irListener.Name
+				if isRuleShared(rule) {
+					domain = stripRuleIndexSuffix(rule.Name)
+				}
+				addRateLimitDescriptor(route, rule, descriptor, domain, domainDesc)
+			}
 		}
 	}
 
@@ -488,7 +475,7 @@ func descriptorsEqual(a, b *rlsconfv3.RateLimitDescriptor) bool {
 	return true
 }
 
-// addRateLimitDescriptor adds rate limit descriptors to the domain descriptor map.
+// addRateLimitDescriptor adds rate limit descriptors from a single rule to the domain descriptor map.
 // Handles both shared and route-specific rate limits.
 //
 // An example of route descriptor looks like this:
@@ -501,54 +488,48 @@ func descriptorsEqual(a, b *rlsconfv3.RateLimitDescriptor) bool {
 //   - ...
 func addRateLimitDescriptor(
 	route *ir.HTTPRoute,
-	serviceDescriptors []*rlsconfv3.RateLimitDescriptor,
+	rule *ir.RateLimitRule,
+	descriptor *rlsconfv3.RateLimitDescriptor,
 	domain string,
 	domainDescriptors map[string][]*rlsconfv3.RateLimitDescriptor,
-	includeShared bool,
 ) {
-	if !isValidGlobalRateLimit(route) || len(serviceDescriptors) == 0 {
+	if !isValidGlobalRateLimit(route) || descriptor == nil {
 		return
 	}
 
-	for i, rule := range route.Traffic.RateLimit.Global.Rules {
-		if i >= len(serviceDescriptors) || (includeShared != isRuleShared(rule)) {
-			continue
-		}
-
-		var descriptorKey string
-		if isRuleShared(rule) {
-			descriptorKey = rule.Name
-		} else {
-			descriptorKey = getRouteDescriptor(route.Name)
-		}
+	var descriptorKey string
+	if isRuleShared(rule) {
+		descriptorKey = rule.Name
+	} else {
+		descriptorKey = getRouteDescriptor(route.Name)
+	}
 
-		// Find or create descriptor in domainDescriptors[domain]
-		var descriptorRule *rlsconfv3.RateLimitDescriptor
-		found := false
-		for _, d := range domainDescriptors[domain] {
-			if d.Key == descriptorKey {
-				descriptorRule = d
-				found = true
-				break
-			}
-		}
-		if !found {
-			descriptorRule = &rlsconfv3.RateLimitDescriptor{Key: descriptorKey, Value: descriptorKey}
-			domainDescriptors[domain] = append(domainDescriptors[domain], descriptorRule)
+	// Find or create descriptor in domainDescriptors[domain]
+	var descriptorRule *rlsconfv3.RateLimitDescriptor
+	found := false
+	for _, d := range domainDescriptors[domain] {
+		if d.Key == descriptorKey {
+			descriptorRule = d
+			found = true
+			break
 		}
+	}
+	if !found {
+		descriptorRule = &rlsconfv3.RateLimitDescriptor{Key: descriptorKey, Value: descriptorKey}
+		domainDescriptors[domain] = append(domainDescriptors[domain], descriptorRule)
+	}
 
-		// Ensure no duplicate descriptors
-		alreadyExists := false
-		for _, existing := range descriptorRule.Descriptors {
-			if descriptorsEqual(existing, serviceDescriptors[i]) {
-				alreadyExists = true
-				break
-			}
-		}
-		if !alreadyExists {
-			descriptorRule.Descriptors = append(descriptorRule.Descriptors, serviceDescriptors[i])
+	// Ensure no duplicate descriptors
+	alreadyExists := false
+	for _, existing := range descriptorRule.Descriptors {
+		if descriptorsEqual(existing, descriptor) {
+			alreadyExists = true
+			break
 		}
 	}
+	if !alreadyExists {
+		descriptorRule.Descriptors = append(descriptorRule.Descriptors, descriptor)
+	}
 }
 
 // isSharedRateLimit checks if a route has at least one shared rate limit rule.
@@ -579,16 +560,6 @@ func isRuleShared(rule *ir.RateLimitRule) bool {
 	return rule != nil && rule.Shared != nil && *rule.Shared
 }
 
-// Helper function to check if a specific rule in a route is shared
-func isRuleAtIndexShared(route *ir.HTTPRoute, ruleIndex int) bool {
-	if route == nil || route.Traffic == nil || route.Traffic.RateLimit == nil ||
-		route.Traffic.RateLimit.Global == nil || len(route.Traffic.RateLimit.Global.Rules) <= ruleIndex || ruleIndex < 0 {
-		return false
-	}
-
-	return isRuleShared(route.Traffic.RateLimit.Global.Rules[ruleIndex])
-}
-
 // Helper function to map a global rule index to a domain-specific rule index
 // This ensures that both shared and non-shared rules have indices starting from 0 in their own domains.
 func getDomainRuleIndex(rules []*ir.RateLimitRule, globalRuleIdx int, ruleIsShared bool) int {
@@ -710,19 +681,8 @@ func buildRateLimitServiceDescriptors(route *ir.HTTPRoute) []*rlsconfv3.RateLimi
 		// 3) No Match (apply to all traffic)
 		if !rule.IsMatchSet() {
 			pbDesc := new(rlsconfv3.RateLimitDescriptor)
-
-			// Determine if we should use the shared rate limit key (rule-based) or a generic route key
-			if isRuleAtIndexShared(route, rIdx) {
-				// For shared rate limits, use rule name
-				pbDesc.Key = rule.Name
-				pbDesc.Value = rule.Name
-			} else {
-				// Use generic key for non-shared rate limits, with prefix for uniqueness
-				descriptorKey := getRouteRuleDescriptor(domainRuleIdx, -1)
-				pbDesc.Key = descriptorKey
-				pbDesc.Value = pbDesc.Key
-			}
-
+			pbDesc.Key = getRouteRuleDescriptor(domainRuleIdx, -1)
+			pbDesc.Value = getRouteRuleDescriptor(domainRuleIdx, -1)
 			head = pbDesc
 			cur = head
 		}
@@ -735,11 +695,6 @@ func buildRateLimitServiceDescriptors(route *ir.HTTPRoute) []*rlsconfv3.RateLimi
 	return pbDescriptors
 }
 
-// getDomainSharedName returns the shared domain (stripped policy name), stripRuleIndexSuffix is used to remove the rule index suffix.
-func getDomainSharedName(route *ir.HTTPRoute) string {
-	return stripRuleIndexSuffix(route.Traffic.RateLimit.Global.Rules[0].Name)
-}
-
 func getRouteRuleDescriptor(ruleIndex, matchIndex int) string {
 	return "rule-" + strconv.Itoa(ruleIndex) + "-match-" + strconv.Itoa(matchIndex)
 }

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

@@ -14,30 +14,30 @@
           initialStreamWindowSize: 65536
           maxConcurrentStreams: 100
         httpFilters:
-        - name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
+        - name: envoy.filters.http.ratelimit
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
-            domain: test-namespace/test-policy-1
+            domain: first-listener
             enableXRatelimitHeaders: DRAFT_VERSION_03
             rateLimitService:
               grpcService:
                 envoyGrpc:
                   clusterName: ratelimit_cluster
               transportApiVersion: V3
-        - name: envoy.filters.http.ratelimit/test-namespace/test-policy-2
+        - name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
-            domain: test-namespace/test-policy-2
+            domain: test-namespace/test-policy-1
             enableXRatelimitHeaders: DRAFT_VERSION_03
             rateLimitService:
               grpcService:
                 envoyGrpc:
                   clusterName: ratelimit_cluster
               transportApiVersion: V3
-        - name: envoy.filters.http.ratelimit
+        - name: envoy.filters.http.ratelimit/test-namespace/test-policy-2
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
-            domain: first-listener
+            domain: test-namespace/test-policy-2
             enableXRatelimitHeaders: DRAFT_VERSION_03
             rateLimitService:
               grpcService:

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

@@ -14,6 +14,16 @@
           initialStreamWindowSize: 65536
           maxConcurrentStreams: 100
         httpFilters:
+        - name: envoy.filters.http.ratelimit
+          typedConfig:
+            '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
+            domain: first-listener
+            enableXRatelimitHeaders: DRAFT_VERSION_03
+            rateLimitService:
+              grpcService:
+                envoyGrpc:
+                  clusterName: ratelimit_cluster
+              transportApiVersion: V3
         - name: envoy.filters.http.ratelimit/test-namespace/test-policy-1
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit

test/e2e/e2e_test.go

@@ -61,6 +61,15 @@ func TestE2E(t *testing.T) {
 		)
 	}
 
+	// TODO: make these tests work in GatewayNamespaceMode
+	if tests.IsGatewayNamespaceMode() {
+		skipTests = append(skipTests,
+			tests.HTTPWasmTest.ShortName,
+			tests.OCIWasmTest.ShortName,
+			tests.ZoneAwareRoutingTest.ShortName,
+		)
+	}
+
 	cSuite, err := suite.NewConformanceTestSuite(suite.ConformanceOptions{
 		Client:               c,
 		RestConfig:           cfg,

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

@@ -93,6 +93,10 @@ spec:
     type: Global
     global:
       rules:
+        - limit:
+            requests: 21
+            unit: Hour
+          shared: true
         - clientSelectors:
             - headers:
                 - name: x-user-id