fix: rate limit doesn't work with two(and more) listeners (#3085)

* fix: rate limit doesn't work with two listeners Signed-off-by: huabing zhao <[email protected]> * add e2e test for rate limit on multiple listeners Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Co-authored-by: Xunzhuo <[email protected]>
envoyproxy · Apr 8, 2024 · a5bedbc · a5bedbc
1 parent 43b7dab
commit a5bedbc
Show file tree

Hide file tree

Showing 12 changed files with 139 additions and 2 deletions.
diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go
@@ -268,7 +268,16 @@ func GetRateLimitServiceConfigStr(pbCfg *rlsconfv3.RateLimitConfig) (string, err
 	enc.SetIndent(2)
 	// Translate pb config to yaml
 	yamlRoot := config.ConfigXdsProtoToYaml(pbCfg)
-	err := enc.Encode(*yamlRoot)
+	rateLimitConfig := &struct {
+		Name        string
+		Domain      string
+		Descriptors []config.YamlDescriptor
+	}{
+		Name:        pbCfg.Name,
+		Domain:      yamlRoot.Domain,
+		Descriptors: yamlRoot.Descriptors,
+	}
+	err := enc.Encode(rateLimitConfig)
 	return buf.String(), err
 }
 
@@ -306,8 +315,10 @@ func BuildRateLimitServiceConfig(irListener *ir.HTTPListener) *rlsconfv3.RateLim
 		return nil
 	}
 
+	domain := getRateLimitDomain(irListener)
 	return &rlsconfv3.RateLimitConfig{
-		Domain:      getRateLimitDomain(irListener),
+		Name:        domain,
+		Domain:      domain,
 		Descriptors: pbDescriptors,
 	}
 }

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/distinct-match.yaml b/internal/xds/translator/testdata/out/ratelimit-config/distinct-match.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/distinct-remote-address-match.yaml b/internal/xds/translator/testdata/out/ratelimit-config/distinct-remote-address-match.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/empty-header-matches.yaml b/internal/xds/translator/testdata/out/ratelimit-config/empty-header-matches.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/masked-remote-address-match.yaml b/internal/xds/translator/testdata/out/ratelimit-config/masked-remote-address-match.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/...or/testdata/out/ratelimit-config/multiple-masked-remote-address-match-with-same-cidr.yaml b/...or/testdata/out/ratelimit-config/multiple-masked-remote-address-match-with-same-cidr.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/multiple-matches.yaml b/internal/xds/translator/testdata/out/ratelimit-config/multiple-matches.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/multiple-routes.yaml b/internal/xds/translator/testdata/out/ratelimit-config/multiple-routes.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/multiple-rules.yaml b/internal/xds/translator/testdata/out/ratelimit-config/multiple-rules.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/internal/xds/translator/testdata/out/ratelimit-config/value-match.yaml b/internal/xds/translator/testdata/out/ratelimit-config/value-match.yaml
@@ -1,3 +1,4 @@
+name: first-listener
 domain: first-listener
 descriptors:
   - key: first-route

diff --git a/test/e2e/testdata/ratelimit-multiple-listeners.yaml b/test/e2e/testdata/ratelimit-multiple-listeners.yaml
@@ -0,0 +1,54 @@
+apiVersion: gateway.networking.k8s.io/v1
+kind: Gateway
+metadata:
+  name: eg-rate-limit
+  namespace: gateway-conformance-infra
+spec:
+  gatewayClassName: "{GATEWAY_CLASS_NAME}"
+  listeners:
+    - name: http-80
+      protocol: HTTP
+      port: 80
+    - name: http-8080
+      protocol: HTTP
+      port: 8080
+---
+apiVersion: gateway.networking.k8s.io/v1
+kind: HTTPRoute
+metadata:
+  name: cidr-ratelimit
+  namespace: gateway-conformance-infra
+spec:
+  parentRefs:
+    - name: eg-rate-limit
+  rules:
+    - matches:
+        - path:
+            type: PathPrefix
+            value: /
+      backendRefs:
+        - name: infra-backend-v1
+          port: 8080
+---
+apiVersion: gateway.envoyproxy.io/v1alpha1
+kind: BackendTrafficPolicy
+metadata:
+  name: ratelimit-all-ips
+  namespace: gateway-conformance-infra
+spec:
+  targetRef:
+    group: gateway.networking.k8s.io
+    kind: HTTPRoute
+    name: cidr-ratelimit
+    namespace: gateway-conformance-infra
+  rateLimit:
+    type: Global
+    global:
+      rules:
+        - clientSelectors:
+            - sourceCIDR:
+                value: 0.0.0.0/0
+                type: distinct
+          limit:
+            requests: 3
+            unit: Hour
diff --git a/test/e2e/tests/ratelimit.go b/test/e2e/tests/ratelimit.go
@@ -9,8 +9,11 @@
 package tests
 
 import (
+	"fmt"
+	"net"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/gateway-api/conformance/utils/http"
 	"sigs.k8s.io/gateway-api/conformance/utils/kubernetes"
@@ -22,6 +25,7 @@ func init() {
 	ConformanceTests = append(ConformanceTests, RateLimitCIDRMatchTest)
 	ConformanceTests = append(ConformanceTests, RateLimitHeaderMatchTest)
 	ConformanceTests = append(ConformanceTests, RateLimitBasedJwtClaimsTest)
+	ConformanceTests = append(ConformanceTests, RateLimitMultipleListenersTest)
 }
 
 var RateLimitCIDRMatchTest = suite.ConformanceTest{
@@ -285,6 +289,65 @@ var RateLimitBasedJwtClaimsTest = suite.ConformanceTest{
 	},
 }
 
+var RateLimitMultipleListenersTest = suite.ConformanceTest{
+	ShortName:   "RateLimitMultipleListeners",
+	Description: "Limit requests on multiple listeners",
+	Manifests:   []string{"testdata/ratelimit-multiple-listeners.yaml"},
+	Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
+		t.Run("block all ips on listener 80 and 8080", func(t *testing.T) {
+			ns := "gateway-conformance-infra"
+			routeNN := types.NamespacedName{Name: "cidr-ratelimit", Namespace: ns}
+			gwNN := types.NamespacedName{Name: "eg-rate-limit", Namespace: ns}
+			gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN)
+			gwIP, _, err := net.SplitHostPort(gwAddr)
+			require.NoError(t, err)
+
+			gwPorts := []string{"80", "8080"}
+			for _, port := range gwPorts {
+				gwAddr = fmt.Sprintf("%s:%s", gwIP, port)
+
+				ratelimitHeader := make(map[string]string)
+				expectOkResp := http.ExpectedResponse{
+					Request: http.Request{
+						Path: "/",
+					},
+					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: "/",
+					},
+					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 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)
+				}
+			}
+		})
+	},
+}
+
 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)