diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2022-07-07 03:49:58 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2022-07-07 03:49:58 +0300 |
commit | 1f9cc507928429fc0a85eaa504bfca692c56ff2a (patch) | |
tree | 7553e1e08205d7b7d4eb3288ec459dd89c80d415 /internal | |
parent | f04653d860c8ea8a204557fc2bb7a2b48b9f2fc5 (diff) | |
parent | 68a1d6a18d37061598bf7d74bec4bda8422f0cd2 (diff) |
Merge branch 'feat/remove-rate-limit-ff' into 'master'
feat: remove rate limits feature flags
See merge request gitlab-org/gitlab-pages!788
Diffstat (limited to 'internal')
-rw-r--r-- | internal/feature/feature.go | 24 | ||||
-rw-r--r-- | internal/handlers/ratelimiter.go | 3 | ||||
-rw-r--r-- | internal/handlers/ratelimiter_test.go | 31 | ||||
-rw-r--r-- | internal/ratelimiter/middleware.go | 11 | ||||
-rw-r--r-- | internal/ratelimiter/middleware_test.go | 17 | ||||
-rw-r--r-- | internal/ratelimiter/ratelimiter.go | 8 | ||||
-rw-r--r-- | internal/ratelimiter/tls.go | 8 | ||||
-rw-r--r-- | internal/ratelimiter/tls_test.go | 26 | ||||
-rw-r--r-- | internal/tls/tls.go | 3 |
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) |