Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scale ratelimit with ingress pods #5113

Merged
merged 7 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/crd/bases/k8s.nginx.org_policies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ spec:
type: string
rejectCode:
type: integer
scale:
type: boolean
zoneSize:
type: string
type: object
Expand Down
2 changes: 2 additions & 0 deletions deploy/crds.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,8 @@ spec:
type: string
rejectCode:
type: integer
scale:
type: boolean
zoneSize:
type: string
type: object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions docs/content/configuration/policy-resource.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 8 additions & 0 deletions internal/configs/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
}

Expand Down
1 change: 1 addition & 0 deletions internal/configs/config_params.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
49 changes: 32 additions & 17 deletions internal/configs/configurator.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ type Configurator struct {
isLatencyMetricsEnabled bool
isReloadsEnabled bool
isDynamicSSLReloadEnabled bool
ingressControllerReplicas int
}

// ConfiguratorParams is a collection of parameters used for the
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
54 changes: 43 additions & 11 deletions internal/configs/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
})
}
}
Expand Down Expand Up @@ -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
}
120 changes: 120 additions & 0 deletions internal/configs/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
}
}
Loading
Loading