Scale ratelimit with ingress pods (#5113)

* Ratelimit Scaling for Ingresses

* Added ratelimit scaling for VS

* Testcases for ratelimit scaling

* Fixed linter errors

* Apply suggestions from code review

Co-authored-by: Jim Ryan <j.ryan@f5.com>
Signed-off-by: dbaumgarten <mail@dbaumgarten.net>

* Clarify documentation about ratelimit-scaling feature

---------

Signed-off-by: dbaumgarten <mail@dbaumgarten.net>
Co-authored-by: Jim Ryan <j.ryan@f5.com>

config/crd/bases/k8s.nginx.org_policies.yaml

@@ -172,6 +172,8 @@ spec:
                     type: string
                   rejectCode:
                     type: integer
+                  scale:
+                    type: boolean
                   zoneSize:
                     type: string
                 type: object

deploy/crds.yaml

@@ -374,6 +374,8 @@ spec:
                     type: string
                   rejectCode:
                     type: integer
+                  scale:
+                    type: boolean
                   zoneSize:
                     type: string
                 type: object

docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md

@@ -187,6 +187,7 @@ The table below summarizes the available annotations.
 |``nginx.org/limit-req-dry-run`` | N/A | Enables the dry run mode. In this mode, the rate limit is not actually applied, but the number of excessive requests is accounted as usual in the shared memory zone. | false | true |
 |``nginx.org/limit-req-log-level`` | N/A | Sets the desired logging level for cases when the server refuses to process requests due to rate exceeding, or delays request processing. Allowed values are info, notice, warn or error. | error | info |
 |``nginx.org/limit-req-reject-code`` | N/A | Sets the status code to return in response to rejected requests. Must fall into the range 400..599. | 429 | 503 |
+|``nginx.org/limit-req-scale`` | N/A | Enables a constant rate-limit by dividing the configured rate by the number of nginx-ingress pods currently serving traffic. This adjustment ensures that the rate-limit remains consistent, even as the number of nginx-pods fluctuates due to autoscaling. Note: This will not work properly if requests from a client are not evenly distributed accross all ingress pods (sticky sessions, long lived TCP-Connections with many requests etc.). In such cases using NGINX+'s zone-sync feature instead would give better results. | false | true |
 {{% /table %}}
 
 ### Snippets and Custom Templates

docs/content/configuration/policy-resource.md

@@ -124,6 +124,7 @@ rateLimit:
 |``dryRun`` | Enables the dry run mode. In this mode, the rate limit is not actually applied, but the number of excessive requests is accounted as usual in the shared memory zone. | ``bool`` | No |
 |``logLevel`` | Sets the desired logging level for cases when the server refuses to process requests due to rate exceeding, or delays request processing. Allowed values are ``info``, ``notice``, ``warn`` or ``error``. Default is ``error``. | ``string`` | No |
 |``rejectCode`` | Sets the status code to return in response to rejected requests. Must fall into the range ``400..599``. Default is ``503``. | ``int`` | No |
+|``scale`` | Enables a constant rate-limit by dividing the configured rate by the number of nginx-ingress pods currently serving traffic. This adjustment ensures that the rate-limit remains consistent, even as the number of nginx-pods fluctuates due to autoscaling. Note: This will not work properly if requests from a client are not evenly distributed accross all ingress pods (sticky sessions, long lived TCP-Connections with many requests etc.). In such cases using NGINX+'s zone-sync feature instead would give better results. | ``bool`` | No |
 {{% /table %}}
 
 > For each policy referenced in a VirtualServer and/or its VirtualServerRoutes, NGINX Ingress Controller will generate a single rate limiting zone defined by the [`limit_req_zone`](http://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone) directive. If two VirtualServer resources reference the same policy, NGINX Ingress Controller will generate two different rate limiting zones, one zone per VirtualServer.

internal/configs/annotations.go

@@ -90,6 +90,7 @@ var minionInheritanceList = map[string]bool{
 	"nginx.org/limit-req-dry-run":        true,
 	"nginx.org/limit-req-log-level":      true,
 	"nginx.org/limit-req-reject-code":    true,
+	"nginx.org/limit-req-scale":          true,
 }
 
 var validPathRegex = map[string]bool{
@@ -510,6 +511,13 @@ func parseRateLimitAnnotations(annotations map[string]string, cfgParams *ConfigP
 			cfgParams.LimitReqRejectCode = requestRateRejectCode
 		}
 	}
+	if requestRateScale, exists, err := GetMapKeyAsBool(annotations, "nginx.org/limit-req-scale", context); exists {
+		if err != nil {
+			errors = append(errors, err)
+		} else {
+			cfgParams.LimitReqScale = requestRateScale
+		}
+	}
 	return errors
 }
 

internal/configs/config_params.go

@@ -125,6 +125,7 @@ type ConfigParams struct {
 	LimitReqDryRun     bool
 	LimitReqLogLevel   string
 	LimitReqRejectCode int
+	LimitReqScale      bool
 }
 
 // StaticConfigParams holds immutable NGINX configuration parameters that affect the main NGINX config.

internal/configs/configurator.go

@@ -136,6 +136,7 @@ type Configurator struct {
 	isLatencyMetricsEnabled   bool
 	isReloadsEnabled          bool
 	isDynamicSSLReloadEnabled bool
+	ingressControllerReplicas int
 }
 
 // ConfiguratorParams is a collection of parameters used for the
@@ -391,15 +392,16 @@ func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) (bool, Warnings, e
 
 	isMinion := false
 	nginxCfg, warnings := generateNginxCfg(NginxCfgParams{
-		staticParams:         cnf.staticCfgParams,
-		ingEx:                ingEx,
-		apResources:          apResources,
-		dosResource:          dosResource,
-		isMinion:             isMinion,
-		isPlus:               cnf.isPlus,
-		baseCfgParams:        cnf.cfgParams,
-		isResolverConfigured: cnf.IsResolverConfigured(),
-		isWildcardEnabled:    cnf.isWildcardEnabled,
+		staticParams:              cnf.staticCfgParams,
+		ingEx:                     ingEx,
+		apResources:               apResources,
+		dosResource:               dosResource,
+		isMinion:                  isMinion,
+		isPlus:                    cnf.isPlus,
+		baseCfgParams:             cnf.cfgParams,
+		isResolverConfigured:      cnf.IsResolverConfigured(),
+		isWildcardEnabled:         cnf.isWildcardEnabled,
+		ingressControllerReplicas: cnf.ingressControllerReplicas,
 	})
 
 	name := objectMetaToFileName(&ingEx.Ingress.ObjectMeta)
@@ -454,14 +456,15 @@ func (cnf *Configurator) addOrUpdateMergeableIngress(mergeableIngs *MergeableIng
 	}
 
 	nginxCfg, warnings := generateNginxCfgForMergeableIngresses(NginxCfgParams{
-		mergeableIngs:        mergeableIngs,
-		apResources:          apResources,
-		dosResource:          dosResource,
-		baseCfgParams:        cnf.cfgParams,
-		isPlus:               cnf.isPlus,
-		isResolverConfigured: cnf.IsResolverConfigured(),
-		staticParams:         cnf.staticCfgParams,
-		isWildcardEnabled:    cnf.isWildcardEnabled,
+		mergeableIngs:             mergeableIngs,
+		apResources:               apResources,
+		dosResource:               dosResource,
+		baseCfgParams:             cnf.cfgParams,
+		isPlus:                    cnf.isPlus,
+		isResolverConfigured:      cnf.IsResolverConfigured(),
+		staticParams:              cnf.staticCfgParams,
+		isWildcardEnabled:         cnf.isWildcardEnabled,
+		ingressControllerReplicas: cnf.ingressControllerReplicas,
 	})
 
 	name := objectMetaToFileName(&mergeableIngs.Master.Ingress.ObjectMeta)
@@ -607,6 +610,7 @@ func (cnf *Configurator) addOrUpdateVirtualServer(virtualServerEx *VirtualServer
 	name := getFileNameForVirtualServer(virtualServerEx.VirtualServer)
 
 	vsc := newVirtualServerConfigurator(cnf.cfgParams, cnf.isPlus, cnf.IsResolverConfigured(), cnf.staticCfgParams, cnf.isWildcardEnabled, nil)
+	vsc.IngressControllerReplicas = cnf.ingressControllerReplicas
 	vsCfg, warnings := vsc.GenerateVirtualServerConfig(virtualServerEx, apResources, dosResources)
 	content, err := cnf.templateExecutorV2.ExecuteVirtualServerTemplate(&vsCfg)
 	if err != nil {
@@ -2009,3 +2013,14 @@ func (cnf *Configurator) DynamicSSLReloadEnabled() bool {
 func (cnf *Configurator) UpsertSplitClientsKeyVal(zoneName, key, value string) {
 	cnf.nginxManager.UpsertSplitClientsKeyVal(zoneName, key, value)
 }
+
+// GetIngressControllerReplicas returns the number of ingresscontroller-replicas (previously stored via SetIngressControllerReplicas)
+func (cnf *Configurator) GetIngressControllerReplicas() int {
+	return cnf.ingressControllerReplicas
+}
+
+// SetIngressControllerReplicas sets the number of ingresscontroller-replicas
+// Is used for calculating ratelimits
+func (cnf *Configurator) SetIngressControllerReplicas(replicas int) {
+	cnf.ingressControllerReplicas = replicas
+}

internal/configs/ingress.go

@@ -78,16 +78,17 @@ type MergeableIngresses struct {
 // NginxCfgParams is a collection of parameters
 // used by generateNginxCfg() and generateNginxCfgForMergeableIngresses()
 type NginxCfgParams struct {
-	staticParams         *StaticConfigParams
-	ingEx                *IngressEx
-	mergeableIngs        *MergeableIngresses
-	apResources          *AppProtectResources
-	dosResource          *appProtectDosResource
-	baseCfgParams        *ConfigParams
-	isMinion             bool
-	isPlus               bool
-	isResolverConfigured bool
-	isWildcardEnabled    bool
+	staticParams              *StaticConfigParams
+	ingEx                     *IngressEx
+	mergeableIngs             *MergeableIngresses
+	apResources               *AppProtectResources
+	dosResource               *appProtectDosResource
+	baseCfgParams             *ConfigParams
+	isMinion                  bool
+	isPlus                    bool
+	isResolverConfigured      bool
+	isWildcardEnabled         bool
+	ingressControllerReplicas int
 }
 
 //nolint:gocyclo
@@ -278,11 +279,15 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings)
 					RejectCode: cfgParams.LimitReqRejectCode,
 				}
 				if !limitReqZoneExists(limitReqZones, zoneName) {
+					rate := cfgParams.LimitReqRate
+					if cfgParams.LimitReqScale && p.ingressControllerReplicas > 0 {
+						rate = scaleRatelimit(rate, p.ingressControllerReplicas)
+					}
 					limitReqZones = append(limitReqZones, version1.LimitReqZone{
 						Name: zoneName,
 						Key:  cfgParams.LimitReqKey,
 						Size: cfgParams.LimitReqZoneSize,
-						Rate: cfgParams.LimitReqRate,
+						Rate: rate,
 					})
 				}
 			}
@@ -754,3 +759,30 @@ func GetBackendPortAsString(port networking.ServiceBackendPort) string {
 	}
 	return strconv.Itoa(int(port.Number))
 }
+
+// scaleRatelimit divides a given ratelimit by the given number of replicas, adjusting the unit and flooring the result as needed
+func scaleRatelimit(ratelimit string, replicas int) string {
+	if replicas < 1 {
+		return ratelimit
+	}
+
+	match := rateRegexp.FindStringSubmatch(ratelimit)
+	if match == nil {
+		return ratelimit
+	}
+
+	number, err := strconv.Atoi(match[1])
+	if err != nil {
+		return ratelimit
+	}
+
+	numberf := float64(number) / float64(replicas)
+
+	unit := match[2]
+	if unit == "r/s" && numberf < 1 {
+		numberf = numberf * 60
+		unit = "r/m"
+	}
+
+	return strconv.Itoa(int(numberf)) + unit
+}

internal/configs/ingress_test.go

@@ -1244,6 +1244,70 @@ func TestGenerateNginxCfgForMergeableIngressesForLimitReq(t *testing.T) {
 	}
 }
 
+func TestGenerateNginxCfgForLimitReqWithScaling(t *testing.T) {
+	t.Parallel()
+	cafeIngressEx := createCafeIngressEx()
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-rate"] = "200r/s"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-key"] = "${request_uri}"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-burst"] = "100"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-no-delay"] = "true"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-delay"] = "80"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-reject-code"] = "503"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-dry-run"] = "true"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-log-level"] = "info"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-zone-size"] = "11m"
+	cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-scale"] = "true"
+
+	isPlus := false
+	configParams := NewDefaultConfigParams(isPlus)
+
+	expectedZones := []version1.LimitReqZone{
+		{
+			Name: "default/cafe-ingress",
+			Key:  "${request_uri}",
+			Size: "11m",
+			Rate: "50r/s",
+		},
+	}
+
+	expectedReqs := &version1.LimitReq{
+		Zone:       "default/cafe-ingress",
+		Burst:      100,
+		Delay:      80,
+		NoDelay:    true,
+		DryRun:     true,
+		LogLevel:   "info",
+		RejectCode: 503,
+	}
+
+	result, warnings := generateNginxCfg(NginxCfgParams{
+		ingEx:                     &cafeIngressEx,
+		baseCfgParams:             configParams,
+		staticParams:              &StaticConfigParams{},
+		isPlus:                    isPlus,
+		ingressControllerReplicas: 4,
+	})
+
+	if !reflect.DeepEqual(result.LimitReqZones, expectedZones) {
+		t.Errorf("generateNginxCfg returned \n%v,  but expected \n%v", result.LimitReqZones, expectedZones)
+	}
+
+	for _, server := range result.Servers {
+		for _, location := range server.Locations {
+			if !reflect.DeepEqual(location.LimitReq, expectedReqs) {
+				t.Errorf("generateNginxCfg returned \n%v,  but expected \n%v", result.LimitReqZones, expectedZones)
+			}
+		}
+	}
+
+	if !reflect.DeepEqual(result.LimitReqZones, expectedZones) {
+		t.Errorf("generateNginxCfg returned \n%v,  but expected \n%v", result.LimitReqZones, expectedZones)
+	}
+	if len(warnings) != 0 {
+		t.Errorf("generateNginxCfg returned warnings: %v", warnings)
+	}
+}
+
 func createMergeableCafeIngress() *MergeableIngresses {
 	master := networking.Ingress{
 		ObjectMeta: meta_v1.ObjectMeta{
@@ -2379,3 +2443,59 @@ func TestGetBackendPortAsString(t *testing.T) {
 		}
 	}
 }
+
+func TestScaleRatelimit(t *testing.T) {
+	tests := []struct {
+		input    string
+		pods     int
+		expected string
+	}{
+		{
+			input:    "10r/s",
+			pods:     0,
+			expected: "10r/s",
+		},
+		{
+			input:    "10r/s",
+			pods:     1,
+			expected: "10r/s",
+		},
+		{
+			input:    "10r/s",
+			pods:     2,
+			expected: "5r/s",
+		},
+		{
+			input:    "10r/s",
+			pods:     3,
+			expected: "3r/s",
+		},
+		{
+			input:    "10r/s",
+			pods:     10,
+			expected: "1r/s",
+		},
+		{
+			input:    "10r/s",
+			pods:     20,
+			expected: "30r/m",
+		},
+		{
+			input:    "10r/m",
+			pods:     0,
+			expected: "10r/m",
+		},
+		{
+			input:    "10r/m",
+			pods:     1,
+			expected: "10r/m",
+		},
+	}
+
+	for _, testcase := range tests {
+		scaled := scaleRatelimit(testcase.input, testcase.pods)
+		if scaled != testcase.expected {
+			t.Errorf("scaleRatelimit(%s,%d) returned %s but expected %s", testcase.input, testcase.pods, scaled, testcase.expected)
+		}
+	}
+}