diff --git a/.golangci.yml b/.golangci.yml index 2b97de091147956e5c0d962e18992c781b509414..21e290d41ce11312dea8757a26b062638cf5f955 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -22,7 +22,7 @@ linters-settings: govet: check-shadowing: false goconst: - min-len: 3 + min-len: 6 min-occurrences: 3 goimports: local-prefixes: gitlab.com/gitlab-org/gitlab-pages diff --git a/app.go b/app.go index c5555fc08de5e7b1f0060d10137e147c265c4f9c..dd6aff7da15c0dcd9e96dfd629a27d395bf79a6f 100644 --- a/app.go +++ b/app.go @@ -491,7 +491,7 @@ func (a *theApp) listenMetricsFD(wg *sync.WaitGroup, fd uintptr) { } func runApp(config *cfg.Config) { - domains, err := source.NewDomains(config) + domains, err := source.NewDomains(config, config.General.EnableDisk) if err != nil { log.WithError(err).Fatal("could not create domains config source") } diff --git a/app_test.go b/app_test.go index b7e9a01cba5c507c3764c4a52764933861281f08..927ecf4378763dba3bbc525f9058e214779eb0b8 100644 --- a/app_test.go +++ b/app_test.go @@ -92,7 +92,7 @@ func TestHealthCheckMiddleware(t *testing.T) { config: cfg, } - domains, err := source.NewDomains(app.config) + domains, err := source.NewDomains(app.config, true) require.NoError(t, err) app.domains = domains diff --git a/internal/config/config.go b/internal/config/config.go index 528b586ab108510823b7033bfbe98c4696427fe3..b4290d200526d241bdb6da476673c1ef0adba8a8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -61,6 +61,7 @@ type General struct { DisableCrossOriginRequests bool InsecureCiphers bool PropagateCorrelationID bool + EnableDisk bool ShowVersion bool @@ -243,6 +244,7 @@ func loadConfig() *Config { General: General{ Domain: strings.ToLower(*pagesDomain), DomainConfigurationSource: *domainConfigSource, + EnableDisk: *enableDisk, UseLegacyStorage: *useLegacyStorage, HTTP2: *useHTTP2, MaxConns: *maxConns, @@ -364,6 +366,7 @@ func LogConfig(config *Config) { "metrics-address": *metricsAddress, "pages-domain": *pagesDomain, "pages-root": *pagesRoot, + "enable-disk": *enableDisk, "pages-status": *pagesStatus, "propagate-correlation-id": *propagateCorrelationID, "redirect-http": config.General.RedirectHTTP, diff --git a/internal/config/flags.go b/internal/config/flags.go index 84eef0f6fac0ef5cddea80005f62bf51b98462e8..47c6d220559707084de7191a7c624fafae9d92ef 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -14,6 +14,7 @@ var ( redirectHTTP = flag.Bool("redirect-http", false, "Redirect pages from HTTP to HTTPS") useHTTP2 = flag.Bool("use-http2", true, "Enable HTTP2 support") pagesRoot = flag.String("pages-root", "shared/pages", "The directory where pages are stored") + enableDisk = flag.Bool("enable-disk", true, "Set to \"false\" to disable disk access") pagesDomain = flag.String("pages-domain", "gitlab-example.com", "The domain to serve static pages") artifactsServer = flag.String("artifacts-server", "", "API URL to proxy artifact requests to, e.g.: 'https://gitlab.com/api/v4'") artifactsServerTimeout = flag.Int("artifacts-server-timeout", 10, "Timeout (in seconds) for a proxied request to the artifacts server") diff --git a/internal/source/domains.go b/internal/source/domains.go index 5254c3b8a97a1e8070d85c46d79893e648e39cd4..3e089ea5cda9fcb4f868da5aed01f8b8bd56c455 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -1,6 +1,7 @@ package source import ( + "errors" "fmt" "regexp" @@ -16,6 +17,8 @@ var ( // is a serverless domain, to short circuit gitlab source rollout. It can be // removed after the rollout is done serverlessDomainRegex = regexp.MustCompile(`^[^.]+-[[:xdigit:]]{2}a1[[:xdigit:]]{10}f2[[:xdigit:]]{2}[[:xdigit:]]+-?.*`) + + errDiskSourceDisabled = errors.New("disk source is disabled via enable-disk=false") ) type configSource int @@ -32,6 +35,7 @@ const ( // currently using two sources during the transition to the new GitLab domains // source. type Domains struct { + enableDisk bool configSource configSource gitlab Source disk *disk.Disk // legacy disk source @@ -40,8 +44,10 @@ type Domains struct { // NewDomains is a factory method for domains initializing a mutex. It should // not initialize `dm` as we later check the readiness by comparing it with a // nil value. -func NewDomains(config Config) (*Domains, error) { - domains := &Domains{} +func NewDomains(config Config, enableDisk bool) (*Domains, error) { + domains := &Domains{ + enableDisk: enableDisk, + } if err := domains.setConfigSource(config); err != nil { return nil, err } @@ -59,10 +65,16 @@ func (d *Domains) setConfigSource(config Config) error { return d.setGitLabClient(config) case "auto": d.configSource = sourceAuto - // enable disk for auto for now - d.disk = disk.New() + if d.enableDisk { + // enable disk for auto when not explicitly disabled + d.disk = disk.New() + } return d.setGitLabClient(config) case "disk": + if !d.enableDisk { + return errDiskSourceDisabled + } + // TODO: disable domains.disk https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382 d.configSource = sourceDisk d.disk = disk.New() @@ -106,7 +118,7 @@ func (d *Domains) GetDomain(name string) (*domain.Domain, error) { // remove it entirely when disk source gets removed. func (d *Domains) Read(rootDomain string) { // start disk.Read for sourceDisk and sourceAuto - if d.configSource != sourceGitlab { + if d.configSource != sourceGitlab && d.enableDisk { d.disk.Read(rootDomain) } } diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index db9c2c23e25ca38391a18050f3dfad772701012e..ae7db87ba94a82059dced46c9124b8a74645bacf 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -53,6 +53,7 @@ func TestNewDomains(t *testing.T) { expectedErr string expectGitlabNil bool expectDiskNil bool + enableDisk bool }{ { name: "no_source_config", @@ -69,18 +70,21 @@ func TestNewDomains(t *testing.T) { sourceConfig: sourceConfig{domainSource: "disk"}, expectGitlabNil: true, expectDiskNil: false, + enableDisk: true, }, { name: "auto_without_api_config", sourceConfig: sourceConfig{domainSource: "auto"}, expectGitlabNil: true, expectDiskNil: false, + enableDisk: true, }, { name: "auto_with_api_config", sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"}, expectGitlabNil: false, expectDiskNil: false, + enableDisk: true, }, { name: "gitlab_source_success", @@ -97,11 +101,24 @@ func TestNewDomains(t *testing.T) { sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "", domainSource: "gitlab"}, expectedErr: "GitLab API URL or API secret has not been provided", }, + { + name: "auto_source_with_disk_disabled", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"}, + expectGitlabNil: false, + expectDiskNil: true, + enableDisk: false, + }, + { + name: "disk_source_with_disk_disabled", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "disk"}, + expectedErr: "disk source is disabled via enable-disk=false", + enableDisk: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - domains, err := NewDomains(tt.sourceConfig) + domains, err := NewDomains(tt.sourceConfig, tt.enableDisk) if tt.expectedErr != "" { require.EqualError(t, err, tt.expectedErr) return diff --git a/main.go b/main.go index 441b3a749f20361f27e1acbe6444431cc99c3fe4..b05b866882e09f18ff023fb185f23ee4daddad28 100644 --- a/main.go +++ b/main.go @@ -31,6 +31,8 @@ func initErrorReporting(sentryDSN, sentryEnvironment string) { errortracking.WithSentryEnvironment(sentryEnvironment)) } +// nolint: gocyclo +// TODO reduce cyclomatic complexity https://gitlab.com/gitlab-org/gitlab-pages/-/issues/557 func appMain() { if err := validateargs.NotAllowed(os.Args[1:]); err != nil { log.WithError(err).Fatal("Using invalid arguments, use -config=gitlab-pages-config file instead") diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go index d2c3c31e41ad38dcb6fa08434510965f9d4e8ff4..72bb41ade42ad43346d879fd2e589b6e3ab33df7 100644 --- a/test/acceptance/helpers_test.go +++ b/test/acceptance/helpers_test.go @@ -219,8 +219,8 @@ func RunPagesProcessWithEnvs(t *testing.T, wait bool, pagesBinary string, listen return cleanup } -func RunPagesProcessWithOutput(t *testing.T, pagesBinary string, listeners []ListenSpec, promPort string, extraArgs ...string) (out *LogCaptureBuffer, teardown func()) { - return runPagesProcess(t, true, pagesBinary, listeners, promPort, nil, extraArgs...) +func RunPagesProcessWithOutput(t *testing.T, wait bool, pagesBinary string, listeners []ListenSpec, promPort string, extraArgs ...string) (out *LogCaptureBuffer, teardown func()) { + return runPagesProcess(t, wait, pagesBinary, listeners, promPort, nil, extraArgs...) } func RunPagesProcessWithStubGitLabServer(t *testing.T, wait bool, pagesBinary string, listeners []ListenSpec, promPort string, envs []string, extraArgs ...string) (teardown func()) { diff --git a/test/acceptance/proxyv2_test.go b/test/acceptance/proxyv2_test.go index 2a42f0f1cf9d355bd3a862b2a8362b417e45604b..a169a05a7b0f8273a1457e1ea6cf48bfd200b517 100644 --- a/test/acceptance/proxyv2_test.go +++ b/test/acceptance/proxyv2_test.go @@ -12,7 +12,7 @@ import ( func TestProxyv2(t *testing.T) { skipUnlessEnabled(t) - logBuf, teardown := RunPagesProcessWithOutput(t, *pagesBinary, listeners, "") + logBuf, teardown := RunPagesProcessWithOutput(t, true, *pagesBinary, listeners, "") defer teardown() // the dummy client IP 10.1.1.1 is set by TestProxyv2Client diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go index 7824dda913b0d5a225c7e5ff45ca9d63d20cf171..776356bfaf5e92c650e98fac73d9478388f5d145 100644 --- a/test/acceptance/serving_test.go +++ b/test/acceptance/serving_test.go @@ -380,6 +380,7 @@ func TestDomainsSource(t *testing.T) { type args struct { configSource string useLegacyStorage bool + enableDisk bool domain string urlSuffix string readyCount int @@ -421,6 +422,7 @@ func TestDomainsSource(t *testing.T) { { name: "disk_source_domain_exists", args: args{ + enableDisk: true, configSource: "disk", // test.domain.com sourced from disk configuration domain: "test.domain.com", @@ -435,6 +437,7 @@ func TestDomainsSource(t *testing.T) { { name: "disk_source_domain_does_not_exist", args: args{ + enableDisk: true, configSource: "disk", domain: "non-existent-domain.gitlab.io", }, @@ -446,6 +449,7 @@ func TestDomainsSource(t *testing.T) { { name: "disk_source_domain_should_not_exist_under_hashed_dir", args: args{ + enableDisk: true, configSource: "disk", domain: "hashed.com", }, @@ -457,6 +461,7 @@ func TestDomainsSource(t *testing.T) { { name: "auto_source_gitlab_is_not_ready", args: args{ + enableDisk: true, configSource: "auto", domain: "test.domain.com", urlSuffix: "/", @@ -471,6 +476,7 @@ func TestDomainsSource(t *testing.T) { { name: "auto_source_gitlab_is_ready", args: args{ + enableDisk: true, configSource: "auto", domain: "new-source-test.gitlab.io", urlSuffix: "/my/pages/project/", @@ -486,6 +492,7 @@ func TestDomainsSource(t *testing.T) { name: "use_legacy_storage_overrides_domain_source", args: args{ useLegacyStorage: true, + enableDisk: true, domain: "test.domain.com", urlSuffix: "/", }, @@ -495,6 +502,47 @@ func TestDomainsSource(t *testing.T) { apiCalled: false, }, }, + { + name: "enable_disk_with_auto_source", + args: args{ + enableDisk: true, + configSource: "auto", + domain: "test.domain.com", + urlSuffix: "/", + readyCount: 100, // big number to ensure the API is in bad state for a while + }, + want: want{ + statusCode: http.StatusOK, + content: "main-dir\n", + apiCalled: false, + }, + }, + { + name: "enable_disk_with_gitlab_source_disk_ignored", + args: args{ + enableDisk: true, + configSource: "gitlab", + domain: "test.domain.com", + urlSuffix: "/", + }, + want: want{ + statusCode: http.StatusNotFound, + apiCalled: true, + }, + }, + { + name: "disable_disk_auto_source", + args: args{ + enableDisk: false, + configSource: "auto", + domain: "test.domain.com", + urlSuffix: "/", + }, + want: want{ + statusCode: http.StatusNotFound, + apiCalled: true, + }, + }, } for _, tt := range tests { @@ -511,7 +559,9 @@ func TestDomainsSource(t *testing.T) { pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", tt.args.configSource, // TODO: remove in https://gitlab.com/gitlab-org/omnibus-gitlab/-/issues/6009 - fmt.Sprintf("-use-legacy-storage=%t", tt.args.useLegacyStorage)} + fmt.Sprintf("-use-legacy-storage=%t", tt.args.useLegacyStorage), + fmt.Sprintf("-enable-disk=%t", tt.args.enableDisk), + } teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, []ListenSpec{httpListener}, "", []string{}, pagesArgs...) defer teardown() @@ -532,6 +582,19 @@ func TestDomainsSource(t *testing.T) { } } +func TestDisabledDiskWithDiskSourceFailsToStart(t *testing.T) { + skipUnlessEnabled(t) + + logBuf, teardown := RunPagesProcessWithOutput(t, false, *pagesBinary, listeners, "", "-enable-disk=false", "-domain-config-source=disk") + defer teardown() + + // give the process enough time to write the log message + require.Eventually(t, func() bool { + require.Contains(t, logBuf.String(), "disk source is disabled via enable-disk=false", "log mismatch") + return true + }, 2*time.Second, 500*time.Millisecond) +} + // TestGitLabSourceBecomesUnauthorized proves workaround for https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535 // The first request will fail and display an error but subsequent requests will // serve from disk source when `domain-config-source=auto`