refactor: extract common logic for env feature flags

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 {