Handle and return errors in internal/config/

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
 }
diff --git a/main.go b/main.go
index 441b3a74..ec4f917f 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")
@@ -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")
 	}