Skip to content

Commit

Permalink
filters/ratelimit: apply ratelimitFailClosed regardless of the positi…
Browse files Browse the repository at this point in the history 
…on in the route (#2667)

* 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 <[email protected]>
  • Loading branch information
@AlexanderYastrebov
AlexanderYastrebov authored Oct 10, 2023
1 parent 56d8708 commit 79a9f35
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 87 deletions.
15 changes: 8 additions & 7 deletions docs/reference/filters.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand All @@ -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) │
Expand Down Expand Up @@ -2255,16 +2255,17 @@ 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. when redis is down) and allow requests.
When this filter is present on the route, rate limit filters will fail closed in case of infrastructure errors and deny requests.

Examples:
```
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
Expand Down Expand Up @@ -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
Expand Down
28 changes: 15 additions & 13 deletions filters/ratelimit/fail_closed.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down
117 changes: 50 additions & 67 deletions filters/ratelimit/fail_closed_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ package ratelimit_test
import (
"net/http"
"net/http/httptest"
"net/url"
"testing"

"github.com/zalando/skipper/eskip"
"github.com/zalando/skipper/filters"
"github.com/zalando/skipper/filters/builtin"
fratelimit "github.com/zalando/skipper/filters/ratelimit"
snet "github.com/zalando/skipper/net"
Expand All @@ -19,68 +17,70 @@ import (

func TestFailureMode(t *testing.T) {
for _, tt := range []struct {
name string
ratelimitFilterName string
failClosed bool
wantLimit bool
limitStatusCode int
name string
filters string
wantLimit bool
limitStatusCode int
}{
{
name: "test clusterRatelimit fail open",
ratelimitFilterName: "clusterRatelimit",
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
name: "test clusterRatelimit fail open",
filters: `clusterRatelimit("t", 1, "1s")`,
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
},
{
name: "test clusterRatelimit fail closed",
ratelimitFilterName: "clusterRatelimit",
failClosed: true,
wantLimit: true,
limitStatusCode: http.StatusTooManyRequests,
name: "test clusterRatelimit fail closed",
filters: `ratelimitFailClosed() -> clusterRatelimit("t", 1, "1s")`,
wantLimit: true,
limitStatusCode: http.StatusTooManyRequests,
},
{
name: "test clusterClientRatelimit fail open",
ratelimitFilterName: "clusterClientRatelimit",
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
name: "test clusterClientRatelimit fail open",
filters: `clusterClientRatelimit("t", 1, "1s", "X-Test")`,
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
},
{
name: "test clusterClientRatelimit fail closed",
ratelimitFilterName: "clusterClientRatelimit",
failClosed: true,
wantLimit: true,
limitStatusCode: http.StatusTooManyRequests,
name: "test clusterClientRatelimit fail closed",
filters: `ratelimitFailClosed() -> clusterClientRatelimit("t", 1, "1s", "X-Test")`,
wantLimit: true,
limitStatusCode: http.StatusTooManyRequests,
},
{
name: "test backendRatelimit fail open",
ratelimitFilterName: "backendRatelimit",
wantLimit: false,
limitStatusCode: http.StatusServiceUnavailable,
name: "test backendRatelimit fail open",
filters: `backendRatelimit("t", 1, "1s")`,
wantLimit: false,
limitStatusCode: http.StatusServiceUnavailable,
},
{
name: "test backendRatelimit fail closed",
ratelimitFilterName: "backendRatelimit",
failClosed: true,
wantLimit: true,
limitStatusCode: http.StatusServiceUnavailable,
name: "test backendRatelimit fail closed",
filters: `ratelimitFailClosed() -> backendRatelimit("t", 1, "1s")`,
wantLimit: true,
limitStatusCode: http.StatusServiceUnavailable,
},
{
name: "test clusterLeakyBucketRatelimit fail open",
ratelimitFilterName: "clusterLeakyBucketRatelimit",
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
name: "test clusterLeakyBucketRatelimit fail open",
filters: `clusterLeakyBucketRatelimit("t", 1, "1s")`,
wantLimit: false,
limitStatusCode: http.StatusTooManyRequests,
},
{
name: "test clusterLeakyBucketRatelimit fail closed",
ratelimitFilterName: "clusterLeakyBucketRatelimit",
failClosed: true,
wantLimit: true,
limitStatusCode: http.StatusTooManyRequests,
}} {
name: "test clusterLeakyBucketRatelimit fail closed",
filters: `ratelimitFailClosed() -> clusterLeakyBucketRatelimit("t", 1, "1s", 10, 1)`,
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()

reg := ratelimit.NewSwarmRegistry(nil, &snet.RedisOptions{Addrs: []string{"127.0.0.2:6379"}}, ratelimit.Settings{})
reg := ratelimit.NewSwarmRegistry(nil, &snet.RedisOptions{Addrs: []string{"fails.test:6379"}}, ratelimit.Settings{})
defer reg.Close()

provider := fratelimit.NewRatelimitProvider(reg)
Expand All @@ -96,19 +96,9 @@ func TestFailureMode(t *testing.T) {
}))
defer backend.Close()

args := []interface{}{"t", 1, "1s"}
switch tt.ratelimitFilterName {
case filters.ClusterLeakyBucketRatelimitName:
args = append(args, 10, 1)
case filters.ClusterClientRatelimitName:
args = append(args, "X-Test")
}

r := &eskip.Route{Filters: []*eskip.Filter{
{Name: tt.ratelimitFilterName, Args: args}}, Backend: backend.URL}
if tt.failClosed {
r.Filters = append([]*eskip.Filter{{Name: fratelimit.NewFailClosed().Name()}},
r.Filters...)
r := &eskip.Route{
Filters: eskip.MustParseFilters(tt.filters),
Backend: backend.URL,
}

proxy := proxytest.WithParamsAndRoutingOptions(
Expand All @@ -123,22 +113,16 @@ func TestFailureMode(t *testing.T) {
}, r)
defer proxy.Close()

reqURL, err := url.Parse(proxy.URL)
if err != nil {
t.Fatalf("Failed to parse url %s: %v", proxy.URL, err)
}
req, err := http.NewRequest("GET", reqURL.String(), nil)
req, err := http.NewRequest("GET", proxy.URL, nil)
if err != nil {
t.Fatal(err)
return
}
req.Header.Set("X-Test", "foo")

rsp, err := http.DefaultClient.Do(req)
rsp, err := proxy.Client().Do(req)
if err != nil {
t.Fatal(err)
}

defer rsp.Body.Close()

limited := rsp.StatusCode == tt.limitStatusCode
Expand All @@ -149,5 +133,4 @@ func TestFailureMode(t *testing.T) {
}
})
}

}

0 comments on commit 79a9f35

Please sign in to comment.