From b8ad9a335c8251691ce3300d2dabea346bc9de94 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 23 Mar 2021 15:19:39 +1100 Subject: [PATCH 1/4] Add flag `-enable-disk` Introduces the flag `enable-disk=true` by default. This flag will allow people use NFS instead of Object Storage. When `enable-disk=false` the NFS mount won't be needed anymore and Pages will not try to scan the directory for files to serve. Changelog: added --- daemon.go | 33 ++++++++++++++++++++------------- internal/config/flags.go | 2 +- internal/config/validate.go | 4 ++++ main.go | 4 +++- 4 files changed, 28 insertions(+), 15 deletions(-) diff --git a/daemon.go b/daemon.go index c2a995d85..a7f104368 100644 --- a/daemon.go +++ b/daemon.go @@ -255,13 +255,17 @@ func jailDaemon(pagesRoot string, cmd *exec.Cmd) (*jail.Jail, error) { return nil, err } - // Bind mount shared folder - cage.MkDirAll(pagesRoot, 0755) - cage.Bind(pagesRoot, pagesRoot) - // Update command to use chroot cmd.SysProcAttr.Chroot = cage.Path() cmd.Path = "/gitlab-pages" + + if pagesRoot == "false" { + return cage, nil + } + + // Bind mount shared folder + cage.MkDirAll(pagesRoot, 0755) + cage.Bind(pagesRoot, pagesRoot) cmd.Dir = pagesRoot return cage, nil @@ -273,15 +277,18 @@ func daemonize(config *config.Config) error { inPlace := config.Daemon.InplaceChroot pagesRoot := config.General.RootDir - // Ensure pagesRoot is an absolute path. This will produce a different path - // if any component of pagesRoot is a symlink (not likely). For example, - // -pages-root=/some-path where ln -s /other-path /some-path - // pagesPath will become: /other-path and we will fail to serve files from /some-path. - // GitLab Rails also resolves the absolute path for `pages_path` - // https://gitlab.com/gitlab-org/gitlab/blob/981ad651d8bd3690e28583eec2363a79f775af89/config/initializers/1_settings.rb#L296 - pagesRoot, err := filepath.Abs(pagesRoot) - if err != nil { - return err + if pagesRoot != "false" { + // Ensure pagesRoot is an absolute path. This will produce a different path + // if any component of pagesRoot is a symlink (not likely). For example, + // -pages-root=/some-path where ln -s /other-path /some-path + // pagesPath will become: /other-path and we will fail to serve files from /some-path. + // GitLab Rails also resolves the absolute path for `pages_path` + // https://gitlab.com/gitlab-org/gitlab/blob/981ad651d8bd3690e28583eec2363a79f775af89/config/initializers/1_settings.rb#L296 + var err error + pagesRoot, err = filepath.Abs(pagesRoot) + if err != nil { + return err + } } log.WithFields(log.Fields{ diff --git a/internal/config/flags.go b/internal/config/flags.go index 84eef0f6f..e4e57dbe3 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -13,7 +13,7 @@ var ( pagesRootKey = flag.String("root-key", "", "The default path to file certificate to serve static pages") 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") + pagesRoot = flag.String("pages-root", "shared/pages", "The directory where pages are stored. 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/config/validate.go b/internal/config/validate.go index 393ff9d29..662bfeb7c 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -9,6 +9,10 @@ import ( ) func validateConfig(config *Config) { + if config.General.RootDir == "false" && config.General.DomainConfigurationSource == "disk" { + log.Fatal("incompatible settings for pages-root=false and domain-config-source=disk, either use domain-config-source=gitlab or set a valid pages-root") + } + validateAuthConfig(config) validateArtifactsServerConfig(config) validateTLSConfig() diff --git a/main.go b/main.go index 441b3a749..72180f10c 100644 --- a/main.go +++ b/main.go @@ -61,7 +61,9 @@ func appMain() { }).Print("GitLab Pages Daemon") log.Printf("URL: https://gitlab.com/gitlab-org/gitlab-pages") - if err := os.Chdir(config.General.RootDir); err != nil { + if config.General.RootDir == "false" { + log.Info("pages-root is disabled!") + } else if err := os.Chdir(config.General.RootDir); err != nil { fatal(err, "could not change directory into pagesRoot") } -- GitLab From 4d71484adeedc3f6c3796e9a1d3298d6d651ed93 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 24 Mar 2021 15:58:06 +1100 Subject: [PATCH 2/4] Disable disk source Fix linting issues --- .golangci.yml | 2 +- app.go | 2 +- app_test.go | 2 +- daemon.go | 48 ++++++++++++++++++--------------- internal/config/config.go | 3 +++ internal/config/flags.go | 3 ++- internal/config/validate.go | 4 --- internal/source/domains.go | 22 +++++++++++---- internal/source/domains_test.go | 16 ++++++++++- main.go | 6 ++--- test/acceptance/helpers_test.go | 4 +-- test/acceptance/proxyv2_test.go | 2 +- test/acceptance/serving_test.go | 37 ++++++++++++++++++++++++- 13 files changed, 109 insertions(+), 42 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 2b97de091..21e290d41 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 c5555fc08..dd6aff7da 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 b7e9a01cb..63d04464f 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, false) require.NoError(t, err) app.domains = domains diff --git a/daemon.go b/daemon.go index a7f104368..f0a691f18 100644 --- a/daemon.go +++ b/daemon.go @@ -117,12 +117,7 @@ func passSignals(cmd *exec.Cmd) { }() } -func chrootDaemon(cmd *exec.Cmd) (*jail.Jail, error) { - wd, err := os.Getwd() - if err != nil { - return nil, err - } - +func chrootDaemon(cmd *exec.Cmd, wd string) (*jail.Jail, error) { chroot := jail.Into(wd) // Generate a probabilistically-unique suffix for the copy of the pages @@ -255,20 +250,37 @@ func jailDaemon(pagesRoot string, cmd *exec.Cmd) (*jail.Jail, error) { return nil, err } + // Bind mount shared folder + cage.MkDirAll(pagesRoot, 0755) + cage.Bind(pagesRoot, pagesRoot) + // Update command to use chroot cmd.SysProcAttr.Chroot = cage.Path() cmd.Path = "/gitlab-pages" + cmd.Dir = pagesRoot - if pagesRoot == "false" { - return cage, nil + return cage, nil +} + +func getJailWrapper(cmd *exec.Cmd, pagesRoot string, inPlace bool) (*jail.Jail, error) { + wd, err := os.Getwd() + if err != nil { + return nil, err } - // Bind mount shared folder - cage.MkDirAll(pagesRoot, 0755) - cage.Bind(pagesRoot, pagesRoot) - cmd.Dir = pagesRoot + // Run daemon in chroot environment + var wrapper *jail.Jail + if inPlace { + wrapper, err = chrootDaemon(cmd, wd) + } else { + wrapper, err = jailDaemon(pagesRoot, cmd) + } + if err != nil { + log.WithError(err).Print("chroot failed") + return nil, err + } - return cage, nil + return wrapper, nil } func daemonize(config *config.Config) error { @@ -304,17 +316,11 @@ func daemonize(config *config.Config) error { } defer killProcess(cmd) - // Run daemon in chroot environment - var wrapper *jail.Jail - if inPlace { - wrapper, err = chrootDaemon(cmd) - } else { - wrapper, err = jailDaemon(pagesRoot, cmd) - } + wrapper, err := getJailWrapper(cmd, pagesRoot, inPlace) if err != nil { - log.WithError(err).Print("chroot failed") return err } + defer wrapper.Dispose() // Unshare mount namespace diff --git a/internal/config/config.go b/internal/config/config.go index 528b586ab..b4290d200 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 e4e57dbe3..642d75fff 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -13,7 +13,8 @@ var ( pagesRootKey = flag.String("root-key", "", "The default path to file certificate to serve static pages") 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. Set to \"false\" to disable disk access.") + 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/config/validate.go b/internal/config/validate.go index 662bfeb7c..393ff9d29 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -9,10 +9,6 @@ import ( ) func validateConfig(config *Config) { - if config.General.RootDir == "false" && config.General.DomainConfigurationSource == "disk" { - log.Fatal("incompatible settings for pages-root=false and domain-config-source=disk, either use domain-config-source=gitlab or set a valid pages-root") - } - validateAuthConfig(config) validateArtifactsServerConfig(config) validateTLSConfig() diff --git a/internal/source/domains.go b/internal/source/domains.go index 5254c3b8a..087108a10 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 db9c2c23e..576e645d3 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 + disableDisk bool }{ { name: "no_source_config", @@ -97,11 +98,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, + disableDisk: true, + }, + { + name: "disk_source_with_disk_disabled", + sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "disk"}, + expectedErr: "disk source is disabled via pages-root=false", + disableDisk: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - domains, err := NewDomains(tt.sourceConfig) + domains, err := NewDomains(tt.sourceConfig, tt.disableDisk) if tt.expectedErr != "" { require.EqualError(t, err, tt.expectedErr) return diff --git a/main.go b/main.go index 72180f10c..b05b86688 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") @@ -61,9 +63,7 @@ func appMain() { }).Print("GitLab Pages Daemon") log.Printf("URL: https://gitlab.com/gitlab-org/gitlab-pages") - if config.General.RootDir == "false" { - log.Info("pages-root is disabled!") - } else if err := os.Chdir(config.General.RootDir); err != nil { + if err := os.Chdir(config.General.RootDir); err != nil { fatal(err, "could not change directory into pagesRoot") } diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go index d2c3c31e4..72bb41ade 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 2a42f0f1c..a169a05a7 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 7824dda91..76120f935 100644 --- a/test/acceptance/serving_test.go +++ b/test/acceptance/serving_test.go @@ -379,6 +379,7 @@ func TestDomainsSource(t *testing.T) { type args struct { configSource string + pagesRoot string useLegacyStorage bool domain string urlSuffix string @@ -495,6 +496,19 @@ func TestDomainsSource(t *testing.T) { apiCalled: false, }, }, + { + name: "pages", + args: args{ + useLegacyStorage: true, + domain: "test.domain.com", + urlSuffix: "/", + }, + want: want{ + statusCode: http.StatusOK, + content: "main-dir\n", + apiCalled: false, + }, + }, } for _, tt := range tests { @@ -509,9 +523,15 @@ func TestDomainsSource(t *testing.T) { gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) + if tt.args.pagesRoot == "" { + tt.args.pagesRoot = "../../shared/pages" + } + 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), + "-pages-root", tt.args.pagesRoot, + } teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, []ListenSpec{httpListener}, "", []string{}, pagesArgs...) defer teardown() @@ -532,6 +552,21 @@ func TestDomainsSource(t *testing.T) { } } +func TestDisabledDiskWithDiskSourceFailsToStart(t *testing.T) { + skipUnlessEnabled(t) + + expectedErrMsg := "incompatible settings for pages-root=false and domain-config-source=disk" + + logBuf, teardown := RunPagesProcessWithOutput(t, false, *pagesBinary, listeners, "", "-pages-root=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(), expectedErrMsg, "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` -- GitLab From 452cdc9e9d193649c2b2b3c2cd32834e86b5a687 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 12 Apr 2021 17:47:26 +1000 Subject: [PATCH 3/4] Fix enable disk logic --- app_test.go | 2 +- daemon.go | 59 +++++++++++++-------------------- internal/source/domains.go | 4 +-- internal/source/domains_test.go | 13 +++++--- test/acceptance/serving_test.go | 6 ++-- 5 files changed, 36 insertions(+), 48 deletions(-) diff --git a/app_test.go b/app_test.go index 63d04464f..927ecf437 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, false) + domains, err := source.NewDomains(app.config, true) require.NoError(t, err) app.domains = domains diff --git a/daemon.go b/daemon.go index f0a691f18..c2a995d85 100644 --- a/daemon.go +++ b/daemon.go @@ -117,7 +117,12 @@ func passSignals(cmd *exec.Cmd) { }() } -func chrootDaemon(cmd *exec.Cmd, wd string) (*jail.Jail, error) { +func chrootDaemon(cmd *exec.Cmd) (*jail.Jail, error) { + wd, err := os.Getwd() + if err != nil { + return nil, err + } + chroot := jail.Into(wd) // Generate a probabilistically-unique suffix for the copy of the pages @@ -262,45 +267,21 @@ func jailDaemon(pagesRoot string, cmd *exec.Cmd) (*jail.Jail, error) { return cage, nil } -func getJailWrapper(cmd *exec.Cmd, pagesRoot string, inPlace bool) (*jail.Jail, error) { - wd, err := os.Getwd() - if err != nil { - return nil, err - } - - // Run daemon in chroot environment - var wrapper *jail.Jail - if inPlace { - wrapper, err = chrootDaemon(cmd, wd) - } else { - wrapper, err = jailDaemon(pagesRoot, cmd) - } - if err != nil { - log.WithError(err).Print("chroot failed") - return nil, err - } - - return wrapper, nil -} - func daemonize(config *config.Config) error { uid := config.Daemon.UID gid := config.Daemon.GID inPlace := config.Daemon.InplaceChroot pagesRoot := config.General.RootDir - if pagesRoot != "false" { - // Ensure pagesRoot is an absolute path. This will produce a different path - // if any component of pagesRoot is a symlink (not likely). For example, - // -pages-root=/some-path where ln -s /other-path /some-path - // pagesPath will become: /other-path and we will fail to serve files from /some-path. - // GitLab Rails also resolves the absolute path for `pages_path` - // https://gitlab.com/gitlab-org/gitlab/blob/981ad651d8bd3690e28583eec2363a79f775af89/config/initializers/1_settings.rb#L296 - var err error - pagesRoot, err = filepath.Abs(pagesRoot) - if err != nil { - return err - } + // Ensure pagesRoot is an absolute path. This will produce a different path + // if any component of pagesRoot is a symlink (not likely). For example, + // -pages-root=/some-path where ln -s /other-path /some-path + // pagesPath will become: /other-path and we will fail to serve files from /some-path. + // GitLab Rails also resolves the absolute path for `pages_path` + // https://gitlab.com/gitlab-org/gitlab/blob/981ad651d8bd3690e28583eec2363a79f775af89/config/initializers/1_settings.rb#L296 + pagesRoot, err := filepath.Abs(pagesRoot) + if err != nil { + return err } log.WithFields(log.Fields{ @@ -316,11 +297,17 @@ func daemonize(config *config.Config) error { } defer killProcess(cmd) - wrapper, err := getJailWrapper(cmd, pagesRoot, inPlace) + // Run daemon in chroot environment + var wrapper *jail.Jail + if inPlace { + wrapper, err = chrootDaemon(cmd) + } else { + wrapper, err = jailDaemon(pagesRoot, cmd) + } if err != nil { + log.WithError(err).Print("chroot failed") return err } - defer wrapper.Dispose() // Unshare mount namespace diff --git a/internal/source/domains.go b/internal/source/domains.go index 087108a10..3e089ea5c 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -65,7 +65,7 @@ func (d *Domains) setConfigSource(config Config) error { return d.setGitLabClient(config) case "auto": d.configSource = sourceAuto - if !d.enableDisk { + if d.enableDisk { // enable disk for auto when not explicitly disabled d.disk = disk.New() } @@ -118,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 && !d.enableDisk { + 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 576e645d3..ae7db87ba 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -53,7 +53,7 @@ func TestNewDomains(t *testing.T) { expectedErr string expectGitlabNil bool expectDiskNil bool - disableDisk bool + enableDisk bool }{ { name: "no_source_config", @@ -70,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", @@ -103,19 +106,19 @@ func TestNewDomains(t *testing.T) { sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"}, expectGitlabNil: false, expectDiskNil: true, - disableDisk: 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 pages-root=false", - disableDisk: true, + 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, tt.disableDisk) + domains, err := NewDomains(tt.sourceConfig, tt.enableDisk) if tt.expectedErr != "" { require.EqualError(t, err, tt.expectedErr) return diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go index 76120f935..da8c87e0b 100644 --- a/test/acceptance/serving_test.go +++ b/test/acceptance/serving_test.go @@ -555,14 +555,12 @@ func TestDomainsSource(t *testing.T) { func TestDisabledDiskWithDiskSourceFailsToStart(t *testing.T) { skipUnlessEnabled(t) - expectedErrMsg := "incompatible settings for pages-root=false and domain-config-source=disk" - - logBuf, teardown := RunPagesProcessWithOutput(t, false, *pagesBinary, listeners, "", "-pages-root=false", "-domain-config-source=disk") + 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(), expectedErrMsg, "log mismatch") + require.Contains(t, logBuf.String(), "disk source is disabled via enable-disk=false", "log mismatch") return true }, 2*time.Second, 500*time.Millisecond) } -- GitLab From 149cfe6fc037eb402002478cc6074280c62766ca Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 13 Apr 2021 16:05:18 +1000 Subject: [PATCH 4/4] Add enable-disk to acceptance tests --- internal/config/flags.go | 4 +-- test/acceptance/serving_test.go | 50 ++++++++++++++++++++++++++------- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/internal/config/flags.go b/internal/config/flags.go index 642d75fff..47c6d2205 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -13,8 +13,8 @@ var ( pagesRootKey = flag.String("root-key", "", "The default path to file certificate to serve static pages") 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.") + 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/test/acceptance/serving_test.go b/test/acceptance/serving_test.go index da8c87e0b..776356bfa 100644 --- a/test/acceptance/serving_test.go +++ b/test/acceptance/serving_test.go @@ -379,8 +379,8 @@ func TestDomainsSource(t *testing.T) { type args struct { configSource string - pagesRoot string useLegacyStorage bool + enableDisk bool domain string urlSuffix string readyCount int @@ -422,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", @@ -436,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", }, @@ -447,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", }, @@ -458,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: "/", @@ -472,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/", @@ -487,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: "/", }, @@ -497,11 +503,13 @@ func TestDomainsSource(t *testing.T) { }, }, { - name: "pages", + name: "enable_disk_with_auto_source", args: args{ - useLegacyStorage: true, - domain: "test.domain.com", - urlSuffix: "/", + 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, @@ -509,6 +517,32 @@ func TestDomainsSource(t *testing.T) { 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 { @@ -523,14 +557,10 @@ func TestDomainsSource(t *testing.T) { gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) - if tt.args.pagesRoot == "" { - tt.args.pagesRoot = "../../shared/pages" - } - 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), - "-pages-root", tt.args.pagesRoot, + fmt.Sprintf("-enable-disk=%t", tt.args.enableDisk), } teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, []ListenSpec{httpListener}, "", []string{}, pagesArgs...) defer teardown() -- GitLab