Merge branch 'fix/gb/improve-error-reporting-in-client' into 'master'

Improve error handing when creating new domains config source

Closes #275

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

10 files changed, 128 insertions, 36 deletions

diff --git a/acceptance_test.go b/acceptance_test.go
index 6f2e5fb4..dfa334b9 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -1536,7 +1536,8 @@ func TestGitlabDomainsSource(t *testing.T) {
 	defer source.Close()
 
 	newSourceDomains := "GITLAB_NEW_SOURCE_DOMAINS=new-source-test.gitlab.io,other-test.gitlab.io"
-	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{newSourceDomains}, "-gitlab-server", source.URL)
+	pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", "README.md"}
+	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{newSourceDomains}, pagesArgs...)
 	defer teardown()
 
 	response, err := GetPageFromListener(t, httpListener, "new-source-test.gitlab.io", "/my/pages/project/")
diff --git a/app.go b/app.go
index eeefaeaf..25a87785 100644
--- a/app.go
+++ b/app.go
@@ -413,9 +413,14 @@ func (a *theApp) listenMetricsFD(wg *sync.WaitGroup, fd uintptr) {
 }
 
 func runApp(config appConfig) {
-	a := theApp{appConfig: config, domains: source.NewDomains(config)}
+	domains, err := source.NewDomains(config)
+	if err != nil {
+		log.WithError(err).Fatal("could not create domains config source")
+	}
+
+	a := theApp{appConfig: config, domains: domains}
 
-	err := logging.ConfigureLogging(a.LogFormat, a.LogVerbose)
+	err = logging.ConfigureLogging(a.LogFormat, a.LogVerbose)
 	if err != nil {
 		log.WithError(err).Fatal("Failed to initialize logging")
 	}
@@ -451,6 +456,6 @@ func runApp(config appConfig) {
 }
 
 // fatal will log a fatal error and exit.
-func fatal(err error) {
-	log.WithError(err).Fatal()
+func fatal(err error, message string) {
+	log.WithError(err).Fatal(message)
 }
diff --git a/daemon.go b/daemon.go
index cc9d3217..9142642c 100644
--- a/daemon.go
+++ b/daemon.go
@@ -34,7 +34,7 @@ func daemonMain() {
 	// read the configuration from the pipe "ExtraFiles"
 	var config appConfig
 	if err := json.NewDecoder(os.NewFile(3, "options")).Decode(&config); err != nil {
-		fatal(err)
+		fatal(err, "could not decode app config")
 	}
 	runApp(config)
 	os.Exit(0)
diff --git a/helpers.go b/helpers.go
index b287dc36..d71f2b2c 100644
--- a/helpers.go
+++ b/helpers.go
@@ -11,7 +11,7 @@ import (
 func readFile(file string) (result []byte) {
 	result, err := ioutil.ReadFile(file)
 	if err != nil {
-		fatal(err)
+		fatal(err, "could not read file")
 	}
 	return
 }
@@ -21,7 +21,7 @@ func readFile(file string) (result []byte) {
 func createSocket(addr string) (net.Listener, *os.File) {
 	l, err := net.Listen("tcp", addr)
 	if err != nil {
-		fatal(err)
+		fatal(err, "could not create socket")
 	}
 
 	return l, fileForListener(l)
@@ -34,7 +34,7 @@ func fileForListener(l net.Listener) *os.File {
 
 	f, err := l.(filer).File()
 	if err != nil {
-		fatal(err)
+		fatal(err, "could not find file for listener")
 	}
 
 	return f
@@ -42,5 +42,5 @@ func fileForListener(l net.Listener) *os.File {
 
 func capturingFatal(err error, fields ...errortracking.CaptureOption) {
 	errortracking.Capture(err, fields...)
-	fatal(err)
+	fatal(err, "capturing fatal")
 }
diff --git a/internal/source/domains.go b/internal/source/domains.go
index f47884ef..85d06152 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -36,11 +36,20 @@ 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 {
+func NewDomains(config Config) (*Domains, error) {
+	if len(config.GitlabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 {
+		return &Domains{disk: disk.New()}, nil
+	}
+
+	gitlab, err := gitlab.New(config)
+	if err != nil {
+		return nil, err
+	}
+
 	return &Domains{
+		gitlab: gitlab,
 		disk:   disk.New(),
-		gitlab: gitlab.New(config),
-	}
+	}, nil
 }
 
 // GetDomain retrieves a domain information from a source. We are using two
@@ -69,6 +78,10 @@ func (d *Domains) IsReady() bool {
 }
 
 func (d *Domains) source(domain string) Source {
+	if d.gitlab == nil {
+		return d.disk
+	}
+
 	for _, name := range newSourceDomains {
 		if domain == name {
 			return d.gitlab
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index 6854c359..378f8c89 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -3,14 +3,44 @@ package source
 import (
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
 )
 
-func TestHasDomain(t *testing.T) {
+type sourceConfig struct {
+	api    string
+	secret string
+}
+
+func (c sourceConfig) GitlabServerURL() string {
+	return c.api
+}
+
+func (c sourceConfig) GitlabAPISecret() []byte {
+	return []byte(c.secret)
+}
+
+func TestDomainSources(t *testing.T) {
+	t.Run("when GitLab API URL has been provided", func(t *testing.T) {
+		domains, err := NewDomains(sourceConfig{api: "https://gitlab.com", secret: "abc"})
+		require.NoError(t, err)
+
+		require.NotNil(t, domains.gitlab)
+		require.NotNil(t, domains.disk)
+	})
+
+	t.Run("when GitLab API has not been provided", func(t *testing.T) {
+		domains, err := NewDomains(sourceConfig{})
+		require.NoError(t, err)
+
+		require.Nil(t, domains.gitlab)
+		require.NotNil(t, domains.disk)
+	})
+}
+
+func TestGetDomain(t *testing.T) {
 	newSourceDomains = []string{"new-source-test.gitlab.io"}
 	brokenSourceDomain = "pages-broken-poc.gitlab.io"
 
@@ -43,7 +73,7 @@ func TestHasDomain(t *testing.T) {
 		domain, err := domains.GetDomain("domain.test.io")
 
 		require.NoError(t, err)
-		assert.Nil(t, domain)
+		require.Nil(t, domain)
 	})
 
 	t.Run("when requesting a broken test domain", func(t *testing.T) {
@@ -57,7 +87,17 @@ func TestHasDomain(t *testing.T) {
 
 		domain, err := domains.GetDomain("pages-broken-poc.gitlab.io")
 
-		assert.Nil(t, domain)
-		assert.EqualError(t, err, "broken test domain used")
+		require.Nil(t, domain)
+		require.EqualError(t, err, "broken test domain used")
+	})
+
+	t.Run("when requesting a test domain in case of the source not being fully configured", func(t *testing.T) {
+		domains, err := NewDomains(sourceConfig{})
+		require.NoError(t, err)
+
+		domain, err := domains.GetDomain("new-source-test.gitlab.io")
+
+		require.Nil(t, domain)
+		require.NoError(t, err)
 	})
 }
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 6c9327dc..1c3024b7 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -9,8 +9,6 @@ import (
 
 	jwt "github.com/dgrijalva/jwt-go"
 
-	"gitlab.com/gitlab-org/labkit/log"
-
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
@@ -35,10 +33,14 @@ var connectionTimeout = 10 * time.Second
 
 // NewClient initializes and returns new Client baseUrl is
 // appConfig.GitLabServer secretKey is appConfig.GitLabAPISecretKey
-func NewClient(baseURL string, secretKey []byte) *Client {
+func NewClient(baseURL string, secretKey []byte) (*Client, error) {
+	if len(baseURL) == 0 || len(secretKey) == 0 {
+		return nil, errors.New("GitLab API URL or API secret has not been provided")
+	}
+
 	url, err := url.Parse(baseURL)
 	if err != nil {
-		log.WithError(err).Fatal("could not parse GitLab server URL")
+		return nil, err
 	}
 
 	return &Client{
@@ -48,11 +50,11 @@ func NewClient(baseURL string, secretKey []byte) *Client {
 			Timeout:   connectionTimeout,
 			Transport: httptransport.Transport,
 		},
-	}
+	}, nil
 }
 
 // NewFromConfig creates a new client from Config struct
-func NewFromConfig(config Config) *Client {
+func NewFromConfig(config Config) (*Client, error) {
 	return NewClient(config.GitlabServerURL(), config.GitlabAPISecret())
 }
 
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index f3ce4fa6..d056f2bd 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -16,6 +16,34 @@ var (
 	encodedSecret = "e41rcFh7XBA7sNABWVCe2AZvxMsy6QDtJ8S9Ql1UiN8=" // 32 bytes, base64 encoded
 )
 
+func TestNewValidBaseURL(t *testing.T) {
+	_, err := NewClient("https://gitlab.com", secretKey())
+	require.NoError(t, err)
+}
+
+func TestNewInvalidBaseURL(t *testing.T) {
+	t.Run("when API URL is not valid", func(t *testing.T) {
+		client, err := NewClient("%", secretKey())
+
+		require.Error(t, err)
+		require.Nil(t, client)
+	})
+
+	t.Run("when API URL is empty", func(t *testing.T) {
+		client, err := NewClient("", secretKey())
+
+		require.Nil(t, client)
+		require.EqualError(t, err, "GitLab API URL or API secret has not been provided")
+	})
+
+	t.Run("when API secret is empty", func(t *testing.T) {
+		client, err := NewClient("https://gitlab.com", []byte{})
+
+		require.Nil(t, client)
+		require.EqualError(t, err, "GitLab API URL or API secret has not been provided")
+	})
+}
+
 func TestGetVirtualDomainForErrorResponses(t *testing.T) {
 	tests := map[int]string{
 		http.StatusNoContent:    "No Content",
@@ -35,7 +63,8 @@ func TestGetVirtualDomainForErrorResponses(t *testing.T) {
 			server := httptest.NewServer(mux)
 			defer server.Close()
 
-			client := NewClient(server.URL, secretKey())
+			client, err := NewClient(server.URL, secretKey())
+			require.NoError(t, err)
 
 			actual, err := client.GetVirtualDomain("group.gitlab.io")
 
@@ -78,7 +107,8 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) {
 	server := httptest.NewServer(mux)
 	defer server.Close()
 
-	client := NewClient(server.URL, secretKey())
+	client, err := NewClient(server.URL, secretKey())
+	require.NoError(t, err)
 
 	actual, err := client.GetVirtualDomain("group.gitlab.io")
 	require.NoError(t, err)
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 7cb88e42..a2911e6b 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -20,8 +20,13 @@ type Gitlab struct {
 }
 
 // New returns a new instance of gitlab domain source.
-func New(config client.Config) *Gitlab {
-	return &Gitlab{client: client.NewFromConfig(config), cache: cache.New()}
+func New(config client.Config) (*Gitlab, error) {
+	client, err := client.NewFromConfig(config)
+	if err != nil {
+		return nil, err
+	}
+
+	return &Gitlab{client: client, cache: cache.New()}, nil
 }
 
 // GetDomain return a representation of a domain that we have fetched from
diff --git a/main.go b/main.go
index dc2d7873..aed0cc27 100644
--- a/main.go
+++ b/main.go
@@ -57,7 +57,7 @@ var (
 	secret                 = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long.")
 	gitLabAuthServer       = flag.String("auth-server", "", "DEPRECATED, use gitlab-server instead. GitLab server, for example https://www.gitlab.com")
 	gitLabServer           = flag.String("gitlab-server", "", "GitLab server, for example https://www.gitlab.com")
-	gitLabAPISecretKey     = flag.String("api-secret-key", "", "File with secret key used to authenticate with the GitLab API (NOT YET IMPLEMENTED)")
+	gitLabAPISecretKey     = flag.String("api-secret-key", "", "File with secret key used to authenticate with the GitLab API")
 	clientID               = flag.String("auth-client-id", "", "GitLab application Client ID")
 	clientSecret           = flag.String("auth-client-secret", "", "GitLab application Client Secret")
 	redirectURI            = flag.String("auth-redirect-uri", "", "GitLab application redirect URI")
@@ -243,10 +243,6 @@ func loadConfig() appConfig {
 		"auth-redirect-uri":             config.RedirectURI,
 	}).Debug("Start daemon with configuration")
 
-	if *gitLabAPISecretKey != "" {
-		log.Warn("api-secret-key parameter is a placeholder for future developments, this option will be ignored.")
-	}
-
 	return config
 }
 
@@ -256,7 +252,7 @@ func appMain() {
 	flag.String(flag.DefaultConfigFlagname, "", "path to config file")
 	flag.Parse()
 	if err := tlsconfig.ValidateTLSVersions(*tlsMinVersion, *tlsMaxVersion); err != nil {
-		fatal(err)
+		fatal(err, "invalid TLS version")
 	}
 
 	printVersion(*showVersion, VERSION)
@@ -273,7 +269,7 @@ func appMain() {
 	log.Printf("URL: https://gitlab.com/gitlab-org/gitlab-pages")
 
 	if err := os.Chdir(*pagesRoot); err != nil {
-		fatal(err)
+		fatal(err, "could not change directory into pagesRoot")
 	}
 
 	config := loadConfig()
@@ -288,7 +284,7 @@ func appMain() {
 	if *daemonUID != 0 || *daemonGID != 0 {
 		if err := daemonize(config, *daemonUID, *daemonGID, *daemonInplaceChroot); err != nil {
 			errortracking.Capture(err)
-			fatal(err)
+			fatal(err, "could not create pages daemon")
 		}
 
 		return