From f92f795302b584d4cdb60c56309bfdabf151530d Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 15 Nov 2023 13:26:56 +0000 Subject: [PATCH 1/5] Add annotations for controlling request rate limiting --- ...advanced-configuration-with-annotations.md | 12 + internal/configs/annotations.go | 34 ++ internal/configs/config_params.go | 8 + internal/configs/ingress.go | 31 ++ internal/configs/ingress_test.go | 130 ++++++++ internal/configs/version1/config.go | 17 + .../configs/version1/nginx-plus.ingress.tmpl | 9 + internal/configs/version1/nginx.ingress.tmpl | 9 + internal/configs/version1/template_test.go | 293 ++++++++++++++++++ 9 files changed, 543 insertions(+) diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md index 2d89fb33d9..6a2c2c13b2 100644 --- a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md +++ b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md @@ -175,6 +175,18 @@ The table below summarizes the available annotations. |``nginx.org/use-cluster-ip`` | N/A | Enables using the Cluster IP and port of the service instead of the default behavior of using the IP and port of the pods. When this field is enabled, the fields that configure NGINX behavior related to multiple upstream servers (like ``lb-method`` and ``next-upstream``) will have no effect, as NGINX Ingress Controller will configure NGINX with only one upstream server that will match the service Cluster IP. | ``False`` | | {{% /table %}} +### Rate limiting + +{{% table %}} +|Annotation | ConfigMap Key | Description | Default | Example | +| ---| ---| ---| ---| --- | +|``nginx.org/limit-req`` | N/A | Enables request-rate-limiting for this ingress by creating a [limit_req_zone](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone) and matching [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) for each location. All servers/locations of one ingress share the same zone. | N/A | 200r/s | +|``nginx.org/limit-req-zone-size`` | N/A | Configures the size of the created [limit_req_zone](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone). | 10m | 20m | +|``nginx.org/limit-req-burst`` | N/A | Configures the burst-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. | N/A | 100 | +|``nginx.org/limit-req-delay`` | N/A | Configures the delay-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. 0 means nodelay. | 0 | 100 | +|``nginx.org/limit-req-status`` | N/A | Configures the [limit_req_status](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_status) directive. | 429 | 503 | +{{% /table %}} + ### Snippets and Custom Templates {{% table %}} diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index a73dbf3d6a..76c01f0d0c 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -78,6 +78,11 @@ var minionInheritanceList = map[string]bool{ "nginx.org/max-fails": true, "nginx.org/max-conns": true, "nginx.org/fail-timeout": true, + "nginx.org/limit-req": true, + "nginx.org/limit-req-zone-size": true, + "nginx.org/limit-req-burst": true, + "nginx.org/limit-req-delay": true, + "nginx.org/limit-req-status": true, } var validPathRegex = map[string]bool{ @@ -413,6 +418,35 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool cfgParams.UseClusterIP = useClusterIP } } + + if requestRateLimit, exists := ingEx.Ingress.Annotations["nginx.org/limit-req"]; exists { + cfgParams.LimitReqRate = requestRateLimit + } + if requestRateZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-zone-size"]; exists { + cfgParams.LimitReqZoneSize = requestRateZoneSize + } + if requestRateBurst, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-burst", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.LimitReqBurst = requestRateBurst + } + } + if requestRateDelay, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-delay", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.LimitReqDelay = requestRateDelay + } + } + if requestRateStatus, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-status", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.LimitReqStatus = requestRateStatus + } + } + return cfgParams } diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index 1028035cca..525f943111 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -113,6 +113,12 @@ type ConfigParams struct { SSLPorts []int SpiffeServerCerts bool + + LimitReqRate string + LimitReqZoneSize string + LimitReqBurst int + LimitReqDelay int + LimitReqStatus int } // StaticConfigParams holds immutable NGINX configuration parameters that affect the main NGINX config. @@ -191,6 +197,8 @@ func NewDefaultConfigParams(isPlus bool) *ConfigParams { MainKeepaliveRequests: 100, VariablesHashBucketSize: 256, VariablesHashMaxSize: 1024, + LimitReqZoneSize: "10m", + LimitReqStatus: 429, } } diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index ff09975fba..71218c6e15 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -128,6 +128,7 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings) allWarnings := newWarnings() var servers []version1.Server + var limitReqZones []version1.LimitReqZone for _, rule := range p.ingEx.Ingress.Spec.Rules { // skipping invalid hosts @@ -265,6 +266,23 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings) allWarnings.Add(warnings) } + if cfgParams.LimitReqRate != "" { + zoneName := p.ingEx.Ingress.Namespace + "/" + p.ingEx.Ingress.Name + loc.LimitReq = &version1.LimitReq{ + Zone: zoneName, + Burst: cfgParams.LimitReqBurst, + Delay: cfgParams.LimitReqDelay, + Status: cfgParams.LimitReqStatus, + } + if !limitReqZoneExists(limitReqZones, zoneName) { + limitReqZones = append(limitReqZones, version1.LimitReqZone{ + Name: zoneName, + Size: cfgParams.LimitReqZoneSize, + Rate: cfgParams.LimitReqRate, + }) + } + } + locations = append(locations, loc) if loc.Path == "/" { @@ -317,6 +335,7 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings) SpiffeClientCerts: p.staticParams.NginxServiceMesh && !cfgParams.SpiffeServerCerts, DynamicSSLReloadEnabled: p.staticParams.DynamicSSLReload, StaticSSLPath: p.staticParams.StaticSSLPath, + LimitReqZones: limitReqZones, }, allWarnings } @@ -609,6 +628,7 @@ func generateNginxCfgForMergeableIngresses(p NginxCfgParams) (version1.IngressNg var locations []version1.Location var upstreams []version1.Upstream healthChecks := make(map[string]version1.HealthCheck) + var limitReqZones []version1.LimitReqZone var keepalive string // replace master with a deepcopy because we will modify it @@ -704,6 +724,7 @@ func generateNginxCfgForMergeableIngresses(p NginxCfgParams) (version1.IngressNg } upstreams = append(upstreams, nginxCfg.Upstreams...) + limitReqZones = append(limitReqZones, nginxCfg.LimitReqZones...) } masterServer.HealthChecks = healthChecks @@ -717,9 +738,19 @@ func generateNginxCfgForMergeableIngresses(p NginxCfgParams) (version1.IngressNg SpiffeClientCerts: p.staticParams.NginxServiceMesh && !p.baseCfgParams.SpiffeServerCerts, DynamicSSLReloadEnabled: p.staticParams.DynamicSSLReload, StaticSSLPath: p.staticParams.StaticSSLPath, + LimitReqZones: limitReqZones, }, warnings } +func limitReqZoneExists(zones []version1.LimitReqZone, zoneName string) bool { + for _, zone := range zones { + if zone.Name == zoneName { + return true + } + } + return false +} + func isSSLEnabled(isSSLService bool, cfgParams ConfigParams, staticCfgParams *StaticConfigParams) bool { return isSSLService || staticCfgParams.NginxServiceMesh && !cfgParams.SpiffeServerCerts } diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index decf9f4a2f..ac265d2641 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -910,6 +910,136 @@ func TestGenerateNginxCfgWithUseClusterIP(t *testing.T) { } } +func TestGenerateNginxCfgForLimitReq(t *testing.T) { + t.Parallel() + cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req"] = "200r/s" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-burst"] = "100" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-delay"] = "80" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-status"] = "429" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-zone-size"] = "11m" + + isPlus := false + configParams := NewDefaultConfigParams(isPlus) + + expectedZones := []version1.LimitReqZone{ + { + Name: "default/cafe-ingress", + Size: "11m", + Rate: "200r/s", + }, + } + + expectedReqs := &version1.LimitReq{ + Zone: "default/cafe-ingress", + Burst: 100, + Delay: 80, + Status: 429, + } + + result, warnings := generateNginxCfg(NginxCfgParams{ + ingEx: &cafeIngressEx, + baseCfgParams: configParams, + staticParams: &StaticConfigParams{}, + isPlus: isPlus, + }) + + 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 TestGenerateNginxCfgForMergeableIngressesForLimitReq(t *testing.T) { + t.Parallel() + mergeableIngresses := createMergeableCafeIngress() + + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req"] = "200r/s" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-burst"] = "100" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-delay"] = "80" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-status"] = "429" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-zone-size"] = "11m" + + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req"] = "400r/s" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-burst"] = "200" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-delay"] = "160" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-status"] = "503" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-zone-size"] = "12m" + + expectedZones := []version1.LimitReqZone{ + { + Name: "default/cafe-ingress-coffee-minion", + Size: "11m", + Rate: "200r/s", + }, + { + Name: "default/cafe-ingress-tea-minion", + Size: "12m", + Rate: "400r/s", + }, + } + + expectedReqs := map[string]*version1.LimitReq{ + "cafe-ingress-coffee-minion": { + Zone: "default/cafe-ingress-coffee-minion", + Burst: 100, + Delay: 80, + Status: 429, + }, + "cafe-ingress-tea-minion": { + Zone: "default/cafe-ingress-tea-minion", + Burst: 200, + Delay: 160, + Status: 503, + }, + } + + isPlus := false + + configParams := NewDefaultConfigParams(isPlus) + + result, warnings := generateNginxCfgForMergeableIngresses(NginxCfgParams{ + mergeableIngs: mergeableIngresses, + baseCfgParams: configParams, + isPlus: isPlus, + staticParams: &StaticConfigParams{}, + }) + + 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 { + expectedLimitReq := expectedReqs[location.MinionIngress.Name] + if !reflect.DeepEqual(location.LimitReq, expectedLimitReq) { + t.Errorf("generateNginxCfg returned \n%v, but expected \n%v", location.LimitReq, expectedLimitReq) + } + } + } + + 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{ diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index 888c19b336..bc46bfa413 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -19,6 +19,7 @@ type IngressNginxConfig struct { SpiffeClientCerts bool DynamicSSLReloadEnabled bool StaticSSLPath string + LimitReqZones []LimitReqZone } // Ingress holds information about an Ingress resource. @@ -63,6 +64,13 @@ type HealthCheck struct { TimeoutSeconds int64 } +// LimitReqZone describes a zone used for request rate limiting +type LimitReqZone struct { + Name string + Size string + Rate string +} + // Server describes an NGINX server. type Server struct { ServerSnippets []string @@ -138,6 +146,14 @@ type JWTAuth struct { RedirectLocationName string } +// LimitReq configures a request rate limit +type LimitReq struct { + Zone string + Burst int + Delay int + Status int +} + // Location describes an NGINX location. type Location struct { LocationSnippets []string @@ -159,6 +175,7 @@ type Location struct { JWTAuth *JWTAuth BasicAuth *BasicAuth ServiceName string + LimitReq *LimitReq MinionIngress *Ingress } diff --git a/internal/configs/version1/nginx-plus.ingress.tmpl b/internal/configs/version1/nginx-plus.ingress.tmpl index bcf82ffb71..5fa7e5361d 100644 --- a/internal/configs/version1/nginx-plus.ingress.tmpl +++ b/internal/configs/version1/nginx-plus.ingress.tmpl @@ -19,6 +19,10 @@ upstream {{$upstream.Name}} { } {{- end}} +{{range $limitReqZone := .LimitReqZones}} +limit_req_zone $binary_remote_addr zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; +{{end}} + {{range $server := .Servers}} server { {{- if $server.SpiffeCerts}} @@ -310,6 +314,11 @@ server { proxy_pass http://{{$location.Upstream.Name}}{{$location.Rewrite}}; {{- end}} {{- end}} + + {{with $location.LimitReq}} + limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{else}}nodelay{{end}}; + {{if $location.LimitReq.Status}}limit_req_status {{$location.LimitReq.Status}};{{end}} + {{end}} } {{end -}} {{- if $server.GRPCOnly}} diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl index ce88993c55..8dc09ca1f1 100644 --- a/internal/configs/version1/nginx.ingress.tmpl +++ b/internal/configs/version1/nginx.ingress.tmpl @@ -10,6 +10,10 @@ upstream {{$upstream.Name}} { } {{end -}} +{{range $limitReqZone := .LimitReqZones}} +limit_req_zone $binary_remote_addr zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; +{{end}} + {{range $server := .Servers}} server { {{- if $server.SpiffeCerts}} @@ -223,6 +227,11 @@ server { proxy_pass http://{{$location.Upstream.Name}}{{$location.Rewrite}}; {{- end}} {{- end}} + + {{with $location.LimitReq}} + limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{else}}nodelay{{end}}; + {{if $location.LimitReq.Status}}limit_req_status {{$location.LimitReq.Status}};{{end}} + {{end}} } {{end -}} {{- if $server.GRPCOnly}} diff --git a/internal/configs/version1/template_test.go b/internal/configs/version1/template_test.go index 9172863df2..296c4b9833 100644 --- a/internal/configs/version1/template_test.go +++ b/internal/configs/version1/template_test.go @@ -2,6 +2,7 @@ package version1 import ( "bytes" + "strconv" "strings" "testing" "text/template" @@ -972,6 +973,126 @@ func TestExecuteTemplate_ForIngressForNGINXWithHTTP2Off(t *testing.T) { } } +func TestExecuteTemplate_ForIngressForNGINXWithRequestRateLimit(t *testing.T) { + t.Parallel() + + tmpl := newNGINXIngressTmpl(t) + buf := &bytes.Buffer{} + + err := tmpl.Execute(buf, ingressCfgRequestRateLimit) + t.Log(buf.String()) + if err != nil { + t.Fatal(err) + } + ingConf := buf.String() + + limitReq := ingressCfgRequestRateLimit.Servers[0].Locations[0].LimitReq + + wantDirectives := []string{ + "limit_req_zone $binary_remote_addr zone=default/myingress:10m rate=200r/s;", + "limit_req zone=default/myingress burst=" + strconv.Itoa(limitReq.Burst) + " delay=" + strconv.Itoa(limitReq.Delay) + ";", + "limit_req_status " + strconv.Itoa(limitReq.Status) + ";", + } + + for _, want := range wantDirectives { + if !strings.Contains(ingConf, want) { + t.Errorf("want %q in generated config", want) + } + } +} + +func TestExecuteTemplate_ForIngressForNGINXWithRequestRateLimitMinions(t *testing.T) { + t.Parallel() + + tmpl := newNGINXIngressTmpl(t) + buf := &bytes.Buffer{} + + err := tmpl.Execute(buf, ingressCfgRequestRateLimitMinions) + t.Log(buf.String()) + if err != nil { + t.Fatal(err) + } + ingConf := buf.String() + + limitReqTea := ingressCfgRequestRateLimitMinions.Servers[0].Locations[0].LimitReq + limitReqCoffee := ingressCfgRequestRateLimitMinions.Servers[0].Locations[1].LimitReq + + wantDirectives := []string{ + "limit_req_zone $binary_remote_addr zone=default/tea-minion:10m rate=200r/s;", + "limit_req_zone $binary_remote_addr zone=default/coffee-minion:20m rate=400r/s;", + "limit_req zone=" + limitReqTea.Zone + " burst=" + strconv.Itoa(limitReqTea.Burst) + " delay=" + strconv.Itoa(limitReqTea.Delay) + ";", + "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " delay=" + strconv.Itoa(limitReqCoffee.Delay) + ";", + "limit_req_status " + strconv.Itoa(limitReqTea.Status) + ";", + "limit_req_status " + strconv.Itoa(limitReqCoffee.Status) + ";", + } + + for _, want := range wantDirectives { + if !strings.Contains(ingConf, want) { + t.Errorf("want %q in generated config", want) + } + } +} + +func TestExecuteTemplate_ForIngressForNGINXPlusWithRequestRateLimit(t *testing.T) { + t.Parallel() + + tmpl := newNGINXPlusIngressTmpl(t) + buf := &bytes.Buffer{} + + err := tmpl.Execute(buf, ingressCfgRequestRateLimit) + t.Log(buf.String()) + if err != nil { + t.Fatal(err) + } + ingConf := buf.String() + + limitReq := ingressCfgRequestRateLimit.Servers[0].Locations[0].LimitReq + + wantDirectives := []string{ + "limit_req_zone $binary_remote_addr zone=default/myingress:10m rate=200r/s;", + "limit_req zone=default/myingress burst=" + strconv.Itoa(limitReq.Burst) + " delay=" + strconv.Itoa(limitReq.Delay) + ";", + "limit_req_status " + strconv.Itoa(limitReq.Status) + ";", + } + + for _, want := range wantDirectives { + if !strings.Contains(ingConf, want) { + t.Errorf("want %q in generated config", want) + } + } +} + +func TestExecuteTemplate_ForIngressForNGINXPlusWithRequestRateLimitMinions(t *testing.T) { + t.Parallel() + + tmpl := newNGINXPlusIngressTmpl(t) + buf := &bytes.Buffer{} + + err := tmpl.Execute(buf, ingressCfgRequestRateLimitMinions) + t.Log(buf.String()) + if err != nil { + t.Fatal(err) + } + ingConf := buf.String() + + limitReqTea := ingressCfgRequestRateLimitMinions.Servers[0].Locations[0].LimitReq + limitReqCoffee := ingressCfgRequestRateLimitMinions.Servers[0].Locations[1].LimitReq + + wantDirectives := []string{ + "limit_req_zone $binary_remote_addr zone=default/tea-minion:10m rate=200r/s;", + "limit_req_zone $binary_remote_addr zone=default/coffee-minion:20m rate=400r/s;", + "limit_req zone=" + limitReqTea.Zone + " burst=" + strconv.Itoa(limitReqTea.Burst) + " delay=" + strconv.Itoa(limitReqTea.Delay) + ";", + "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " delay=" + strconv.Itoa(limitReqCoffee.Delay) + ";", + "limit_req_status " + strconv.Itoa(limitReqTea.Status) + ";", + "limit_req_status " + strconv.Itoa(limitReqCoffee.Status) + ";", + } + + for _, want := range wantDirectives { + if !strings.Contains(ingConf, want) { + t.Errorf("want %q in generated config", want) + } + } +} + func newNGINXPlusIngressTmpl(t *testing.T) *template.Template { t.Helper() tmpl, err := template.New("nginx-plus.ingress.tmpl").Funcs(helperFunctions).ParseFiles("nginx-plus.ingress.tmpl") @@ -2119,6 +2240,178 @@ var ( Namespace: "default", }, } + + // Ingress Config that includes a request rate limit + ingressCfgRequestRateLimit = IngressNginxConfig{ + Ingress: Ingress{ + Name: "myingress", + Namespace: "default", + }, + Servers: []Server{ + { + Name: "test.example.com", + ServerTokens: "off", + StatusZone: "test.example.com", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + RedirectLocationName: "@login_url-default-cafe-ingress", + }, + SSL: true, + SSLCertificate: "secret.pem", + SSLCertificateKey: "secret.pem", + SSLPorts: []int{443}, + SSLRedirect: true, + Locations: []Location{ + { + Path: "/tea", + Upstream: testUpstream, + ProxyConnectTimeout: "10s", + ProxyReadTimeout: "10s", + ProxySendTimeout: "10s", + ClientMaxBodySize: "2m", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/location-key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + }, + LimitReq: &LimitReq{ + Zone: "default/myingress", + Burst: 100, + Delay: 50, + Status: 429, + }, + }, + { + Path: "/coffee", + Upstream: testUpstream, + ProxyConnectTimeout: "10s", + ProxyReadTimeout: "10s", + ProxySendTimeout: "10s", + ClientMaxBodySize: "2m", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/location-key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + }, + LimitReq: &LimitReq{ + Zone: "default/myingress", + Burst: 100, + Delay: 50, + Status: 429, + }, + }, + }, + HealthChecks: map[string]HealthCheck{"test": healthCheck}, + JWTRedirectLocations: []JWTRedirectLocation{ + { + Name: "@login_url-default-cafe-ingress", + LoginURL: "https://test.example.com/login", + }, + }, + }, + }, + LimitReqZones: []LimitReqZone{ + { + Name: "default/myingress", + Size: "10m", + Rate: "200r/s", + }, + }, + } + + ingressCfgRequestRateLimitMinions = IngressNginxConfig{ + Ingress: Ingress{ + Name: "myingress", + Namespace: "default", + }, + Servers: []Server{ + { + Name: "test.example.com", + ServerTokens: "off", + StatusZone: "test.example.com", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + RedirectLocationName: "@login_url-default-cafe-ingress", + }, + SSL: true, + SSLCertificate: "secret.pem", + SSLCertificateKey: "secret.pem", + SSLPorts: []int{443}, + SSLRedirect: true, + Locations: []Location{ + { + Path: "/tea", + Upstream: testUpstream, + ProxyConnectTimeout: "10s", + ProxyReadTimeout: "10s", + ProxySendTimeout: "10s", + ClientMaxBodySize: "2m", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/location-key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + }, + MinionIngress: &Ingress{ + Name: "tea-minion", + Namespace: "default", + }, + LimitReq: &LimitReq{ + Zone: "default/tea-minion", + Burst: 100, + Delay: 50, + Status: 429, + }, + }, + { + Path: "/coffee", + Upstream: testUpstream, + ProxyConnectTimeout: "10s", + ProxyReadTimeout: "10s", + ProxySendTimeout: "10s", + ClientMaxBodySize: "2m", + JWTAuth: &JWTAuth{ + Key: "/etc/nginx/secrets/location-key.jwk", + Realm: "closed site", + Token: "$cookie_auth_token", + }, + MinionIngress: &Ingress{ + Name: "coffee-minion", + Namespace: "default", + }, + LimitReq: &LimitReq{ + Zone: "default/coffee-minion", + Burst: 200, + Delay: 100, + Status: 503, + }, + }, + }, + HealthChecks: map[string]HealthCheck{"test": healthCheck}, + JWTRedirectLocations: []JWTRedirectLocation{ + { + Name: "@login_url-default-cafe-ingress", + LoginURL: "https://test.example.com/login", + }, + }, + }, + }, + LimitReqZones: []LimitReqZone{ + { + Name: "default/tea-minion", + Size: "10m", + Rate: "200r/s", + }, + { + Name: "default/coffee-minion", + Size: "20m", + Rate: "400r/s", + }, + }, + } ) var testUpstream = Upstream{ From 44d77301a3ae3faf840eaa652c0648254bc8dd1e Mon Sep 17 00:00:00 2001 From: vw485wx Date: Tue, 9 Jan 2024 09:37:45 +0000 Subject: [PATCH 2/5] Align limit-req annotations with virtualservers --- ...advanced-configuration-with-annotations.md | 10 +- internal/configs/annotations.go | 40 +++++-- internal/configs/config_params.go | 18 ++- internal/configs/ingress.go | 12 +- internal/configs/ingress_test.go | 108 +++++++++++++++--- internal/configs/version1/config.go | 12 +- .../configs/version1/nginx-plus.ingress.tmpl | 8 +- internal/configs/version1/nginx.ingress.tmpl | 8 +- internal/configs/version1/template_test.go | 80 ++++++++----- 9 files changed, 217 insertions(+), 79 deletions(-) diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md index 6a2c2c13b2..d1c756e770 100644 --- a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md +++ b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md @@ -180,11 +180,15 @@ The table below summarizes the available annotations. {{% table %}} |Annotation | ConfigMap Key | Description | Default | Example | | ---| ---| ---| ---| --- | -|``nginx.org/limit-req`` | N/A | Enables request-rate-limiting for this ingress by creating a [limit_req_zone](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone) and matching [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) for each location. All servers/locations of one ingress share the same zone. | N/A | 200r/s | +|``nginx.org/limit-req-rate`` | N/A | Enables request-rate-limiting for this ingress by creating a [limit_req_zone](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone) and matching [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) for each location. All servers/locations of one ingress share the same zone. Must have unit r/s or r/m. | N/A | 200r/s | +|``nginx.org/limit-req-key`` | N/A | The key to which the rate limit is applied. Can contain text, variables, or a combination of them. Variables must be surrounded by ${}. | ${binary_remote_addr} | ${binary_remote_addr} | |``nginx.org/limit-req-zone-size`` | N/A | Configures the size of the created [limit_req_zone](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_zone). | 10m | 20m | +|``nginx.org/limit-req-delay`` | N/A | Configures the delay-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. | 0 | 100 | +|``nginx.org/limit-req-no-delay`` | N/A | Configures the nodelay-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. | false | true | |``nginx.org/limit-req-burst`` | N/A | Configures the burst-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. | N/A | 100 | -|``nginx.org/limit-req-delay`` | N/A | Configures the delay-parameter of the [limit_req](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req) directive. 0 means nodelay. | 0 | 100 | -|``nginx.org/limit-req-status`` | N/A | Configures the [limit_req_status](https://nginx.org/en/docs/http/ngx_http_limit_req_module.html#limit_req_status) directive. | 429 | 503 | +|``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 | {{% /table %}} ### Snippets and Custom Templates diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 76c01f0d0c..4ba79b30d9 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -78,11 +78,15 @@ var minionInheritanceList = map[string]bool{ "nginx.org/max-fails": true, "nginx.org/max-conns": true, "nginx.org/fail-timeout": true, - "nginx.org/limit-req": true, + "nginx.org/limit-req-rate": true, + "nginx.org/limit-req-key": true, "nginx.org/limit-req-zone-size": true, - "nginx.org/limit-req-burst": true, "nginx.org/limit-req-delay": true, - "nginx.org/limit-req-status": true, + "nginx.org/limit-req-no-delay": true, + "nginx.org/limit-req-burst": true, + "nginx.org/limit-req-dry-run": true, + "nginx.org/limit-req-log-level": true, + "nginx.org/limit-req-reject-code": true, } var validPathRegex = map[string]bool{ @@ -419,12 +423,29 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } } - if requestRateLimit, exists := ingEx.Ingress.Annotations["nginx.org/limit-req"]; exists { + if requestRateLimit, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-rate"]; exists { cfgParams.LimitReqRate = requestRateLimit } + if requestRateKey, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-key"]; exists { + cfgParams.LimitReqKey = requestRateKey + } if requestRateZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-zone-size"]; exists { cfgParams.LimitReqZoneSize = requestRateZoneSize } + if requestRateDelay, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-delay", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.LimitReqDelay = requestRateDelay + } + } + if requestRateNoDelay, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/limit-req-no-delay", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.LimitReqNoDelay = requestRateNoDelay + } + } if requestRateBurst, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-burst", ingEx.Ingress); exists { if err != nil { glog.Error(err) @@ -432,18 +453,21 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool cfgParams.LimitReqBurst = requestRateBurst } } - if requestRateDelay, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-delay", ingEx.Ingress); exists { + if requestRateDryRun, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/limit-req-dry-run", ingEx.Ingress); exists { if err != nil { glog.Error(err) } else { - cfgParams.LimitReqDelay = requestRateDelay + cfgParams.LimitReqDryRun = requestRateDryRun } } - if requestRateStatus, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-status", ingEx.Ingress); exists { + if requestRateLogLevel, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-log-level"]; exists { + cfgParams.LimitReqLogLevel = requestRateLogLevel + } + if requestRateRejectCode, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-reject-code", ingEx.Ingress); exists { if err != nil { glog.Error(err) } else { - cfgParams.LimitReqStatus = requestRateStatus + cfgParams.LimitReqRejectCode = requestRateRejectCode } } diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index 525f943111..d03f008431 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -114,11 +114,15 @@ type ConfigParams struct { SpiffeServerCerts bool - LimitReqRate string - LimitReqZoneSize string - LimitReqBurst int - LimitReqDelay int - LimitReqStatus int + LimitReqRate string + LimitReqKey string + LimitReqZoneSize string + LimitReqDelay int + LimitReqNoDelay bool + LimitReqBurst int + LimitReqDryRun bool + LimitReqLogLevel string + LimitReqRejectCode int } // StaticConfigParams holds immutable NGINX configuration parameters that affect the main NGINX config. @@ -197,8 +201,10 @@ func NewDefaultConfigParams(isPlus bool) *ConfigParams { MainKeepaliveRequests: 100, VariablesHashBucketSize: 256, VariablesHashMaxSize: 1024, + LimitReqKey: "${binary_remote_addr}", LimitReqZoneSize: "10m", - LimitReqStatus: 429, + LimitReqLogLevel: "error", + LimitReqRejectCode: 429, } } diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 71218c6e15..3a19025f85 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -269,14 +269,18 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings) if cfgParams.LimitReqRate != "" { zoneName := p.ingEx.Ingress.Namespace + "/" + p.ingEx.Ingress.Name loc.LimitReq = &version1.LimitReq{ - Zone: zoneName, - Burst: cfgParams.LimitReqBurst, - Delay: cfgParams.LimitReqDelay, - Status: cfgParams.LimitReqStatus, + Zone: zoneName, + Burst: cfgParams.LimitReqBurst, + Delay: cfgParams.LimitReqDelay, + NoDelay: cfgParams.LimitReqNoDelay, + DryRun: cfgParams.LimitReqDryRun, + LogLevel: cfgParams.LimitReqLogLevel, + RejectCode: cfgParams.LimitReqRejectCode, } if !limitReqZoneExists(limitReqZones, zoneName) { limitReqZones = append(limitReqZones, version1.LimitReqZone{ Name: zoneName, + Key: cfgParams.LimitReqKey, Size: cfgParams.LimitReqZoneSize, Rate: cfgParams.LimitReqRate, }) diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index ac265d2641..af727c9378 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -913,10 +913,14 @@ func TestGenerateNginxCfgWithUseClusterIP(t *testing.T) { func TestGenerateNginxCfgForLimitReq(t *testing.T) { t.Parallel() cafeIngressEx := createCafeIngressEx() - cafeIngressEx.Ingress.Annotations["nginx.org/limit-req"] = "200r/s" + 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-status"] = "429" + 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" isPlus := false @@ -925,16 +929,74 @@ func TestGenerateNginxCfgForLimitReq(t *testing.T) { expectedZones := []version1.LimitReqZone{ { Name: "default/cafe-ingress", + Key: "${request_uri}", Size: "11m", Rate: "200r/s", }, } expectedReqs := &version1.LimitReq{ - Zone: "default/cafe-ingress", - Burst: 100, - Delay: 80, - Status: 429, + 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, + }) + + 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 TestGenerateNginxCfgForLimitReqDefaults(t *testing.T) { + t.Parallel() + cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-rate"] = "200r/s" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-burst"] = "100" + cafeIngressEx.Ingress.Annotations["nginx.org/limit-req-delay"] = "80" + + isPlus := false + configParams := NewDefaultConfigParams(isPlus) + + expectedZones := []version1.LimitReqZone{ + { + Name: "default/cafe-ingress", + Key: "${binary_remote_addr}", + Size: "10m", + Rate: "200r/s", + }, + } + + expectedReqs := &version1.LimitReq{ + Zone: "default/cafe-ingress", + Burst: 100, + Delay: 80, + LogLevel: "error", + RejectCode: 429, } result, warnings := generateNginxCfg(NginxCfgParams{ @@ -968,26 +1030,32 @@ func TestGenerateNginxCfgForMergeableIngressesForLimitReq(t *testing.T) { t.Parallel() mergeableIngresses := createMergeableCafeIngress() - mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req"] = "200r/s" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-rate"] = "200r/s" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-key"] = "${request_uri}" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-burst"] = "100" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-delay"] = "80" - mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-status"] = "429" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-no-delay"] = "true" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-reject-code"] = "429" mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-zone-size"] = "11m" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-dry-run"] = "true" + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/limit-req-log-level"] = "info" - mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req"] = "400r/s" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-rate"] = "400r/s" mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-burst"] = "200" mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-delay"] = "160" - mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-status"] = "503" + mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-reject-code"] = "503" mergeableIngresses.Minions[1].Ingress.Annotations["nginx.org/limit-req-zone-size"] = "12m" expectedZones := []version1.LimitReqZone{ { Name: "default/cafe-ingress-coffee-minion", + Key: "${request_uri}", Size: "11m", Rate: "200r/s", }, { Name: "default/cafe-ingress-tea-minion", + Key: "${binary_remote_addr}", Size: "12m", Rate: "400r/s", }, @@ -995,16 +1063,20 @@ func TestGenerateNginxCfgForMergeableIngressesForLimitReq(t *testing.T) { expectedReqs := map[string]*version1.LimitReq{ "cafe-ingress-coffee-minion": { - Zone: "default/cafe-ingress-coffee-minion", - Burst: 100, - Delay: 80, - Status: 429, + Zone: "default/cafe-ingress-coffee-minion", + Burst: 100, + Delay: 80, + LogLevel: "info", + RejectCode: 429, + NoDelay: true, + DryRun: true, }, "cafe-ingress-tea-minion": { - Zone: "default/cafe-ingress-tea-minion", - Burst: 200, - Delay: 160, - Status: 503, + Zone: "default/cafe-ingress-tea-minion", + Burst: 200, + Delay: 160, + LogLevel: "error", + RejectCode: 503, }, } diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index bc46bfa413..bf376fe5be 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -67,6 +67,7 @@ type HealthCheck struct { // LimitReqZone describes a zone used for request rate limiting type LimitReqZone struct { Name string + Key string Size string Rate string } @@ -148,10 +149,13 @@ type JWTAuth struct { // LimitReq configures a request rate limit type LimitReq struct { - Zone string - Burst int - Delay int - Status int + Zone string + Burst int + Delay int + NoDelay bool + RejectCode int + DryRun bool + LogLevel string } // Location describes an NGINX location. diff --git a/internal/configs/version1/nginx-plus.ingress.tmpl b/internal/configs/version1/nginx-plus.ingress.tmpl index 5fa7e5361d..eec61ca468 100644 --- a/internal/configs/version1/nginx-plus.ingress.tmpl +++ b/internal/configs/version1/nginx-plus.ingress.tmpl @@ -20,7 +20,7 @@ upstream {{$upstream.Name}} { {{- end}} {{range $limitReqZone := .LimitReqZones}} -limit_req_zone $binary_remote_addr zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; +limit_req_zone {{ $limitReqZone.Key }} zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; {{end}} {{range $server := .Servers}} @@ -316,8 +316,10 @@ server { {{- end}} {{with $location.LimitReq}} - limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{else}}nodelay{{end}}; - {{if $location.LimitReq.Status}}limit_req_status {{$location.LimitReq.Status}};{{end}} + limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.NoDelay}}nodelay{{else if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{end}}; + {{if $location.LimitReq.DryRun}}limit_req_dry_run on;{{end}} + {{if $location.LimitReq.LogLevel}}limit_req_log_level {{$location.LimitReq.LogLevel}};{{end}} + {{if $location.LimitReq.RejectCode}}limit_req_status {{$location.LimitReq.RejectCode}};{{end}} {{end}} } {{end -}} diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl index 8dc09ca1f1..02f46cbf37 100644 --- a/internal/configs/version1/nginx.ingress.tmpl +++ b/internal/configs/version1/nginx.ingress.tmpl @@ -11,7 +11,7 @@ upstream {{$upstream.Name}} { {{end -}} {{range $limitReqZone := .LimitReqZones}} -limit_req_zone $binary_remote_addr zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; +limit_req_zone {{ $limitReqZone.Key }} zone={{ $limitReqZone.Name }}:{{$limitReqZone.Size}} rate={{$limitReqZone.Rate}}; {{end}} {{range $server := .Servers}} @@ -229,8 +229,10 @@ server { {{- end}} {{with $location.LimitReq}} - limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{else}}nodelay{{end}}; - {{if $location.LimitReq.Status}}limit_req_status {{$location.LimitReq.Status}};{{end}} + limit_req zone={{ $location.LimitReq.Zone }} {{if $location.LimitReq.Burst}}burst={{$location.LimitReq.Burst}}{{end}} {{if $location.LimitReq.NoDelay}}nodelay{{else if $location.LimitReq.Delay}}delay={{$location.LimitReq.Delay}}{{end}}; + {{if $location.LimitReq.DryRun}}limit_req_dry_run on;{{end}} + {{if $location.LimitReq.LogLevel}}limit_req_log_level {{$location.LimitReq.LogLevel}};{{end}} + {{if $location.LimitReq.RejectCode}}limit_req_status {{$location.LimitReq.RejectCode}};{{end}} {{end}} } {{end -}} diff --git a/internal/configs/version1/template_test.go b/internal/configs/version1/template_test.go index 296c4b9833..95b39cb873 100644 --- a/internal/configs/version1/template_test.go +++ b/internal/configs/version1/template_test.go @@ -989,9 +989,11 @@ func TestExecuteTemplate_ForIngressForNGINXWithRequestRateLimit(t *testing.T) { limitReq := ingressCfgRequestRateLimit.Servers[0].Locations[0].LimitReq wantDirectives := []string{ - "limit_req_zone $binary_remote_addr zone=default/myingress:10m rate=200r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/myingress:10m rate=200r/s;", "limit_req zone=default/myingress burst=" + strconv.Itoa(limitReq.Burst) + " delay=" + strconv.Itoa(limitReq.Delay) + ";", - "limit_req_status " + strconv.Itoa(limitReq.Status) + ";", + "limit_req_status " + strconv.Itoa(limitReq.RejectCode) + ";", + "limit_req_dry_run on;", + "limit_req_log_level info;", } for _, want := range wantDirectives { @@ -1018,12 +1020,15 @@ func TestExecuteTemplate_ForIngressForNGINXWithRequestRateLimitMinions(t *testin limitReqCoffee := ingressCfgRequestRateLimitMinions.Servers[0].Locations[1].LimitReq wantDirectives := []string{ - "limit_req_zone $binary_remote_addr zone=default/tea-minion:10m rate=200r/s;", - "limit_req_zone $binary_remote_addr zone=default/coffee-minion:20m rate=400r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/tea-minion:10m rate=200r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/coffee-minion:20m rate=400r/s;", "limit_req zone=" + limitReqTea.Zone + " burst=" + strconv.Itoa(limitReqTea.Burst) + " delay=" + strconv.Itoa(limitReqTea.Delay) + ";", - "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " delay=" + strconv.Itoa(limitReqCoffee.Delay) + ";", - "limit_req_status " + strconv.Itoa(limitReqTea.Status) + ";", - "limit_req_status " + strconv.Itoa(limitReqCoffee.Status) + ";", + "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " nodelay;", + "limit_req_status " + strconv.Itoa(limitReqTea.RejectCode) + ";", + "limit_req_status " + strconv.Itoa(limitReqCoffee.RejectCode) + ";", + "limit_req_log_level " + limitReqTea.LogLevel + ";", + "limit_req_log_level " + limitReqCoffee.LogLevel + ";", + "limit_req_dry_run on;", } for _, want := range wantDirectives { @@ -1049,9 +1054,11 @@ func TestExecuteTemplate_ForIngressForNGINXPlusWithRequestRateLimit(t *testing.T limitReq := ingressCfgRequestRateLimit.Servers[0].Locations[0].LimitReq wantDirectives := []string{ - "limit_req_zone $binary_remote_addr zone=default/myingress:10m rate=200r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/myingress:10m rate=200r/s;", "limit_req zone=default/myingress burst=" + strconv.Itoa(limitReq.Burst) + " delay=" + strconv.Itoa(limitReq.Delay) + ";", - "limit_req_status " + strconv.Itoa(limitReq.Status) + ";", + "limit_req_status " + strconv.Itoa(limitReq.RejectCode) + ";", + "limit_req_dry_run on;", + "limit_req_log_level info;", } for _, want := range wantDirectives { @@ -1078,12 +1085,15 @@ func TestExecuteTemplate_ForIngressForNGINXPlusWithRequestRateLimitMinions(t *te limitReqCoffee := ingressCfgRequestRateLimitMinions.Servers[0].Locations[1].LimitReq wantDirectives := []string{ - "limit_req_zone $binary_remote_addr zone=default/tea-minion:10m rate=200r/s;", - "limit_req_zone $binary_remote_addr zone=default/coffee-minion:20m rate=400r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/tea-minion:10m rate=200r/s;", + "limit_req_zone ${binary_remote_addr} zone=default/coffee-minion:20m rate=400r/s;", "limit_req zone=" + limitReqTea.Zone + " burst=" + strconv.Itoa(limitReqTea.Burst) + " delay=" + strconv.Itoa(limitReqTea.Delay) + ";", - "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " delay=" + strconv.Itoa(limitReqCoffee.Delay) + ";", - "limit_req_status " + strconv.Itoa(limitReqTea.Status) + ";", - "limit_req_status " + strconv.Itoa(limitReqCoffee.Status) + ";", + "limit_req zone=" + limitReqCoffee.Zone + " burst=" + strconv.Itoa(limitReqCoffee.Burst) + " nodelay;", + "limit_req_status " + strconv.Itoa(limitReqTea.RejectCode) + ";", + "limit_req_status " + strconv.Itoa(limitReqCoffee.RejectCode) + ";", + "limit_req_log_level " + limitReqTea.LogLevel + ";", + "limit_req_log_level " + limitReqCoffee.LogLevel + ";", + "limit_req_dry_run on;", } for _, want := range wantDirectives { @@ -2277,10 +2287,12 @@ var ( Token: "$cookie_auth_token", }, LimitReq: &LimitReq{ - Zone: "default/myingress", - Burst: 100, - Delay: 50, - Status: 429, + Zone: "default/myingress", + Burst: 100, + Delay: 50, + RejectCode: 429, + DryRun: true, + LogLevel: "info", }, }, { @@ -2296,10 +2308,12 @@ var ( Token: "$cookie_auth_token", }, LimitReq: &LimitReq{ - Zone: "default/myingress", - Burst: 100, - Delay: 50, - Status: 429, + Zone: "default/myingress", + Burst: 100, + Delay: 50, + RejectCode: 429, + DryRun: true, + LogLevel: "info", }, }, }, @@ -2315,6 +2329,7 @@ var ( LimitReqZones: []LimitReqZone{ { Name: "default/myingress", + Key: "${binary_remote_addr}", Size: "10m", Rate: "200r/s", }, @@ -2360,10 +2375,12 @@ var ( Namespace: "default", }, LimitReq: &LimitReq{ - Zone: "default/tea-minion", - Burst: 100, - Delay: 50, - Status: 429, + Zone: "default/tea-minion", + Burst: 100, + Delay: 10, + LogLevel: "info", + DryRun: true, + RejectCode: 429, }, }, { @@ -2383,10 +2400,11 @@ var ( Namespace: "default", }, LimitReq: &LimitReq{ - Zone: "default/coffee-minion", - Burst: 200, - Delay: 100, - Status: 503, + Zone: "default/coffee-minion", + Burst: 200, + NoDelay: true, + LogLevel: "error", + RejectCode: 503, }, }, }, @@ -2402,11 +2420,13 @@ var ( LimitReqZones: []LimitReqZone{ { Name: "default/tea-minion", + Key: "${binary_remote_addr}", Size: "10m", Rate: "200r/s", }, { Name: "default/coffee-minion", + Key: "${binary_remote_addr}", Size: "20m", Rate: "400r/s", }, From 5369006b3a4067a1ebb74cfc72a0db2b43b1b5b5 Mon Sep 17 00:00:00 2001 From: vw485wx Date: Tue, 9 Jan 2024 11:01:30 +0000 Subject: [PATCH 3/5] Validate rate-limit annotations --- internal/configs/annotations.go | 62 +++++++++++++++++++--------- internal/configs/annotations_test.go | 57 +++++++++++++++++++++++++ internal/configs/parsing_helpers.go | 24 +++++++++++ 3 files changed, 124 insertions(+), 19 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 4ba79b30d9..43d36a6ddd 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -1,6 +1,9 @@ package configs import ( + "fmt" + "slices" + "github.com/golang/glog" ) @@ -423,55 +426,76 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } } - if requestRateLimit, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-rate"]; exists { - cfgParams.LimitReqRate = requestRateLimit + for _, err := range parseRateLimitAnnotations(ingEx.Ingress.Annotations, &cfgParams, ingEx.Ingress) { + glog.Error(err) + } + + return cfgParams +} + +// parseRateLimitAnnotations parses rate-limiting-related annotations and places them into cfgParams. Occuring errors are collected and returned, but do not abort parsing. +func parseRateLimitAnnotations(annotations map[string]string, cfgParams *ConfigParams, context apiObject) []error { + errors := make([]error, 0) + if requestRateLimit, exists := annotations["nginx.org/limit-req-rate"]; exists { + if rate, err := ParseRequestRate(requestRateLimit); err != nil { + errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-rate: got %s: %v", context.GetNamespace(), context.GetName(), requestRateLimit, err)) + } else { + cfgParams.LimitReqRate = rate + } } - if requestRateKey, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-key"]; exists { + if requestRateKey, exists := annotations["nginx.org/limit-req-key"]; exists { cfgParams.LimitReqKey = requestRateKey } - if requestRateZoneSize, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-zone-size"]; exists { - cfgParams.LimitReqZoneSize = requestRateZoneSize + if requestRateZoneSize, exists := annotations["nginx.org/limit-req-zone-size"]; exists { + if size, err := ParseSize(requestRateZoneSize); err != nil { + errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-zone-size: got %s: %v", context.GetNamespace(), context.GetName(), requestRateZoneSize, err)) + } else { + cfgParams.LimitReqZoneSize = size + } } - if requestRateDelay, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-delay", ingEx.Ingress); exists { + if requestRateDelay, exists, err := GetMapKeyAsInt(annotations, "nginx.org/limit-req-delay", context); exists { if err != nil { - glog.Error(err) + errors = append(errors, err) } else { cfgParams.LimitReqDelay = requestRateDelay } } - if requestRateNoDelay, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/limit-req-no-delay", ingEx.Ingress); exists { + if requestRateNoDelay, exists, err := GetMapKeyAsBool(annotations, "nginx.org/limit-req-no-delay", context); exists { if err != nil { - glog.Error(err) + errors = append(errors, err) } else { cfgParams.LimitReqNoDelay = requestRateNoDelay } } - if requestRateBurst, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-burst", ingEx.Ingress); exists { + if requestRateBurst, exists, err := GetMapKeyAsInt(annotations, "nginx.org/limit-req-burst", context); exists { if err != nil { - glog.Error(err) + errors = append(errors, err) } else { cfgParams.LimitReqBurst = requestRateBurst } } - if requestRateDryRun, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/limit-req-dry-run", ingEx.Ingress); exists { + if requestRateDryRun, exists, err := GetMapKeyAsBool(annotations, "nginx.org/limit-req-dry-run", context); exists { if err != nil { - glog.Error(err) + errors = append(errors, err) } else { cfgParams.LimitReqDryRun = requestRateDryRun } } - if requestRateLogLevel, exists := ingEx.Ingress.Annotations["nginx.org/limit-req-log-level"]; exists { - cfgParams.LimitReqLogLevel = requestRateLogLevel + if requestRateLogLevel, exists := annotations["nginx.org/limit-req-log-level"]; exists { + if !slices.Contains([]string{"info", "notice", "warn", "error"}, requestRateLogLevel) { + errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-log-level: got %s", context.GetNamespace(), context.GetName(), requestRateLogLevel)) + } else { + cfgParams.LimitReqLogLevel = requestRateLogLevel + } } - if requestRateRejectCode, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/limit-req-reject-code", ingEx.Ingress); exists { + if requestRateRejectCode, exists, err := GetMapKeyAsInt(annotations, "nginx.org/limit-req-reject-code", context); exists { if err != nil { - glog.Error(err) + errors = append(errors, err) } else { cfgParams.LimitReqRejectCode = requestRateRejectCode } } - - return cfgParams + return errors } func getWebsocketServices(ingEx *IngressEx) map[string]bool { diff --git a/internal/configs/annotations_test.go b/internal/configs/annotations_test.go index 7a4cebcbaa..81fa52f550 100644 --- a/internal/configs/annotations_test.go +++ b/internal/configs/annotations_test.go @@ -4,6 +4,9 @@ import ( "reflect" "sort" "testing" + + networking "k8s.io/api/networking/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestParseRewrites(t *testing.T) { @@ -159,3 +162,57 @@ func TestMergeMasterAnnotationsIntoMinion(t *testing.T) { t.Errorf("mergeMasterAnnotationsIntoMinion returned %v, but expected %v", minionAnnotations, expectedMergedAnnotations) } } + +func TestParseRateLimitAnnotations(t *testing.T) { + context := &networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "context", + }, + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-rate": "200r/s", + "nginx.org/limit-req-key": "${request_uri}", + "nginx.org/limit-req-burst": "100", + "nginx.org/limit-req-delay": "80", + "nginx.org/limit-req-no-delay": "true", + "nginx.org/limit-req-reject-code": "429", + "nginx.org/limit-req-zone-size": "11m", + "nginx.org/limit-req-dry-run": "true", + "nginx.org/limit-req-log-level": "info", + }, NewDefaultConfigParams(false), context); len(errors) > 0 { + t.Errorf("Errors when parsing valid limit-req annotations") + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-rate": "200", + }, NewDefaultConfigParams(false), context); len(errors) == 0 { + t.Errorf("No Errors when parsing invalid request rate") + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-rate": "200r/h", + }, NewDefaultConfigParams(false), context); len(errors) == 0 { + t.Errorf("No Errors when parsing invalid request rate") + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-rate": "0r/s", + }, NewDefaultConfigParams(false), context); len(errors) == 0 { + t.Errorf("No Errors when parsing invalid request rate") + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-zone-size": "10abc", + }, NewDefaultConfigParams(false), context); len(errors) == 0 { + t.Errorf("No Errors when parsing invalid zone size") + } + + if errors := parseRateLimitAnnotations(map[string]string{ + "nginx.org/limit-req-log-level": "foobar", + }, NewDefaultConfigParams(false), context); len(errors) == 0 { + t.Errorf("No Errors when parsing invalid log level") + } + +} diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index 831c30f034..b48032e555 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -239,6 +239,30 @@ func ParseSize(s string) (string, error) { return "", errors.New("invalid size string") } +var rateRegexp = regexp.MustCompile(`^(\d+)(r/s|r/m)$`) + +// ParseRequestRate ensures that the string value is a valid request rate in r/s or r/m and > 0 +func ParseRequestRate(s string) (string, error) { + s = strings.TrimSpace(s) + + match := rateRegexp.FindStringSubmatch(s) + + if match == nil { + return "", errors.New("String does not match rate-pattern: ^(\\d+)(r/s|r/m)$") + } + + number, err := strconv.Atoi(match[1]) + if err != nil { + return "", errors.New("String does not match rate-pattern") + } + + if number <= 0 { + return "", errors.New("Rate must be >0") + } + + return s, nil +} + // https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers var proxyBuffersRegexp = regexp.MustCompile(`^\d+ \d+[kKmM]?$`) From 205f821efef0769c5659259dacb8f6ec090f7d9b Mon Sep 17 00:00:00 2001 From: vw485wx Date: Tue, 9 Jan 2024 11:38:09 +0000 Subject: [PATCH 4/5] Fixed linter-errors --- internal/configs/annotations.go | 8 +++++--- internal/configs/annotations_test.go | 3 +-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 43d36a6ddd..d9c2e526fc 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -433,12 +433,14 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool return cfgParams } -// parseRateLimitAnnotations parses rate-limiting-related annotations and places them into cfgParams. Occuring errors are collected and returned, but do not abort parsing. +// parseRateLimitAnnotations parses rate-limiting-related annotations and places them into cfgParams. Occurring errors are collected and returned, but do not abort parsing. +// +//gocyclo:ignore func parseRateLimitAnnotations(annotations map[string]string, cfgParams *ConfigParams, context apiObject) []error { errors := make([]error, 0) if requestRateLimit, exists := annotations["nginx.org/limit-req-rate"]; exists { if rate, err := ParseRequestRate(requestRateLimit); err != nil { - errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-rate: got %s: %v", context.GetNamespace(), context.GetName(), requestRateLimit, err)) + errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-rate: got %s: %w", context.GetNamespace(), context.GetName(), requestRateLimit, err)) } else { cfgParams.LimitReqRate = rate } @@ -448,7 +450,7 @@ func parseRateLimitAnnotations(annotations map[string]string, cfgParams *ConfigP } if requestRateZoneSize, exists := annotations["nginx.org/limit-req-zone-size"]; exists { if size, err := ParseSize(requestRateZoneSize); err != nil { - errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-zone-size: got %s: %v", context.GetNamespace(), context.GetName(), requestRateZoneSize, err)) + errors = append(errors, fmt.Errorf("Ingress %s/%s: Invalid value for nginx.org/limit-req-zone-size: got %s: %w", context.GetNamespace(), context.GetName(), requestRateZoneSize, err)) } else { cfgParams.LimitReqZoneSize = size } diff --git a/internal/configs/annotations_test.go b/internal/configs/annotations_test.go index 81fa52f550..fd711c49e3 100644 --- a/internal/configs/annotations_test.go +++ b/internal/configs/annotations_test.go @@ -182,7 +182,7 @@ func TestParseRateLimitAnnotations(t *testing.T) { "nginx.org/limit-req-dry-run": "true", "nginx.org/limit-req-log-level": "info", }, NewDefaultConfigParams(false), context); len(errors) > 0 { - t.Errorf("Errors when parsing valid limit-req annotations") + t.Error("Errors when parsing valid limit-req annotations") } if errors := parseRateLimitAnnotations(map[string]string{ @@ -214,5 +214,4 @@ func TestParseRateLimitAnnotations(t *testing.T) { }, NewDefaultConfigParams(false), context); len(errors) == 0 { t.Errorf("No Errors when parsing invalid log level") } - } From ce6f7743f7d67efcd976b8132c4c11bdebf30041 Mon Sep 17 00:00:00 2001 From: vw485wx Date: Tue, 9 Jan 2024 12:41:08 +0000 Subject: [PATCH 5/5] Replaced t.Errorf with t.Error where needed --- internal/configs/annotations_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/configs/annotations_test.go b/internal/configs/annotations_test.go index fd711c49e3..b2216ccd62 100644 --- a/internal/configs/annotations_test.go +++ b/internal/configs/annotations_test.go @@ -188,30 +188,30 @@ func TestParseRateLimitAnnotations(t *testing.T) { if errors := parseRateLimitAnnotations(map[string]string{ "nginx.org/limit-req-rate": "200", }, NewDefaultConfigParams(false), context); len(errors) == 0 { - t.Errorf("No Errors when parsing invalid request rate") + t.Error("No Errors when parsing invalid request rate") } if errors := parseRateLimitAnnotations(map[string]string{ "nginx.org/limit-req-rate": "200r/h", }, NewDefaultConfigParams(false), context); len(errors) == 0 { - t.Errorf("No Errors when parsing invalid request rate") + t.Error("No Errors when parsing invalid request rate") } if errors := parseRateLimitAnnotations(map[string]string{ "nginx.org/limit-req-rate": "0r/s", }, NewDefaultConfigParams(false), context); len(errors) == 0 { - t.Errorf("No Errors when parsing invalid request rate") + t.Error("No Errors when parsing invalid request rate") } if errors := parseRateLimitAnnotations(map[string]string{ "nginx.org/limit-req-zone-size": "10abc", }, NewDefaultConfigParams(false), context); len(errors) == 0 { - t.Errorf("No Errors when parsing invalid zone size") + t.Error("No Errors when parsing invalid zone size") } if errors := parseRateLimitAnnotations(map[string]string{ "nginx.org/limit-req-log-level": "foobar", }, NewDefaultConfigParams(false), context); len(errors) == 0 { - t.Errorf("No Errors when parsing invalid log level") + t.Error("No Errors when parsing invalid log level") } }