fix: ensure ANDed matches for ratelimit rules (envoyproxy#2607)

* move route descriptor to first and add e2e test for this Signed-off-by: shawnh2 <[email protected]> * rename global ratelimit e2e test name Signed-off-by: shawnh2 <[email protected]> * address comments Signed-off-by: shawnh2 <[email protected]> * correct comments Signed-off-by: shawnh2 <[email protected]> * fix failed ratelimit e2e test case Signed-off-by: shawnh2 <[email protected]> --------- Signed-off-by: shawnh2 <[email protected]>
vixns · Feb 15, 2024 · ac02437 · ac02437
1 parent b2bfdbf
commit ac02437
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 17 deletions.
diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go
@@ -152,8 +152,8 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 
 	// Rules are ORed
 	for rIdx, rule := range global.Rules {
-		var rlActions []*routev3.RateLimit_Action
 		// Matches are ANDed
+		rlActions := []*routev3.RateLimit_Action{routeDescriptor}
 		for mIdx, match := range rule.HeaderMatches {
 			// Case for distinct match
 			if match.Distinct {
@@ -167,7 +167,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 						},
 					},
 				}
-				rlActions = append(rlActions, routeDescriptor, action)
+				rlActions = append(rlActions, action)
 			} else {
 				// Setup HeaderValueMatch actions
 				descriptorKey := getRouteRuleDescriptor(rIdx, mIdx)
@@ -190,7 +190,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 						},
 					},
 				}
-				rlActions = append(rlActions, routeDescriptor, action)
+				rlActions = append(rlActions, action)
 			}
 		}
 
@@ -225,7 +225,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 					MaskedRemoteAddress: mra,
 				},
 			}
-			rlActions = append(rlActions, routeDescriptor, action)
+			rlActions = append(rlActions, action)
 
 			// Setup RemoteAddress action if distinct match is set
 			if rule.CIDRMatch.Distinct {
@@ -251,7 +251,7 @@ func buildRouteRateLimits(descriptorPrefix string, global *ir.GlobalRateLimit) [
 					},
 				},
 			}
-			rlActions = append(rlActions, routeDescriptor, action)
+			rlActions = append(rlActions, action)
 		}
 
 		rateLimit := &routev3.RateLimit{Actions: rlActions}
@@ -291,6 +291,7 @@ func BuildRateLimitServiceConfig(irListener *ir.HTTPListener) *rlsconfv3.RateLim
 			//     descriptors:
 			//       - key:   ${RouteRuleDescriptor}
 			//         value: ${RouteRuleDescriptor}
+			//       - ...
 			//
 			routeDescriptor := &rlsconfv3.RateLimitDescriptor{
 				Key:         getRouteDescriptor(route.Name),

diff --git a/...e2e/testdata/ratelimit-block-all-ips.yaml → test/e2e/testdata/ratelimit-cidr-match.yaml b/...e2e/testdata/ratelimit-block-all-ips.yaml → test/e2e/testdata/ratelimit-cidr-match.yaml
@@ -7,7 +7,7 @@ spec:
   targetRef:
     group: gateway.networking.k8s.io
     kind: HTTPRoute
-    name: http-ratelimit
+    name: cidr-ratelimit
     namespace: gateway-conformance-infra
   rateLimit:
     type: Global
@@ -24,7 +24,7 @@ spec:
 apiVersion: gateway.networking.k8s.io/v1
 kind: HTTPRoute
 metadata:
-  name: http-ratelimit
+  name: cidr-ratelimit
   namespace: gateway-conformance-infra
 spec:
   parentRefs:

diff --git a/test/e2e/testdata/ratelimit-header-match.yaml b/test/e2e/testdata/ratelimit-header-match.yaml
@@ -0,0 +1,43 @@
+apiVersion: gateway.envoyproxy.io/v1alpha1
+kind: BackendTrafficPolicy
+metadata:
+  name: ratelimit-anded-headers
+  namespace: gateway-conformance-infra
+spec:
+  targetRef:
+    group: gateway.networking.k8s.io
+    kind: HTTPRoute
+    name: header-ratelimit
+    namespace: gateway-conformance-infra
+  rateLimit:
+    type: Global
+    global:
+      rules:
+        - clientSelectors:
+            - headers:
+                - name: x-user-id
+                  type: Exact
+                  value: one
+                - name: x-user-org
+                  type: Exact
+                  value: acme
+          limit:
+            requests: 3
+            unit: Hour
+---
+apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+  name: header-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
@@ -19,18 +19,19 @@ import (
 )
 
 func init() {
-	ConformanceTests = append(ConformanceTests, RateLimitTest)
+	ConformanceTests = append(ConformanceTests, RateLimitCIDRMatchTest)
+	ConformanceTests = append(ConformanceTests, RateLimitHeaderMatchTest)
 	ConformanceTests = append(ConformanceTests, RateLimitBasedJwtClaimsTest)
 }
 
-var RateLimitTest = suite.ConformanceTest{
-	ShortName:   "RateLimit",
-	Description: "Limit all requests",
-	Manifests:   []string{"testdata/ratelimit-block-all-ips.yaml"},
+var RateLimitCIDRMatchTest = suite.ConformanceTest{
+	ShortName:   "RateLimitCIDRMatch",
+	Description: "Limit all requests that match CIDR",
+	Manifests:   []string{"testdata/ratelimit-cidr-match.yaml"},
 	Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
 		t.Run("block all ips", func(t *testing.T) {
 			ns := "gateway-conformance-infra"
-			routeNN := types.NamespacedName{Name: "http-ratelimit", Namespace: ns}
+			routeNN := types.NamespacedName{Name: "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)
 			ratelimitHeader := make(map[string]string)
@@ -63,12 +64,100 @@ var RateLimitTest = suite.ConformanceTest{
 			// keep sending requests till get 200 first, that will cost one 200
 			http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, expectOkResp)
 
-			// fire the rest request
+			// fire the rest of requests
 			if err := GotExactExpectedResponse(t, 2, suite.RoundTripper, expectOkReq, expectOkResp); err != nil {
-				t.Errorf("fail to get expected response at first three request: %v", err)
+				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("fail to get expected response at last fourth request: %v", err)
+				t.Errorf("failed to get expected response for the last (fourth) request: %v", err)
+			}
+		})
+	},
+}
+
+var RateLimitHeaderMatchTest = suite.ConformanceTest{
+	ShortName:   "RateLimitHeaderMatch",
+	Description: "Limit all requests that match headers",
+	Manifests:   []string{"testdata/ratelimit-header-match.yaml"},
+	Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
+		ns := "gateway-conformance-infra"
+		routeNN := types.NamespacedName{Name: "header-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 headers 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 one matched header cannot got limited", func(t *testing.T) {
+			requestHeaders := map[string]string{
+				"x-user-id": "one",
+			}
+
+			// 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)
 			}
 		})
 	},
@@ -164,7 +253,7 @@ var RateLimitBasedJwtClaimsTest = suite.ConformanceTest{
 			// keep sending requests till get 200 first, that will cost one 200
 			http.MakeRequestAndExpectEventuallyConsistentResponse(t, suite.RoundTripper, suite.TimeoutConfig, gwAddr, JwtOkResp)
 
-			// fire the rest request
+			// fire the rest of requests
 			if err := GotExactExpectedResponse(t, 2, suite.RoundTripper, JwtReq, JwtOkResp); err != nil {
 				t.Errorf("failed to get expected response at third request: %v", err)
 			}