fix: ratelimit not working with both headers and cidr matches

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,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
+		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)
 	}
 

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

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

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