Merge branch 'feat/remove-rate-limit-ff' into 'master'

feat: remove rate limits feature flags

See merge request gitlab-org/gitlab-pages!788

9 files changed, 8 insertions, 123 deletions

diff --git a/internal/feature/feature.go b/internal/feature/feature.go
index 337bf2f1..d5c340ff 100644
--- a/internal/feature/feature.go
+++ b/internal/feature/feature.go
@@ -7,30 +7,6 @@ type Feature struct {
 	defaultEnabled bool
 }
 
-// EnforceIPRateLimits enforces IP rate limiter to drop requests
-// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/706
-var EnforceIPRateLimits = Feature{
-	EnvVariable: "FF_ENFORCE_IP_RATE_LIMITS",
-}
-
-// EnforceDomainRateLimits enforces domain rate limiter to drop requests
-// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/706
-var EnforceDomainRateLimits = Feature{
-	EnvVariable: "FF_ENFORCE_DOMAIN_RATE_LIMITS",
-}
-
-// EnforceDomainTLSRateLimits enforces domain rate limits on establishing new TLS connections
-// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/706
-var EnforceDomainTLSRateLimits = Feature{
-	EnvVariable: "FF_ENFORCE_DOMAIN_TLS_RATE_LIMITS",
-}
-
-// EnforceIPTLSRateLimits enforces domain rate limits on establishing new TLS connections
-// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/706
-var EnforceIPTLSRateLimits = Feature{
-	EnvVariable: "FF_ENFORCE_IP_TLS_RATE_LIMITS",
-}
-
 // RedirectsPlaceholders enables support for placeholders in redirects file
 // TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/620
 var RedirectsPlaceholders = Feature{
diff --git a/internal/handlers/ratelimiter.go b/internal/handlers/ratelimiter.go
index a8eee005..7574e170 100644
--- a/internal/handlers/ratelimiter.go
+++ b/internal/handlers/ratelimiter.go
@@ -4,7 +4,6 @@ import (
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/feature"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/ratelimiter"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
@@ -21,7 +20,6 @@ func Ratelimiter(handler http.Handler, config *config.RateLimit) http.Handler {
 		ratelimiter.WithBlockedCountMetric(metrics.RateLimitBlockedCount),
 		ratelimiter.WithLimitPerSecond(config.SourceIPLimitPerSecond),
 		ratelimiter.WithBurstSize(config.SourceIPBurst),
-		ratelimiter.WithEnforce(feature.EnforceIPRateLimits.Enabled()),
 	)
 
 	handler = sourceIPLimiter.Middleware(handler)
@@ -35,7 +33,6 @@ func Ratelimiter(handler http.Handler, config *config.RateLimit) http.Handler {
 		ratelimiter.WithBlockedCountMetric(metrics.RateLimitBlockedCount),
 		ratelimiter.WithLimitPerSecond(config.DomainLimitPerSecond),
 		ratelimiter.WithBurstSize(config.DomainBurst),
-		ratelimiter.WithEnforce(feature.EnforceDomainRateLimits.Enabled()),
 	)
 
 	return domainLimiter.Middleware(handler)
diff --git a/internal/handlers/ratelimiter_test.go b/internal/handlers/ratelimiter_test.go
index 1ff1c0e5..19414942 100644
--- a/internal/handlers/ratelimiter_test.go
+++ b/internal/handlers/ratelimiter_test.go
@@ -3,13 +3,11 @@ package handlers
 import (
 	"net/http"
 	"net/http/httptest"
-	"strconv"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/feature"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
 
@@ -23,8 +21,6 @@ func TestRatelimiter(t *testing.T) {
 		firstTarget        string
 		secondRemoteAddr   string
 		secondTarget       string
-		sourceIPEnforced   bool
-		domainEnforced     bool
 		expectedSecondCode int
 	}{
 		"rejected_by_ip": {
@@ -32,8 +28,6 @@ func TestRatelimiter(t *testing.T) {
 			firstTarget:        "https://domain.gitlab.io",
 			secondRemoteAddr:   "10.0.0.1",
 			secondTarget:       "https://different.gitlab.io",
-			sourceIPEnforced:   true,
-			domainEnforced:     true,
 			expectedSecondCode: http.StatusTooManyRequests,
 		},
 		"rejected_by_domain": {
@@ -41,44 +35,19 @@ func TestRatelimiter(t *testing.T) {
 			firstTarget:        "https://domain.gitlab.io",
 			secondRemoteAddr:   "10.0.0.2",
 			secondTarget:       "https://domain.gitlab.io",
-			sourceIPEnforced:   true,
-			domainEnforced:     true,
 			expectedSecondCode: http.StatusTooManyRequests,
 		},
-		"ip_rate_limiter_disabled": {
-			firstRemoteAddr:    "10.0.0.1",
-			firstTarget:        "https://domain.gitlab.io",
-			secondRemoteAddr:   "10.0.0.1",
-			secondTarget:       "https://different.gitlab.io",
-			sourceIPEnforced:   false,
-			domainEnforced:     true,
-			expectedSecondCode: http.StatusNoContent,
-		},
-		"domain_rate_limiter_disabled": {
-			firstRemoteAddr:    "10.0.0.1",
-			firstTarget:        "https://domain.gitlab.io",
-			secondRemoteAddr:   "10.0.0.2",
-			secondTarget:       "https://domain.gitlab.io",
-			sourceIPEnforced:   true,
-			domainEnforced:     false,
-			expectedSecondCode: http.StatusNoContent,
-		},
 		"different_ip_and_domain_passes": {
 			firstRemoteAddr:    "10.0.0.1",
 			firstTarget:        "https://domain.gitlab.io",
 			secondRemoteAddr:   "10.0.0.2",
 			secondTarget:       "https://different.gitlab.io",
-			sourceIPEnforced:   true,
-			domainEnforced:     true,
 			expectedSecondCode: http.StatusNoContent,
 		},
 	}
 
 	for name, tc := range tt {
 		t.Run(name, func(t *testing.T) {
-			t.Setenv(feature.EnforceIPRateLimits.EnvVariable, strconv.FormatBool(tc.sourceIPEnforced))
-			t.Setenv(feature.EnforceDomainRateLimits.EnvVariable, strconv.FormatBool(tc.domainEnforced))
-
 			conf := config.RateLimit{
 				SourceIPLimitPerSecond: 0.1,
 				SourceIPBurst:          1,
diff --git a/internal/ratelimiter/middleware.go b/internal/ratelimiter/middleware.go
index d710b193..fa56f5e4 100644
--- a/internal/ratelimiter/middleware.go
+++ b/internal/ratelimiter/middleware.go
@@ -2,7 +2,6 @@ package ratelimiter
 
 import (
 	"net/http"
-	"strconv"
 
 	"github.com/sirupsen/logrus"
 
@@ -32,15 +31,10 @@ func (rl *RateLimiter) Middleware(handler http.Handler) http.Handler {
 		rl.logRateLimitedRequest(r)
 
 		if rl.blockedCount != nil {
-			rl.blockedCount.WithLabelValues(rl.name, strconv.FormatBool(rl.enforce)).Inc()
+			rl.blockedCount.WithLabelValues(rl.name).Inc()
 		}
 
-		if rl.enforce {
-			httperrors.Serve429(w)
-			return
-		}
-
-		handler.ServeHTTP(w, r)
+		httperrors.Serve429(w)
 	})
 }
 
@@ -53,7 +47,6 @@ func (rl *RateLimiter) logRateLimitedRequest(r *http.Request) {
 		"x_forwarded_proto":             r.Header.Get(headerXForwardedProto),
 		"x_forwarded_for":               r.Header.Get(headerXForwardedFor),
 		"gitlab_real_ip":                r.Header.Get(headerGitLabRealIP),
-		"enforced":                      rl.enforce,
 		"rate_limiter_limit_per_second": rl.limitPerSecond,
 		"rate_limiter_burst_size":       rl.burstSize,
 	}). // TODO: change to Debug with https://gitlab.com/gitlab-org/gitlab-pages/-/issues/629
diff --git a/internal/ratelimiter/middleware_test.go b/internal/ratelimiter/middleware_test.go
index 167a9e68..fb557462 100644
--- a/internal/ratelimiter/middleware_test.go
+++ b/internal/ratelimiter/middleware_test.go
@@ -3,7 +3,6 @@ package ratelimiter
 import (
 	"net/http"
 	"net/http/httptest"
-	"strconv"
 	"testing"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -11,7 +10,6 @@ import (
 	testlog "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/feature"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers"
 )
@@ -34,7 +32,6 @@ func TestMiddlewareWithDifferentLimits(t *testing.T) {
 				WithNow(mockNow),
 				WithLimitPerSecond(tc.limit),
 				WithBurstSize(tc.burstSize),
-				WithEnforce(true),
 			)
 
 			handler := rl.Middleware(next)
@@ -62,23 +59,15 @@ func TestMiddlewareDenyRequestsAfterBurst(t *testing.T) {
 	blocked, cachedEntries, cacheReqs := newTestMetrics(t)
 
 	tcs := map[string]struct {
-		enforce        bool
 		expectedStatus int
 	}{
-		"disabled_rate_limit_http": {
-			enforce:        false,
-			expectedStatus: http.StatusNoContent,
-		},
 		"enabled_rate_limit_http_blocks": {
-			enforce:        true,
 			expectedStatus: http.StatusTooManyRequests,
 		},
 	}
 
 	for tn, tc := range tcs {
 		t.Run(tn, func(t *testing.T) {
-			t.Setenv(feature.EnforceIPRateLimits.EnvVariable, strconv.FormatBool(tc.enforce))
-
 			rl := New(
 				"rate_limiter",
 				WithCachedEntriesMetric(cachedEntries),
@@ -87,7 +76,6 @@ func TestMiddlewareDenyRequestsAfterBurst(t *testing.T) {
 				WithNow(mockNow),
 				WithLimitPerSecond(1),
 				WithBurstSize(1),
-				WithEnforce(tc.enforce),
 			)
 
 			// middleware is evaluated in reverse order
@@ -107,7 +95,7 @@ func TestMiddlewareDenyRequestsAfterBurst(t *testing.T) {
 				assertSourceIPLog(t, remoteAddr, hook)
 			}
 
-			blockedCount := testutil.ToFloat64(blocked.WithLabelValues("rate_limiter", strconv.FormatBool(tc.enforce)))
+			blockedCount := testutil.ToFloat64(blocked.WithLabelValues("rate_limiter"))
 			require.Equal(t, float64(4), blockedCount, "blocked count")
 			blocked.Reset()
 
@@ -191,7 +179,6 @@ func TestKeyFunc(t *testing.T) {
 				WithLimitPerSecond(1),
 				WithBurstSize(1),
 				WithKeyFunc(tc.keyFunc),
-				WithEnforce(true),
 			).Middleware(next)
 
 			r1 := httptest.NewRequest(http.MethodGet, tc.firstTarget, nil)
@@ -226,7 +213,7 @@ func newTestMetrics(t *testing.T) (*prometheus.GaugeVec, *prometheus.GaugeVec, *
 		prometheus.GaugeOpts{
 			Name: t.Name(),
 		},
-		[]string{"limit_name", "enforced"},
+		[]string{"limit_name"},
 	)
 
 	cachedEntries := prometheus.NewGaugeVec(prometheus.GaugeOpts{
diff --git a/internal/ratelimiter/ratelimiter.go b/internal/ratelimiter/ratelimiter.go
index b64cb4ce..bf49cc95 100644
--- a/internal/ratelimiter/ratelimiter.go
+++ b/internal/ratelimiter/ratelimiter.go
@@ -45,7 +45,6 @@ type RateLimiter struct {
 	burstSize      int
 	blockedCount   *prometheus.GaugeVec
 	cache          *lru.Cache
-	enforce        bool
 
 	cacheOptions []lru.Option
 }
@@ -146,13 +145,6 @@ func WithTLSKeyFunc(keyFunc TLSKeyFunc) Option {
 	}
 }
 
-// WithEnforce configures if requests are actually rejected, or we just report them as rejected in metrics
-func WithEnforce(enforce bool) Option {
-	return func(rl *RateLimiter) {
-		rl.enforce = enforce
-	}
-}
-
 func (rl *RateLimiter) limiter(key string) *rate.Limiter {
 	limiterI, _ := rl.cache.FindOrFetch(key, key, func() (interface{}, error) {
 		return rate.NewLimiter(rate.Limit(rl.limitPerSecond), rl.burstSize), nil
diff --git a/internal/ratelimiter/tls.go b/internal/ratelimiter/tls.go
index 3bebbc38..fbf956a5 100644
--- a/internal/ratelimiter/tls.go
+++ b/internal/ratelimiter/tls.go
@@ -3,7 +3,6 @@ package ratelimiter
 import (
 	"crypto/tls"
 	"errors"
-	"strconv"
 
 	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/labkit/log"
@@ -26,11 +25,7 @@ func (rl *RateLimiter) GetCertificateMiddleware(getCertificate GetCertificateFun
 		rl.logRateLimitedTLS(info)
 
 		if rl.blockedCount != nil {
-			rl.blockedCount.WithLabelValues(rl.name, strconv.FormatBool(rl.enforce)).Inc()
-		}
-
-		if !rl.enforce {
-			return getCertificate(info)
+			rl.blockedCount.WithLabelValues(rl.name).Inc()
 		}
 
 		return nil, ErrTLSRateLimited
@@ -44,6 +39,5 @@ func (rl *RateLimiter) logRateLimitedTLS(info *tls.ClientHelloInfo) {
 		"req_host":                      info.ServerName,
 		"rate_limiter_limit_per_second": rl.limitPerSecond,
 		"rate_limiter_burst_size":       rl.burstSize,
-		"enforced":                      rl.enforce,
 	}).Info("TLS connection rate-limited")
 }
diff --git a/internal/ratelimiter/tls_test.go b/internal/ratelimiter/tls_test.go
index 6763514b..f198df47 100644
--- a/internal/ratelimiter/tls_test.go
+++ b/internal/ratelimiter/tls_test.go
@@ -4,7 +4,6 @@ import (
 	"crypto/tls"
 	"errors"
 	"net"
-	"strconv"
 	"testing"
 	"time"
 
@@ -48,42 +47,30 @@ func TestTLSClientIPKey(t *testing.T) {
 func TestGetCertificateMiddleware(t *testing.T) {
 	tests := map[string]struct {
 		useHostnameAsKey bool
-		enforced         bool
 		limitPerSecond   float64
 		burst            int
 		successfulReqCnt int
 	}{
 		"ip_limiter": {
 			useHostnameAsKey: false,
-			enforced:         true,
 			limitPerSecond:   0.1,
 			burst:            5,
 			successfulReqCnt: 5,
 		},
 		"hostname_limiter": {
 			useHostnameAsKey: true,
-			enforced:         true,
-			limitPerSecond:   0.1,
-			burst:            5,
-			successfulReqCnt: 5,
-		},
-		"not_enforced": {
-			useHostnameAsKey: false,
-			enforced:         false,
 			limitPerSecond:   0.1,
 			burst:            5,
 			successfulReqCnt: 5,
 		},
 		"disabled": {
 			useHostnameAsKey: false,
-			enforced:         true,
 			limitPerSecond:   0,
 			burst:            5,
 			successfulReqCnt: 10,
 		},
 		"slowly_approach_limit": {
 			useHostnameAsKey: false,
-			enforced:         true,
 			limitPerSecond:   0.2,
 			burst:            5,
 			successfulReqCnt: 6, // 5 * 0.2 gives another 1 request
@@ -114,7 +101,6 @@ func TestGetCertificateMiddleware(t *testing.T) {
 				WithNow(stubNow()),
 				WithLimitPerSecond(tt.limitPerSecond),
 				WithBurstSize(tt.burst),
-				WithEnforce(tt.enforced),
 				WithTLSKeyFunc(keyFunc))
 
 			middlewareGetCert := rl.GetCertificateMiddleware(getCertificate)
@@ -136,13 +122,8 @@ func TestGetCertificateMiddleware(t *testing.T) {
 			}
 
 			cert, err := middlewareGetCert(info)
-			if tt.enforced {
-				require.Nil(t, cert)
-				require.Equal(t, err, ErrTLSRateLimited)
-			} else {
-				require.Equal(t, expectedCert, cert)
-				require.Equal(t, expectedErr, err)
-			}
+			require.Nil(t, cert)
+			require.Equal(t, err, ErrTLSRateLimited)
 
 			require.NotNil(t, hook.LastEntry())
 			require.Equal(t, "TLS connection rate-limited", hook.LastEntry().Message)
@@ -152,7 +133,6 @@ func TestGetCertificateMiddleware(t *testing.T) {
 				"req_host":                      "group.gitlab.io",
 				"rate_limiter_limit_per_second": tt.limitPerSecond,
 				"rate_limiter_burst_size":       tt.burst,
-				"enforced":                      tt.enforced,
 			}
 			require.Equal(t, expectedFields, hook.LastEntry().Data)
 
@@ -170,7 +150,7 @@ func TestGetCertificateMiddleware(t *testing.T) {
 			require.Equal(t, expectedCert, cert)
 			require.Equal(t, expectedErr, err)
 
-			blockedCount := testutil.ToFloat64(blocked.WithLabelValues("limit_name", strconv.FormatBool(tt.enforced)))
+			blockedCount := testutil.ToFloat64(blocked.WithLabelValues("limit_name"))
 			require.Equal(t, float64(1), blockedCount, "blocked count")
 
 			cachedCount := testutil.ToFloat64(cachedEntries.WithLabelValues("limit_name"))
diff --git a/internal/tls/tls.go b/internal/tls/tls.go
index ce16843a..eb8e4e64 100644
--- a/internal/tls/tls.go
+++ b/internal/tls/tls.go
@@ -4,7 +4,6 @@ import (
 	"crypto/tls"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/feature"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/ratelimiter"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
@@ -59,7 +58,6 @@ func GetTLSConfig(cfg *config.Config, getCertificateByServerName GetCertificateF
 		ratelimiter.WithBlockedCountMetric(metrics.RateLimitBlockedCount),
 		ratelimiter.WithLimitPerSecond(cfg.RateLimit.TLSDomainLimitPerSecond),
 		ratelimiter.WithBurstSize(cfg.RateLimit.TLSDomainBurst),
-		ratelimiter.WithEnforce(feature.EnforceDomainTLSRateLimits.Enabled()),
 	)
 
 	TLSSourceIPRateLimiter := ratelimiter.New(
@@ -71,7 +69,6 @@ func GetTLSConfig(cfg *config.Config, getCertificateByServerName GetCertificateF
 		ratelimiter.WithBlockedCountMetric(metrics.RateLimitBlockedCount),
 		ratelimiter.WithLimitPerSecond(cfg.RateLimit.TLSSourceIPLimitPerSecond),
 		ratelimiter.WithBurstSize(cfg.RateLimit.TLSSourceIPBurst),
-		ratelimiter.WithEnforce(feature.EnforceIPTLSRateLimits.Enabled()),
 	)
 
 	getCertificate = TLSDomainRateLimiter.GetCertificateMiddleware(getCertificate)