From bbf1ee2111bdbb997b6fb9846f56c40fdec422a9 Mon Sep 17 00:00:00 2001 From: Alexander Yastrebov Date: Mon, 9 Oct 2023 18:06:34 +0200 Subject: [PATCH] filters/ratelimit: apply ratelimitFailClosed regardless of the position in the route ratelimitFailClosed is a route-wide configuration filter so it should apply to all ratelimit filters regardless of its position. Signed-off-by: Alexander Yastrebov --- docs/reference/filters.md | 15 +++++++------- filters/ratelimit/fail_closed.go | 28 ++++++++++++++------------- filters/ratelimit/fail_closed_test.go | 6 ++++++ 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/docs/reference/filters.md b/docs/reference/filters.md index 1fde77fe41..ffcf92268f 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -1721,7 +1721,7 @@ As of now there is no negative/deny rule possible. The first matching path is ev ### Open Policy Agent -To get started with [Open Policy Agent](https://www.openpolicyagent.org/), also have a look at the [tutorial](../tutorials/auth.md#open-policy-agent). This section is only a reference for the implemented filters. +To get started with [Open Policy Agent](https://www.openpolicyagent.org/), also have a look at the [tutorial](../tutorials/auth.md#open-policy-agent). This section is only a reference for the implemented filters. #### opaAuthorizeRequest @@ -1796,7 +1796,7 @@ The difference is that if the decision in (3) is equivalent to false, the respon Headers both to the upstream and the downstream service can be manipulated the same way this works for [Envoy external authorization](https://www.openpolicyagent.org/docs/latest/envoy-primer/#example-policy-with-additional-controls) -This allows both to add and remove unwanted headers in allow/deny cases. +This allows both to add and remove unwanted headers in allow/deny cases. #### opaServeResponse @@ -1821,7 +1821,7 @@ For this filter, the data flow looks like this independent of an allow/deny deci ```ascii ┌──────────────────┐ - (1) Request │ Skipper │ + (1) Request │ Skipper │ ─────────────┤ ├ │ │ (4) Response│ (2)│ ▲ (3) │ @@ -2255,8 +2255,9 @@ Path("/expensive") -> clusterLeakyBucketRatelimit("user-${request.cookie.Authori ### ratelimitFailClosed -This filter changes the failure mode for rate limit filters. If the -filter is present, infrastructure issues will lead to rate limit. +This filter changes the failure mode for all rate limit filters of the route. +By default rate limit filters fail open on infrastructure errors (e.g. Redis is down). +When filter is present, infrastructure errors will result in request rejected, i.e. fail closed. Examples: ``` @@ -2264,7 +2265,7 @@ fail_open: * -> clusterRatelimit("g",10, "1s") fail_closed: * -> ratelimitFailClosed() -> clusterRatelimit("g", 10, "1s") ``` -In case `clusterRatelimit` could not reach the swarm (f.e. redis): +In case `clusterRatelimit` could not reach the swarm (e.g. redis): * Route `fail_open` will allow the request * Route `fail_closed` will deny the request @@ -3015,7 +3016,7 @@ tracingTag("http.flow_id", "${request.header.X-Flow-Id}") ### tracingTagFromResponse -This filter works just like [tracingTag](#tracingTag), but is applied after the request was processed. In particular, [template placeholders](#template-placeholders) referencing the response can be used in the parameters. +This filter works just like [tracingTag](#tracingTag), but is applied after the request was processed. In particular, [template placeholders](#template-placeholders) referencing the response can be used in the parameters. ### tracingSpanName diff --git a/filters/ratelimit/fail_closed.go b/filters/ratelimit/fail_closed.go index 1bd675d7a5..4dd75fa31f 100644 --- a/filters/ratelimit/fail_closed.go +++ b/filters/ratelimit/fail_closed.go @@ -19,18 +19,20 @@ func NewFailClosedPostProcessor() *FailClosedPostProcessor { // new routes. func (*FailClosedPostProcessor) Do(routes []*routing.Route) []*routing.Route { for _, r := range routes { - var found bool - + var failClosed bool for _, f := range r.Filters { if f.Name == filters.RatelimitFailClosedName { - found = true - continue - } - // no config changes detected - if !found { - continue + failClosed = true + break } + } + // no config changes detected + if !failClosed { + continue + } + + for _, f := range r.Filters { switch f.Name { // leaky bucket has no Settings case filters.ClusterLeakyBucketRatelimitName: @@ -45,11 +47,11 @@ func (*FailClosedPostProcessor) Do(routes []*routing.Route) []*routing.Route { bf.Settings.FailClosed = true } - case filters.ClientRatelimitName: - fallthrough - case filters.ClusterClientRatelimitName: - fallthrough - case filters.ClusterRatelimitName: + case + filters.ClientRatelimitName, + filters.ClusterClientRatelimitName, + filters.ClusterRatelimitName: + ff, ok := f.Filter.(*filter) if ok { ff.settings.FailClosed = true diff --git a/filters/ratelimit/fail_closed_test.go b/filters/ratelimit/fail_closed_test.go index c2a7d073b8..16ed149821 100644 --- a/filters/ratelimit/fail_closed_test.go +++ b/filters/ratelimit/fail_closed_test.go @@ -70,6 +70,12 @@ func TestFailureMode(t *testing.T) { wantLimit: true, limitStatusCode: http.StatusTooManyRequests, }, + { + name: "test ratelimitFailClosed applies when placed after ratelimit filter", + filters: `clusterRatelimit("t", 1, "1s") -> ratelimitFailClosed()`, + wantLimit: true, + limitStatusCode: http.StatusTooManyRequests, + }, } { t.Run(tt.name, func(t *testing.T) { fr := builtin.MakeRegistry()