diff --git a/charts/ingress-nginx/tests/controller-configmap_test.yaml b/charts/ingress-nginx/tests/controller-configmap_test.yaml index 9cfea9800b..f0fc1c730e 100644 --- a/charts/ingress-nginx/tests/controller-configmap_test.yaml +++ b/charts/ingress-nginx/tests/controller-configmap_test.yaml @@ -16,16 +16,8 @@ tests: - it: should create a ConfigMap with templated values if `controller.config` contains templates set: controller.config: - global-rate-limit-memcached-host: "memcached.{{ .Release.Namespace }}.svc.kubernetes.local" - global-rate-limit-memcached-port: 11211 use-gzip: true asserts: - - equal: - path: data.global-rate-limit-memcached-host - value: memcached.NAMESPACE.svc.kubernetes.local - - equal: - path: data.global-rate-limit-memcached-port - value: "11211" - equal: path: data.use-gzip value: "true" diff --git a/docs/e2e-tests.md b/docs/e2e-tests.md index f288ec82f3..c956038531 100644 --- a/docs/e2e-tests.md +++ b/docs/e2e-tests.md @@ -7,7 +7,6 @@ Do not try to edit it manually. ### [[Admission] admission controller](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/admission/admission.go#L39) -- [reject ingress with global-rate-limit annotations when memcached is not configured](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/admission/admission.go#L47) - [should not allow overlaps of host and paths without canary annotations](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/admission/admission.go#L74) - [should allow overlaps of host and paths with canary annotation](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/admission/admission.go#L91) - [should block ingress with invalid path](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/admission/admission.go#L112) @@ -173,8 +172,6 @@ Do not try to edit it manually. ### [from-to-www-redirect](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/fromtowwwredirect.go#L31) - [should redirect from www HTTP to HTTP](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/fromtowwwredirect.go#L38) - [should redirect from www HTTPS to HTTPS](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/fromtowwwredirect.go#L64) -### [annotation-global-rate-limit](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/globalratelimit.go#L30) -- [generates correct configuration](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/globalratelimit.go#L38) ### [backend-protocol - GRPC](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/grpc.go#L45) - [should use grpc_pass in the configuration file](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/grpc.go#L48) - [should return OK for service with backend protocol GRPC](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/annotations/grpc.go#L71) @@ -420,8 +417,6 @@ Do not try to edit it manually. ### [global-options](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/global_options.go#L28) - [should have worker_rlimit_nofile option](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/global_options.go#L31) - [should have worker_rlimit_nofile option and be independent on amount of worker processes](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/global_options.go#L37) -### [settings-global-rate-limit](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/globalratelimit.go#L30) -- [generates correct NGINX configuration](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/globalratelimit.go#L38) ### [GRPC](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/grpc.go#L39) - [should set the correct GRPC Buffer Size](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/grpc.go#L42) ### [gzip](https://github.com/kubernetes/ingress-nginx/tree/main//test/e2e/settings/gzip.go#L30) diff --git a/docs/user-guide/nginx-configuration/annotations-risk.md b/docs/user-guide/nginx-configuration/annotations-risk.md index 890608b4b2..3e3b93986b 100755 --- a/docs/user-guide/nginx-configuration/annotations-risk.md +++ b/docs/user-guide/nginx-configuration/annotations-risk.md @@ -55,10 +55,6 @@ | ExternalAuth | auth-url | High | location | | FastCGI | fastcgi-index | Medium | location | | FastCGI | fastcgi-params-configmap | Medium | location | -| GlobalRateLimit | global-rate-limit | Low | ingress | -| GlobalRateLimit | global-rate-limit-ignored-cidrs | Medium | ingress | -| GlobalRateLimit | global-rate-limit-key | High | ingress | -| GlobalRateLimit | global-rate-limit-window | Low | ingress | | HTTP2PushPreload | http2-push-preload | Low | location | | LoadBalancing | load-balance | Low | location | | Logs | enable-access-log | Low | location | diff --git a/docs/user-guide/nginx-configuration/annotations.md b/docs/user-guide/nginx-configuration/annotations.md index 7de05d32c1..80ceccfa9d 100755 --- a/docs/user-guide/nginx-configuration/annotations.md +++ b/docs/user-guide/nginx-configuration/annotations.md @@ -64,10 +64,6 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz |[nginx.ingress.kubernetes.io/http2-push-preload](#http2-push-preload)|"true" or "false"| |[nginx.ingress.kubernetes.io/limit-connections](#rate-limiting)|number| |[nginx.ingress.kubernetes.io/limit-rps](#rate-limiting)|number| -|[nginx.ingress.kubernetes.io/global-rate-limit](#global-rate-limiting)|number| -|[nginx.ingress.kubernetes.io/global-rate-limit-window](#global-rate-limiting)|duration| -|[nginx.ingress.kubernetes.io/global-rate-limit-key](#global-rate-limiting)|string| -|[nginx.ingress.kubernetes.io/global-rate-limit-ignored-cidrs](#global-rate-limiting)|string| |[nginx.ingress.kubernetes.io/permanent-redirect](#permanent-redirect)|string| |[nginx.ingress.kubernetes.io/permanent-redirect-code](#permanent-redirect-code)|number| |[nginx.ingress.kubernetes.io/temporal-redirect](#temporal-redirect)|string| @@ -560,46 +556,6 @@ To configure settings globally for all Ingress rules, the `limit-rate-after` and The client IP address will be set based on the use of [PROXY protocol](./configmap.md#use-proxy-protocol) or from the `X-Forwarded-For` header value when [use-forwarded-headers](./configmap.md#use-forwarded-headers) is enabled. -### Global Rate Limiting - -**Note:** Be careful when configuring both (Local) Rate Limiting and Global Rate Limiting at the same time. -They are two completely different rate limiting implementations. Whichever limit exceeds first will reject the -requests. It might be a good idea to configure both of them to ease load on Global Rate Limiting backend -in cases of spike in traffic. - -The stock NGINX rate limiting does not share its counters among different NGINX instances. -Given that most ingress-nginx deployments are elastic and number of replicas can change any day -it is impossible to configure a proper rate limit using stock NGINX functionalities. -Global Rate Limiting overcome this by using [lua-resty-global-throttle](https://github.com/ElvinEfendi/lua-resty-global-throttle). `lua-resty-global-throttle` shares its counters via a central store such as `memcached`. -The obvious shortcoming of this is users have to deploy and operate a `memcached` instance -in order to benefit from this functionality. Configure the `memcached` -using [these configmap settings](./configmap.md#global-rate-limit). - -**Here are a few remarks for ingress-nginx integration of `lua-resty-global-throttle`:** - -1. We minimize `memcached` access by caching exceeding limit decisions. The expiry of -cache entry is the desired delay `lua-resty-global-throttle` calculates for us. -The Lua Shared Dictionary used for that is `global_throttle_cache`. Currently its size defaults to 10M. -Customize it as per your needs using [lua-shared-dicts](./configmap.md#lua-shared-dicts). -When we fail to cache the exceeding limit decision then we log an NGINX error. You can monitor -for that error to decide if you need to bump the cache size. Without cache the cost of processing a -request is two memcached commands: `GET`, and `INCR`. With the cache it is only `INCR`. -1. Log NGINX variable `$global_rate_limit_exceeding`'s value to have some visibility into -what portion of requests are rejected (value `y`), whether they are rejected using cached decision (value `c`), -or if they are not rejected (default value `n`). You can use [log-format-upstream](./configmap.md#log-format-upstream) -to include that in access logs. -1. In case of an error it will log the error message and **fail open**. -1. The annotations below creates Global Rate Limiting instance per ingress. -That means if there are multiple paths configured under the same ingress, -the Global Rate Limiting will count requests to all the paths under the same counter. -Extract a path out into its own ingress if you need to isolate a certain path. - - -* `nginx.ingress.kubernetes.io/global-rate-limit`: Configures maximum allowed number of requests per window. Required. -* `nginx.ingress.kubernetes.io/global-rate-limit-window`: Configures a time window (i.e `1m`) that the limit is applied. Required. -* `nginx.ingress.kubernetes.io/global-rate-limit-key`: Configures a key for counting the samples. Defaults to `$remote_addr`. You can also combine multiple NGINX variables here, like `${remote_addr}-${http_x_api_client}` which would mean the limit will be applied to requests coming from the same API client (indicated by `X-API-Client` HTTP request header) with the same source IP address. -* `nginx.ingress.kubernetes.io/global-rate-limit-ignored-cidrs`: comma separated list of IPs and CIDRs to match client IP against. When there's a match request is not considered for rate limiting. - ### Permanent Redirect This annotation allows to return a permanent redirect (Return Code 301) instead of sending data to the upstream. For example `nginx.ingress.kubernetes.io/permanent-redirect: https://www.google.com` would redirect everything to Google. diff --git a/docs/user-guide/nginx-configuration/configmap.md b/docs/user-guide/nginx-configuration/configmap.md index 51e4edfa36..ab96a17d1e 100644 --- a/docs/user-guide/nginx-configuration/configmap.md +++ b/docs/user-guide/nginx-configuration/configmap.md @@ -218,12 +218,6 @@ The following table shows a configuration option's name, type, and the default v | [block-referers](#block-referers) | []string | "" | | | [proxy-ssl-location-only](#proxy-ssl-location-only) | bool | "false" | | | [default-type](#default-type) | string | "text/html" | | -| [global-rate-limit-memcached-host](#global-rate-limit) | string | "" | | -| [global-rate-limit-memcached-port](#global-rate-limit) | int | 11211 | | -| [global-rate-limit-memcached-connect-timeout](#global-rate-limit) | int | 50 | | -| [global-rate-limit-memcached-max-idle-timeout](#global-rate-limit) | int | 10000 | | -| [global-rate-limit-memcached-pool-size](#global-rate-limit) | int | 50 | | -| [global-rate-limit-status-code](#global-rate-limit) | int | 429 | | | [service-upstream](#service-upstream) | bool | "false" | | | [ssl-reject-handshake](#ssl-reject-handshake) | bool | "false" | | | [debug-connections](#debug-connections) | []string | "127.0.0.1,1.1.1.1/24" | | @@ -1349,22 +1343,6 @@ _**default:**_ text/html _References:_ [https://nginx.org/en/docs/http/ngx_http_core_module.html#default_type](https://nginx.org/en/docs/http/ngx_http_core_module.html#default_type) -## global-rate-limit - -* `global-rate-limit-status-code`: configure HTTP status code to return when rejecting requests. Defaults to 429. - -Configure `memcached` client for [Global Rate Limiting](https://github.com/kubernetes/ingress-nginx/blob/main/docs/user-guide/nginx-configuration/annotations.md#global-rate-limiting). - -* `global-rate-limit-memcached-host`: IP/FQDN of memcached server to use. Required to enable Global Rate Limiting. -* `global-rate-limit-memcached-port`: port of memcached server to use. Defaults default memcached port of `11211`. -* `global-rate-limit-memcached-connect-timeout`: configure timeout for connect, send and receive operations. Unit is millisecond. Defaults to 50ms. -* `global-rate-limit-memcached-max-idle-timeout`: configure timeout for cleaning idle connections. Unit is millisecond. Defaults to 50ms. -* `global-rate-limit-memcached-pool-size`: configure number of max connections to keep alive. Make sure your `memcached` server can handle -`global-rate-limit-memcached-pool-size * worker-processes * ` simultaneous connections. - -These settings get used by [lua-resty-global-throttle](https://github.com/ElvinEfendi/lua-resty-global-throttle) -that ingress-nginx includes. Refer to the link to learn more about `lua-resty-global-throttle`. - ## service-upstream Set if the service's Cluster IP and port should be used instead of a list of all endpoints. This can be overwritten by an annotation on an Ingress rule. diff --git a/images/nginx-1.25/rootfs/build.sh b/images/nginx-1.25/rootfs/build.sh index f60a260f96..ca636d03a0 100755 --- a/images/nginx-1.25/rootfs/build.sh +++ b/images/nginx-1.25/rootfs/build.sh @@ -98,9 +98,6 @@ export LUA_RESTY_REDIS_VERSION=8641b9f1b6f75cca50c90cf8ca5c502ad8950aa8 # Check for recent changes: https://github.com/api7/lua-resty-ipmatcher/compare/v0.6.1...master export LUA_RESTY_IPMATCHER_VERSION=3e93c53eb8c9884efe939ef070486a0e507cc5be -# Check for recent changes: https://github.com/ElvinEfendi/lua-resty-global-throttle/compare/v0.2.0...main -export LUA_RESTY_GLOBAL_THROTTLE_VERSION=v0.2.0 - # Check for recent changes: https://github.com/microsoft/mimalloc/compare/v2.1.7...master export MIMALOC_VERSION=v2.1.7 @@ -276,9 +273,6 @@ get_src c15aed1a01c88a3a6387d9af67a957dff670357f5fdb4ee182beb44635eef3f1 \ get_src efb767487ea3f6031577b9b224467ddbda2ad51a41c5867a47582d4ad85d609e \ "https://github.com/api7/lua-resty-ipmatcher/archive/$LUA_RESTY_IPMATCHER_VERSION.tar.gz" "lua-resty-ipmatcher" -get_src 0fb790e394510e73fdba1492e576aaec0b8ee9ef08e3e821ce253a07719cf7ea \ - "https://github.com/ElvinEfendi/lua-resty-global-throttle/archive/$LUA_RESTY_GLOBAL_THROTTLE_VERSION.tar.gz" "lua-resty-global-throttle" - get_src d74f86ada2329016068bc5a243268f1f555edd620b6a7d6ce89295e7d6cf18da \ "https://github.com/microsoft/mimalloc/archive/${MIMALOC_VERSION}.tar.gz" "mimalloc" @@ -591,9 +585,6 @@ make install cd "$BUILD_PATH/lua-resty-ipmatcher" INST_LUADIR=/usr/local/lib/lua make install -cd "$BUILD_PATH/lua-resty-global-throttle" -make install - cd "$BUILD_PATH/mimalloc" mkdir -p out/release cd out/release diff --git a/images/nginx/rootfs/build.sh b/images/nginx/rootfs/build.sh index 2f5f3c66fd..cfd6493e34 100755 --- a/images/nginx/rootfs/build.sh +++ b/images/nginx/rootfs/build.sh @@ -119,9 +119,6 @@ export LUA_RESTY_REDIS_VERSION=0.30 # Check for recent changes: https://github.com/api7/lua-resty-ipmatcher/compare/v0.6.1...master export LUA_RESTY_IPMATCHER_VERSION=0.6.1 -# Check for recent changes: https://github.com/ElvinEfendi/lua-resty-global-throttle/compare/v0.2.0...main -export LUA_RESTY_GLOBAL_THROTTLE_VERSION=0.2.0 - # Check for recent changes: https://github.com/microsoft/mimalloc/compare/v1.7.6...master export MIMALOC_VERSION=1.7.6 @@ -309,9 +306,6 @@ get_src c15aed1a01c88a3a6387d9af67a957dff670357f5fdb4ee182beb44635eef3f1 \ get_src efb767487ea3f6031577b9b224467ddbda2ad51a41c5867a47582d4ad85d609e \ "https://github.com/api7/lua-resty-ipmatcher/archive/v$LUA_RESTY_IPMATCHER_VERSION.tar.gz" -get_src 0fb790e394510e73fdba1492e576aaec0b8ee9ef08e3e821ce253a07719cf7ea \ - "https://github.com/ElvinEfendi/lua-resty-global-throttle/archive/v$LUA_RESTY_GLOBAL_THROTTLE_VERSION.tar.gz" - get_src d74f86ada2329016068bc5a243268f1f555edd620b6a7d6ce89295e7d6cf18da \ "https://github.com/microsoft/mimalloc/archive/refs/tags/v${MIMALOC_VERSION}.tar.gz" @@ -704,9 +698,6 @@ make install cd "$BUILD_PATH/lua-resty-ipmatcher-$LUA_RESTY_IPMATCHER_VERSION" INST_LUADIR=/usr/local/lib/lua make install -cd "$BUILD_PATH/lua-resty-global-throttle-$LUA_RESTY_GLOBAL_THROTTLE_VERSION" -make install - cd "$BUILD_PATH/mimalloc-$MIMALOC_VERSION" mkdir -p out/release cd out/release diff --git a/internal/ingress/annotations/annotations.go b/internal/ingress/annotations/annotations.go index 4c073246c8..e10cc9be17 100644 --- a/internal/ingress/annotations/annotations.go +++ b/internal/ingress/annotations/annotations.go @@ -39,7 +39,6 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/defaultbackend" "k8s.io/ingress-nginx/internal/ingress/annotations/disableproxyintercepterrors" "k8s.io/ingress-nginx/internal/ingress/annotations/fastcgi" - "k8s.io/ingress-nginx/internal/ingress/annotations/globalratelimit" "k8s.io/ingress-nginx/internal/ingress/annotations/http2pushpreload" "k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist" "k8s.io/ingress-nginx/internal/ingress/annotations/ipdenylist" @@ -98,7 +97,6 @@ type Ingress struct { Proxy proxy.Config ProxySSL proxyssl.Config RateLimit ratelimit.Config - GlobalRateLimit globalratelimit.Config Redirect redirect.Config Rewrite rewrite.Config Satisfy string @@ -147,7 +145,6 @@ func NewAnnotationFactory(cfg resolver.Resolver) map[string]parser.IngressAnnota "Proxy": proxy.NewParser(cfg), "ProxySSL": proxyssl.NewParser(cfg), "RateLimit": ratelimit.NewParser(cfg), - "GlobalRateLimit": globalratelimit.NewParser(cfg), "Redirect": redirect.NewParser(cfg), "Rewrite": rewrite.NewParser(cfg), "Satisfy": satisfy.NewParser(cfg), diff --git a/internal/ingress/annotations/globalratelimit/main.go b/internal/ingress/annotations/globalratelimit/main.go deleted file mode 100644 index 0aec29f66d..0000000000 --- a/internal/ingress/annotations/globalratelimit/main.go +++ /dev/null @@ -1,179 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package globalratelimit - -import ( - "fmt" - "strings" - "time" - - networking "k8s.io/api/networking/v1" - "k8s.io/klog/v2" - - "k8s.io/ingress-nginx/internal/ingress/annotations/parser" - ing_errors "k8s.io/ingress-nginx/internal/ingress/errors" - "k8s.io/ingress-nginx/internal/ingress/resolver" - "k8s.io/ingress-nginx/internal/net" - "k8s.io/ingress-nginx/pkg/util/sets" -) - -const defaultKey = "$remote_addr" - -const ( - globalRateLimitAnnotation = "global-rate-limit" - globalRateLimitWindowAnnotation = "global-rate-limit-window" - globalRateLimitKeyAnnotation = "global-rate-limit-key" - globalRateLimitIgnoredCidrsAnnotation = "global-rate-limit-ignored-cidrs" -) - -var globalRateLimitAnnotationConfig = parser.Annotation{ - Group: "ratelimit", - Annotations: parser.AnnotationFields{ - globalRateLimitAnnotation: { - Validator: parser.ValidateInt, - Scope: parser.AnnotationScopeIngress, - Risk: parser.AnnotationRiskLow, - Documentation: `This annotation configures maximum allowed number of requests per window`, - }, - globalRateLimitWindowAnnotation: { - Validator: parser.ValidateDuration, - Scope: parser.AnnotationScopeIngress, - Risk: parser.AnnotationRiskLow, - Documentation: `Configures a time window (i.e 1m) that the limit is applied`, - }, - globalRateLimitKeyAnnotation: { - Validator: parser.ValidateRegex(parser.NGINXVariable, true), - Scope: parser.AnnotationScopeIngress, - Risk: parser.AnnotationRiskHigh, - Documentation: `This annotation Configures a key for counting the samples. Defaults to $remote_addr. - You can also combine multiple NGINX variables here, like ${remote_addr}-${http_x_api_client} which would mean the limit will be applied to - requests coming from the same API client (indicated by X-API-Client HTTP request header) with the same source IP address`, - }, - globalRateLimitIgnoredCidrsAnnotation: { - Validator: parser.ValidateCIDRs, - Scope: parser.AnnotationScopeIngress, - Risk: parser.AnnotationRiskMedium, - Documentation: `This annotation defines a comma separated list of IPs and CIDRs to match client IP against. - When there's a match request is not considered for rate limiting.`, - }, - }, -} - -// Config encapsulates all global rate limit attributes -type Config struct { - Namespace string `json:"namespace"` - Limit int `json:"limit"` - WindowSize int `json:"window-size"` - Key string `json:"key"` - IgnoredCIDRs []string `json:"ignored-cidrs"` -} - -// Equal tests for equality between two Config types -func (l *Config) Equal(r *Config) bool { - if l.Namespace != r.Namespace { - return false - } - if l.Limit != r.Limit { - return false - } - if l.WindowSize != r.WindowSize { - return false - } - if l.Key != r.Key { - return false - } - if len(l.IgnoredCIDRs) != len(r.IgnoredCIDRs) || !sets.StringElementsMatch(l.IgnoredCIDRs, r.IgnoredCIDRs) { - return false - } - - return true -} - -type globalratelimit struct { - r resolver.Resolver - annotationConfig parser.Annotation -} - -// NewParser creates a new globalratelimit annotation parser -func NewParser(r resolver.Resolver) parser.IngressAnnotation { - return globalratelimit{ - r: r, - annotationConfig: globalRateLimitAnnotationConfig, - } -} - -// Parse extracts globalratelimit annotations from the given ingress -// and returns them structured as Config type -func (a globalratelimit) Parse(ing *networking.Ingress) (interface{}, error) { - config := &Config{} - - limit, err := parser.GetIntAnnotation(globalRateLimitAnnotation, ing, a.annotationConfig.Annotations) - if err != nil && ing_errors.IsInvalidContent(err) { - return nil, err - } - rawWindowSize, err := parser.GetStringAnnotation(globalRateLimitWindowAnnotation, ing, a.annotationConfig.Annotations) - if err != nil && ing_errors.IsValidationError(err) { - return config, ing_errors.LocationDeniedError{ - Reason: fmt.Errorf("failed to parse 'global-rate-limit-window' value: %w", err), - } - } - - if limit == 0 || rawWindowSize == "" { - return config, nil - } - - windowSize, err := time.ParseDuration(rawWindowSize) - if err != nil { - return config, ing_errors.LocationDeniedError{ - Reason: fmt.Errorf("failed to parse 'global-rate-limit-window' value: %w", err), - } - } - - key, err := parser.GetStringAnnotation(globalRateLimitKeyAnnotation, ing, a.annotationConfig.Annotations) - if err != nil { - klog.Warningf("invalid %s, defaulting to %s", globalRateLimitKeyAnnotation, defaultKey) - } - if key == "" { - key = defaultKey - } - - rawIgnoredCIDRs, err := parser.GetStringAnnotation(globalRateLimitIgnoredCidrsAnnotation, ing, a.annotationConfig.Annotations) - if err != nil && ing_errors.IsInvalidContent(err) { - return nil, err - } - ignoredCIDRs, err := net.ParseCIDRs(rawIgnoredCIDRs) - if err != nil { - return nil, err - } - - config.Namespace = strings.ReplaceAll(string(ing.UID), "-", "") - config.Limit = limit - config.WindowSize = int(windowSize.Seconds()) - config.Key = key - config.IgnoredCIDRs = ignoredCIDRs - - return config, nil -} - -func (a globalratelimit) GetDocumentation() parser.AnnotationFields { - return a.annotationConfig.Annotations -} - -func (a globalratelimit) Validate(anns map[string]string) error { - maxrisk := parser.StringRiskToRisk(a.r.GetSecurityConfiguration().AnnotationsRiskLevel) - return parser.CheckAnnotationRisk(anns, maxrisk, globalRateLimitAnnotationConfig.Annotations) -} diff --git a/internal/ingress/annotations/globalratelimit/main_test.go b/internal/ingress/annotations/globalratelimit/main_test.go deleted file mode 100644 index b1a7ab71b2..0000000000 --- a/internal/ingress/annotations/globalratelimit/main_test.go +++ /dev/null @@ -1,211 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package globalratelimit - -import ( - "encoding/json" - "fmt" - "testing" - - api "k8s.io/api/core/v1" - networking "k8s.io/api/networking/v1" - meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "k8s.io/ingress-nginx/internal/ingress/annotations/parser" - ing_errors "k8s.io/ingress-nginx/internal/ingress/errors" - "k8s.io/ingress-nginx/internal/ingress/resolver" -) - -const ( - UID = "31285d47-b150-4dcf-bd6f-12c46d769f6e" - expectedUID = "31285d47b1504dcfbd6f12c46d769f6e" -) - -func buildIngress() *networking.Ingress { - defaultBackend := networking.IngressBackend{ - Service: &networking.IngressServiceBackend{ - Name: "default-backend", - Port: networking.ServiceBackendPort{ - Number: 80, - }, - }, - } - - return &networking.Ingress{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: "foo", - Namespace: api.NamespaceDefault, - UID: UID, - }, - Spec: networking.IngressSpec{ - DefaultBackend: &networking.IngressBackend{ - Service: &networking.IngressServiceBackend{ - Name: "default-backend", - Port: networking.ServiceBackendPort{ - Number: 80, - }, - }, - }, - Rules: []networking.IngressRule{ - { - Host: "foo.bar.com", - IngressRuleValue: networking.IngressRuleValue{ - HTTP: &networking.HTTPIngressRuleValue{ - Paths: []networking.HTTPIngressPath{ - { - Path: "/foo", - Backend: defaultBackend, - }, - }, - }, - }, - }, - }, - }, - } -} - -type mockBackend struct { - resolver.Mock -} - -func TestGlobalRateLimiting(t *testing.T) { - ing := buildIngress() - - annRateLimit := parser.GetAnnotationWithPrefix("global-rate-limit") - annRateLimitWindow := parser.GetAnnotationWithPrefix("global-rate-limit-window") - annRateLimitKey := parser.GetAnnotationWithPrefix("global-rate-limit-key") - annRateLimitIgnoredCIDRs := parser.GetAnnotationWithPrefix("global-rate-limit-ignored-cidrs") - - testCases := []struct { - title string - annotations map[string]string - expectedConfig *Config - expectedErr error - }{ - { - "no annotation", - nil, - &Config{}, - nil, - }, - { - "minimum required annotations", - map[string]string{ - annRateLimit: "100", - annRateLimitWindow: "2m", - }, - &Config{ - Namespace: expectedUID, - Limit: 100, - WindowSize: 120, - Key: "$remote_addr", - IgnoredCIDRs: make([]string, 0), - }, - nil, - }, - { - "global-rate-limit-key annotation", - map[string]string{ - annRateLimit: "100", - annRateLimitWindow: "2m", - annRateLimitKey: "$http_x_api_user", - }, - &Config{ - Namespace: expectedUID, - Limit: 100, - WindowSize: 120, - Key: "$http_x_api_user", - IgnoredCIDRs: make([]string, 0), - }, - nil, - }, - { - "global-rate-limit-ignored-cidrs annotation", - map[string]string{ - annRateLimit: "100", - annRateLimitWindow: "2m", - annRateLimitKey: "$http_x_api_user", - annRateLimitIgnoredCIDRs: "127.0.0.1, 200.200.24.0/24", - }, - &Config{ - Namespace: expectedUID, - Limit: 100, - WindowSize: 120, - Key: "$http_x_api_user", - IgnoredCIDRs: []string{"127.0.0.1", "200.200.24.0/24"}, - }, - nil, - }, - { - "global-rate-limit-complex-key", - map[string]string{ - annRateLimit: "100", - annRateLimitWindow: "2m", - annRateLimitKey: "${http_x_api_user}${otherinfo}", - }, - &Config{ - Namespace: expectedUID, - Limit: 100, - WindowSize: 120, - Key: "${http_x_api_user}${otherinfo}", - IgnoredCIDRs: make([]string, 0), - }, - nil, - }, - { - "incorrect duration for window", - map[string]string{ - annRateLimit: "100", - annRateLimitWindow: "2mb", - annRateLimitKey: "$http_x_api_user", - }, - &Config{}, - ing_errors.ValidationError{ - Reason: fmt.Errorf("failed to parse 'global-rate-limit-window' value: annotation nginx.ingress.kubernetes.io/global-rate-limit-window contains invalid value"), - }, - }, - } - - for _, testCase := range testCases { - ing.SetAnnotations(testCase.annotations) - - i, actualErr := NewParser(mockBackend{}).Parse(ing) - if (testCase.expectedErr == nil || actualErr == nil) && testCase.expectedErr != actualErr { - t.Errorf("%s expected error '%v' but got '%v'", testCase.title, testCase.expectedErr, actualErr) - } else if testCase.expectedErr != nil && actualErr != nil && - testCase.expectedErr.Error() != actualErr.Error() { - t.Errorf("expected error '%v' but got '%v'", testCase.expectedErr, actualErr) - } - - actualConfig, ok := i.(*Config) - if !ok { - t.Errorf("expected Config type but got %T", i) - } - if !testCase.expectedConfig.Equal(actualConfig) { - expectedJSON, err := json.Marshal(testCase.expectedConfig) - if err != nil { - t.Errorf("failed to marshal expected config: %v", err) - } - actualJSON, err := json.Marshal(actualConfig) - if err != nil { - t.Errorf("failed to marshal actual config: %v", err) - } - t.Errorf("%v: expected config '%s' but got '%s'", testCase.title, expectedJSON, actualJSON) - } - } -} diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 63029bbbe7..9b36667c1a 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -718,31 +718,6 @@ type Configuration struct { // Default: text/html DefaultType string `json:"default-type"` - // GlobalRateLimitMemcachedHost configures memcached host. - GlobalRateLimitMemcachedHost string `json:"global-rate-limit-memcached-host"` - - // GlobalRateLimitMemcachedPort configures memcached port. - GlobalRateLimitMemcachedPort int `json:"global-rate-limit-memcached-port"` - - // GlobalRateLimitMemcachedConnectTimeout configures timeout when connecting to memcached. - // The unit is millisecond. - GlobalRateLimitMemcachedConnectTimeout int `json:"global-rate-limit-memcached-connect-timeout"` - - // GlobalRateLimitMemcachedMaxIdleTimeout configured how long connections - // should be kept alive in idle state. The unit is millisecond. - GlobalRateLimitMemcachedMaxIdleTimeout int `json:"global-rate-limit-memcached-max-idle-timeout"` - - // GlobalRateLimitMemcachedPoolSize configures how many connections - // should be kept alive in the pool. - // Note that this is per NGINX worker. Make sure your memcached server can - // handle `MemcachedPoolSize * * ` - // simultaneous connections. - GlobalRateLimitMemcachedPoolSize int `json:"global-rate-limit-memcached-pool-size"` - - // GlobalRateLimitStatusCode determines the HTTP status code to return - // when limit is exceeding during global rate limiting. - GlobalRateLimitStatusCode int `json:"global-rate-limit-status-code"` - // DebugConnections Enables debugging log for selected client connections // http://nginx.org/en/docs/ngx_core_module.html#debug_connection // Default: "" @@ -893,39 +868,34 @@ func NewDefault() Configuration { ServiceUpstream: false, AllowedResponseHeaders: []string{}, }, - UpstreamKeepaliveConnections: 320, - UpstreamKeepaliveTime: "1h", - UpstreamKeepaliveTimeout: 60, - UpstreamKeepaliveRequests: 10000, - LimitConnZoneVariable: defaultLimitConnZoneVariable, - BindAddressIpv4: defBindAddress, - BindAddressIpv6: defBindAddress, - OpentelemetryTrustIncomingSpan: true, - OpentelemetryConfig: "/etc/ingress-controller/telemetry/opentelemetry.toml", - OtlpCollectorPort: "4317", - OtelServiceName: "nginx", - OtelSampler: "AlwaysOn", - OtelSamplerRatio: 0.01, - OtelSamplerParentBased: true, - OtelScheduleDelayMillis: 5000, - OtelMaxExportBatchSize: 512, - OtelMaxQueueSize: 2048, - LimitReqStatusCode: 503, - LimitConnStatusCode: 503, - SyslogPort: 514, - NoTLSRedirectLocations: "/.well-known/acme-challenge", - NoAuthLocations: "/.well-known/acme-challenge", - GlobalExternalAuth: defGlobalExternalAuth, - ProxySSLLocationOnly: false, - DefaultType: "text/html", - GlobalRateLimitMemcachedPort: 11211, - GlobalRateLimitMemcachedConnectTimeout: 50, - GlobalRateLimitMemcachedMaxIdleTimeout: 10000, - GlobalRateLimitMemcachedPoolSize: 50, - GlobalRateLimitStatusCode: 429, - DebugConnections: []string{}, - StrictValidatePathType: true, - GRPCBufferSizeKb: 0, + UpstreamKeepaliveConnections: 320, + UpstreamKeepaliveTime: "1h", + UpstreamKeepaliveTimeout: 60, + UpstreamKeepaliveRequests: 10000, + LimitConnZoneVariable: defaultLimitConnZoneVariable, + BindAddressIpv4: defBindAddress, + BindAddressIpv6: defBindAddress, + OpentelemetryTrustIncomingSpan: true, + OpentelemetryConfig: "/etc/ingress-controller/telemetry/opentelemetry.toml", + OtlpCollectorPort: "4317", + OtelServiceName: "nginx", + OtelSampler: "AlwaysOn", + OtelSamplerRatio: 0.01, + OtelSamplerParentBased: true, + OtelScheduleDelayMillis: 5000, + OtelMaxExportBatchSize: 512, + OtelMaxQueueSize: 2048, + LimitReqStatusCode: 503, + LimitConnStatusCode: 503, + SyslogPort: 514, + NoTLSRedirectLocations: "/.well-known/acme-challenge", + NoAuthLocations: "/.well-known/acme-challenge", + GlobalExternalAuth: defGlobalExternalAuth, + ProxySSLLocationOnly: false, + DefaultType: "text/html", + DebugConnections: []string{}, + StrictValidatePathType: true, + GRPCBufferSizeKb: 0, } if klog.V(5).Enabled() { diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go index 568edc483e..45e11268cc 100644 --- a/internal/ingress/controller/controller.go +++ b/internal/ingress/controller/controller.go @@ -379,10 +379,6 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error { if !cfg.AllowSnippetAnnotations && strings.HasSuffix(key, "-snippet") { return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key) } - - if cfg.GlobalRateLimitMemcachedHost == "" && strings.HasPrefix(key, fmt.Sprintf("%s/%s", parser.AnnotationsPrefix, "global-rate-limit")) { - return fmt.Errorf("'global-rate-limit*' annotations require 'global-rate-limit-memcached-host' settings configured in the global configmap") - } } k8s.SetDefaultNGINXPathType(ing) @@ -1516,7 +1512,6 @@ func locationApplyAnnotations(loc *ingress.Location, anns *annotations.Ingress) loc.Proxy = anns.Proxy loc.ProxySSL = anns.ProxySSL loc.RateLimit = anns.RateLimit - loc.GlobalRateLimit = anns.GlobalRateLimit loc.Redirect = anns.Redirect loc.Rewrite = anns.Rewrite loc.UpstreamVhost = anns.UpstreamVhost diff --git a/internal/ingress/controller/template/configmap.go b/internal/ingress/controller/template/configmap.go index 2f7b0a09c5..481724793e 100644 --- a/internal/ingress/controller/template/configmap.go +++ b/internal/ingress/controller/template/configmap.go @@ -82,7 +82,6 @@ var ( "balancer_ewma_locks": 1024, "certificate_servers": 5120, "ocsp_response_cache": 5120, // keep this same as certificate_servers - "global_throttle_cache": 10240, } defaultGlobalAuthRedirectParam = "rd" ) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 8628f80903..42ff7417e8 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -403,12 +403,6 @@ func configForLua(input interface{}) string { hsts_include_subdomains = %t, hsts_preload = %t, - global_throttle = { - memcached = { - host = "%v", port = %d, connect_timeout = %d, max_idle_timeout = %d, pool_size = %d, - }, - status_code = %d, - } }`, all.Cfg.UseForwardedHeaders, all.Cfg.UseProxyProtocol, @@ -421,13 +415,6 @@ func configForLua(input interface{}) string { all.Cfg.HSTSMaxAge, all.Cfg.HSTSIncludeSubdomains, all.Cfg.HSTSPreload, - - all.Cfg.GlobalRateLimitMemcachedHost, - all.Cfg.GlobalRateLimitMemcachedPort, - all.Cfg.GlobalRateLimitMemcachedConnectTimeout, - all.Cfg.GlobalRateLimitMemcachedMaxIdleTimeout, - all.Cfg.GlobalRateLimitMemcachedPoolSize, - all.Cfg.GlobalRateLimitStatusCode, ) } @@ -445,30 +432,18 @@ func locationConfigForLua(l, a interface{}) string { return "{}" } - ignoredCIDRs, err := convertGoSliceIntoLuaTable(location.GlobalRateLimit.IgnoredCIDRs, false) - if err != nil { - klog.Errorf("failed to convert %v into Lua table: %q", location.GlobalRateLimit.IgnoredCIDRs, err) - ignoredCIDRs = "{}" - } - return fmt.Sprintf(`{ force_ssl_redirect = %t, ssl_redirect = %t, force_no_ssl_redirect = %t, preserve_trailing_slash = %t, use_port_in_redirects = %t, - global_throttle = { namespace = "%v", limit = %d, window_size = %d, key = %v, ignored_cidrs = %v }, }`, location.Rewrite.ForceSSLRedirect, location.Rewrite.SSLRedirect, isLocationInLocationList(l, all.Cfg.NoTLSRedirectLocations), location.Rewrite.PreserveTrailingSlash, location.UsePortInRedirects, - location.GlobalRateLimit.Namespace, - location.GlobalRateLimit.Limit, - location.GlobalRateLimit.WindowSize, - parseComplexNginxVarIntoLuaTable(location.GlobalRateLimit.Key), - ignoredCIDRs, ) } @@ -1690,54 +1665,6 @@ func buildServerName(hostname string) string { return `~^(?[\w-]+)\.` + strings.Join(parts, "\\.") + `$` } -// parseComplexNginxVarIntoLuaTable parses things like "$my${complex}ngx\$var" into -// [["$var", "complex", "my", "ngx"]]. In other words, 2nd and 3rd elements -// in the result are actual NGINX variable names, whereas first and 4th elements -// are string literals. -func parseComplexNginxVarIntoLuaTable(ngxVar string) string { - r := regexp.MustCompile(`(\\\$[0-9a-zA-Z_]+)|\$\{([0-9a-zA-Z_]+)\}|\$([0-9a-zA-Z_]+)|(\$|[^$\\]+)`) - matches := r.FindAllStringSubmatch(ngxVar, -1) - components := make([][]string, len(matches)) - for i, match := range matches { - components[i] = match[1:] - } - - luaTable, err := convertGoSliceIntoLuaTable(components, true) - if err != nil { - klog.Errorf("unexpected error: %v", err) - luaTable = "{}" - } - return luaTable -} - -func convertGoSliceIntoLuaTable(goSliceInterface interface{}, emptyStringAsNil bool) (string, error) { - goSlice := reflect.ValueOf(goSliceInterface) - kind := goSlice.Kind() - - switch kind { - case reflect.String: - if emptyStringAsNil && goSlice.Interface().(string) == "" { - return "nil", nil - } - return fmt.Sprintf(`"%v"`, goSlice.Interface()), nil - case reflect.Int, reflect.Bool: - return fmt.Sprintf(`%v`, goSlice.Interface()), nil - case reflect.Slice, reflect.Array: - luaTable := "{ " - for i := 0; i < goSlice.Len(); i++ { - luaEl, err := convertGoSliceIntoLuaTable(goSlice.Index(i).Interface(), emptyStringAsNil) - if err != nil { - return "", err - } - luaTable = luaTable + luaEl + ", " - } - luaTable += "}" - return luaTable, nil - default: - return "", fmt.Errorf("could not process type: %s", kind) - } -} - func buildOriginRegex(origin string) string { origin = regexp.QuoteMeta(origin) origin = strings.Replace(origin, "\\*", `[A-Za-z0-9\-]+`, 1) diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 3089e3b32f..59d2d62563 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -1926,89 +1926,6 @@ func TestBuildServerName(t *testing.T) { } } -func TestParseComplexNginxVarIntoLuaTable(t *testing.T) { - testCases := []struct { - ngxVar string - expectedLuaTable string - }{ - {"foo", `{ { nil, nil, nil, "foo", }, }`}, - {"$foo", `{ { nil, nil, "foo", nil, }, }`}, - {"${foo}", `{ { nil, "foo", nil, nil, }, }`}, - {"\\$foo", `{ { "\$foo", nil, nil, nil, }, }`}, - { - "foo\\$bar$baz${daz}xiyar$pomidor", - `{ { nil, nil, nil, "foo", }, { "\$bar", nil, nil, nil, }, { nil, nil, "baz", nil, }, ` + - `{ nil, "daz", nil, nil, }, { nil, nil, nil, "xiyar", }, { nil, nil, "pomidor", nil, }, }`, - }, - } - - for _, testCase := range testCases { - actualLuaTable := parseComplexNginxVarIntoLuaTable(testCase.ngxVar) - if actualLuaTable != testCase.expectedLuaTable { - t.Errorf("expected %v but returned %v", testCase.expectedLuaTable, actualLuaTable) - } - } -} - -func TestConvertGoSliceIntoLuaTablet(t *testing.T) { - testCases := []struct { - title string - goSlice interface{} - emptyStringAsNil bool - expectedLuaTable string - expectedErr error - }{ - { - "flat string slice", - []string{"one", "two", "three"}, - false, - `{ "one", "two", "three", }`, - nil, - }, - { - "nested string slice", - [][]string{{"one", "", "three"}, {"foo", "bar"}}, - false, - `{ { "one", "", "three", }, { "foo", "bar", }, }`, - nil, - }, - { - "converts empty string to nil when enabled", - [][]string{{"one", "", "three"}, {"foo", "bar"}}, - true, - `{ { "one", nil, "three", }, { "foo", "bar", }, }`, - nil, - }, - { - "boolean slice", - []bool{true, true, false}, - false, - `{ true, true, false, }`, - nil, - }, - { - "integer slice", - []int{4, 3, 6}, - false, - `{ 4, 3, 6, }`, - nil, - }, - } - - for _, testCase := range testCases { - actualLuaTable, err := convertGoSliceIntoLuaTable(testCase.goSlice, testCase.emptyStringAsNil) - if testCase.expectedErr != nil && err != nil && testCase.expectedErr.Error() != err.Error() { - t.Errorf("expected error '%v' but returned '%v'", testCase.expectedErr, err) - } - if testCase.expectedErr == nil && err != nil { - t.Errorf("expected error to be nil but returned '%v'", err) - } - if testCase.expectedLuaTable != actualLuaTable { - t.Errorf("%v: expected '%v' but returned '%v'", testCase.title, testCase.expectedLuaTable, actualLuaTable) - } - } -} - func TestCleanConf(t *testing.T) { testDataDir, err := getTestDataDir() if err != nil { diff --git a/pkg/apis/ingress/types.go b/pkg/apis/ingress/types.go index 36067732ea..9370333c18 100644 --- a/pkg/apis/ingress/types.go +++ b/pkg/apis/ingress/types.go @@ -29,7 +29,6 @@ import ( "k8s.io/ingress-nginx/internal/ingress/annotations/cors" "k8s.io/ingress-nginx/internal/ingress/annotations/customheaders" "k8s.io/ingress-nginx/internal/ingress/annotations/fastcgi" - "k8s.io/ingress-nginx/internal/ingress/annotations/globalratelimit" "k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist" "k8s.io/ingress-nginx/internal/ingress/annotations/ipdenylist" "k8s.io/ingress-nginx/internal/ingress/annotations/log" @@ -285,10 +284,6 @@ type Location struct { // The Redirect annotation precedes RateLimit // +optional RateLimit ratelimit.Config `json:"rateLimit,omitempty"` - // GlobalRateLimit similar to RateLimit - // but this is applied globally across multiple replicas. - // +optional - GlobalRateLimit globalratelimit.Config `json:"globalRateLimit,omitempty"` // Redirect describes a temporal o permanent redirection this location. // +optional Redirect redirect.Config `json:"redirect,omitempty"` diff --git a/pkg/apis/ingress/types_equals.go b/pkg/apis/ingress/types_equals.go index eeed9a06e4..aa6c3efa76 100644 --- a/pkg/apis/ingress/types_equals.go +++ b/pkg/apis/ingress/types_equals.go @@ -390,9 +390,6 @@ func (l1 *Location) Equal(l2 *Location) bool { if !(&l1.RateLimit).Equal(&l2.RateLimit) { return false } - if !(&l1.GlobalRateLimit).Equal(&l2.GlobalRateLimit) { - return false - } if !(&l1.Redirect).Equal(&l2.Redirect) { return false } diff --git a/rootfs/etc/nginx/lua/global_throttle.lua b/rootfs/etc/nginx/lua/global_throttle.lua deleted file mode 100644 index bea8cfd17b..0000000000 --- a/rootfs/etc/nginx/lua/global_throttle.lua +++ /dev/null @@ -1,131 +0,0 @@ -local resty_global_throttle = require("resty.global_throttle") -local resty_ipmatcher = require("resty.ipmatcher") -local util = require("util") - -local ngx = ngx -local ngx_exit = ngx.exit -local ngx_log = ngx.log -local ngx_ERR = ngx.ERR -local ngx_INFO = ngx.INFO - -local _M = {} - -local DECISION_CACHE = ngx.shared.global_throttle_cache - --- it does not make sense to cache decision for too little time --- the benefit of caching likely is negated if we cache for too little time --- Lua Shared Dict's time resolution for expiry is 0.001. -local CACHE_THRESHOLD = 0.001 - -local DEFAULT_RAW_KEY = "remote_addr" - -local function should_ignore_request(ignored_cidrs) - if not ignored_cidrs or #ignored_cidrs == 0 then - return false - end - - local ignored_cidrs_matcher, err = resty_ipmatcher.new(ignored_cidrs) - if not ignored_cidrs_matcher then - ngx_log(ngx_ERR, "failed to initialize resty-ipmatcher: ", err) - return false - end - - local is_ignored - is_ignored, err = ignored_cidrs_matcher:match(ngx.var.remote_addr) - if err then - ngx_log(ngx_ERR, "failed to match ip: '", - ngx.var.remote_addr, "': ", err) - return false - end - - return is_ignored -end - -local function is_enabled(config, location_config) - if config.memcached.host == "" or config.memcached.port == 0 then - return false - end - if location_config.limit == 0 or - location_config.window_size == 0 then - return false - end - - if should_ignore_request(location_config.ignored_cidrs) then - return false - end - - return true -end - -local function get_namespaced_key_value(namespace, key_value) - return namespace .. key_value -end - -function _M.throttle(config, location_config) - if not is_enabled(config, location_config) then - return - end - - local key_value = util.generate_var_value(location_config.key) - if not key_value or key_value == "" then - key_value = ngx.var[DEFAULT_RAW_KEY] - end - - local namespaced_key_value = - get_namespaced_key_value(location_config.namespace, key_value) - - local is_limit_exceeding = DECISION_CACHE:get(namespaced_key_value) - if is_limit_exceeding then - ngx.var.global_rate_limit_exceeding = "c" - return ngx_exit(config.status_code) - end - - local my_throttle, err = resty_global_throttle.new( - location_config.namespace, - location_config.limit, - location_config.window_size, - { - provider = "memcached", - host = config.memcached.host, - port = config.memcached.port, - connect_timeout = config.memcached.connect_timeout, - max_idle_timeout = config.memcached.max_idle_timeout, - pool_size = config.memcached.pool_size, - } - ) - if err then - ngx.log(ngx.ERR, "faled to initialize resty_global_throttle: ", err) - -- fail open - return - end - - local desired_delay, estimated_final_count - estimated_final_count, desired_delay, err = my_throttle:process(key_value) - if err then - ngx.log(ngx.ERR, "error while processing key: ", err) - -- fail open - return - end - - if desired_delay then - if desired_delay > CACHE_THRESHOLD then - local ok - ok, err = - DECISION_CACHE:safe_add(namespaced_key_value, true, desired_delay) - if not ok then - if err ~= "exists" then - ngx_log(ngx_ERR, "failed to cache decision: ", err) - end - end - end - - ngx.var.global_rate_limit_exceeding = "y" - ngx_log(ngx_INFO, "limit is exceeding for ", - location_config.namespace, "/", key_value, - " with estimated_final_count: ", estimated_final_count) - - return ngx_exit(config.status_code) - end -end - -return _M diff --git a/rootfs/etc/nginx/lua/lua_ingress.lua b/rootfs/etc/nginx/lua/lua_ingress.lua index 49e0f5b05a..65cf5a2577 100644 --- a/rootfs/etc/nginx/lua/lua_ingress.lua +++ b/rootfs/etc/nginx/lua/lua_ingress.lua @@ -2,7 +2,6 @@ local ngx_re_split = require("ngx.re").split local certificate_configured_for_current_request = require("certificate").configured_for_current_request -local global_throttle = require("global_throttle") local ngx = ngx local io = io @@ -164,7 +163,6 @@ function _M.rewrite(location_config) return ngx_redirect(uri, config.http_redirect_code) end - global_throttle.throttle(config.global_throttle, location_config.global_throttle) end function _M.header() diff --git a/rootfs/etc/nginx/lua/test/global_throttle_test.lua b/rootfs/etc/nginx/lua/test/global_throttle_test.lua deleted file mode 100644 index b8db740ade..0000000000 --- a/rootfs/etc/nginx/lua/test/global_throttle_test.lua +++ /dev/null @@ -1,258 +0,0 @@ -local util = require("util") - -local function assert_request_rejected(config, location_config, opts) - stub(ngx, "exit") - - local global_throttle = require_without_cache("global_throttle") - assert.has_no.errors(function() - global_throttle.throttle(config, location_config) - end) - - assert.stub(ngx.exit).was_called_with(config.status_code) - if opts.with_cache then - assert.are.same("c", ngx.var.global_rate_limit_exceeding) - else - assert.are.same("y", ngx.var.global_rate_limit_exceeding) - end -end - -local function assert_request_not_rejected(config, location_config) - stub(ngx, "exit") - local cache_safe_add_spy = spy.on(ngx.shared.global_throttle_cache, "safe_add") - - local global_throttle = require_without_cache("global_throttle") - assert.has_no.errors(function() - global_throttle.throttle(config, location_config) - end) - - assert.stub(ngx.exit).was_not_called() - assert.is_nil(ngx.var.global_rate_limit_exceeding) - assert.spy(cache_safe_add_spy).was_not_called() -end - -local function assert_short_circuits(f) - local cache_get_spy = spy.on(ngx.shared.global_throttle_cache, "get") - - local resty_global_throttle = require_without_cache("resty.global_throttle") - local resty_global_throttle_new_spy = spy.on(resty_global_throttle, "new") - - local global_throttle = require_without_cache("global_throttle") - - f(global_throttle) - - assert.spy(resty_global_throttle_new_spy).was_not_called() - assert.spy(cache_get_spy).was_not_called() -end - -local function assert_fails_open(config, location_config, ...) - stub(ngx, "exit") - stub(ngx, "log") - - local global_throttle = require_without_cache("global_throttle") - - assert.has_no.errors(function() - global_throttle.throttle(config, location_config) - end) - - assert.stub(ngx.exit).was_not_called() - assert.stub(ngx.log).was_called_with(ngx.ERR, ...) - assert.is_nil(ngx.var.global_rate_limit_exceeding) -end - -local function stub_resty_global_throttle_process(ret1, ret2, ret3, f) - local resty_global_throttle = require_without_cache("resty.global_throttle") - local resty_global_throttle_mock = { - process = function(self, key) return ret1, ret2, ret3 end - } - stub(resty_global_throttle, "new", resty_global_throttle_mock) - - f() - - assert.stub(resty_global_throttle.new).was_called() -end - -local function cache_rejection_decision(namespace, key_value, desired_delay) - local namespaced_key_value = namespace .. key_value - local ok, err = ngx.shared.global_throttle_cache:safe_add(namespaced_key_value, true, desired_delay) - assert.is_nil(err) - assert.is_true(ok) - assert.is_true(ngx.shared.global_throttle_cache:get(namespaced_key_value)) -end - -describe("global_throttle", function() - local snapshot - - local NAMESPACE = "31285d47b1504dcfbd6f12c46d769f6e" - local LOCATION_CONFIG = { - namespace = NAMESPACE, - limit = 10, - window_size = 60, - key = {}, - ignored_cidrs = {}, - } - local CONFIG = { - memcached = { - host = "memc.default.svc.cluster.local", port = 11211, - connect_timeout = 50, max_idle_timeout = 10000, pool_size = 50, - }, - status_code = 429, - } - - before_each(function() - snapshot = assert:snapshot() - - ngx.var = { remote_addr = "127.0.0.1", global_rate_limit_exceeding = nil } - end) - - after_each(function() - snapshot:revert() - - ngx.shared.global_throttle_cache:flush_all() - reset_ngx() - end) - - it("short circuits when memcached is not configured", function() - assert_short_circuits(function(global_throttle) - assert.has_no.errors(function() - global_throttle.throttle({ memcached = { host = "", port = 0 } }, LOCATION_CONFIG) - end) - end) - end) - - it("short circuits when limit or window_size is not configured", function() - assert_short_circuits(function(global_throttle) - local location_config_copy = util.deepcopy(LOCATION_CONFIG) - location_config_copy.limit = 0 - assert.has_no.errors(function() - global_throttle.throttle(CONFIG, location_config_copy) - end) - end) - - assert_short_circuits(function(global_throttle) - local location_config_copy = util.deepcopy(LOCATION_CONFIG) - location_config_copy.window_size = 0 - assert.has_no.errors(function() - global_throttle.throttle(CONFIG, location_config_copy) - end) - end) - end) - - it("short circuits when remote_addr is in ignored_cidrs", function() - local global_throttle = require_without_cache("global_throttle") - local location_config = util.deepcopy(LOCATION_CONFIG) - location_config.ignored_cidrs = { ngx.var.remote_addr } - assert_short_circuits(function(global_throttle) - assert.has_no.errors(function() - global_throttle.throttle(CONFIG, location_config) - end) - end) - end) - - it("rejects when exceeding limit has already been cached", function() - local key_value = "foo" - local location_config = util.deepcopy(LOCATION_CONFIG) - location_config.key = { { nil, nil, nil, key_value } } - cache_rejection_decision(NAMESPACE, key_value, 0.5) - - assert_request_rejected(CONFIG, location_config, { with_cache = true }) - end) - - describe("when resty_global_throttle fails", function() - it("fails open in case of initialization error", function() - local too_long_namespace = "" - for i=1,36,1 do - too_long_namespace = too_long_namespace .. "a" - end - - local location_config = util.deepcopy(LOCATION_CONFIG) - location_config.namespace = too_long_namespace - - assert_fails_open(CONFIG, location_config, "faled to initialize resty_global_throttle: ", "'namespace' can be at most 35 characters") - end) - - it("fails open in case of key processing error", function() - stub_resty_global_throttle_process(nil, nil, "failed to process", function() - assert_fails_open(CONFIG, LOCATION_CONFIG, "error while processing key: ", "failed to process") - end) - end) - end) - - it("initializes resty_global_throttle with the right parameters", function() - local resty_global_throttle = require_without_cache("resty.global_throttle") - local resty_global_throttle_original_new = resty_global_throttle.new - resty_global_throttle.new = function(namespace, limit, window_size, store_opts) - local o, err = resty_global_throttle_original_new(namespace, limit, window_size, store_opts) - if not o then - return nil, err - end - o.process = function(self, key) return 1, nil, nil end - - local expected = LOCATION_CONFIG - assert.are.same(expected.namespace, namespace) - assert.are.same(expected.limit, limit) - assert.are.same(expected.window_size, window_size) - - assert.are.same("memcached", store_opts.provider) - assert.are.same(CONFIG.memcached.host, store_opts.host) - assert.are.same(CONFIG.memcached.port, store_opts.port) - assert.are.same(CONFIG.memcached.connect_timeout, store_opts.connect_timeout) - assert.are.same(CONFIG.memcached.max_idle_timeout, store_opts.max_idle_timeout) - assert.are.same(CONFIG.memcached.pool_size, store_opts.pool_size) - - return o, nil - end - local resty_global_throttle_new_spy = spy.on(resty_global_throttle, "new") - - local global_throttle = require_without_cache("global_throttle") - - assert.has_no.errors(function() - global_throttle.throttle(CONFIG, LOCATION_CONFIG) - end) - - assert.spy(resty_global_throttle_new_spy).was_called() - end) - - it("rejects request and caches decision when limit is exceeding after processing a key", function() - local desired_delay = 0.015 - - stub_resty_global_throttle_process(LOCATION_CONFIG.limit + 1, desired_delay, nil, function() - assert_request_rejected(CONFIG, LOCATION_CONFIG, { with_cache = false }) - - local cache_key = LOCATION_CONFIG.namespace .. ngx.var.remote_addr - assert.is_true(ngx.shared.global_throttle_cache:get(cache_key)) - - -- we assume it won't take more than this after caching - -- until we execute the assertion below - local delta = 0.001 - local ttl = ngx.shared.global_throttle_cache:ttl(cache_key) - assert.is_true(ttl > desired_delay - delta) - assert.is_true(ttl <= desired_delay) - end) - end) - - it("rejects request and skip caching of decision when limit is exceeding after processing a key but desired delay is lower than the threshold", function() - local desired_delay = 0.0009 - - stub_resty_global_throttle_process(LOCATION_CONFIG.limit, desired_delay, nil, function() - assert_request_rejected(CONFIG, LOCATION_CONFIG, { with_cache = false }) - - local cache_key = LOCATION_CONFIG.namespace .. ngx.var.remote_addr - assert.is_nil(ngx.shared.global_throttle_cache:get(cache_key)) - end) - end) - - it("allows the request when limit is not exceeding after processing a key", function() - stub_resty_global_throttle_process(LOCATION_CONFIG.limit - 3, nil, nil, - function() - assert_request_not_rejected(CONFIG, LOCATION_CONFIG) - end - ) - end) - - it("rejects with custom status code", function() - cache_rejection_decision(NAMESPACE, ngx.var.remote_addr, 0.3) - local config = util.deepcopy(CONFIG) - config.status_code = 503 - assert_request_rejected(config, LOCATION_CONFIG, { with_cache = true }) - end) -end) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 1c630bb4d7..4f705976ff 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -1242,7 +1242,6 @@ stream { set $service_name {{ $ing.Service | quote }}; set $service_port {{ $ing.ServicePort | quote }}; set $location_path {{ $ing.Path | escapeLiteralDollar | quote }}; - set $global_rate_limit_exceeding n; {{ buildOpentelemetryForLocation $all.Cfg.EnableOpentelemetry $all.Cfg.OpentelemetryTrustIncomingSpan $location }} diff --git a/test/e2e/admission/admission.go b/test/e2e/admission/admission.go index 124e1784de..873e6719da 100644 --- a/test/e2e/admission/admission.go +++ b/test/e2e/admission/admission.go @@ -44,33 +44,6 @@ var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller", f.NewSlowEchoDeployment() }) - ginkgo.It("reject ingress with global-rate-limit annotations when memcached is not configured", func() { - host := admissionTestHost - - annotations := map[string]string{ - "nginx.ingress.kubernetes.io/global-rate-limit": "100", - "nginx.ingress.kubernetes.io/global-rate-limit-window": "1m", - } - ing := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations) - - ginkgo.By("rejects ingress when memcached is not configured") - - _, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), ing, metav1.CreateOptions{}) - assert.NotNil(ginkgo.GinkgoT(), err, "creating ingress with global throttle annotations when memcached is not configured") - - ginkgo.By("accepts ingress when memcached is not configured") - - f.UpdateNginxConfigMapData("global-rate-limit-memcached-host", "memc.default.svc.cluster.local") - - _, err = f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), ing, metav1.CreateOptions{}) - assert.Nil(ginkgo.GinkgoT(), err, "creating ingress with global throttle annotations when memcached is configured") - - f.WaitForNginxServer(host, - func(server string) bool { - return strings.Contains(server, fmt.Sprintf("server_name %v", host)) - }) - }) - ginkgo.It("should not allow overlaps of host and paths without canary annotations", func() { host := admissionTestHost diff --git a/test/e2e/annotations/globalratelimit.go b/test/e2e/annotations/globalratelimit.go deleted file mode 100644 index 96be467fe0..0000000000 --- a/test/e2e/annotations/globalratelimit.go +++ /dev/null @@ -1,88 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package annotations - -import ( - "fmt" - "net/http" - "strings" - - "github.com/onsi/ginkgo/v2" - "github.com/stretchr/testify/assert" - - "k8s.io/ingress-nginx/test/e2e/framework" -) - -var _ = framework.DescribeAnnotation("annotation-global-rate-limit", func() { - f := framework.NewDefaultFramework("global-rate-limit") - host := "global-rate-limit-annotation" - - ginkgo.BeforeEach(func() { - f.NewEchoDeployment() - }) - - ginkgo.It("generates correct configuration", func() { - annotations := make(map[string]string) - annotations["nginx.ingress.kubernetes.io/global-rate-limit"] = "5" - annotations["nginx.ingress.kubernetes.io/global-rate-limit-window"] = "2m" - - // We need to allow { and } characters for this annotation to work - f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root") - // Sleep a while just to guarantee that the configmap is applied - framework.Sleep() - - ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) - ing = f.EnsureIngress(ing) - namespace := strings.ReplaceAll(string(ing.UID), "-", "") - - serverConfig := "" - f.WaitForNginxServer(host, func(server string) bool { - serverConfig = server - return true - }) - assert.Contains(ginkgo.GinkgoT(), serverConfig, - fmt.Sprintf(`global_throttle = { namespace = "%v", `+ - `limit = 5, window_size = 120, key = { { nil, nil, "remote_addr", nil, }, }, `+ - `ignored_cidrs = { } }`, - namespace)) - - f.HTTPTestClient().GET("/").WithHeader("Host", host).Expect().Status(http.StatusOK) - - ginkgo.By("regenerating the correct configuration after update") - annotations["nginx.ingress.kubernetes.io/global-rate-limit-key"] = "${remote_addr}${http_x_api_client}" - annotations["nginx.ingress.kubernetes.io/global-rate-limit-ignored-cidrs"] = "192.168.1.1, 234.234.234.0/24" - ing.SetAnnotations(annotations) - - f.WaitForReload(func() { - ing = f.UpdateIngress(ing) - }) - - serverConfig = "" - f.WaitForNginxServer(host, func(server string) bool { - serverConfig = server - return true - }) - assert.Contains(ginkgo.GinkgoT(), serverConfig, - fmt.Sprintf(`global_throttle = { namespace = "%v", `+ - `limit = 5, window_size = 120, `+ - `key = { { nil, "remote_addr", nil, nil, }, { nil, "http_x_api_client", nil, nil, }, }, `+ - `ignored_cidrs = { "192.168.1.1", "234.234.234.0/24", } }`, - namespace)) - - f.HTTPTestClient().GET("/").WithHeader("Host", host).Expect().Status(http.StatusOK) - }) -}) diff --git a/test/e2e/settings/globalratelimit.go b/test/e2e/settings/globalratelimit.go deleted file mode 100644 index e266350adf..0000000000 --- a/test/e2e/settings/globalratelimit.go +++ /dev/null @@ -1,96 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package settings - -import ( - "fmt" - "net/http" - "strconv" - "strings" - - "github.com/onsi/ginkgo/v2" - "github.com/stretchr/testify/assert" - "k8s.io/ingress-nginx/test/e2e/framework" -) - -var _ = framework.DescribeSetting("settings-global-rate-limit", func() { - f := framework.NewDefaultFramework("global-rate-limit") - host := "global-rate-limit" - - ginkgo.BeforeEach(func() { - f.NewEchoDeployment() - }) - - ginkgo.It("generates correct NGINX configuration", func() { - annotations := make(map[string]string) - ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations) - f.EnsureIngress(ing) - - ginkgo.By("generating correct defaults") - - ngxCfg := "" - f.WaitForNginxConfiguration(func(cfg string) bool { - if strings.Contains(cfg, "global_throttle") { - ngxCfg = cfg - return true - } - return false - }) - - assert.Contains(ginkgo.GinkgoT(), ngxCfg, fmt.Sprintf(`global_throttle = { `+ - `memcached = { host = "%v", port = %d, connect_timeout = %d, max_idle_timeout = %d, `+ - `pool_size = %d, }, status_code = %d, }`, - "", 11211, 50, 10000, 50, 429)) - - f.HTTPTestClient().GET("/").WithHeader("Host", host).Expect().Status(http.StatusOK) - - ginkgo.By("applying customizations") - - memcachedHost := "memc.default.svc.cluster.local" - memcachedPort := 11211 - memcachedConnectTimeout := 100 - memcachedMaxIdleTimeout := 5000 - memcachedPoolSize := 100 - statusCode := 503 - - f.SetNginxConfigMapData(map[string]string{ - "global-rate-limit-memcached-host": memcachedHost, - "global-rate-limit-memcached-port": strconv.Itoa(memcachedPort), - "global-rate-limit-memcached-connect-timeout": strconv.Itoa(memcachedConnectTimeout), - "global-rate-limit-memcached-max-idle-timeout": strconv.Itoa(memcachedMaxIdleTimeout), - "global-rate-limit-memcached-pool-size": strconv.Itoa(memcachedPoolSize), - "global-rate-limit-status-code": strconv.Itoa(statusCode), - }) - - ngxCfg = "" - f.WaitForNginxConfiguration(func(cfg string) bool { - if strings.Contains(cfg, "global_throttle") { - ngxCfg = cfg - return true - } - return false - }) - - assert.Contains(ginkgo.GinkgoT(), ngxCfg, fmt.Sprintf(`global_throttle = { `+ - `memcached = { host = "%v", port = %d, connect_timeout = %d, max_idle_timeout = %d, `+ - `pool_size = %d, }, status_code = %d, }`, - memcachedHost, memcachedPort, memcachedConnectTimeout, memcachedMaxIdleTimeout, - memcachedPoolSize, statusCode)) - - f.HTTPTestClient().GET("/").WithHeader("Host", host).Expect().Status(http.StatusOK) - }) -}) diff --git a/test/test-lua.sh b/test/test-lua.sh index 1aff5f30c2..e7ee5843ea 100755 --- a/test/test-lua.sh +++ b/test/test-lua.sh @@ -36,7 +36,6 @@ SHDICT_ARGS=( "--shdict" "high_throughput_tracker 1M" "--shdict" "balancer_ewma_last_touched_at 1M" "--shdict" "balancer_ewma_locks 512k" - "--shdict" "global_throttle_cache 5M" "./rootfs/etc/nginx/lua/test/run.lua" )