ratelimit: do not default to ipstrategy too early

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

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 {

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")

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")
+			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)
+			}
 		})
 	}
 }

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,
-								},
 							},
 						},
 					},

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,
-								},
 							},
 						},
 					},

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,
-								},
 							},
 						},
 					},