Setting disabled = true on a route should disable the virtual host …

…global rate limit policy (#5657) Support disabling global rate limiting on individual routes by setting disabled=true. Fixes #5685. Signed-off-by: shadi-altarsha <shadi.altarsha@reddit.com>
projectcontour · Sep 25, 2023 · 1d0a774 · 1d0a774
1 parent 6219d90
commit 1d0a774
Show file tree

Hide file tree

Showing 11 changed files with 561 additions and 3 deletions.
diff --git a/changelogs/unreleased/5657-shadialtarsha-minor.md b/changelogs/unreleased/5657-shadialtarsha-minor.md
@@ -0,0 +1,38 @@
+## Specific routes can now opt out of the virtual host's global rate limit policy
+
+Setting `rateLimitPolicy.global.disabled` flag to true on a specific route now disables the global rate limit policy inherited from the virtual host for that route.
+
+### Sample Configurations
+In the example below, `/foo` route is opted out from the global rate limit policy defined by the virtualhost.
+#### httpproxy.yaml
+```yaml
+apiVersion: projectcontour.io/v1
+kind: HTTPProxy
+metadata:
+  name: echo
+spec:
+  virtualhost:
+    fqdn: local.projectcontour.io
+    rateLimitPolicy:
+      global:
+        descriptors:
+          - entries:
+            - remoteAddress: {}
+            - genericKey:
+                key: vhost
+                value: local.projectcontour.io
+  routes:
+    - conditions:
+        - prefix: /
+      services:
+        - name: ingress-conformance-echo
+          port: 80
+    - conditions:
+        - prefix: /foo
+      rateLimitPolicy:
+        global:
+          disabled: true
+      services:
+        - name: ingress-conformance-echo
+          port: 80
+```
diff --git a/internal/dag/dag.go b/internal/dag/dag.go
@@ -335,6 +335,9 @@ type Route struct {
 	// RateLimitPolicy defines if/how requests for the route are rate limited.
 	RateLimitPolicy *RateLimitPolicy
 
+	// RateLimitPerRoute defines how the route should handle rate limits defined by the virtual host.
+	RateLimitPerRoute *RateLimitPerRoute
+
 	// RequestHashPolicies is a list of policies for configuring hashes on
 	// request attributes.
 	RequestHashPolicies []RequestHashPolicy
@@ -571,6 +574,24 @@ type HeaderValueMatchDescriptorEntry struct {
 	Value       string
 }
 
+type VhRateLimitsType int
+
+const (
+	// VhRateLimitsOverride (Default) will use the virtual host rate limits unless the route has a rate limit policy.
+	VhRateLimitsOverride VhRateLimitsType = iota
+
+	// VhRateLimitsInclude will use the virtual host rate limits even if the route has a rate limit policy.
+	VhRateLimitsInclude
+
+	// VhRateLimitsIgnore will ignore the virtual host rate limits even if the route does not have a rate limit policy.
+	VhRateLimitsIgnore
+)
+
+// RateLimitPerRoute configures how the route should handle the rate limits defined by the virtual host.
+type RateLimitPerRoute struct {
+	VhRateLimits VhRateLimitsType
+}
+
 // RemoteAddressDescriptorEntry configures a descriptor entry
 // that contains the remote address (i.e. client IP).
 type RemoteAddressDescriptorEntry struct{}

diff --git a/internal/dag/httpproxy_processor.go b/internal/dag/httpproxy_processor.go
@@ -798,6 +798,8 @@ func (p *HTTPProxyProcessor) computeRoutes(
 			return nil
 		}
 
+		vrl := rateLimitPerRoute(route.RateLimitPolicy)
+
 		requestHashPolicies, lbPolicy := loadBalancerRequestHashPolicies(route.LoadBalancerPolicy, validCond)
 
 		redirectPolicy, err := redirectRoutePolicy(route.RequestRedirectPolicy)
@@ -807,7 +809,7 @@ func (p *HTTPProxyProcessor) computeRoutes(
 			return nil
 		}
 
-		internalRedirectPolicy := internalRedirectPolicy(route.InternalRedirectPolicy)
+		irp := internalRedirectPolicy(route.InternalRedirectPolicy)
 
 		directPolicy := directResponsePolicy(route.DirectResponsePolicy)
 
@@ -823,10 +825,11 @@ func (p *HTTPProxyProcessor) computeRoutes(
 			ResponseHeadersPolicy:     respHP,
 			CookieRewritePolicies:     cookieRP,
 			RateLimitPolicy:           rlp,
+			RateLimitPerRoute:         vrl,
 			RequestHashPolicies:       requestHashPolicies,
 			Redirect:                  redirectPolicy,
 			DirectResponse:            directPolicy,
-			InternalRedirectPolicy:    internalRedirectPolicy,
+			InternalRedirectPolicy:    irp,
 		}
 
 		if p.SetSourceMetadataOnRoutes {
@@ -1968,3 +1971,14 @@ func slowStartConfig(slowStart *contour_api_v1.SlowStartPolicy) (*SlowStartConfi
 		MinWeightPercent: slowStart.MinimumWeightPercent,
 	}, nil
 }
+
+func rateLimitPerRoute(in *contour_api_v1.RateLimitPolicy) *RateLimitPerRoute {
+	// Ignore the virtual host global rate limit policy if disabled is true
+	if in != nil && in.Global != nil && in.Global.Disabled {
+		return &RateLimitPerRoute{
+			VhRateLimits: VhRateLimitsIgnore,
+		}
+	}
+
+	return nil
+}
diff --git a/internal/dag/httpproxy_processor_test.go b/internal/dag/httpproxy_processor_test.go
@@ -1386,3 +1386,116 @@ func TestValidateVirtualHostRateLimitPolicy(t *testing.T) {
 		})
 	}
 }
+
+func TestRateLimitPerRoute(t *testing.T) {
+	tests := map[string]struct {
+		httpproxy *contour_api_v1.HTTPProxy
+		want      *RateLimitPerRoute
+	}{
+		"route doesn't disable the global rate limit functionality": {
+			httpproxy: &contour_api_v1.HTTPProxy{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "ns",
+				},
+				Spec: contour_api_v1.HTTPProxySpec{
+					VirtualHost: &contour_api_v1.VirtualHost{
+						Fqdn: "foo.projectcontour.io",
+					},
+					Routes: []contour_api_v1.Route{
+						{
+							Services: []contour_api_v1.Service{
+								{
+									Name: "foo",
+									Port: 80,
+								},
+							},
+							Conditions: []contour_api_v1.MatchCondition{
+								{
+									Prefix: "/bar",
+								},
+							},
+						},
+					},
+				},
+			},
+			want: nil,
+		},
+		"route disables the global rate limit functionality": {
+			httpproxy: &contour_api_v1.HTTPProxy{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "ns",
+				},
+				Spec: contour_api_v1.HTTPProxySpec{
+					VirtualHost: &contour_api_v1.VirtualHost{
+						Fqdn: "foo.projectcontour.io",
+					},
+					Routes: []contour_api_v1.Route{
+						{
+							Services: []contour_api_v1.Service{
+								{
+									Name: "foo",
+									Port: 80,
+								},
+							},
+							Conditions: []contour_api_v1.MatchCondition{
+								{
+									Prefix: "/bar",
+								},
+							},
+							RateLimitPolicy: &contour_api_v1.RateLimitPolicy{
+								Global: &contour_api_v1.GlobalRateLimitPolicy{
+									Disabled: true,
+								},
+							},
+						},
+					},
+				},
+			},
+			want: &RateLimitPerRoute{
+				VhRateLimits: 2,
+			},
+		},
+		"route doesn't disable the global rate limit functionality explicitly": {
+			httpproxy: &contour_api_v1.HTTPProxy{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "ns",
+				},
+				Spec: contour_api_v1.HTTPProxySpec{
+					VirtualHost: &contour_api_v1.VirtualHost{
+						Fqdn: "foo.projectcontour.io",
+					},
+					Routes: []contour_api_v1.Route{
+						{
+							Services: []contour_api_v1.Service{
+								{
+									Name: "foo",
+									Port: 80,
+								},
+							},
+							Conditions: []contour_api_v1.MatchCondition{
+								{
+									Prefix: "/bar",
+								},
+							},
+							RateLimitPolicy: &contour_api_v1.RateLimitPolicy{
+								Global: &contour_api_v1.GlobalRateLimitPolicy{
+									Disabled: false,
+								},
+							},
+						},
+					},
+				},
+			},
+			want: nil,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			for _, route := range tc.httpproxy.Spec.Routes {
+				got := rateLimitPerRoute(route.RateLimitPolicy)
+				require.Equal(t, tc.want, got)
+			}
+		})
+	}
+}
diff --git a/internal/envoy/v3/ratelimit.go b/internal/envoy/v3/ratelimit.go
@@ -163,3 +163,12 @@ func enableXRateLimitHeaders(enable bool) ratelimit_filter_v3.RateLimit_XRateLim
 	}
 	return ratelimit_filter_v3.RateLimit_OFF
 }
+
+// rateLimitPerRoute returns a per-route config to configure vhost rate limits.
+func rateLimitPerRoute(r *dag.RateLimitPerRoute) *anypb.Any {
+	return protobuf.MustMarshalAny(
+		&ratelimit_filter_v3.RateLimitPerRoute{
+			VhRateLimits: ratelimit_filter_v3.RateLimitPerRoute_VhRateLimitsOptions(r.VhRateLimits),
+		},
+	)
+}
diff --git a/internal/envoy/v3/ratelimit_test.go b/internal/envoy/v3/ratelimit_test.go
@@ -411,3 +411,40 @@ func TestGlobalRateLimitFilter(t *testing.T) {
 		})
 	}
 }
+
+func TestRateLimitPerRoute(t *testing.T) {
+	tests := map[string]struct {
+		name string
+		cfg  *dag.RateLimitPerRoute
+		want *anypb.Any
+	}{
+		"VhRateLimits in Override mode": {
+			cfg: &dag.RateLimitPerRoute{
+				VhRateLimits: dag.VhRateLimitsOverride,
+			},
+			want: protobuf.MustMarshalAny(&ratelimit_filter_v3.RateLimitPerRoute{
+				VhRateLimits: 0,
+			}),
+		}, "VhRateLimits in Include mode": {
+			cfg: &dag.RateLimitPerRoute{
+				VhRateLimits: dag.VhRateLimitsInclude,
+			},
+			want: protobuf.MustMarshalAny(&ratelimit_filter_v3.RateLimitPerRoute{
+				VhRateLimits: 1,
+			}),
+		}, "VhRateLimits in Ignore mode": {
+			cfg: &dag.RateLimitPerRoute{
+				VhRateLimits: dag.VhRateLimitsIgnore,
+			},
+			want: protobuf.MustMarshalAny(&ratelimit_filter_v3.RateLimitPerRoute{
+				VhRateLimits: 2,
+			}),
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, tc.want, rateLimitPerRoute(tc.cfg))
+		})
+	}
+}
diff --git a/internal/envoy/v3/route.go b/internal/envoy/v3/route.go
@@ -145,6 +145,10 @@ func buildRoute(dagRoute *dag.Route, vhostName string, secure bool) *envoy_route
 			route.TypedPerFilterConfig["envoy.filters.http.local_ratelimit"] = LocalRateLimitConfig(dagRoute.RateLimitPolicy.Local, "vhost."+vhostName)
 		}
 
+		if dagRoute.RateLimitPerRoute != nil {
+			route.TypedPerFilterConfig["envoy.filters.http.ratelimit"] = rateLimitPerRoute(dagRoute.RateLimitPerRoute)
+		}
+
 		// Apply per-route authorization policy modifications.
 		if dagRoute.AuthDisabled {
 			route.TypedPerFilterConfig["envoy.filters.http.ext_authz"] = routeAuthzDisabled()

diff --git a/internal/featuretests/v3/globalratelimit_test.go b/internal/featuretests/v3/globalratelimit_test.go
@@ -188,7 +188,6 @@ func globalRateLimitNoRateLimitsDefined(t *testing.T, rh ResourceEventHandlerWra
 			),
 		})
 	}
-
 }
 
 func globalRateLimitVhostRateLimitDefined(t *testing.T, rh ResourceEventHandlerWrapper, c *Contour, tls tlsConfig) {