From 0b64a6c14deaa36d07f207bd2421774ba903b6f9 Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Tue, 1 Oct 2024 12:01:01 +0800 Subject: [PATCH 1/4] fix ratelimit descriptors do not respect both headers and cidr match for one rule Signed-off-by: shawnh2 --- internal/xds/translator/ratelimit.go | 70 +++++------ .../header-and-cidr-matches.yaml | 36 ++++++ .../in/xds-ir/ratelimit-headers-and-cidr.yaml | 88 ++++++++++++++ .../header-and-cidr-matches.yaml | 32 +++++ .../ratelimit-headers-and-cidr.clusters.yaml | 98 +++++++++++++++ .../ratelimit-headers-and-cidr.endpoints.yaml | 36 ++++++ .../ratelimit-headers-and-cidr.listeners.yaml | 44 +++++++ .../ratelimit-headers-and-cidr.routes.yaml | 88 ++++++++++++++ .../latest/tasks/traffic/global-rate-limit.md | 2 +- .../v1.1/tasks/traffic/global-rate-limit.md | 2 +- .../ratelimit-headers-and-cidr-match.yaml | 45 +++++++ test/e2e/tests/ratelimit.go | 114 ++++++++++++++++++ 12 files changed, 615 insertions(+), 40 deletions(-) create mode 100644 internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml create mode 100644 internal/xds/translator/testdata/in/xds-ir/ratelimit-headers-and-cidr.yaml create mode 100644 internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.clusters.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.endpoints.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.listeners.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.routes.yaml create mode 100644 test/e2e/testdata/ratelimit-headers-and-cidr-match.yaml diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 8e3e661f9d7..153f753ca02 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -157,11 +157,12 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ // Matches are ANDed rlActions := []*routev3.RateLimit_Action{routeDescriptor} for mIdx, match := range rule.HeaderMatches { + var action *routev3.RateLimit_Action // Case for distinct match if match.Distinct { // Setup RequestHeader actions descriptorKey := getRouteRuleDescriptor(rIdx, mIdx) - action := &routev3.RateLimit_Action{ + action = &routev3.RateLimit_Action{ ActionSpecifier: &routev3.RateLimit_Action_RequestHeaders_{ RequestHeaders: &routev3.RateLimit_Action_RequestHeaders{ HeaderName: match.Name, @@ -169,7 +170,6 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ }, }, } - rlActions = append(rlActions, action) } else { // Setup HeaderValueMatch actions descriptorKey := getRouteRuleDescriptor(rIdx, mIdx) @@ -180,7 +180,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ StringMatch: buildXdsStringMatcher(match), }, } - action := &routev3.RateLimit_Action{ + action = &routev3.RateLimit_Action{ ActionSpecifier: &routev3.RateLimit_Action_HeaderValueMatch_{ HeaderValueMatch: &routev3.RateLimit_Action_HeaderValueMatch{ DescriptorKey: descriptorKey, @@ -192,8 +192,8 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ }, }, } - rlActions = append(rlActions, action) } + rlActions = append(rlActions, action) } // To be able to rate limit each individual IP, we need to use a nested descriptors structure in the configuration @@ -232,7 +232,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ // Setup RemoteAddress action if distinct match is set if rule.CIDRMatch.Distinct { // Setup RemoteAddress action - action := &routev3.RateLimit_Action{ + action = &routev3.RateLimit_Action{ ActionSpecifier: &routev3.RateLimit_Action_RemoteAddress_{ RemoteAddress: &routev3.RateLimit_Action_RemoteAddress{}, }, @@ -241,7 +241,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ } } - // Case when header match is not set and the rate limit is applied + // Case when header/cidr match is not set and the rate limit is applied // to all traffic. if !rule.IsMatchSet() { // Setup GenericKey action @@ -329,20 +329,12 @@ func BuildRateLimitServiceConfig(irListener *ir.HTTPListener) *rlsconfv3.RateLim func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.RateLimitDescriptor { pbDescriptors := make([]*rlsconfv3.RateLimitDescriptor, 0, len(global.Rules)) + // Descriptors for matches are built corresponding to ratelimit actions. for rIdx, rule := range global.Rules { var head, cur *rlsconfv3.RateLimitDescriptor - if !rule.IsMatchSet() { - pbDesc := new(rlsconfv3.RateLimitDescriptor) - // GenericKey case - pbDesc.Key = getRouteRuleDescriptor(rIdx, -1) - pbDesc.Value = getRouteRuleDescriptor(rIdx, -1) - rateLimit := rlsconfv3.RateLimitPolicy{ - RequestsPerUnit: uint32(rule.Limit.Requests), - Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), - } - pbDesc.RateLimit = &rateLimit - head = pbDesc - cur = head + var rateLimitPolicy = &rlsconfv3.RateLimitPolicy{ + RequestsPerUnit: uint32(rule.Limit.Requests), + Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), } for mIdx, match := range rule.HeaderMatches { @@ -357,15 +349,6 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R pbDesc.Value = getRouteRuleDescriptor(rIdx, mIdx) } - // Add the ratelimit values to the last descriptor - if mIdx == len(rule.HeaderMatches)-1 { - rateLimit := rlsconfv3.RateLimitPolicy{ - RequestsPerUnit: uint32(rule.Limit.Requests), - Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), - } - pbDesc.RateLimit = &rateLimit - } - if mIdx == 0 { head = pbDesc } else { @@ -401,25 +384,36 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R pbDesc := new(rlsconfv3.RateLimitDescriptor) pbDesc.Key = "masked_remote_address" pbDesc.Value = rule.CIDRMatch.CIDR - rateLimit := rlsconfv3.RateLimitPolicy{ - RequestsPerUnit: uint32(rule.Limit.Requests), - Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), + + if cur != nil { + cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc} + cur = pbDesc + } else { + head = pbDesc + cur = head } if rule.CIDRMatch.Distinct { - pbDesc.Descriptors = []*rlsconfv3.RateLimitDescriptor{ - { - Key: "remote_address", - RateLimit: &rateLimit, - }, - } - } else { - pbDesc.RateLimit = &rateLimit + pbDesc := new(rlsconfv3.RateLimitDescriptor) + pbDesc.Key = "remote_address" + cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc} + cur = pbDesc } + } + + // Case when header/cidr match is not set and the rate limit is applied + // to all traffic. + if !rule.IsMatchSet() { + pbDesc := new(rlsconfv3.RateLimitDescriptor) + // GenericKey case + pbDesc.Key = getRouteRuleDescriptor(rIdx, -1) + pbDesc.Value = getRouteRuleDescriptor(rIdx, -1) head = pbDesc cur = head } + // Add the ratelimit values to the last descriptor + cur.RateLimit = rateLimitPolicy pbDescriptors = append(pbDescriptors, head) } diff --git a/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml b/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml new file mode 100644 index 00000000000..3e5d2b35b5a --- /dev/null +++ b/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml @@ -0,0 +1,36 @@ +name: "first-listener" +address: "0.0.0.0" +port: 10080 +hostnames: +- "*" +path: + mergeSlashes: true + escapedSlashesAction: UnescapeAndRedirect +routes: +- name: "first-route" + traffic: + rateLimit: + global: + rules: + - headerMatches: + - name: "x-user-id" + exact: "one" + - name: "x-user-id" + exact: "two" + cidrMatch: + cidr: 0.0.0.0/0 + ip: 0.0.0.0 + maskLen: 0 + isIPv6: false + distinct: false + limit: + requests: 5 + unit: second + pathMatch: + exact: "foo/bar" + destination: + name: "first-route-dest" + settings: + - endpoints: + - host: "1.2.3.4" + port: 50000 diff --git a/internal/xds/translator/testdata/in/xds-ir/ratelimit-headers-and-cidr.yaml b/internal/xds/translator/testdata/in/xds-ir/ratelimit-headers-and-cidr.yaml new file mode 100644 index 00000000000..fa9b6f31ae5 --- /dev/null +++ b/internal/xds/translator/testdata/in/xds-ir/ratelimit-headers-and-cidr.yaml @@ -0,0 +1,88 @@ +http: +- name: "first-listener" + address: "0.0.0.0" + port: 10080 + hostnames: + - "*" + path: + mergeSlashes: true + escapedSlashesAction: UnescapeAndRedirect + routes: + - name: "first-route" + hostname: "*" + traffic: + rateLimit: + global: + rules: + - headerMatches: + - name: "x-user-id" + exact: "one" + cidrMatch: + cidr: 192.168.0.0/16 + maskLen: 16 + limit: + requests: 5 + unit: second + destination: + name: "first-route-dest" + settings: + - endpoints: + - host: "1.2.3.4" + port: 50000 + - name: "second-route" + hostname: "*" + traffic: + rateLimit: + global: + rules: + - headerMatches: + - name: "x-user-id" + distinct: true + - name: "foobar" + distinct: true + cidrMatch: + cidr: 192.168.0.0/16 + maskLen: 16 + limit: + requests: 5 + unit: second + pathMatch: + exact: "example" + destination: + name: "second-route-dest" + settings: + - endpoints: + - host: "1.2.3.4" + port: 50000 + - name: "third-route" + hostname: "*" + traffic: + rateLimit: + global: + rules: + - headerMatches: + - name: "x-user-id" + exact: "one" + cidrMatch: + cidr: 192.168.0.0/16 + maskLen: 16 + limit: + requests: 5 + unit: second + - headerMatches: + - name: "x-user-id" + exact: "two" + - name: "foobar" + distinct: true + cidrMatch: + cidr: 192.169.0.0/16 + maskLen: 16 + limit: + requests: 10 + unit: second + destination: + name: "third-route-dest" + settings: + - endpoints: + - host: "1.2.3.4" + port: 50000 diff --git a/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml b/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml new file mode 100644 index 00000000000..87596e15a59 --- /dev/null +++ b/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml @@ -0,0 +1,32 @@ +name: first-listener +domain: first-listener +descriptors: + - key: first-route + value: first-route + rate_limit: null + descriptors: + - key: rule-0-match-0 + value: rule-0-match-0 + rate_limit: null + descriptors: + - key: rule-0-match-1 + value: rule-0-match-1 + rate_limit: null + descriptors: + - key: masked_remote_address + value: 0.0.0.0/0 + rate_limit: + requests_per_unit: 5 + unit: SECOND + unlimited: false + name: "" + replaces: [] + descriptors: [] + shadow_mode: false + detailed_metric: false + shadow_mode: false + detailed_metric: false + shadow_mode: false + detailed_metric: false + shadow_mode: false + detailed_metric: false diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.clusters.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.clusters.yaml new file mode 100644 index 00000000000..0ba1749076a --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.clusters.yaml @@ -0,0 +1,98 @@ +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + edsClusterConfig: + edsConfig: + ads: {} + resourceApiVersion: V3 + serviceName: first-route-dest + lbPolicy: LEAST_REQUEST + name: first-route-dest + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + type: EDS +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + edsClusterConfig: + edsConfig: + ads: {} + resourceApiVersion: V3 + serviceName: second-route-dest + lbPolicy: LEAST_REQUEST + name: second-route-dest + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + type: EDS +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + edsClusterConfig: + edsConfig: + ads: {} + resourceApiVersion: V3 + serviceName: third-route-dest + lbPolicy: LEAST_REQUEST + name: third-route-dest + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + type: EDS +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + dnsRefreshRate: 30s + lbPolicy: LEAST_REQUEST + loadAssignment: + clusterName: ratelimit_cluster + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: envoy-ratelimit.envoy-gateway-system.svc.cluster.local + portValue: 8081 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: ratelimit_cluster/backend/0 + name: ratelimit_cluster + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + respectDnsTtl: true + transportSocket: + name: envoy.transport_sockets.tls + typedConfig: + '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext + commonTlsContext: + tlsCertificates: + - certificateChain: + filename: /certs/tls.crt + privateKey: + filename: /certs/tls.key + validationContext: + trustedCa: + filename: /certs/ca.crt + type: STRICT_DNS + typedExtensionProtocolOptions: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + '@type': type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + explicitHttpConfig: + http2ProtocolOptions: + initialConnectionWindowSize: 1048576 + initialStreamWindowSize: 65536 diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.endpoints.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.endpoints.yaml new file mode 100644 index 00000000000..475b89a087c --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.endpoints.yaml @@ -0,0 +1,36 @@ +- clusterName: first-route-dest + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: 1.2.3.4 + portValue: 50000 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: first-route-dest/backend/0 +- clusterName: second-route-dest + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: 1.2.3.4 + portValue: 50000 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: second-route-dest/backend/0 +- clusterName: third-route-dest + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: 1.2.3.4 + portValue: 50000 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: third-route-dest/backend/0 diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.listeners.yaml new file mode 100644 index 00000000000..a80f448f017 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.listeners.yaml @@ -0,0 +1,44 @@ +- address: + socketAddress: + address: 0.0.0.0 + portValue: 10080 + defaultFilterChain: + filters: + - name: envoy.filters.network.http_connection_manager + typedConfig: + '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager + commonHttpProtocolOptions: + headersWithUnderscoresAction: REJECT_REQUEST + http2ProtocolOptions: + initialConnectionWindowSize: 1048576 + 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.router + typedConfig: + '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router + suppressEnvoyHeaders: true + mergeSlashes: true + normalizePath: true + pathWithEscapedSlashesAction: UNESCAPE_AND_REDIRECT + rds: + configSource: + ads: {} + resourceApiVersion: V3 + routeConfigName: first-listener + serverHeaderTransformation: PASS_THROUGH + statPrefix: http-10080 + useRemoteAddress: true + name: first-listener + name: first-listener + perConnectionBufferLimitBytes: 32768 diff --git a/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.routes.yaml new file mode 100644 index 00000000000..459d975a9b0 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/ratelimit-headers-and-cidr.routes.yaml @@ -0,0 +1,88 @@ +- ignorePortInHostMatching: true + name: first-listener + virtualHosts: + - domains: + - '*' + name: first-listener/* + routes: + - match: + prefix: / + name: first-route + route: + cluster: first-route-dest + rateLimits: + - actions: + - genericKey: + descriptorKey: first-route + descriptorValue: first-route + - headerValueMatch: + descriptorKey: rule-0-match-0 + descriptorValue: rule-0-match-0 + expectMatch: true + headers: + - name: x-user-id + stringMatch: + exact: one + - maskedRemoteAddress: + v4PrefixMaskLen: 16 + upgradeConfigs: + - upgradeType: websocket + - match: + path: example + name: second-route + route: + cluster: second-route-dest + rateLimits: + - actions: + - genericKey: + descriptorKey: second-route + descriptorValue: second-route + - requestHeaders: + descriptorKey: rule-0-match-0 + headerName: x-user-id + - requestHeaders: + descriptorKey: rule-0-match-1 + headerName: foobar + - maskedRemoteAddress: + v4PrefixMaskLen: 16 + upgradeConfigs: + - upgradeType: websocket + - match: + prefix: / + name: third-route + route: + cluster: third-route-dest + rateLimits: + - actions: + - genericKey: + descriptorKey: third-route + descriptorValue: third-route + - headerValueMatch: + descriptorKey: rule-0-match-0 + descriptorValue: rule-0-match-0 + expectMatch: true + headers: + - name: x-user-id + stringMatch: + exact: one + - maskedRemoteAddress: + v4PrefixMaskLen: 16 + - actions: + - genericKey: + descriptorKey: third-route + descriptorValue: third-route + - headerValueMatch: + descriptorKey: rule-1-match-0 + descriptorValue: rule-1-match-0 + expectMatch: true + headers: + - name: x-user-id + stringMatch: + exact: two + - requestHeaders: + descriptorKey: rule-1-match-1 + headerName: foobar + - maskedRemoteAddress: + v4PrefixMaskLen: 16 + upgradeConfigs: + - upgradeType: websocket diff --git a/site/content/en/latest/tasks/traffic/global-rate-limit.md b/site/content/en/latest/tasks/traffic/global-rate-limit.md index f105de880cd..6c96b12efe7 100644 --- a/site/content/en/latest/tasks/traffic/global-rate-limit.md +++ b/site/content/en/latest/tasks/traffic/global-rate-limit.md @@ -870,7 +870,7 @@ spec: - clientSelectors: - sourceCIDR: value: 0.0.0.0/0 - type: distinct + type: Distinct limit: requests: 3 unit: Hour diff --git a/site/content/en/v1.1/tasks/traffic/global-rate-limit.md b/site/content/en/v1.1/tasks/traffic/global-rate-limit.md index bb87c47de49..da00334b296 100644 --- a/site/content/en/v1.1/tasks/traffic/global-rate-limit.md +++ b/site/content/en/v1.1/tasks/traffic/global-rate-limit.md @@ -871,7 +871,7 @@ spec: - clientSelectors: - sourceCIDR: value: 0.0.0.0/0 - type: distinct + type: Distinct limit: requests: 3 unit: Hour diff --git a/test/e2e/testdata/ratelimit-headers-and-cidr-match.yaml b/test/e2e/testdata/ratelimit-headers-and-cidr-match.yaml new file mode 100644 index 00000000000..fef2f645a2b --- /dev/null +++ b/test/e2e/testdata/ratelimit-headers-and-cidr-match.yaml @@ -0,0 +1,45 @@ +apiVersion: gateway.envoyproxy.io/v1alpha1 +kind: BackendTrafficPolicy +metadata: + name: ratelimit-headers-and-cidr + namespace: gateway-conformance-infra +spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: header-and-cidr-ratelimit + rateLimit: + type: Global + global: + rules: + - clientSelectors: + - headers: + - name: x-user-id + type: Exact + value: one + - name: x-user-org + type: Exact + value: acme + sourceCIDR: + value: 0.0.0.0/0 + type: Distinct + limit: + requests: 3 + unit: Hour +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: header-and-cidr-ratelimit + namespace: gateway-conformance-infra +spec: + parentRefs: + - name: same-namespace + rules: + - matches: + - path: + type: PathPrefix + value: /get + backendRefs: + - name: infra-backend-v1 + port: 8080 diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index f564eec6dd4..95d99632f41 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -30,6 +30,7 @@ func init() { ConformanceTests = append(ConformanceTests, RateLimitHeadersDisabled) ConformanceTests = append(ConformanceTests, RateLimitBasedJwtClaimsTest) ConformanceTests = append(ConformanceTests, RateLimitMultipleListenersTest) + ConformanceTests = append(ConformanceTests, RateLimitHeadersAndCIDRMatchTest) } var RateLimitCIDRMatchTest = suite.ConformanceTest{ @@ -451,6 +452,119 @@ var RateLimitMultipleListenersTest = suite.ConformanceTest{ }, } +var RateLimitHeadersAndCIDRMatchTest = suite.ConformanceTest{ + ShortName: "RateLimitHeadersAndCIDRMatch", + Description: "Limit requests on rule that has both headers and cidr matches", + Manifests: []string{"testdata/ratelimit-headers-and-cidr-match.yaml"}, + Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { + ns := "gateway-conformance-infra" + routeNN := types.NamespacedName{Name: "header-and-cidr-ratelimit", Namespace: ns} + gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} + gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) + + t.Run("all matched both headers and cidr can got limited", func(t *testing.T) { + requestHeaders := map[string]string{ + "x-user-id": "one", + "x-user-org": "acme", + } + + ratelimitHeader := make(map[string]string) + expectOkResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/get", + Headers: requestHeaders, + }, + Response: http.Response{ + StatusCode: 200, + Headers: ratelimitHeader, + }, + Namespace: ns, + } + expectOkResp.Response.Headers["X-Ratelimit-Limit"] = "3, 3;w=3600" + expectOkReq := http.MakeRequest(t, &expectOkResp, gwAddr, "HTTP", "http") + + expectLimitResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/get", + Headers: requestHeaders, + }, + Response: http.Response{ + StatusCode: 429, + }, + Namespace: ns, + } + expectLimitReq := http.MakeRequest(t, &expectLimitResp, gwAddr, "HTTP", "http") + + // should just send exactly 4 requests, and expect 429 + + // keep sending requests till get 200 first, that will cost one 200 + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectOkResp) + + // fire the rest of the requests + if err := GotExactExpectedResponse(t, 2, suite.RoundTripper, expectOkReq, expectOkResp); err != nil { + t.Errorf("failed to get expected response for the first three requests: %v", err) + } + if err := GotExactExpectedResponse(t, 1, suite.RoundTripper, expectLimitReq, expectLimitResp); err != nil { + t.Errorf("failed to get expected response for the last (fourth) request: %v", err) + } + }) + + t.Run("only matched headers cannot got limited", func(t *testing.T) { + requestHeaders := map[string]string{ + "x-user-id": "one", + "x-user-org": "acme", + } + + // it does not require any rate limit header, since this request never be rate limited. + expectOkResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/get", + Headers: requestHeaders, + }, + Response: http.Response{ + StatusCode: 200, + }, + Namespace: ns, + } + expectOkReq := http.MakeRequest(t, &expectOkResp, gwAddr, "HTTP", "http") + + // send exactly 4 requests, and still expect 200 + + // keep sending requests till get 200 first, that will cost one 200 + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectOkResp) + + // fire the rest of the requests + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, expectOkReq, expectOkResp); err != nil { + t.Errorf("failed to get expected responses for the request: %v", err) + } + }) + + t.Run("only matched cidr cannot got limited", func(t *testing.T) { + // it does not require any rate limit header, since this request never be rate limited. + expectOkResp := http.ExpectedResponse{ + Request: http.Request{ + Path: "/get", + }, + Response: http.Response{ + StatusCode: 200, + }, + Namespace: ns, + } + expectOkReq := http.MakeRequest(t, &expectOkResp, gwAddr, "HTTP", "http") + + // send exactly 4 requests, and still expect 200 + + // keep sending requests till get 200 first, that will cost one 200 + http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectOkResp) + + // fire the rest of the requests + if err := GotExactExpectedResponse(t, 3, suite.RoundTripper, expectOkReq, expectOkResp); err != nil { + t.Errorf("failed to get expected responses for the request: %v", err) + } + }) + }, +} + 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) From a5fb19f75d085b5ac11acc4155f9d730c9560575 Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Tue, 1 Oct 2024 12:21:25 +0800 Subject: [PATCH 2/4] fix gen-check and lint Signed-off-by: shawnh2 --- internal/xds/translator/ratelimit.go | 2 +- site/content/en/docs/tasks/traffic/global-rate-limit.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 153f753ca02..6556ff5c240 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -332,7 +332,7 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R // Descriptors for matches are built corresponding to ratelimit actions. for rIdx, rule := range global.Rules { var head, cur *rlsconfv3.RateLimitDescriptor - var rateLimitPolicy = &rlsconfv3.RateLimitPolicy{ + rateLimitPolicy := &rlsconfv3.RateLimitPolicy{ RequestsPerUnit: uint32(rule.Limit.Requests), Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), } diff --git a/site/content/en/docs/tasks/traffic/global-rate-limit.md b/site/content/en/docs/tasks/traffic/global-rate-limit.md index bb87c47de49..da00334b296 100644 --- a/site/content/en/docs/tasks/traffic/global-rate-limit.md +++ b/site/content/en/docs/tasks/traffic/global-rate-limit.md @@ -871,7 +871,7 @@ spec: - clientSelectors: - sourceCIDR: value: 0.0.0.0/0 - type: distinct + type: Distinct limit: requests: 3 unit: Hour From 3760327b7b0c8247e4cea57de06cbca76b2ea5f4 Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Tue, 8 Oct 2024 21:53:18 +0800 Subject: [PATCH 3/4] fix ratelimit e2e test Signed-off-by: shawnh2 --- test/e2e/tests/ratelimit.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go index 278106ed4fe..e4792698973 100644 --- a/test/e2e/tests/ratelimit.go +++ b/test/e2e/tests/ratelimit.go @@ -508,10 +508,9 @@ var RateLimitHeadersAndCIDRMatchTest = suite.ConformanceTest{ } }) - t.Run("only matched headers cannot got limited", func(t *testing.T) { + t.Run("only partly matched headers cannot got limited", func(t *testing.T) { requestHeaders := map[string]string{ - "x-user-id": "one", - "x-user-org": "acme", + "x-user-id": "one", } // it does not require any rate limit header, since this request never be rate limited. From 53cd6f580d3c2975b3f5ba9693466bdfd8b9199a Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Sat, 19 Oct 2024 15:55:30 +0800 Subject: [PATCH 4/4] add more comment and update test case Signed-off-by: shawnh2 --- internal/xds/translator/ratelimit.go | 29 +++++++++++++------ .../header-and-cidr-matches.yaml | 2 ++ .../header-and-cidr-matches.yaml | 24 +++++++++------ 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 6556ff5c240..62e671f1f64 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -241,8 +241,8 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [ } } - // Case when header/cidr match is not set and the rate limit is applied - // to all traffic. + // Case when both header and cidr match are not set and the ratelimit + // will be applied to all traffic. if !rule.IsMatchSet() { // Setup GenericKey action action := &routev3.RateLimit_Action{ @@ -329,14 +329,21 @@ func BuildRateLimitServiceConfig(irListener *ir.HTTPListener) *rlsconfv3.RateLim func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.RateLimitDescriptor { pbDescriptors := make([]*rlsconfv3.RateLimitDescriptor, 0, len(global.Rules)) - // Descriptors for matches are built corresponding to ratelimit actions. + // The order in which matching descriptors are built is consistent with + // the order in which ratelimit actions are built: + // 1) Header Matches + // 2) CIDR Match + // 3) No Match for rIdx, rule := range global.Rules { - var head, cur *rlsconfv3.RateLimitDescriptor rateLimitPolicy := &rlsconfv3.RateLimitPolicy{ RequestsPerUnit: uint32(rule.Limit.Requests), Unit: rlsconfv3.RateLimitUnit(rlsconfv3.RateLimitUnit_value[strings.ToUpper(string(rule.Limit.Unit))]), } + // We use a chain structure to describe the matching descriptors for one rule. + // The RateLimitPolicy should be added to the last descriptor in the chain. + var head, cur *rlsconfv3.RateLimitDescriptor + for mIdx, match := range rule.HeaderMatches { pbDesc := new(rlsconfv3.RateLimitDescriptor) // Case for distinct match @@ -356,6 +363,9 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R } cur = pbDesc + + // Do not add the RateLimitPolicy to the last header match descriptor yet, + // as it is also possible that CIDR match descriptor also exist. } // EG supports two kinds of rate limit descriptors for the source IP: exact and distinct. @@ -386,12 +396,13 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R pbDesc.Value = rule.CIDRMatch.CIDR if cur != nil { + // The header match descriptor chain exist, add current + // descriptor to the chain. cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc} - cur = pbDesc } else { head = pbDesc - cur = head } + cur = pbDesc if rule.CIDRMatch.Distinct { pbDesc := new(rlsconfv3.RateLimitDescriptor) @@ -401,8 +412,8 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R } } - // Case when header/cidr match is not set and the rate limit is applied - // to all traffic. + // Case when both header and cidr match are not set and the ratelimit + // will be applied to all traffic. if !rule.IsMatchSet() { pbDesc := new(rlsconfv3.RateLimitDescriptor) // GenericKey case @@ -412,7 +423,7 @@ func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.R cur = head } - // Add the ratelimit values to the last descriptor + // Add the ratelimit policy to the last descriptor of chain. cur.RateLimit = rateLimitPolicy pbDescriptors = append(pbDescriptors, head) } diff --git a/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml b/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml index 3e5d2b35b5a..481b8598695 100644 --- a/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml +++ b/internal/xds/translator/testdata/in/ratelimit-config/header-and-cidr-matches.yaml @@ -17,6 +17,8 @@ routes: exact: "one" - name: "x-user-id" exact: "two" + - name: "x-org-id" + exact: "three" cidrMatch: cidr: 0.0.0.0/0 ip: 0.0.0.0 diff --git a/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml b/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml index 87596e15a59..83f5376dade 100644 --- a/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml +++ b/internal/xds/translator/testdata/out/ratelimit-config/header-and-cidr-matches.yaml @@ -13,15 +13,21 @@ descriptors: value: rule-0-match-1 rate_limit: null descriptors: - - key: masked_remote_address - value: 0.0.0.0/0 - rate_limit: - requests_per_unit: 5 - unit: SECOND - unlimited: false - name: "" - replaces: [] - descriptors: [] + - key: rule-0-match-2 + value: rule-0-match-2 + rate_limit: null + descriptors: + - key: masked_remote_address + value: 0.0.0.0/0 + rate_limit: + requests_per_unit: 5 + unit: SECOND + unlimited: false + name: "" + replaces: [] + descriptors: [] + shadow_mode: false + detailed_metric: false shadow_mode: false detailed_metric: false shadow_mode: false