diff options
-rw-r--r-- | internal/feature/feature.go | 33 | ||||
-rw-r--r-- | internal/feature/feature_test.go | 48 | ||||
-rw-r--r-- | internal/ratelimiter/middleware.go | 13 | ||||
-rw-r--r-- | internal/ratelimiter/middleware_test.go | 9 | ||||
-rw-r--r-- | internal/redirects/helpers.go | 7 | ||||
-rw-r--r-- | internal/redirects/matching.go | 4 | ||||
-rw-r--r-- | internal/redirects/redirects.go | 3 | ||||
-rw-r--r-- | internal/redirects/redirects_benchmark_test.go | 3 | ||||
-rw-r--r-- | internal/redirects/redirects_test.go | 9 | ||||
-rw-r--r-- | internal/redirects/validations.go | 4 | ||||
-rw-r--r-- | internal/testhelpers/testhelpers.go | 13 | ||||
-rw-r--r-- | test/acceptance/ratelimiter_test.go | 3 | ||||
-rw-r--r-- | test/acceptance/redirects_test.go | 8 | ||||
-rw-r--r-- | test/acceptance/rewrites_test.go | 4 |
14 files changed, 113 insertions, 48 deletions
diff --git a/internal/feature/feature.go b/internal/feature/feature.go new file mode 100644 index 00000000..0a29f4a4 --- /dev/null +++ b/internal/feature/feature.go @@ -0,0 +1,33 @@ +package feature + +import "os" + +type Feature struct { + EnvVariable string + defaultEnabled bool +} + +// EnforceIPRateLimits enforces ratelimiter package to drop requests +// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/629 +var EnforceIPRateLimits = Feature{ + EnvVariable: "FF_ENFORCE_IP_RATE_LIMITS", +} + +// RedirectsPlaceholders enables support for placeholders in redirects file +// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/620 +var RedirectsPlaceholders = Feature{ + EnvVariable: "FF_ENABLE_PLACEHOLDERS", +} + +// Enabled reads the environment variable responsible for the feature flag +// if FF is disabled by default, the environment variable needs to be "true" to explicitly enable it +// if FF is enabled by default, variable needs to be "false" to explicitly disable it +func (f Feature) Enabled() bool { + env := os.Getenv(f.EnvVariable) + + if f.defaultEnabled { + return env != "false" + } + + return env == "true" +} diff --git a/internal/feature/feature_test.go b/internal/feature/feature_test.go new file mode 100644 index 00000000..88de8419 --- /dev/null +++ b/internal/feature/feature_test.go @@ -0,0 +1,48 @@ +package feature + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" +) + +func TestEnabled(t *testing.T) { + cases := map[string]struct { + envVal string + defaultEnabled bool + expected bool + }{ + "disabled_by_default": { + expected: false, + }, + "enabled_by_env_variable": { + envVal: "true", + expected: true, + }, + "disabled_by_env_variable": { + envVal: "false", + expected: false, + }, + "enabled_by_default": { + defaultEnabled: true, + expected: true, + }, + "enabled_by_default_but_disabled_by_env_variable": { + envVal: "false", + defaultEnabled: true, + }, + } + + for name, tt := range cases { + t.Run(name, func(t *testing.T) { + feature := Feature{ + EnvVariable: "testFeatureFlag", + defaultEnabled: tt.defaultEnabled, + } + testhelpers.SetEnvironmentVariable(t, feature.EnvVariable, tt.envVal) + require.Equal(t, tt.expected, feature.Enabled()) + }) + } +} diff --git a/internal/ratelimiter/middleware.go b/internal/ratelimiter/middleware.go index 86beaf39..a0fccff4 100644 --- a/internal/ratelimiter/middleware.go +++ b/internal/ratelimiter/middleware.go @@ -2,12 +2,12 @@ package ratelimiter import ( "net/http" - "os" "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/labkit/correlation" "gitlab.com/gitlab-org/labkit/log" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/request" ) @@ -24,9 +24,7 @@ func (rl *RateLimiter) Middleware(handler http.Handler) http.Handler { if !rl.RequestAllowed(r) { rl.logRateLimitedRequest(r) - // Only drop requests once FF_ENABLE_RATE_LIMITER is enabled - // https://gitlab.com/gitlab-org/gitlab-pages/-/issues/629 - if rateLimiterEnabled() { + if feature.EnforceIPRateLimits.Enabled() { if rl.blockedCount != nil { rl.blockedCount.WithLabelValues("true").Inc() } @@ -56,14 +54,9 @@ 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), - "rate_limiter_enabled": rateLimiterEnabled(), + "rate_limiter_enabled": feature.EnforceIPRateLimits.Enabled(), "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 Info("request hit rate limit") } - -// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/629 -func rateLimiterEnabled() bool { - return os.Getenv("FF_ENABLE_RATE_LIMITER") == "true" -} diff --git a/internal/ratelimiter/middleware_test.go b/internal/ratelimiter/middleware_test.go index a66f7523..95d95e95 100644 --- a/internal/ratelimiter/middleware_test.go +++ b/internal/ratelimiter/middleware_test.go @@ -10,6 +10,7 @@ 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/testhelpers" ) @@ -23,7 +24,7 @@ var next = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { func TestSourceIPLimiterWithDifferentLimits(t *testing.T) { hook := testlog.NewGlobal() - testhelpers.SetEnvironmentVariable(t, testhelpers.FFEnableRateLimiter, "true") + testhelpers.StubFeatureFlagValue(t, feature.EnforceIPRateLimits.EnvVariable, true) for tn, tc := range sharedTestCases { t.Run(tn, func(t *testing.T) { @@ -96,11 +97,7 @@ func TestSourceIPLimiterDenyRequestsAfterBurst(t *testing.T) { for i := 0; i < 5; i++ { ww := httptest.NewRecorder() rr := httptest.NewRequest(http.MethodGet, "http://gitlab.com", nil) - if tc.enabled { - testhelpers.SetEnvironmentVariable(t, testhelpers.FFEnableRateLimiter, "true") - } else { - testhelpers.SetEnvironmentVariable(t, testhelpers.FFEnableRateLimiter, "false") - } + testhelpers.StubFeatureFlagValue(t, feature.EnforceIPRateLimits.EnvVariable, tc.enabled) rr.RemoteAddr = remoteAddr diff --git a/internal/redirects/helpers.go b/internal/redirects/helpers.go index 52ae7991..4223faae 100644 --- a/internal/redirects/helpers.go +++ b/internal/redirects/helpers.go @@ -1,7 +1,6 @@ package redirects import ( - "os" "strings" ) @@ -10,9 +9,3 @@ import ( func normalizePath(path string) string { return strings.TrimSuffix(path, "/") + "/" } - -// placeholdersEnabled returns whether or not placeholders and splats -// are enabled for use in the _redirects file. -func placeholdersEnabled() bool { - return os.Getenv(FFEnablePlaceholders) == "true" -} diff --git a/internal/redirects/matching.go b/internal/redirects/matching.go index b5bceb71..a75255bb 100644 --- a/internal/redirects/matching.go +++ b/internal/redirects/matching.go @@ -7,6 +7,8 @@ import ( netlifyRedirects "github.com/tj/go-redirects" "gitlab.com/gitlab-org/labkit/log" + + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" ) var ( @@ -43,7 +45,7 @@ func matchesRule(rule *netlifyRedirects.Rule, path string) (bool, string) { // Any logic beyond this point handles placeholders and splats. // If the FF_ENABLE_PLACEHOLDERS feature flag isn't enabled, exit now. - if !placeholdersEnabled() { + if !feature.RedirectsPlaceholders.Enabled() { return false, "" } diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go index 24ce8692..1de02b61 100644 --- a/internal/redirects/redirects.go +++ b/internal/redirects/redirects.go @@ -32,9 +32,6 @@ const ( // maxRuleCount is used to limit the total number of rules allowed in _redirects maxRuleCount = 1000 - - // FFEnablePlaceholders used to check whether placeholder matching is enabled or not - FFEnablePlaceholders = "FF_ENABLE_PLACEHOLDERS" ) var ( diff --git a/internal/redirects/redirects_benchmark_test.go b/internal/redirects/redirects_benchmark_test.go index d1c16a1c..1e679785 100644 --- a/internal/redirects/redirects_benchmark_test.go +++ b/internal/redirects/redirects_benchmark_test.go @@ -10,12 +10,13 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) func generateRedirectsFile(dirPath string, count int) error { content := "/start.html /redirect.html 301\n" - if placeholdersEnabled() { + if feature.RedirectsPlaceholders.Enabled() { content += strings.Repeat("/foo/*/bar /foo/:splat/qux 200\n", count/2) content += strings.Repeat("/foo/:placeholder /qux/:placeholder 200\n", count/2) } else { diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go index 8cad98f8..b51043f7 100644 --- a/internal/redirects/redirects_test.go +++ b/internal/redirects/redirects_test.go @@ -12,16 +12,13 @@ import ( "github.com/stretchr/testify/require" netlifyRedirects "github.com/tj/go-redirects" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) -// enablePlaceholders enables the FF_ENABLE_PLACEHOLDERS in tests +// enablePlaceholders enables redirect placeholders in tests func enablePlaceholders(t testing.TB) { - t.Helper() - - orig := os.Getenv(FFEnablePlaceholders) - os.Setenv(FFEnablePlaceholders, "true") - t.Cleanup(func() { os.Setenv(FFEnablePlaceholders, orig) }) + testhelpers.StubFeatureFlagValue(t, feature.RedirectsPlaceholders.EnvVariable, true) } func TestRedirectsRewrite(t *testing.T) { diff --git a/internal/redirects/validations.go b/internal/redirects/validations.go index 1af5c5c5..5264f731 100644 --- a/internal/redirects/validations.go +++ b/internal/redirects/validations.go @@ -7,6 +7,8 @@ import ( "strings" netlifyRedirects "github.com/tj/go-redirects" + + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" ) var ( @@ -36,7 +38,7 @@ func validateURL(urlText string) error { return errNoStartingForwardSlashInURLPath } - if placeholdersEnabled() { + if feature.RedirectsPlaceholders.Enabled() { // Limit the number of path segments a rule can contain. // This prevents the matching logic from generating regular // expressions that are too large/complex. diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index de48cd7a..df2252d7 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -7,16 +7,13 @@ import ( "net/http/httptest" "net/url" "os" + "strconv" "testing" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) -// FFEnableRateLimiter enforces ratelimiter package to drop requests -// TODO: remove https://gitlab.com/gitlab-org/gitlab-pages/-/issues/629 -const FFEnableRateLimiter = "FF_ENABLE_RATE_LIMITER" - // AssertHTTP404 asserts handler returns 404 with provided str body func AssertHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, str interface{}) { w := httptest.NewRecorder() @@ -83,7 +80,7 @@ func Getwd(t *testing.T) string { } // SetEnvironmentVariable for testing, restoring the original value on t.Cleanup -func SetEnvironmentVariable(t *testing.T, key, value string) { +func SetEnvironmentVariable(t testing.TB, key, value string) { t.Helper() orig := os.Getenv(key) @@ -92,6 +89,10 @@ func SetEnvironmentVariable(t *testing.T, key, value string) { require.NoError(t, err) t.Cleanup(func() { - os.Setenv(FFEnableRateLimiter, orig) + os.Setenv(key, orig) }) } + +func StubFeatureFlagValue(t testing.TB, envVar string, value bool) { + SetEnvironmentVariable(t, envVar, strconv.FormatBool(value)) +} diff --git a/test/acceptance/ratelimiter_test.go b/test/acceptance/ratelimiter_test.go index 5b3be98a..ae6184ca 100644 --- a/test/acceptance/ratelimiter_test.go +++ b/test/acceptance/ratelimiter_test.go @@ -8,11 +8,12 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) func TestSourceIPRateLimitMiddleware(t *testing.T) { - testhelpers.SetEnvironmentVariable(t, testhelpers.FFEnableRateLimiter, "true") + testhelpers.StubFeatureFlagValue(t, feature.EnforceIPRateLimits.EnvVariable, true) tcs := map[string]struct { listener ListenSpec diff --git a/test/acceptance/redirects_test.go b/test/acceptance/redirects_test.go index 0cadea16..685e0fcb 100644 --- a/test/acceptance/redirects_test.go +++ b/test/acceptance/redirects_test.go @@ -6,15 +6,15 @@ import ( "net/http" "testing" - "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" - redirects "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" + "github.com/stretchr/testify/require" ) func TestRedirectStatusPage(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), - withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), + withEnv([]string{feature.RedirectsPlaceholders.EnvVariable + "=true"}), ) rsp, err := GetPageFromListener(t, httpListener, "group.redirects.gitlab-example.com", "/project-redirects/_redirects") @@ -31,7 +31,7 @@ func TestRedirectStatusPage(t *testing.T) { func TestRedirect(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), - withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), + withEnv([]string{feature.RedirectsPlaceholders.EnvVariable + "=true"}), ) // Test that serving a file still works with redirects enabled diff --git a/test/acceptance/rewrites_test.go b/test/acceptance/rewrites_test.go index 5ac16070..aa105789 100644 --- a/test/acceptance/rewrites_test.go +++ b/test/acceptance/rewrites_test.go @@ -7,13 +7,13 @@ import ( "github.com/stretchr/testify/require" - redirects "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" + "gitlab.com/gitlab-org/gitlab-pages/internal/feature" ) func TestRewrites(t *testing.T) { RunPagesProcess(t, withListeners([]ListenSpec{httpListener}), - withEnv([]string{redirects.FFEnablePlaceholders + "=true"}), + withEnv([]string{feature.RedirectsPlaceholders.EnvVariable + "=true"}), ) tests := map[string]struct { |