Use config package in GitLab client

Uses the `internal/config/` client inside the `internal/source/gitlab/`
package which makes it easier to extend the configuration.

This is an iteration of https://gitlab.com/gitlab-org/gitlab-pages/-/issues/543.

Changelog: other

6 files changed, 58 insertions, 105 deletions

diff --git a/internal/source/config.go b/internal/source/config.go
deleted file mode 100644
index 9cf87bc6..00000000
--- a/internal/source/config.go
+++ /dev/null
@@ -1,7 +0,0 @@
-package source
-
-import "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
-
-// Config represents an interface that is configuration provider for client
-// capable of comunicating with GitLab
-type Config client.Config
diff --git a/internal/source/domains.go b/internal/source/domains.go
index 5254c3b8..7507938a 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -6,6 +6,7 @@ import (
 
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab"
@@ -40,9 +41,9 @@ 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) {
+func NewDomains(source string, cfg *config.GitLab) (*Domains, error) {
 	domains := &Domains{}
-	if err := domains.setConfigSource(config); err != nil {
+	if err := domains.setConfigSource(source, cfg); err != nil {
 		return nil, err
 	}
 
@@ -52,33 +53,33 @@ func NewDomains(config Config) (*Domains, error) {
 // setConfigSource and initialize gitlab source
 // returns error if -domain-config-source is not valid
 // returns error if -domain-config-source=gitlab and init fails
-func (d *Domains) setConfigSource(config Config) error {
-	switch config.DomainConfigSource() {
+func (d *Domains) setConfigSource(source string, cfg *config.GitLab) error {
+	switch source {
 	case "gitlab":
 		d.configSource = sourceGitlab
-		return d.setGitLabClient(config)
+		return d.setGitLabClient(cfg)
 	case "auto":
 		d.configSource = sourceAuto
 		// enable disk for auto for now
 		d.disk = disk.New()
-		return d.setGitLabClient(config)
+		return d.setGitLabClient(cfg)
 	case "disk":
 		// TODO: disable domains.disk https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382
 		d.configSource = sourceDisk
 		d.disk = disk.New()
 	default:
-		return fmt.Errorf("invalid option for -domain-config-source: %q", config.DomainConfigSource())
+		return fmt.Errorf("invalid option for -domain-config-source: %q", source)
 	}
 
 	return nil
 }
 
 // setGitLabClient when domain-config-source is `gitlab` or `auto`, only return error for `gitlab` source
-func (d *Domains) setGitLabClient(config Config) error {
+func (d *Domains) setGitLabClient(cfg *config.GitLab) error {
 	// We want to notify users about any API issues
 	// Creating a glClient will start polling connectivity in the background
 	// and spam errors in log
-	glClient, err := gitlab.New(config)
+	glClient, err := gitlab.New(cfg)
 	if err != nil {
 		if d.configSource == sourceGitlab {
 			return err
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index db9c2c23..a9404eef 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -11,105 +11,92 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
 )
 
-type sourceConfig struct {
-	api          string
-	secret       string
-	domainSource string
-}
-
-func (c sourceConfig) InternalGitLabServerURL() string {
-	return c.api
-}
-
-func (c sourceConfig) GitlabAPISecret() []byte {
-	return []byte(c.secret)
-}
-func (c sourceConfig) GitlabClientConnectionTimeout() time.Duration {
-	return 10 * time.Second
-}
-
-func (c sourceConfig) GitlabJWTTokenExpiry() time.Duration {
-	return 30 * time.Second
-}
-
-func (c sourceConfig) DomainConfigSource() string {
-	return c.domainSource
-}
-func (c sourceConfig) Cache() *config.Cache {
-	return &config.Cache{
-		CacheExpiry:          10 * time.Minute,
-		CacheCleanupInterval: time.Minute,
-		EntryRefreshTimeout:  60 * time.Second,
-		RetrievalTimeout:     30 * time.Second,
-		MaxRetrievalInterval: time.Second,
-		MaxRetrievalRetries:  3,
+func TestNewDomains(t *testing.T) {
+	validCfg := config.GitLab{
+		InternalServer:     "https://gitlab.com",
+		APISecretKey:       []byte("abc"),
+		ClientHTTPTimeout:  time.Second,
+		JWTTokenExpiration: time.Second,
 	}
-}
 
-func TestNewDomains(t *testing.T) {
 	tests := []struct {
 		name            string
-		sourceConfig    sourceConfig
+		source          string
+		config          config.GitLab
 		expectedErr     string
 		expectGitlabNil bool
 		expectDiskNil   bool
 	}{
 		{
-			name:         "no_source_config",
-			sourceConfig: sourceConfig{},
-			expectedErr:  "invalid option for -domain-config-source: \"\"",
+			name:        "no_source_config",
+			source:      "",
+			expectedErr: "invalid option for -domain-config-source: \"\"",
 		},
 		{
-			name:         "invalid_source_config",
-			sourceConfig: sourceConfig{domainSource: "invalid"},
-			expectedErr:  "invalid option for -domain-config-source: \"invalid\"",
+			name:        "invalid_source_config",
+			source:      "invalid",
+			expectedErr: "invalid option for -domain-config-source: \"invalid\"",
 		},
 		{
 			name:            "disk_source",
-			sourceConfig:    sourceConfig{domainSource: "disk"},
+			source:          "disk",
 			expectGitlabNil: true,
 			expectDiskNil:   false,
 		},
 		{
 			name:            "auto_without_api_config",
-			sourceConfig:    sourceConfig{domainSource: "auto"},
+			source:          "auto",
 			expectGitlabNil: true,
 			expectDiskNil:   false,
 		},
 		{
 			name:            "auto_with_api_config",
-			sourceConfig:    sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"},
+			source:          "auto",
+			config:          validCfg,
 			expectGitlabNil: false,
 			expectDiskNil:   false,
 		},
 		{
 			name:          "gitlab_source_success",
-			sourceConfig:  sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"},
+			source:        "gitlab",
+			config:        validCfg,
 			expectDiskNil: true,
 		},
 		{
-			name:         "gitlab_source_no_url",
-			sourceConfig: sourceConfig{api: "", secret: "abc", domainSource: "gitlab"},
-			expectedErr:  "GitLab API URL or API secret has not been provided",
+			name:   "gitlab_source_no_url",
+			source: "gitlab",
+			config: func() config.GitLab {
+				cfg := validCfg
+				cfg.InternalServer = ""
+
+				return cfg
+			}(),
+			expectedErr: "GitLab API URL or API secret has not been provided",
 		},
 		{
-			name:         "gitlab_source_no_secret",
-			sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "", domainSource: "gitlab"},
-			expectedErr:  "GitLab API URL or API secret has not been provided",
+			name:   "gitlab_source_no_secret",
+			source: "gitlab",
+			config: func() config.GitLab {
+				cfg := validCfg
+				cfg.APISecretKey = []byte{}
+
+				return cfg
+			}(),
+			expectedErr: "GitLab API URL or API secret has not been provided",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			domains, err := NewDomains(tt.sourceConfig)
+			domains, err := NewDomains(tt.source, &tt.config)
 			if tt.expectedErr != "" {
 				require.EqualError(t, err, tt.expectedErr)
 				return
 			}
 			require.NoError(t, err)
 
-			require.Equal(t, tt.expectGitlabNil, domains.gitlab == nil)
-			require.Equal(t, tt.expectDiskNil, domains.disk == nil)
+			require.Equal(t, tt.expectGitlabNil, domains.gitlab == nil, "mismatch gitlab nil")
+			require.Equal(t, tt.expectDiskNil, domains.disk == nil, "mismatch disk nil")
 		})
 	}
 }
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 99d53fb7..182b11ae 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -15,6 +15,7 @@ import (
 
 	"gitlab.com/gitlab-org/labkit/correlation"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
@@ -79,8 +80,8 @@ func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpi
 }
 
 // NewFromConfig creates a new client from Config struct
-func NewFromConfig(config Config) (*Client, error) {
-	return NewClient(config.InternalGitLabServerURL(), config.GitlabAPISecret(), config.GitlabClientConnectionTimeout(), config.GitlabJWTTokenExpiry())
+func NewFromConfig(cfg *config.GitLab) (*Client, error) {
+	return NewClient(cfg.InternalServer, cfg.APISecretKey, cfg.ClientHTTPTimeout, cfg.JWTTokenExpiration)
 }
 
 // Resolve returns a VirtualDomain configuration wrapped into a Lookup for a
diff --git a/internal/source/gitlab/client/config.go b/internal/source/gitlab/client/config.go
deleted file mode 100644
index 76e85cc4..00000000
--- a/internal/source/gitlab/client/config.go
+++ /dev/null
@@ -1,18 +0,0 @@
-package client
-
-import (
-	"time"
-
-	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
-)
-
-// Config represents an interface that is configuration provider for client
-// capable of comunicating with GitLab
-type Config interface {
-	InternalGitLabServerURL() string
-	GitlabAPISecret() []byte
-	GitlabClientConnectionTimeout() time.Duration
-	GitlabJWTTokenExpiry() time.Duration
-	DomainConfigSource() string
-	Cache() *config.Cache
-}
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 7acad679..fdcbd88f 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -11,6 +11,7 @@ import (
 	"github.com/cenkalti/backoff/v4"
 	"gitlab.com/gitlab-org/labkit/log"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
@@ -19,8 +20,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
-var errCacheNotConfigured = errors.New("cache not configured")
-
 // Gitlab source represent a new domains configuration source. We fetch all the
 // information about domains from GitLab instance.
 type Gitlab struct {
@@ -30,24 +29,14 @@ type Gitlab struct {
 }
 
 // New returns a new instance of gitlab domain source.
-func New(config client.Config) (*Gitlab, error) {
-	client, err := client.NewFromConfig(config)
-	if err != nil {
-		return nil, err
-	}
-
-	cc := config.Cache()
-	if cc == nil {
-		return nil, errCacheNotConfigured
-	}
-
-	cachedClient := cache.NewCache(client, cc)
+func New(cfg *config.GitLab) (*Gitlab, error) {
+	glClient, err := client.NewFromConfig(cfg)
 	if err != nil {
 		return nil, err
 	}
 
 	g := &Gitlab{
-		client: cachedClient,
+		client: cache.NewCache(glClient, &cfg.Cache),
 	}
 
 	go g.poll(backoff.DefaultInitialInterval, maxPollingTime)