From 4da63c92370758da56cf033104c2d42aff1bc4b5 Mon Sep 17 00:00:00 2001 From: mpl Date: Wed, 29 Apr 2020 18:32:05 +0200 Subject: [PATCH] ratelimit: do not default to ipstrategy too early --- pkg/config/dynamic/middlewares.go | 12 +---- pkg/middlewares/extractor.go | 13 ++++++ pkg/middlewares/inflightreq/inflight_req.go | 1 + .../ratelimiter/rate_limiter_test.go | 44 ++++++++++++++++++- pkg/provider/consulcatalog/config_test.go | 6 --- pkg/provider/docker/config_test.go | 6 --- pkg/provider/marathon/config_test.go | 3 -- 7 files changed, 58 insertions(+), 27 deletions(-) diff --git a/pkg/config/dynamic/middlewares.go b/pkg/config/dynamic/middlewares.go index 85c93d1f3..cba305a77 100644 --- a/pkg/config/dynamic/middlewares.go +++ b/pkg/config/dynamic/middlewares.go @@ -286,13 +286,6 @@ type InFlightReq struct { SourceCriterion *SourceCriterion `json:"sourceCriterion,omitempty" toml:"sourceCriterion,omitempty" yaml:"sourceCriterion,omitempty"` } -// SetDefaults Default values for a InFlightReq. -func (i *InFlightReq) SetDefaults() { - i.SourceCriterion = &SourceCriterion{ - RequestHost: true, - } -} - // +k8s:deepcopy-gen=true // PassTLSClientCert holds the TLS client cert headers configuration. @@ -304,8 +297,8 @@ type PassTLSClientCert struct { // +k8s:deepcopy-gen=true // SourceCriterion defines what criterion is used to group requests as originating from a common source. -// The precedence order is IPStrategy, then RequestHeaderName. // If none are set, the default is to use the request's remote address field. +// All fields are mutually exclusive. type SourceCriterion struct { IPStrategy *IPStrategy `json:"ipStrategy" toml:"ipStrategy, omitempty"` RequestHeaderName string `json:"requestHeaderName,omitempty" toml:"requestHeaderName,omitempty" yaml:"requestHeaderName,omitempty"` @@ -337,9 +330,6 @@ type RateLimit struct { func (r *RateLimit) SetDefaults() { r.Burst = 1 r.Period = types.Duration(time.Second) - r.SourceCriterion = &SourceCriterion{ - IPStrategy: &IPStrategy{}, - } } // +k8s:deepcopy-gen=true diff --git a/pkg/middlewares/extractor.go b/pkg/middlewares/extractor.go index 75b700523..1a886bfad 100644 --- a/pkg/middlewares/extractor.go +++ b/pkg/middlewares/extractor.go @@ -13,7 +13,20 @@ import ( // GetSourceExtractor returns the SourceExtractor function corresponding to the given sourceMatcher. // It defaults to a RemoteAddrStrategy IPStrategy if need be. +// It returns an error if more than one source criterion is provided. func GetSourceExtractor(ctx context.Context, sourceMatcher *dynamic.SourceCriterion) (utils.SourceExtractor, error) { + if sourceMatcher != nil { + if sourceMatcher.IPStrategy != nil && sourceMatcher.RequestHeaderName != "" { + return nil, errors.New("iPStrategy and RequestHeaderName are mutually exclusive") + } + if sourceMatcher.IPStrategy != nil && sourceMatcher.RequestHost { + return nil, errors.New("iPStrategy and RequestHost are mutually exclusive") + } + if sourceMatcher.RequestHeaderName != "" && sourceMatcher.RequestHost { + return nil, errors.New("requestHost and RequestHeaderName are mutually exclusive") + } + } + if sourceMatcher == nil || sourceMatcher.IPStrategy == nil && sourceMatcher.RequestHeaderName == "" && !sourceMatcher.RequestHost { diff --git a/pkg/middlewares/inflightreq/inflight_req.go b/pkg/middlewares/inflightreq/inflight_req.go index 4be488bd8..8856858a6 100644 --- a/pkg/middlewares/inflightreq/inflight_req.go +++ b/pkg/middlewares/inflightreq/inflight_req.go @@ -23,6 +23,7 @@ type inFlightReq struct { } // New creates a max request middleware. +// If no source criterion is provided in the config, it defaults to RequestHost. func New(ctx context.Context, next http.Handler, config dynamic.InFlightReq, name string) (http.Handler, error) { ctxLog := log.With(ctx, log.Str(log.MiddlewareName, name), log.Str(log.MiddlewareType, typeName)) log.FromContext(ctxLog).Debug("Creating middleware") diff --git a/pkg/middlewares/ratelimiter/rate_limiter_test.go b/pkg/middlewares/ratelimiter/rate_limiter_test.go index 7d9213db3..c0f4be1b2 100644 --- a/pkg/middlewares/ratelimiter/rate_limiter_test.go +++ b/pkg/middlewares/ratelimiter/rate_limiter_test.go @@ -22,6 +22,8 @@ func TestNewRateLimiter(t *testing.T) { config dynamic.RateLimit expectedMaxDelay time.Duration expectedSourceIP string + requestHeader string + expectedError string }{ { desc: "maxDelay computation", @@ -48,6 +50,29 @@ func TestNewRateLimiter(t *testing.T) { }, expectedSourceIP: "127.0.0.1", }, + { + desc: "SourceCriterion in config is respected", + config: dynamic.RateLimit{ + Average: 200, + Burst: 10, + SourceCriterion: &dynamic.SourceCriterion{ + RequestHeaderName: "Foo", + }, + }, + requestHeader: "bar", + }, + { + desc: "SourceCriteria are mutually exclusive", + config: dynamic.RateLimit{ + Average: 200, + Burst: 10, + SourceCriterion: &dynamic.SourceCriterion{ + IPStrategy: &dynamic.IPStrategy{}, + RequestHeaderName: "Foo", + }, + }, + expectedError: "iPStrategy and RequestHeaderName are mutually exclusive", + }, } for _, test := range testCases { @@ -58,7 +83,11 @@ func TestNewRateLimiter(t *testing.T) { next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) h, err := New(context.Background(), next, test.config, "rate-limiter") - require.NoError(t, err) + if test.expectedError != "" { + assert.EqualError(t, err, test.expectedError) + } else { + require.NoError(t, err) + } rtl, _ := h.(*rateLimiter) if test.expectedMaxDelay != 0 { @@ -77,6 +106,19 @@ func TestNewRateLimiter(t *testing.T) { assert.NoError(t, err) assert.Equal(t, test.expectedSourceIP, ip) } + if test.requestHeader != "" { + extractor, ok := rtl.sourceMatcher.(utils.ExtractorFunc) + require.True(t, ok, "Not an ExtractorFunc") + + req := http.Request{ + Header: map[string][]string{ + test.config.SourceCriterion.RequestHeaderName: {test.requestHeader}, + }, + } + hd, _, err := extractor(&req) + assert.NoError(t, err) + assert.Equal(t, test.requestHeader, hd) + } }) } } diff --git a/pkg/provider/consulcatalog/config_test.go b/pkg/provider/consulcatalog/config_test.go index 973a1e5ef..732fe9dcc 100644 --- a/pkg/provider/consulcatalog/config_test.go +++ b/pkg/provider/consulcatalog/config_test.go @@ -1003,9 +1003,6 @@ func Test_buildConfiguration(t *testing.T) { "Middleware1": { InFlightReq: &dynamic.InFlightReq{ Amount: 42, - SourceCriterion: &dynamic.SourceCriterion{ - RequestHost: true, - }, }, }, }, @@ -1055,9 +1052,6 @@ func Test_buildConfiguration(t *testing.T) { "Middleware1": { InFlightReq: &dynamic.InFlightReq{ Amount: 42, - SourceCriterion: &dynamic.SourceCriterion{ - RequestHost: true, - }, }, }, }, diff --git a/pkg/provider/docker/config_test.go b/pkg/provider/docker/config_test.go index 1617bbd8b..c49594d1b 100644 --- a/pkg/provider/docker/config_test.go +++ b/pkg/provider/docker/config_test.go @@ -1301,9 +1301,6 @@ func Test_buildConfiguration(t *testing.T) { "Middleware1": { InFlightReq: &dynamic.InFlightReq{ Amount: 42, - SourceCriterion: &dynamic.SourceCriterion{ - RequestHost: true, - }, }, }, }, @@ -1372,9 +1369,6 @@ func Test_buildConfiguration(t *testing.T) { "Middleware1": { InFlightReq: &dynamic.InFlightReq{ Amount: 42, - SourceCriterion: &dynamic.SourceCriterion{ - RequestHost: true, - }, }, }, }, diff --git a/pkg/provider/marathon/config_test.go b/pkg/provider/marathon/config_test.go index cb553b47c..d8862e78e 100644 --- a/pkg/provider/marathon/config_test.go +++ b/pkg/provider/marathon/config_test.go @@ -686,9 +686,6 @@ func TestBuildConfiguration(t *testing.T) { "Middleware1": { InFlightReq: &dynamic.InFlightReq{ Amount: 42, - SourceCriterion: &dynamic.SourceCriterion{ - RequestHost: true, - }, }, }, },