From a030575c27bad4e2ee12fcffff1b33a37df88777 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 17:28:12 -0400 Subject: [PATCH 01/12] Fix shared=true when no clientSelector, cleanup filter logic, fix rl descriptor logic Signed-off-by: Ryan Hristovski --- internal/xds/translator/ratelimit.go | 146 +++++++----------- ...obal-shared-and-unshared-header-match.yaml | 4 + test/e2e/tests/ratelimit.go | 74 +++++++-- 3 files changed, 119 insertions(+), 105 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index e2d8ff8f77..96ba648566 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -85,49 +85,30 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3. return nil } - filters := []*hcmv3.HttpFilter{} - created := make(map[string]bool) - + domains := make(map[string]struct{}) 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[stripRuleIndexSuffix(rule.Name)] = struct{}{} } } + } + // Always include the non-shared listener domain + domains[irListener.Name] = struct{}{} - 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 - } + var filters []*hcmv3.HttpFilter + for domain := range domains { + filterName := egv1a1.EnvoyFilterRateLimit.String() + if domain != irListener.Name { + filterName += "/" + domain } - - 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 - } + if filter := createRateLimitFilter(t, irListener, domain, filterName); filter != nil { + filters = append(filters, filter) } } - return filters } @@ -450,13 +431,17 @@ 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] + if isRuleShared(rule) { + sharedDomain := stripRuleIndexSuffix(rule.Name) + addRateLimitDescriptor(route, rule, descriptor, sharedDomain, domainDesc) + } else { + listenerDomain := irListener.Name + addRateLimitDescriptor(route, rule, descriptor, listenerDomain, domainDesc) + } + } } } @@ -500,7 +485,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: @@ -513,54 +498,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. @@ -722,19 +701,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 } diff --git a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml index 639bbd7257..9b7ec8daea 100644 --- a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml +++ b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml @@ -93,6 +93,10 @@ spec: type: Global global: rules: + - limit: + requests: 24 + unit: Hour + shared: true - clientSelectors: - headers: - name: x-user-id diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index b3522af687..0f0ce5246f 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -970,10 +970,18 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, expectOk1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http"), expectOk1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk2, gwAddr1, "HTTP", "http"), expectOk2) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk3, gwAddr2, "HTTP", "http"), expectOk3) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectLimit, gwAddr1, "HTTP", "http"), expectLimit) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http"), expectOk1); err != nil { + t.Errorf("failed to get expected OK response for first /bar request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk2, gwAddr1, "HTTP", "http"), expectOk2); err != nil { + t.Errorf("failed to get expected OK response for /foo request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk3, gwAddr2, "HTTP", "http"), expectOk3); err != nil { + t.Errorf("failed to get expected OK response for second /bar request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectLimit, gwAddr1, "HTTP", "http"), expectLimit); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth request: %v", err) + } }) t.Run("unshared_route_policy_x-user-id=two", func(t *testing.T) { @@ -988,11 +996,19 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) - _ = GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1) + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { + t.Errorf("failed to get expected OK responses for first three /foo requests: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth /foo request: %v", err) + } - _ = GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2) + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { + t.Errorf("failed to get expected OK responses for first three /bar requests: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + } }) t.Run("shared_gateway_policy_x-user-id=three", func(t *testing.T) { @@ -1005,10 +1021,18 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, ok1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr2, "HTTP", "http"), ok1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr1, "HTTP", "http"), ok2) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok3, gwAddr2, "HTTP", "http"), ok3) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit, gwAddr1, "HTTP", "http"), limit) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr2, "HTTP", "http"), ok1); err != nil { + t.Errorf("failed to get expected OK response for first /bar request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr1, "HTTP", "http"), ok2); err != nil { + t.Errorf("failed to get expected OK response for /foo request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok3, gwAddr2, "HTTP", "http"), ok3); err != nil { + t.Errorf("failed to get expected OK response for second /bar request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit, gwAddr1, "HTTP", "http"), limit); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth request: %v", err) + } }) t.Run("unshared_gateway_policy__x-user-id=four", func(t *testing.T) { @@ -1021,11 +1045,29 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) - _ = GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1) + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { + t.Errorf("failed to get expected OK responses for first three /foo requests: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth /foo request: %v", err) + } - _ = GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2) - _ = GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2) + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { + t.Errorf("failed to get expected OK responses for first three /bar requests: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + } + }) + + t.Run("shared_no_client_selectors", func(t *testing.T) { + limit1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 429}, Namespace: ns} + + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) + + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr2, "HTTP", "http"), limit1); err != nil { + t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + } }) }, } From 964660259adda8e1222a89f65bb6aa8e7101d374 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 17:33:05 -0400 Subject: [PATCH 02/12] testdata update Signed-off-by: Ryan Hristovski --- .../xds-ir/ratelimit-global-shared.listeners.yaml | 12 ++++++------ .../ratelimit-multi-global-shared.listeners.yaml | 10 ++++++++++ 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml index fd03d88dca..f5e957d470 100644 --- a/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml +++ b/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/test-namespace/test-policy-2 typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: test-namespace/test-policy-1 + domain: test-namespace/test-policy-2 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 typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: test-namespace/test-policy-2 + domain: first-listener 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-1 typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: first-listener + domain: test-namespace/test-policy-1 enableXRatelimitHeaders: DRAFT_VERSION_03 rateLimitService: grpcService: diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-multi-global-shared.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-multi-global-shared.listeners.yaml index 0e76130891..fd3c8f9467 100644 --- a/internal/xds/translator/testdata/out/xds-ir/ratelimit-multi-global-shared.listeners.yaml +++ b/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 From 94a71c997ac91e5034f16988ddfb93f27f9be9a5 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 17:54:29 -0400 Subject: [PATCH 03/12] Linting, remove unused funcs Signed-off-by: Ryan Hristovski --- internal/xds/translator/ratelimit.go | 27 +++++++------------ .../ratelimit-global-shared.listeners.yaml | 12 ++++----- test/e2e/tests/ratelimit.go | 1 + 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 96ba648566..5a3ce37d55 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -9,6 +9,7 @@ import ( "bytes" "fmt" "net/url" + "sort" "strconv" "strings" @@ -84,7 +85,6 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3. if irListener == nil || irListener.Routes == nil { return nil } - domains := make(map[string]struct{}) for _, route := range irListener.Routes { if !isValidGlobalRateLimit(route) { @@ -96,11 +96,17 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3. } } } - // Always include the non-shared listener domain domains[irListener.Name] = struct{}{} + // Deterministic order otherwise tests break + domainList := make([]string, 0, len(domains)) + for d := range domains { + domainList = append(domainList, d) + } + sort.Strings(domainList) + var filters []*hcmv3.HttpFilter - for domain := range domains { + for _, domain := range domainList { filterName := egv1a1.EnvoyFilterRateLimit.String() if domain != irListener.Name { filterName += "/" + domain @@ -570,16 +576,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 { @@ -782,11 +778,6 @@ func (t *Translator) createRateLimitServiceCluster(tCtx *types.ResourceVersionTa }) } -// 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) } diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml index f5e957d470..fd3c8f9467 100644 --- a/internal/xds/translator/testdata/out/xds-ir/ratelimit-global-shared.listeners.yaml +++ b/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-2 + - name: envoy.filters.http.ratelimit typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: test-namespace/test-policy-2 + domain: first-listener 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-1 typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: first-listener + domain: test-namespace/test-policy-1 enableXRatelimitHeaders: DRAFT_VERSION_03 rateLimitService: grpcService: envoyGrpc: clusterName: ratelimit_cluster transportApiVersion: V3 - - name: envoy.filters.http.ratelimit/test-namespace/test-policy-1 + - name: envoy.filters.http.ratelimit/test-namespace/test-policy-2 typedConfig: '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit - domain: test-namespace/test-policy-1 + domain: test-namespace/test-policy-2 enableXRatelimitHeaders: DRAFT_VERSION_03 rateLimitService: grpcService: diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 0f0ce5246f..08cb7311be 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -1061,6 +1061,7 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ }) t.Run("shared_no_client_selectors", func(t *testing.T) { + ok1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 200}, Namespace: ns} limit1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) From 8277fc1a3b8104b610bc4d5c32aa45d60d0f132b Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 19:17:22 -0400 Subject: [PATCH 04/12] fix e2e Signed-off-by: Ryan Hristovski --- test/e2e/e2e_test.go | 1 + test/e2e/tests/ratelimit.go | 111 +++++++++++++++++++----------------- 2 files changed, 59 insertions(+), 53 deletions(-) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 399087cb71..e73d50f558 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -80,6 +80,7 @@ func TestE2E(t *testing.T) { tests.UsageRateLimitTest.ShortName, tests.RateLimitGlobalSharedCidrMatchTest.ShortName, tests.RateLimitGlobalSharedGatewayHeaderMatchTest.ShortName, + tests.RateLimitGlobalMergeTest.ShortName, ) } diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 08cb7311be..686c82bcaa 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -964,50 +964,48 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ headers := map[string]string{"x-user-id": "one"} expectOk1 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - expectOk2 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - expectOk3 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + expectOk2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + expectOk3 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} expectLimit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, expectOk1) - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http"), expectOk1); err != nil { - t.Errorf("failed to get expected OK response for first /bar request: %v", err) - } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk2, gwAddr1, "HTTP", "http"), expectOk2); err != nil { - t.Errorf("failed to get expected OK response for /foo request: %v", err) - } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectOk3, gwAddr2, "HTTP", "http"), expectOk3); err != nil { - t.Errorf("failed to get expected OK response for second /bar request: %v", err) + for _, expect := range []http.ExpectedResponse{expectOk1, expectOk2, expectOk3} { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { + t.Errorf("expected 200 response: %v", err) + } } if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expectLimit, gwAddr1, "HTTP", "http"), expectLimit); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth request: %v", err) + t.Errorf("expected 429 response: %v", err) } }) t.Run("unshared_route_policy_x-user-id=two", func(t *testing.T) { headers := map[string]string{"x-user-id": "two"} - // Route 1 (/foo) - ok1 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - limit1 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} - // Route 2 (/bar) - ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - limit2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} + okFoo := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + limitFoo := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} + okBar := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + limitBar := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, okFoo) - if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { - t.Errorf("failed to get expected OK responses for first three /foo requests: %v", err) + for i := 0; i < 3; i++ { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okFoo, gwAddr1, "HTTP", "http"), okFoo); err != nil { + t.Errorf("foo request #%d failed: %v", i+1, err) + } } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth /foo request: %v", err) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limitFoo, gwAddr1, "HTTP", "http"), limitFoo); err != nil { + t.Errorf("expected 429 on 4th foo: %v", err) } - if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { - t.Errorf("failed to get expected OK responses for first three /bar requests: %v", err) + for i := 0; i < 3; i++ { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okBar, gwAddr2, "HTTP", "http"), okBar); err != nil { + t.Errorf("bar request #%d failed: %v", i+1, err) + } } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limitBar, gwAddr2, "HTTP", "http"), limitBar); err != nil { + t.Errorf("expected 429 on 4th bar: %v", err) } }) @@ -1015,59 +1013,66 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ headers := map[string]string{"x-user-id": "three"} ok1 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - ok2 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - ok3 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + ok3 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} limit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, ok1) - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr2, "HTTP", "http"), ok1); err != nil { - t.Errorf("failed to get expected OK response for first /bar request: %v", err) - } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr1, "HTTP", "http"), ok2); err != nil { - t.Errorf("failed to get expected OK response for /foo request: %v", err) - } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok3, gwAddr2, "HTTP", "http"), ok3); err != nil { - t.Errorf("failed to get expected OK response for second /bar request: %v", err) + for _, expect := range []http.ExpectedResponse{ok1, ok2, ok3} { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { + t.Errorf("expected 200 response: %v", err) + } } if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit, gwAddr1, "HTTP", "http"), limit); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth request: %v", err) + t.Errorf("expected 429 response: %v", err) } }) t.Run("unshared_gateway_policy__x-user-id=four", func(t *testing.T) { headers := map[string]string{"x-user-id": "four"} - ok1 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - limit1 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} - ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - limit2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} + okFoo := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + limitFoo := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} + okBar := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + limitBar := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, okFoo) - if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { - t.Errorf("failed to get expected OK responses for first three /foo requests: %v", err) + for i := 0; i < 3; i++ { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okFoo, gwAddr1, "HTTP", "http"), okFoo); err != nil { + t.Errorf("foo request #%d failed: %v", i+1, err) + } } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr1, "HTTP", "http"), limit1); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth /foo request: %v", err) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limitFoo, gwAddr1, "HTTP", "http"), limitFoo); err != nil { + t.Errorf("expected 429 on 4th foo: %v", err) } - if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { - t.Errorf("failed to get expected OK responses for first three /bar requests: %v", err) + for i := 0; i < 3; i++ { + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okBar, gwAddr2, "HTTP", "http"), okBar); err != nil { + t.Errorf("bar request #%d failed: %v", i+1, err) + } } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit2, gwAddr2, "HTTP", "http"), limit2); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limitBar, gwAddr2, "HTTP", "http"), limitBar); err != nil { + t.Errorf("expected 429 on 4th bar: %v", err) } }) t.Run("shared_no_client_selectors", func(t *testing.T) { ok1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 200}, Namespace: ns} - limit1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 429}, Namespace: ns} + ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 200}, Namespace: ns} + limit := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit1, gwAddr2, "HTTP", "http"), limit1); err != nil { - t.Errorf("failed to get expected rate limit (429) response for fourth /bar request: %v", err) + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { + t.Errorf("expected 200 for first request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { + t.Errorf("expected 200 for second request: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit, gwAddr1, "HTTP", "http"), limit); err != nil { + t.Errorf("expected 429 for third request: %v", err) } }) }, From 0fb2dfd86ea81ea298d259e5e8a7e459867971a5 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 19:35:26 -0400 Subject: [PATCH 05/12] test requests are real requests Signed-off-by: Ryan Hristovski --- test/e2e/tests/ratelimit.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 686c82bcaa..3df22f4f69 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -964,13 +964,12 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ headers := map[string]string{"x-user-id": "one"} expectOk1 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - expectOk2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - expectOk3 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + expectOk2 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} expectLimit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, expectOk1) - for _, expect := range []http.ExpectedResponse{expectOk1, expectOk2, expectOk3} { + for _, expect := range []http.ExpectedResponse{expectOk1, expectOk2} { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { t.Errorf("expected 200 response: %v", err) } @@ -990,7 +989,7 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, okFoo) - for i := 0; i < 3; i++ { + for i := 0; i < 2; i++ { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okFoo, gwAddr1, "HTTP", "http"), okFoo); err != nil { t.Errorf("foo request #%d failed: %v", i+1, err) } @@ -1013,13 +1012,12 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ headers := map[string]string{"x-user-id": "three"} ok1 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} - ok3 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} + ok2 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} limit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, ok1) - for _, expect := range []http.ExpectedResponse{ok1, ok2, ok3} { + for _, expect := range []http.ExpectedResponse{ok1, ok2} { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { t.Errorf("expected 200 response: %v", err) } @@ -1039,7 +1037,7 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, okFoo) - for i := 0; i < 3; i++ { + for i := 0; i < 2; i++ { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &okFoo, gwAddr1, "HTTP", "http"), okFoo); err != nil { t.Errorf("foo request #%d failed: %v", i+1, err) } From c376e61e469333600c671c96cb39bf35ce13a9cc Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 20:25:01 -0400 Subject: [PATCH 06/12] Fix test Signed-off-by: Ryan Hristovski --- .../ratelimit-global-shared-and-unshared-header-match.yaml | 2 +- test/e2e/tests/ratelimit.go | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml index 9b7ec8daea..ef7500d496 100644 --- a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml +++ b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml @@ -94,7 +94,7 @@ spec: global: rules: - limit: - requests: 24 + requests: 19 unit: Hour shared: true - clientSelectors: diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 3df22f4f69..cb72564722 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -1058,17 +1058,10 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ t.Run("shared_no_client_selectors", func(t *testing.T) { ok1 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 200}, Namespace: ns} - ok2 := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 200}, Namespace: ns} limit := http.ExpectedResponse{Request: http.Request{Path: "/bar"}, Response: http.Response{StatusCode: 429}, Namespace: ns} http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr1, ok1) - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok1, gwAddr1, "HTTP", "http"), ok1); err != nil { - t.Errorf("expected 200 for first request: %v", err) - } - if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &ok2, gwAddr2, "HTTP", "http"), ok2); err != nil { - t.Errorf("expected 200 for second request: %v", err) - } if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &limit, gwAddr1, "HTTP", "http"), limit); err != nil { t.Errorf("expected 429 for third request: %v", err) } From 997c97f9cd194e848fd14d00db76a7d78900784a Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Wed, 14 May 2025 21:18:28 -0400 Subject: [PATCH 07/12] increase test limit Signed-off-by: Ryan Hristovski --- .../ratelimit-global-shared-and-unshared-header-match.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml index ef7500d496..c51ef1fac8 100644 --- a/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml +++ b/test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml @@ -94,7 +94,7 @@ spec: global: rules: - limit: - requests: 19 + requests: 21 unit: Hour shared: true - clientSelectors: From bd021af4ab27aa05953241e36e69fd4384de4da3 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Thu, 15 May 2025 09:32:13 -0400 Subject: [PATCH 08/12] fix test, clean up nit Signed-off-by: Ryan Hristovski --- internal/xds/translator/ratelimit.go | 8 +++----- test/e2e/tests/ratelimit.go | 10 +++++++++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 5a3ce37d55..7c243b43e1 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -440,13 +440,11 @@ func BuildRateLimitServiceConfig(irListeners []*ir.HTTPListener) []*rlsconfv3.Ra // 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) { - sharedDomain := stripRuleIndexSuffix(rule.Name) - addRateLimitDescriptor(route, rule, descriptor, sharedDomain, domainDesc) - } else { - listenerDomain := irListener.Name - addRateLimitDescriptor(route, rule, descriptor, listenerDomain, domainDesc) + domain = stripRuleIndexSuffix(rule.Name) } + addRateLimitDescriptor(route, rule, descriptor, domain, domainDesc) } } } diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index cb72564722..1a60f352b6 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -967,7 +967,15 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ expectOk2 := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 200}, Namespace: ns} expectLimit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} - http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr2, expectOk1) + require.Eventually(t, func() bool { + req := http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http") + resp, err := suite.RoundTripper.RoundTrip(req) + if err != nil { + return false + } + defer resp.Body.Close() + return resp.Header.Get("X-Ratelimit-Limit") == "3, 3;w=3600" + }, suite.TimeoutConfig.MaxTimeToConsistency, suite.TimeoutConfig.PollInterval, "rate limit header not yet present") for _, expect := range []http.ExpectedResponse{expectOk1, expectOk2} { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { From 9dedbb8505462f496c2f26bc459311df15cb6698 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Thu, 15 May 2025 09:56:19 -0400 Subject: [PATCH 09/12] Linting Signed-off-by: Ryan Hristovski --- test/e2e/tests/ratelimit.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 1a60f352b6..d64bd0c78c 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -968,14 +968,13 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ expectLimit := http.ExpectedResponse{Request: http.Request{Path: "/foo", Headers: headers}, Response: http.Response{StatusCode: 429}, Namespace: ns} require.Eventually(t, func() bool { - req := http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http") - resp, err := suite.RoundTripper.RoundTrip(req) + _, cRes, err := suite.RoundTripper.CaptureRoundTrip(http.MakeRequest(t, &expectOk1, gwAddr2, "HTTP", "http")) if err != nil { return false } - defer resp.Body.Close() - return resp.Header.Get("X-Ratelimit-Limit") == "3, 3;w=3600" - }, suite.TimeoutConfig.MaxTimeToConsistency, suite.TimeoutConfig.PollInterval, "rate limit header not yet present") + vals := cRes.Headers["X-Ratelimit-Limit"] + return len(vals) > 0 && vals[0] == "3, 3;w=3600" + }, suite.TimeoutConfig.MaxTimeToConsistency, suite.TimeoutConfig.RequestTimeout, "rate limit header not yet present") for _, expect := range []http.ExpectedResponse{expectOk1, expectOk2} { if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, http.MakeRequest(t, &expect, gwAddr2, "HTTP", "http"), expect); err != nil { @@ -1079,12 +1078,12 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ func GotExactExpectedResponse(t *testing.T, n int, r roundtripper.RoundTripper, req roundtripper.Request, resp http.ExpectedResponse) error { for i := 0; i < n; i++ { - cReq, cRes, err := r.CaptureRoundTrip(req) + _, cRes, err := r.CaptureRoundTrip(req) if err != nil { return err } - if err = http.CompareRequest(t, &req, cReq, cRes, resp); err != nil { + if err = http.CompareRequest(t, &req, nil, cRes, resp); err != nil { return err } } From c7bdd83131f7d72dec66a60754511a561cf38215 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Thu, 15 May 2025 10:35:21 -0400 Subject: [PATCH 10/12] Fix GotExactExpectedResponse Signed-off-by: Ryan Hristovski --- test/e2e/tests/ratelimit.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index d64bd0c78c..1a96198aa0 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -1078,12 +1078,12 @@ var RateLimitGlobalMergeTest = suite.ConformanceTest{ func GotExactExpectedResponse(t *testing.T, n int, r roundtripper.RoundTripper, req roundtripper.Request, resp http.ExpectedResponse) error { for i := 0; i < n; i++ { - _, cRes, err := r.CaptureRoundTrip(req) + cReq, cRes, err := r.CaptureRoundTrip(req) if err != nil { return err } - if err = http.CompareRequest(t, &req, nil, cRes, resp); err != nil { + if err = http.CompareRequest(t, &req, cReq, cRes, resp); err != nil { return err } } From b73c4d12423bfabee3379f2a9d3ee357ac4d0a0e Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Tue, 27 May 2025 08:40:45 -0400 Subject: [PATCH 11/12] use k8s.io/apimachinery/pkg/util/set instead Signed-off-by: Ryan Hristovski --- internal/xds/translator/ratelimit.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index a90317f16a..5fdf18ef6e 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -25,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" @@ -73,24 +74,21 @@ func (t *Translator) buildRateLimitFilter(irListener *ir.HTTPListener) []*hcmv3. if irListener == nil || irListener.Routes == nil { return nil } - domains := make(map[string]struct{}) + domains := sets.New[string]() for _, route := range irListener.Routes { if !isValidGlobalRateLimit(route) { continue } for _, rule := range route.Traffic.RateLimit.Global.Rules { if isRuleShared(rule) { - domains[stripRuleIndexSuffix(rule.Name)] = struct{}{} + domains.Insert(stripRuleIndexSuffix(rule.Name)) } } } - domains[irListener.Name] = struct{}{} + domains.Insert(irListener.Name) // Deterministic order otherwise tests break - domainList := make([]string, 0, len(domains)) - for d := range domains { - domainList = append(domainList, d) - } + domainList := sets.List(domains) sort.Strings(domainList) var filters []*hcmv3.HttpFilter From 793224b2383ea7dee81b22e07765cc69cbcbcd69 Mon Sep 17 00:00:00 2001 From: Ryan Hristovski Date: Tue, 27 May 2025 09:10:28 -0400 Subject: [PATCH 12/12] merge conflict fixes Signed-off-by: Ryan Hristovski --- internal/xds/translator/ratelimit.go | 70 ---------------------------- test/e2e/e2e_test.go | 13 ------ 2 files changed, 83 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 5fdf18ef6e..9f8501c5d5 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -695,76 +695,6 @@ func buildRateLimitServiceDescriptors(route *ir.HTTPRoute) []*rlsconfv3.RateLimi return pbDescriptors } -// buildRateLimitTLSocket builds the TLS socket for the rate limit service. -func buildRateLimitTLSocket() (*corev3.TransportSocket, error) { - tlsCtx := &tlsv3.UpstreamTlsContext{ - CommonTlsContext: &tlsv3.CommonTlsContext{ - TlsCertificates: []*tlsv3.TlsCertificate{}, - ValidationContextType: &tlsv3.CommonTlsContext_ValidationContext{ - ValidationContext: &tlsv3.CertificateValidationContext{ - TrustedCa: &corev3.DataSource{ - Specifier: &corev3.DataSource_Filename{Filename: rateLimitClientTLSCACertFilename}, - }, - }, - }, - }, - } - - tlsCert := &tlsv3.TlsCertificate{ - CertificateChain: &corev3.DataSource{ - Specifier: &corev3.DataSource_Filename{Filename: rateLimitClientTLSCertFilename}, - }, - PrivateKey: &corev3.DataSource{ - Specifier: &corev3.DataSource_Filename{Filename: rateLimitClientTLSKeyFilename}, - }, - } - tlsCtx.CommonTlsContext.TlsCertificates = append(tlsCtx.CommonTlsContext.TlsCertificates, tlsCert) - - tlsCtxAny, err := anypb.New(tlsCtx) - if err != nil { - return nil, err - } - - return &corev3.TransportSocket{ - Name: wellknown.TransportSocketTls, - ConfigType: &corev3.TransportSocket_TypedConfig{ - TypedConfig: tlsCtxAny, - }, - }, nil -} - -func (t *Translator) createRateLimitServiceCluster(tCtx *types.ResourceVersionTable, irListener *ir.HTTPListener, metrics *ir.Metrics) error { - // Return early if rate limits don't exist. - if !t.isRateLimitPresent(irListener) { - return nil - } - clusterName := getRateLimitServiceClusterName() - // Create cluster if it does not exist - host, port := t.getRateLimitServiceGrpcHostPort() - ds := &ir.DestinationSetting{ - Weight: ptr.To[uint32](1), - Protocol: ir.GRPC, - Endpoints: []*ir.DestinationEndpoint{ir.NewDestEndpoint(host, port, false, nil)}, - Name: destinationSettingName(clusterName), - } - - tSocket, err := buildRateLimitTLSocket() - if err != nil { - return err - } - - return addXdsCluster(tCtx, &xdsClusterArgs{ - name: clusterName, - settings: []*ir.DestinationSetting{ds}, - tSocket: tSocket, - endpointType: EndpointTypeDNS, - metrics: metrics, - }) -// 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) } diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 71c70a1138..1016bbfff9 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -67,19 +67,6 @@ func TestE2E(t *testing.T) { tests.HTTPWasmTest.ShortName, tests.OCIWasmTest.ShortName, tests.ZoneAwareRoutingTest.ShortName, - - // Skip RateLimit tests because they are not supported in GatewayNamespaceMode - tests.RateLimitCIDRMatchTest.ShortName, - tests.RateLimitHeaderMatchTest.ShortName, - tests.GlobalRateLimitHeaderInvertMatchTest.ShortName, - tests.RateLimitHeadersDisabled.ShortName, - tests.RateLimitBasedJwtClaimsTest.ShortName, - tests.RateLimitMultipleListenersTest.ShortName, - tests.RateLimitHeadersAndCIDRMatchTest.ShortName, - tests.UsageRateLimitTest.ShortName, - tests.RateLimitGlobalSharedCidrMatchTest.ShortName, - tests.RateLimitGlobalSharedGatewayHeaderMatchTest.ShortName, - tests.RateLimitGlobalMergeTest.ShortName, ) }