Merge branch 'feat/configurable-redirects' into 'master'

feat: make _redirects limits configurable

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

8 files changed, 56 insertions, 15 deletions

diff --git a/app.go b/app.go
index c08dfb29..7a22a3fd 100644
--- a/app.go
+++ b/app.go
@@ -32,6 +32,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/logging"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/netutil"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/redirects"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/rejectmethods"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/routing"
@@ -349,6 +350,8 @@ func (a *theApp) listenMetrics(eg *errgroup.Group, config cfg.Metrics) *http.Ser
 }
 
 func runApp(config *cfg.Config) error {
+	redirects.SetConfig(config.Redirects)
+
 	source, err := gitlab.New(&config.GitLab)
 	if err != nil {
 		return fmt.Errorf("could not create domains config source: %w", err)
diff --git a/internal/config/config.go b/internal/config/config.go
index 18fe33eb..fcaa1d5e 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -21,6 +21,7 @@ type Config struct {
 	Authentication  Auth
 	GitLab          GitLab
 	Log             Log
+	Redirects       Redirects
 	Sentry          Sentry
 	Server          Server
 	TLS             TLS
@@ -119,6 +120,13 @@ type Log struct {
 	Verbose bool
 }
 
+// Redirects groups settings related to configuring _redirects limits
+type Redirects struct {
+	MaxConfigSize   int
+	MaxPathSegments int
+	MaxRuleCount    int
+}
+
 // Sentry groups settings related to configuring Sentry
 type Sentry struct {
 	DSN         string
@@ -287,6 +295,11 @@ func loadConfig() (*Config, error) {
 			Format:  *logFormat,
 			Verbose: *logVerbose,
 		},
+		Redirects: Redirects{
+			MaxConfigSize:   *redirectsMaxConfigSize,
+			MaxPathSegments: *redirectsMaxPathSegments,
+			MaxRuleCount:    *redirectsMaxRuleCount,
+		},
 		Sentry: Sentry{
 			DSN:         *sentryDSN,
 			Environment: *sentryEnvironment,
@@ -401,6 +414,9 @@ func LogConfig(config *Config) {
 		"rate-limit-tls-source-ip-burst": config.RateLimit.TLSSourceIPBurst,
 		"rate-limit-tls-domain":          config.RateLimit.TLSDomainLimitPerSecond,
 		"rate-limit-tls-domain-burst":    config.RateLimit.TLSDomainBurst,
+		"redirects-max-config-size":      config.Redirects.MaxConfigSize,
+		"redirects-max-path-segments":    config.Redirects.MaxPathSegments,
+		"redirects-max-rule-count":       config.Redirects.MaxRuleCount,
 		"server-read-timeout":            config.Server.ReadTimeout,
 		"server-read-header-timeout":     config.Server.ReadHeaderTimeout,
 		"server-write-timeout":           config.Server.WriteTimeout,
diff --git a/internal/config/flags.go b/internal/config/flags.go
index 88ebbb15..9d79f0bc 100644
--- a/internal/config/flags.go
+++ b/internal/config/flags.go
@@ -61,6 +61,11 @@ var (
 	gitlabRetrievalInterval = flag.Duration("gitlab-retrieval-interval", time.Second, "The interval to wait before retrying to resolve a domain's configuration via the GitLab API")
 	gitlabRetrievalRetries  = flag.Int("gitlab-retrieval-retries", 3, "The maximum number of times to retry to resolve a domain's configuration via the API")
 
+	// Check https://gitlab.com/gitlab-org/gitlab-pages/-/issues/472 before increasing default redirectsMaxConfigSize value
+	redirectsMaxConfigSize   = flag.Int("redirects-max-config-size", 64*1024, "The maximum size of the _redirects file, in bytes")
+	redirectsMaxPathSegments = flag.Int("redirects-max-path-segments", 25, "The maximum number of path segments allowed in _redirects rules URLs")
+	redirectsMaxRuleCount    = flag.Int("redirects-max-rule-count", 1000, "The maximum number of rules allowed in _redirects")
+
 	enableDisk = flag.Bool("enable-disk", true, "Enable disk access, shall be disabled in environments where shared disk storage isn't available")
 
 	clientID             = flag.String("auth-client-id", "", "GitLab application Client ID")
diff --git a/internal/redirects/matching.go b/internal/redirects/matching.go
index a75255bb..25e2ce2e 100644
--- a/internal/redirects/matching.go
+++ b/internal/redirects/matching.go
@@ -103,7 +103,7 @@ func matchesRule(rule *netlifyRedirects.Rule, path string) (bool, string) {
 // If no rule matches, this function returns `nil` and an empty string
 func (r *Redirects) match(path string) (*netlifyRedirects.Rule, string) {
 	for i := range r.rules {
-		if i >= maxRuleCount {
+		if i >= cfg.MaxRuleCount {
 			// do not process any more rules
 			return nil, ""
 		}
diff --git a/internal/redirects/redirects.go b/internal/redirects/redirects.go
index f3c4e491..4add6537 100644
--- a/internal/redirects/redirects.go
+++ b/internal/redirects/redirects.go
@@ -13,6 +13,7 @@ import (
 	netlifyRedirects "github.com/tj/go-redirects"
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
 )
 
@@ -24,16 +25,22 @@ const (
 	ConfigFile = "_redirects"
 
 	// Check https://gitlab.com/gitlab-org/gitlab-pages/-/issues/472 before increasing this value
-	maxConfigSize = 64 * 1024
+	defaultMaxConfigSize = 64 * 1024
 
 	// maxPathSegments is used to limit the number of path segments allowed in rules URLs
-	maxPathSegments = 25
+	defaultMaxPathSegments = 25
 
 	// maxRuleCount is used to limit the total number of rules allowed in _redirects
-	maxRuleCount = 1000
+	defaultMaxRuleCount = 1000
 )
 
 var (
+	cfg = config.Redirects{
+		MaxConfigSize:   defaultMaxConfigSize,
+		MaxPathSegments: defaultMaxPathSegments,
+		MaxRuleCount:    defaultMaxRuleCount,
+	}
+
 	// ErrNoRedirect is the error thrown when a no redirect rule matches while trying to Rewrite URL.
 	// This means that no redirect applies to the URL and you can fallback to serving actual content instead.
 	ErrNoRedirect                      = errors.New("no redirect found")
@@ -50,10 +57,13 @@ var (
 	errNoParams                        = errors.New("params not supported")
 	errUnsupportedStatus               = errors.New("status not supported")
 	errNoForce                         = errors.New("force! not supported")
-	errTooManyPathSegments             = fmt.Errorf("url path cannot contain more than %d forward slashes", maxPathSegments)
 	regexpPlaceholder                  = regexp.MustCompile(`(?i)/:[a-z]+`)
 )
 
+func SetConfig(redirectsConfig config.Redirects) {
+	cfg = redirectsConfig
+}
+
 type Redirects struct {
 	rules []netlifyRedirects.Rule
 	error error
@@ -69,13 +79,13 @@ func (r *Redirects) Status() string {
 	messages = append(messages, fmt.Sprintf("%d rules", len(r.rules)))
 
 	for i, rule := range r.rules {
-		if i >= maxRuleCount {
+		if i >= cfg.MaxRuleCount {
 			messages = append([]string{
 				fmt.Sprintf(
 					"The _redirects file contains (%d) rules, more than the maximum of %d rules. Only the first %d rules will be processed.",
 					len(r.rules),
-					maxRuleCount,
-					maxRuleCount,
+					cfg.MaxRuleCount,
+					cfg.MaxRuleCount,
 				)},
 				messages...,
 			)
@@ -126,7 +136,7 @@ func ParseRedirects(ctx context.Context, root vfs.Root) *Redirects {
 		return &Redirects{error: errNeedRegularFile}
 	}
 
-	if fi.Size() > maxConfigSize {
+	if fi.Size() > int64(cfg.MaxConfigSize) {
 		return &Redirects{error: errFileTooLarge}
 	}
 
diff --git a/internal/redirects/redirects_test.go b/internal/redirects/redirects_test.go
index 66c5d733..d40b065b 100644
--- a/internal/redirects/redirects_test.go
+++ b/internal/redirects/redirects_test.go
@@ -165,7 +165,7 @@ func TestRedirectsParseRedirects(t *testing.T) {
 		},
 		{
 			name:          "Config file too big",
-			redirectsFile: strings.Repeat("a", 2*maxConfigSize),
+			redirectsFile: strings.Repeat("a", 2*cfg.MaxConfigSize),
 			expectedRules: 0,
 			expectedErr:   errFileTooLarge,
 		},
@@ -197,7 +197,7 @@ func TestRedirectsParseRedirects(t *testing.T) {
 func TestMaxRuleCount(t *testing.T) {
 	root, tmpDir := testhelpers.TmpDir(t)
 
-	err := os.WriteFile(path.Join(tmpDir, ConfigFile), []byte(strings.Repeat("/goto.html /target.html 301\n", maxRuleCount-1)+
+	err := os.WriteFile(path.Join(tmpDir, ConfigFile), []byte(strings.Repeat("/goto.html /target.html 301\n", cfg.MaxRuleCount-1)+
 		"/1000.html /target1000 301\n"+
 		"/1001.html /target1001 301\n",
 	), 0600)
diff --git a/internal/redirects/validations.go b/internal/redirects/validations.go
index ed022f52..86fb0212 100644
--- a/internal/redirects/validations.go
+++ b/internal/redirects/validations.go
@@ -1,6 +1,7 @@
 package redirects
 
 import (
+	"fmt"
 	"net/http"
 	"net/url"
 	"regexp"
@@ -43,8 +44,8 @@ func validateURL(urlText string) error {
 		// Limit the number of path segments a rule can contain.
 		// This prevents the matching logic from generating regular
 		// expressions that are too large/complex.
-		if strings.Count(url.Path, "/") > maxPathSegments {
-			return errTooManyPathSegments
+		if strings.Count(url.Path, "/") > cfg.MaxPathSegments {
+			return fmt.Errorf("url path cannot contain more than %d forward slashes", cfg.MaxPathSegments)
 		}
 	} else {
 		// No support for splats, https://docs.netlify.com/routing/redirects/redirect-options/#splats
diff --git a/internal/redirects/validations_test.go b/internal/redirects/validations_test.go
index 296be511..0d6df3ec 100644
--- a/internal/redirects/validations_test.go
+++ b/internal/redirects/validations_test.go
@@ -1,6 +1,7 @@
 package redirects
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -42,7 +43,7 @@ func TestRedirectsValidateUrl(t *testing.T) {
 		},
 		"too_many_slashes": {
 			url:         strings.Repeat("/a", 26),
-			expectedErr: errTooManyPathSegments,
+			expectedErr: fmt.Errorf("url path cannot contain more than %d forward slashes", defaultMaxPathSegments),
 		},
 		"placeholders": {
 			url: "/news/:year/:month/:date/:slug",
@@ -58,7 +59,12 @@ func TestRedirectsValidateUrl(t *testing.T) {
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
 			err := validateURL(tt.url)
-			require.ErrorIs(t, err, tt.expectedErr)
+			if tt.expectedErr != nil {
+				require.EqualError(t, err, tt.expectedErr.Error())
+				return
+			}
+
+			require.NoError(t, err)
 		})
 	}
 }