diff options
author | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2019-12-05 14:49:15 +0300 |
---|---|---|
committer | Grzegorz Bizon <grzesiek.bizon@gmail.com> | 2019-12-05 14:49:15 +0300 |
commit | 70634f8693394cca63bfe84d63888992f756ad5a (patch) | |
tree | 5e8078af69febf318ef6330b6c1aadaf685c6db8 | |
parent | cafc4369750e608ac4be5b61240e90265fdcb5a5 (diff) | |
parent | 7f35a7b7c1dde36f695fd7f1627fa77d9d8d2be0 (diff) |
Merge branch 'master' into feature/gb/gitlab-domains-source
* master:
Check presence of GitLab API secret when building a domains source
Make GitLab API Secret a supported parameter
Improve error reporting in the main package
Check if GitLab API secret has been provided too
Avoid using `testify/assert` in favor of `require`
Test domains source not fully configured
Improve error handing when creating new domains config source
Fix formatting in internal/source/gitlab/client/client_test.go
Conflicts:
acceptance_test.go
internal/source/gitlab/client/client_test.go
-rw-r--r-- | acceptance_test.go | 3 | ||||
-rw-r--r-- | app.go | 13 | ||||
-rw-r--r-- | daemon.go | 2 | ||||
-rw-r--r-- | helpers.go | 8 | ||||
-rw-r--r-- | internal/source/domains.go | 19 | ||||
-rw-r--r-- | internal/source/domains_test.go | 50 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 14 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_test.go | 40 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 9 | ||||
-rw-r--r-- | main.go | 12 |
10 files changed, 131 insertions, 39 deletions
diff --git a/acceptance_test.go b/acceptance_test.go index 2598f404..af6d86bc 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1535,7 +1535,8 @@ func TestGitlabDomainsSource(t *testing.T) { defer source.Close() newSourceDomains := "GITLAB_NEW_SOURCE_DOMAINS=new-source-test.gitlab.io,non-existent-domain.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() t.Run("when a domain exists", func(t *testing.T) { @@ -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) } @@ -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) @@ -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 382cc412..a230e529 100644 --- a/internal/source/gitlab/client/client.go +++ b/internal/source/gitlab/client/client.go @@ -10,8 +10,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" ) @@ -36,10 +34,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{ @@ -49,11 +51,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 f91f241a..df4d080f 100644 --- a/internal/source/gitlab/client/client_test.go +++ b/internal/source/gitlab/client/client_test.go @@ -18,7 +18,35 @@ var ( encodedSecret = "e41rcFh7XBA7sNABWVCe2AZvxMsy6QDtJ8S9Ql1UiN8=" // 32 bytes, base64 encoded ) -func TestGetLookupForErrorResponses(t *testing.T) { +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 TestLookupForErrorResponses(t *testing.T) { tests := map[int]string{ http.StatusNoContent: "No Content", http.StatusUnauthorized: "Unauthorized", @@ -37,7 +65,8 @@ func TestGetLookupForErrorResponses(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) lookup := client.GetLookup(context.Background(), "group.gitlab.io") @@ -65,8 +94,8 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) { "access_control": false, "source": { "type": "file", - "path": "mygroup/myproject/public/" - }, + "path": "mygroup/myproject/public/" + }, "https_only": true, "prefix": "/myproject/" } @@ -80,7 +109,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) lookup := client.GetLookup(context.Background(), "group.gitlab.io") require.NoError(t, lookup.Error) diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 44e42f14..88b4004b 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -22,8 +22,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 @@ -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 |