fix: ratelimit not working with both headers and cidr matches (envoyp…

…roxy#4377) * fix ratelimit descriptors do not respect both headers and cidr match for one rule Signed-off-by: shawnh2 <[email protected]> * fix gen-check and lint Signed-off-by: shawnh2 <[email protected]> * fix ratelimit e2e test Signed-off-by: shawnh2 <[email protected]> * add more comment and update test case Signed-off-by: shawnh2 <[email protected]> --------- Signed-off-by: shawnh2 <[email protected]> Co-authored-by: Huabing Zhao <[email protected]> (cherry picked from commit 66c0b51) Signed-off-by: Guy Daich <[email protected]>
guydc · Oct 30, 2024 · 1b4c7d9 · 1b4c7d9
1 parent 8f6e69b
commit 1b4c7d9
Show file tree

Hide file tree

Showing 10 changed files with 633 additions and 40 deletions.
diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go
@@ -157,19 +157,19 @@ 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,
 							DescriptorKey: descriptorKey,
 						},
 					},
 				}
-				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,8 +241,8 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 			}
 		}
 
-		// Case when header 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,22 +329,21 @@ func BuildRateLimitServiceConfig(irListener *ir.HTTPListener) *rlsconfv3.RateLim
 func buildRateLimitServiceDescriptors(global *ir.GlobalRateLimit) []*rlsconfv3.RateLimitDescriptor {
 	pbDescriptors := make([]*rlsconfv3.RateLimitDescriptor, 0, len(global.Rules))
 
+	// 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
-		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
+		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
@@ -357,22 +356,16 @@ 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 {
 				cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc}
 			}
 
 			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.
@@ -401,25 +394,37 @@ 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 {
+				// The header match descriptor chain exist, add current
+				// descriptor to the chain.
+				cur.Descriptors = []*rlsconfv3.RateLimitDescriptor{pbDesc}
+			} else {
+				head = pbDesc
 			}
+			cur = pbDesc
 
 			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 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
+			pbDesc.Key = getRouteRuleDescriptor(rIdx, -1)
+			pbDesc.Value = getRouteRuleDescriptor(rIdx, -1)
 			head = pbDesc
 			cur = head
 		}
 
+		// 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
@@ -0,0 +1,38 @@
+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"
+          - name: "x-org-id"
+            exact: "three"
+          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
@@ -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
@@ -0,0 +1,38 @@
+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: 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
+            detailed_metric: false
+        shadow_mode: false
+        detailed_metric: false
+    shadow_mode: false
+    detailed_metric: false