diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2021-04-28 04:40:28 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2021-04-28 04:40:28 +0300 |
commit | ddb27f943109777cc67f1ff77b2091b72e485a30 (patch) | |
tree | 1a28728d43f89b6e0398240a2824848d661e4583 | |
parent | 8add308dfab8173282566ca053d8b3bb5219945c (diff) | |
parent | 75bcb4dc938e0e8ad4224d102b2ba8057fe34aaf (diff) |
Merge branch 'fix/config-errors' into 'master'
Handle and return errors in internal/config/
Closes #550
See merge request gitlab-org/gitlab-pages!465
-rw-r--r-- | app_test.go | 3 | ||||
-rw-r--r-- | internal/config/config.go | 73 | ||||
-rw-r--r-- | internal/config/config_test.go | 4 | ||||
-rw-r--r-- | internal/config/validate.go | 48 | ||||
-rw-r--r-- | main.go | 9 |
5 files changed, 77 insertions, 60 deletions
diff --git a/app_test.go b/app_test.go index b7e9a01c..48c981cf 100644 --- a/app_test.go +++ b/app_test.go @@ -84,7 +84,8 @@ func TestHealthCheckMiddleware(t *testing.T) { }, } - cfg := config.LoadConfig() + cfg, err := config.LoadConfig() + require.NoError(t, err) cfg.General.StatusPath = "/-/healthcheck" cfg.General.DomainConfigurationSource = "auto" diff --git a/internal/config/config.go b/internal/config/config.go index 528b586a..3733bfb2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -152,60 +152,55 @@ type ZipServing struct { ChrootPath string } -func gitlabServerFromFlags() string { +func gitlabServerFromFlags() (string, error) { if *gitLabServer != "" { - return *gitLabServer + return *gitLabServer, nil } if *gitLabAuthServer != "" { log.Warn("auth-server parameter is deprecated, use gitlab-server instead") - return *gitLabAuthServer + return *gitLabAuthServer, nil } u, err := url.Parse(*artifactsServer) if err != nil { - return "" + return "", fmt.Errorf("parsing artifact server: %w", err) } u.Path = "" - return u.String() + return u.String(), nil } -func internalGitlabServerFromFlags() string { +func internalGitlabServerFromFlags() (string, error) { if *internalGitLabServer != "" { - return *internalGitLabServer + return *internalGitLabServer, nil } return gitlabServerFromFlags() } -func setGitLabAPISecretKey(secretFile string, config *Config) { - encoded := readFile(secretFile) +func setGitLabAPISecretKey(secretFile string, config *Config) error { + if secretFile == "" { + return nil + } + + encoded, err := ioutil.ReadFile(secretFile) + if err != nil { + return fmt.Errorf("reading secret file: %w", err) + } decoded := make([]byte, base64.StdEncoding.DecodedLen(len(encoded))) secretLength, err := base64.StdEncoding.Decode(decoded, encoded) if err != nil { - log.WithError(err).Fatal("Failed to decode GitLab API secret") + return fmt.Errorf("decoding GitLab API secret: %w", err) } if secretLength != 32 { - log.WithError(fmt.Errorf("expected 32 bytes GitLab API secret but got %d bytes", secretLength)).Fatal("Failed to decode GitLab API secret") + return fmt.Errorf("expected 32 bytes GitLab API secret but got %d bytes", secretLength) } config.GitLab.APISecretKey = decoded -} - -// fatal will log a fatal error and exit. -func fatal(err error, message string) { - log.WithError(err).Fatal(message) -} - -func readFile(file string) (result []byte) { - result, err := ioutil.ReadFile(file) - if err != nil { - fatal(err, "could not read file") - } - return + return nil } // InternalGitLabServerURL returns URL to a GitLab instance. @@ -238,7 +233,7 @@ func (config *Config) Cache() *Cache { return &config.GitLab.Cache } -func loadConfig() *Config { +func loadConfig() (*Config, error) { config := &Config{ General: General{ Domain: strings.ToLower(*pagesDomain), @@ -313,6 +308,8 @@ func loadConfig() *Config { Listeners: Listeners{}, } + var err error + // Populating remaining General settings for _, file := range []struct { contents *[]byte @@ -322,15 +319,23 @@ func loadConfig() *Config { {&config.General.RootKey, *pagesRootKey}, } { if file.path != "" { - *file.contents = readFile(file.path) + if *file.contents, err = ioutil.ReadFile(file.path); err != nil { + return nil, err + } } } // Populating remaining GitLab settings - config.GitLab.Server = gitlabServerFromFlags() - config.GitLab.InternalServer = internalGitlabServerFromFlags() - if *gitLabAPISecretKey != "" { - setGitLabAPISecretKey(*gitLabAPISecretKey, config) + if config.GitLab.Server, err = gitlabServerFromFlags(); err != nil { + return nil, err + } + + if config.GitLab.InternalServer, err = internalGitlabServerFromFlags(); err != nil { + return nil, err + } + + if err = setGitLabAPISecretKey(*gitLabAPISecretKey, config); err != nil { + return nil, err } // TODO: this is a temporary workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101 @@ -340,9 +345,11 @@ func loadConfig() *Config { config.Zip.ChrootPath = *pagesRoot } - validateConfig(config) + if err := validateConfig(config); err != nil { + return nil, err + } - return config + return config, nil } func LogConfig(config *Config) { @@ -389,7 +396,7 @@ func LogConfig(config *Config) { // LoadConfig parses configuration settings passed as command line arguments or // via config file, and populates a Config object with those values -func LoadConfig() *Config { +func LoadConfig() (*Config, error) { initFlags() return loadConfig() diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7fa2b253..f9e9b2a0 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -48,7 +48,9 @@ func TestGitLabServerFromFlags(t *testing.T) { gitLabServer = &test.gitLabServer gitLabAuthServer = &test.gitLabAuthServer artifactsServer = &test.artifactsServer - require.Equal(t, test.expected, gitlabServerFromFlags()) + gServer, err := gitlabServerFromFlags() + require.NoError(t, err) + require.Equal(t, test.expected, gServer) }) } } diff --git a/internal/config/validate.go b/internal/config/validate.go index 393ff9d2..cf8025eb 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -1,63 +1,65 @@ package config import ( + "errors" "net/url" - log "github.com/sirupsen/logrus" - "gitlab.com/gitlab-org/gitlab-pages/internal/config/tls" ) -func validateConfig(config *Config) { - validateAuthConfig(config) - validateArtifactsServerConfig(config) - validateTLSConfig() +func validateConfig(config *Config) error { + if err := validateAuthConfig(config); err != nil { + return err + } + + if err := validateArtifactsServerConfig(config); err != nil { + return err + } + + return tls.ValidateTLSVersions(*tlsMinVersion, *tlsMaxVersion) } -func validateAuthConfig(config *Config) { +func validateAuthConfig(config *Config) error { if config.Authentication.Secret == "" && config.Authentication.ClientID == "" && config.Authentication.ClientSecret == "" && config.Authentication.RedirectURI == "" { - return + return nil } if config.Authentication.Secret == "" { - log.Fatal("auth-secret must be defined if authentication is supported") + return errors.New("auth-secret must be defined if authentication is supported") } if config.Authentication.ClientID == "" { - log.Fatal("auth-client-id must be defined if authentication is supported") + return errors.New("auth-client-id must be defined if authentication is supported") } if config.Authentication.ClientSecret == "" { - log.Fatal("auth-client-secret must be defined if authentication is supported") + return errors.New("auth-client-secret must be defined if authentication is supported") } if config.GitLab.Server == "" { - log.Fatal("gitlab-server must be defined if authentication is supported") + return errors.New("gitlab-server must be defined if authentication is supported") } if config.Authentication.RedirectURI == "" { - log.Fatal("auth-redirect-uri must be defined if authentication is supported") + return errors.New("auth-redirect-uri must be defined if authentication is supported") } + return nil } -func validateArtifactsServerConfig(config *Config) { +func validateArtifactsServerConfig(config *Config) error { if config.ArtifactsServer.URL == "" { - return + return nil } u, err := url.Parse(config.ArtifactsServer.URL) if err != nil { - log.Fatal(err) + return err } // url.Parse ensures that the Scheme attribute is always lower case. if u.Scheme != "http" && u.Scheme != "https" { - log.Fatal("artifacts-server scheme must be either http:// or https://") + return errors.New("artifacts-server scheme must be either http:// or https://") } if config.ArtifactsServer.TimeoutSeconds < 1 { - log.Fatal("artifacts-server-timeout must be greater than or equal to 1") + return errors.New("artifacts-server-timeout must be greater than or equal to 1") } -} -func validateTLSConfig() { - if err := tls.ValidateTLSVersions(*tlsMinVersion, *tlsMaxVersion); err != nil { - fatal(err, "invalid TLS version") - } + return nil } @@ -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") @@ -40,7 +42,10 @@ func appMain() { log.WithError(err).Warn("Using deprecated arguments") } - config := cfg.LoadConfig() + config, err := cfg.LoadConfig() + if err != nil { + log.WithError(err).Fatal("Failed to load config") + } printVersion(config.General.ShowVersion, VERSION) @@ -48,7 +53,7 @@ func appMain() { initErrorReporting(config.Sentry.DSN, config.Sentry.Environment) } - err := logging.ConfigureLogging(config.Log.Format, config.Log.Verbose) + err = logging.ConfigureLogging(config.Log.Format, config.Log.Verbose) if err != nil { log.WithError(err).Fatal("Failed to initialize logging") } |