Merge branch 'remove-disk-new-for-gitlab-source' into 'master'

Fully support  `domain-config-source=gitlab`

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

5 files changed, 121 insertions, 36 deletions

diff --git a/acceptance_test.go b/acceptance_test.go
index abcf3592..1fae9c8f 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -1708,36 +1708,101 @@ func TestTLSVersions(t *testing.T) {
 	}
 }
 
-func TestGitlabDomainsSource(t *testing.T) {
+func TestDomainsSource(t *testing.T) {
 	skipUnlessEnabled(t)
 
-	source := NewGitlabDomainsSourceStub(t)
-	defer source.Close()
-
-	gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t)
+	type args struct {
+		configSource string
+		domain       string
+		urlSuffix    string
+	}
+	type want struct {
+		statusCode int
+		content    string
+		apiCalled  bool
+	}
+	tests := []struct {
+		name string
+		args args
+		want want
+	}{
+		{
+			name: "gitlab_source_domain_exists",
+			args: args{
+				configSource: "gitlab",
+				domain:       "new-source-test.gitlab.io",
+				urlSuffix:    "/my/pages/project/",
+			},
+			want: want{
+				statusCode: http.StatusOK,
+				content:    "New Pages GitLab Source TEST OK\n",
+				apiCalled:  true,
+			},
+		},
+		{
+			name: "gitlab_source_domain_does_not_exist",
+			args: args{
+				configSource: "gitlab",
+				domain:       "non-existent-domain.gitlab.io",
+			},
+			want: want{
+				statusCode: http.StatusNotFound,
+				apiCalled:  true,
+			},
+		},
+		{
+			name: "disk_source_domain_exists",
+			args: args{
+				configSource: "disk",
+				// test.domain.com sourced from disk configuration
+				domain:    "test.domain.com",
+				urlSuffix: "/",
+			},
+			want: want{
+				statusCode: http.StatusOK,
+				content:    "main-dir\n",
+				apiCalled:  false,
+			},
+		},
+		{
+			name: "disk_source_domain_does_not_exist",
+			args: args{
+				configSource: "disk",
+				domain:       "non-existent-domain.gitlab.io",
+			},
+			want: want{
+				statusCode: http.StatusNotFound,
+				apiCalled:  false,
+			},
+		},
+		// TODO: modify mock so we can test domain-config-source=auto when API/disk is not ready https://gitlab.com/gitlab-org/gitlab/-/issues/218358
+	}
 
-	pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "gitlab"}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var apiCalled bool
+			source := NewGitlabDomainsSourceStub(t, &apiCalled)
+			defer source.Close()
 
-	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...)
-	defer teardown()
+			gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t)
 
-	t.Run("when a domain exists", func(t *testing.T) {
-		response, err := GetPageFromListener(t, httpListener, "new-source-test.gitlab.io", "/my/pages/project/")
-		require.NoError(t, err)
+			pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", tt.args.configSource}
+			teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...)
+			defer teardown()
 
-		defer response.Body.Close()
-		body, err := ioutil.ReadAll(response.Body)
-		require.NoError(t, err)
+			response, err := GetPageFromListener(t, httpListener, tt.args.domain, tt.args.urlSuffix)
+			require.NoError(t, err)
 
-		require.Equal(t, http.StatusOK, response.StatusCode)
-		require.Equal(t, "New Pages GitLab Source TEST OK\n", string(body))
-	})
+			require.Equal(t, tt.want.statusCode, response.StatusCode)
+			if tt.want.statusCode == http.StatusOK {
+				defer response.Body.Close()
+				body, err := ioutil.ReadAll(response.Body)
+				require.NoError(t, err)
 
-	t.Run("when a domain does not exists", func(t *testing.T) {
-		response, err := GetPageFromListener(t, httpListener, "non-existent-domain.gitlab.io", "/path")
-		defer response.Body.Close()
-		require.NoError(t, err)
+				require.Equal(t, tt.want.content, string(body), "content mismatch")
+			}
 
-		require.Equal(t, http.StatusNotFound, response.StatusCode)
-	})
+			require.Equal(t, tt.want.apiCalled, apiCalled, "api called mismatch")
+		})
+	}
 }
diff --git a/helpers_test.go b/helpers_test.go
index a55399f4..926fc372 100644
--- a/helpers_test.go
+++ b/helpers_test.go
@@ -420,13 +420,15 @@ func waitForRoundtrips(t *testing.T, listeners []ListenSpec, timeout time.Durati
 	require.Equal(t, len(listeners), nListening, "all listeners must be accepting TCP connections")
 }
 
-func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server {
+func NewGitlabDomainsSourceStub(t *testing.T, apiCalled *bool) *httptest.Server {
+	*apiCalled = false
 	mux := http.NewServeMux()
 	mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusNoContent)
 	})
 
 	handler := func(w http.ResponseWriter, r *http.Request) {
+		*apiCalled = true
 		domain := r.URL.Query().Get("host")
 		path := "shared/lookups/" + domain + ".json"
 
diff --git a/internal/source/domains.go b/internal/source/domains.go
index e4a582ea..2a7a317c 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -39,11 +39,7 @@ type Domains struct {
 // not initialize `dm` as we later check the readiness by comparing it with a
 // nil value.
 func NewDomains(config Config) (*Domains, error) {
-	domains := &Domains{
-		// TODO: disable domains.disk https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382
-		disk: disk.New(),
-	}
-
+	domains := &Domains{}
 	if err := domains.setConfigSource(config); err != nil {
 		return nil, err
 	}
@@ -59,15 +55,18 @@ func (d *Domains) setConfigSource(config Config) error {
 	// attach gitlab by default when source is not disk (auto, gitlab)
 	switch config.DomainConfigSource() {
 	case "gitlab":
-		// TODO:  https://gitlab.com/gitlab-org/gitlab/-/issues/218357
 		d.configSource = sourceGitlab
 		return d.setGitLabClient(config)
 	case "auto":
 		// TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358
 		d.configSource = sourceAuto
+		// enable disk for auto for now
+		d.disk = disk.New()
 		return d.setGitLabClient(config)
 	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())
 	}
@@ -107,14 +106,27 @@ func (d *Domains) GetDomain(name string) (*domain.Domain, error) {
 // Read starts the disk domain source. It is DEPRECATED, because we want to
 // remove it entirely when disk source gets removed.
 func (d *Domains) Read(rootDomain string) {
-	d.disk.Read(rootDomain)
+	// start disk.Read for sourceDisk and sourceAuto
+	if d.configSource != sourceGitlab {
+		d.disk.Read(rootDomain)
+	}
 }
 
 // IsReady checks if the disk domain source managed to traverse entire pages
 // filesystem and is ready for use. It is DEPRECATED, because we want to remove
 // it entirely when disk source gets removed.
 func (d *Domains) IsReady() bool {
-	return d.disk.IsReady()
+	switch d.configSource {
+	case sourceGitlab:
+		return d.gitlab.IsReady()
+	case sourceDisk:
+		return d.disk.IsReady()
+	case sourceAuto:
+		// TODO: implement auto https://gitlab.com/gitlab-org/gitlab/-/issues/218358, default to disk for now
+		return d.disk.IsReady()
+	}
+
+	return false
 }
 
 func (d *Domains) source(domain string) Source {
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index 173fc1d3..36c53e6f 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -41,6 +41,7 @@ func TestNewDomains(t *testing.T) {
 		sourceConfig    sourceConfig
 		expectedErr     string
 		expectGitlabNil bool
+		expectDiskNil   bool
 	}{
 		{
 			name:         "no_source_config",
@@ -56,20 +57,25 @@ func TestNewDomains(t *testing.T) {
 			name:            "disk_source",
 			sourceConfig:    sourceConfig{domainSource: "disk"},
 			expectGitlabNil: true,
+			expectDiskNil:   false,
 		},
 		{
+			// TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218358
 			name:            "auto_without_api_config",
 			sourceConfig:    sourceConfig{domainSource: "auto"},
 			expectGitlabNil: true,
+			expectDiskNil:   false,
 		},
 		{
 			name:            "auto_with_api_config",
 			sourceConfig:    sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"},
 			expectGitlabNil: false,
+			expectDiskNil:   false,
 		},
 		{
-			name:         "gitlab_source_success",
-			sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"},
+			name:          "gitlab_source_success",
+			sourceConfig:  sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"},
+			expectDiskNil: true,
 		},
 		{
 			name:         "gitlab_source_no_url",
@@ -93,7 +99,7 @@ func TestNewDomains(t *testing.T) {
 			require.NoError(t, err)
 
 			require.Equal(t, tt.expectGitlabNil, domains.gitlab == nil)
-			require.NotNil(t, domains.disk)
+			require.Equal(t, tt.expectDiskNil, domains.disk == nil)
 		})
 	}
 }
diff --git a/internal/source/gitlab/gitlab_poll.go b/internal/source/gitlab/gitlab_poll.go
index bc1611ef..0284449a 100644
--- a/internal/source/gitlab/gitlab_poll.go
+++ b/internal/source/gitlab/gitlab_poll.go
@@ -14,7 +14,7 @@ const (
 
 // Poll tries to call the /internal/pages/status API endpoint once plus
 // for `maxElapsedTime`
-// TODO: Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/218357
+// TODO: Remove in https://gitlab.com/gitlab-org/gitlab-pages/-/issues/449
 func (g *Gitlab) poll(interval, maxElapsedTime time.Duration) {
 	backOff := backoff.NewExponentialBackOff()
 	backOff.InitialInterval = interval