Move polling out of the client

Let the `package gitlab` handle the polling instead of the client.

10 files changed, 52 insertions, 64 deletions

diff --git a/internal/source/domains.go b/internal/source/domains.go
index 9bb5f38e..16ba683c 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -13,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/domains/gitlabsourceconfig"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 var (
@@ -47,33 +46,36 @@ func NewDomains(config Config) (*Domains, error) {
 	// TODO: choose domain source config via config.DomainConfigSource()
 	// https://gitlab.com/gitlab-org/gitlab/-/issues/217912
 
-	domains := &Domains{
+	d := &Domains{
 		mu:   &sync.RWMutex{},
 		disk: disk.New(),
 	}
 
 	if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 {
-		return domains, nil
+		return d, nil
 	}
 
 	gitlabClient, err := gitlab.New(config)
 	if err != nil {
 		return nil, err
 	}
-	gitlabErr := make(chan error)
-	gitlabClient.Poll(client.DefaultPollingMaxRetries, client.DefaultPollingInterval, gitlabErr)
 
-	domains.enableGitLabSource(gitlabClient)
+	d.enableGitLabSource(gitlabClient)
+
+	return d, nil
+}
+
+func (d *Domains) pollGitLabAPI(gitlabClient *gitlab.Gitlab) {
+	gitlabErr := make(chan error)
+	go gitlabClient.Poll(gitlab.DefaultPollingMaxRetries, gitlab.DefaultPollingInterval, gitlabErr)
 
 	go func() {
 		err := <-gitlabErr
 		if err != nil {
 			log.WithError(err).Error("failed to connect to the GitLab API")
-			domains.disableGitLabSource()
+			d.disableGitLabSource()
 		}
 	}()
-
-	return domains, nil
 }
 
 func (d *Domains) enableGitLabSource(gitlabClient *gitlab.Gitlab) {
@@ -81,6 +83,8 @@ func (d *Domains) enableGitLabSource(gitlabClient *gitlab.Gitlab) {
 	defer d.mu.Unlock()
 
 	d.gitlab = gitlabClient
+
+	d.pollGitLabAPI(gitlabClient)
 }
 
 func (d *Domains) disableGitLabSource() {
diff --git a/internal/source/gitlab/api/client.go b/internal/source/gitlab/api/client.go
index 4a6c0893..00c14a05 100644
--- a/internal/source/gitlab/api/client.go
+++ b/internal/source/gitlab/api/client.go
@@ -2,13 +2,12 @@ package api
 
 import (
 	"context"
-	"time"
 )
 
 // Client represents an interface we use to retrieve information from GitLab
 type Client interface {
-	// GetLookup retrives an VirtualDomain from GitLab API and wraps it into Lookup
+	// GetLookup retrieves a VirtualDomain from the GitLab API and wraps it into Lookup
 	GetLookup(ctx context.Context, domain string) Lookup
-
-	Poll(retries int, interval time.Duration, errCh chan error)
+	// Status checks that Pages can reach the rails internal Pages API for source domain configuration.
+	Status() error
 }
diff --git a/internal/source/gitlab/api/resolver.go b/internal/source/gitlab/api/resolver.go
index 78cbc20b..2285e1a0 100644
--- a/internal/source/gitlab/api/resolver.go
+++ b/internal/source/gitlab/api/resolver.go
@@ -2,14 +2,13 @@ package api
 
 import (
 	"context"
-	"time"
 )
 
 // Resolver represents an interface we use to retrieve information from GitLab
 // in a more generic way. It can be a concrete API client or cached client.
 type Resolver interface {
-	// Resolve retrieves an VirtualDomain from GitLab API and wraps it into Lookup
+	// Resolve retrieves a VirtualDomain from the GitLab API and wraps it into Lookup
 	Resolve(ctx context.Context, domain string) *Lookup
-	// Poll test
-	Poll(retries int, interval time.Duration, errCh chan error)
+	// Status checks that Pages can reach the rails internal Pages API for source domain configuration.
+	Status() error
 }
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index de91e8b6..e3bf85cd 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -110,6 +110,7 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
 	return entry.Retrieve(ctx, c.client)
 }
 
-func (c *Cache) Poll(retries int, interval time.Duration, errCh chan error) {
-	c.client.Poll(retries, interval, errCh)
+// Status returns the client Status
+func (c *Cache) Status() error {
+	return c.client.Status()
 }
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 27bd449a..c921e31a 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -69,7 +69,7 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	return lookup
 }
 
-func (c *client) Poll(int, time.Duration, chan error) {}
+func (c *client) Status() error { return nil }
 
 func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
 	var chanSize int
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 1d4cde9f..e06a87d1 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -109,13 +109,15 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 // for source domain configuration.
 // Timeout is the same as -gitlab-client-http-timeout
 func (gc *Client) Status() error {
-	// nolint: bodyclose
-	// this endpoint returns a http.StatusNoContent response
-	_, err := gc.get(context.Background(), "/api/v4/internal/pages/status", url.Values{})
+	res, err := gc.get(context.Background(), "/api/v4/internal/pages/status", url.Values{})
 	if err != nil {
 		return fmt.Errorf("%s: %v", ConnectionErrorMsg, err)
 	}
 
+	if res != nil && res.Body != nil {
+		res.Body.Close()
+	}
+
 	return nil
 }
 
diff --git a/internal/source/gitlab/client/client_stub.go b/internal/source/gitlab/client/client_stub.go
index c3879a33..d8e1196c 100644
--- a/internal/source/gitlab/client/client_stub.go
+++ b/internal/source/gitlab/client/client_stub.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"os"
-	"time"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
@@ -12,6 +11,7 @@ import (
 // StubClient is a stubbed client used for testing
 type StubClient struct {
 	File string
+	Err  func() error
 }
 
 // Resolve implements api.Resolver
@@ -37,6 +37,6 @@ func (c StubClient) GetLookup(ctx context.Context, host string) api.Lookup {
 	return lookup
 }
 
-func (c StubClient) Poll(r int, i time.Duration, errCh chan error) {
-	errCh <- nil
+func (c StubClient) Status() error {
+	return c.Err()
 }
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index c5095fb7..12da9af1 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -6,7 +6,6 @@ import (
 	"net/http"
 	"path"
 	"strings"
-	"time"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
@@ -95,7 +94,3 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) {
 	return &serving.Request{Serving: defaultServing()},
 		errors.New("could not match lookup path")
 }
-
-func (g *Gitlab) Poll(retries int, interval time.Duration, errCh chan error) {
-	go g.client.Poll(retries, interval, errCh)
-}
diff --git a/internal/source/gitlab/client/client_poll.go b/internal/source/gitlab/gitlab_poll.go
index cca7b731..d62ef5e1 100644
--- a/internal/source/gitlab/client/client_poll.go
+++ b/internal/source/gitlab/gitlab_poll.go
@@ -1,4 +1,4 @@
-package client
+package gitlab
 
 import (
 	"fmt"
@@ -11,20 +11,19 @@ const (
 	// DefaultPollingMaxRetries to be used by Poll
 	DefaultPollingMaxRetries = 10
 	// DefaultPollingInterval to be used by Poll
-	DefaultPollingInterval = 10 * time.Second
+	DefaultPollingInterval = 30 * time.Second
 )
 
 // Poll tries to call the /internal/pages/status API endpoint once plus
 // `retries` every `interval`.
-// TODO: should we consider using an exponential back-off approach?
-// https://pkg.go.dev/github.com/cenkalti/backoff/v4?tab=doc#pkg-examples
-func (gc *Client) Poll(retries int, interval time.Duration, errCh chan error) {
+// TODO: Remove in https://gitlab.com/gitlab-org/gitlab/-/issues/218357
+func (g *Gitlab) Poll(retries int, interval time.Duration, errCh chan error) {
 	defer close(errCh)
 
 	var err error
 	for i := 0; i <= retries; i++ {
 		log.Info("polling GitLab internal pages status API")
-		err = gc.Status()
+		err = g.client.Status()
 		if err == nil {
 			log.Info("GitLab internal pages status API connected successfully")
 
diff --git a/internal/source/gitlab/client/client_poll_test.go b/internal/source/gitlab/gitlab_poll_test.go
index 5bda4518..d5f93dea 100644
--- a/internal/source/gitlab/client/client_poll_test.go
+++ b/internal/source/gitlab/gitlab_poll_test.go
@@ -1,12 +1,13 @@
-package client
+package gitlab
 
 import (
-	"net/http"
-	"net/http/httptest"
+	"fmt"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 func TestClient_Poll(t *testing.T) {
@@ -14,76 +15,64 @@ func TestClient_Poll(t *testing.T) {
 		name     string
 		retries  int
 		interval time.Duration
-		timeout  time.Duration
-		status   int
 		wantErr  bool
 	}{
 		{
 			name:     "success_with_no_retry",
 			retries:  0,
 			interval: 5 * time.Millisecond,
-			timeout:  100 * time.Millisecond,
-			status:   http.StatusOK,
 			wantErr:  false,
 		},
 		{
 			name:     "success_after_N_retries",
 			retries:  3,
 			interval: 10 * time.Millisecond,
-			timeout:  100 * time.Millisecond,
-			status:   http.StatusOK,
 			wantErr:  false,
 		},
 		{
 			name:     "fail_with_no_retries",
 			retries:  0,
 			interval: 5 * time.Millisecond,
-			timeout:  100 * time.Millisecond,
-			status:   http.StatusUnauthorized,
 			wantErr:  true,
 		},
 		{
 			name:     "fail_after_N_retries",
 			retries:  3,
 			interval: 5 * time.Millisecond,
-			timeout:  100 * time.Millisecond,
-			status:   http.StatusUnauthorized,
 			wantErr:  true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			var counter int
-			mux := http.NewServeMux()
-			mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) {
+			clientMock := client.StubClient{Err: func() error {
+				if tt.wantErr {
+					return fmt.Errorf(client.ConnectionErrorMsg)
+				}
+
 				if counter < tt.retries {
 					counter++
-					// fail on purpose until we reach the max retry
-					w.WriteHeader(http.StatusInternalServerError)
-					return
+					return fmt.Errorf(client.ConnectionErrorMsg)
 				}
 
-				w.WriteHeader(tt.status)
-			})
-
-			server := httptest.NewServer(mux)
-			defer server.Close()
+				return nil
+			}}
 
-			client := defaultClient(t, server.URL)
 			errCh := make(chan error)
+			glClient := Gitlab{client: clientMock}
 
-			go client.Poll(tt.retries, tt.interval, errCh)
+			go glClient.Poll(tt.retries, tt.interval, errCh)
 
 			select {
 			case err := <-errCh:
 				if tt.wantErr {
 					require.Error(t, err)
 					require.Contains(t, err.Error(), "polling failed after")
-					require.Contains(t, err.Error(), ConnectionErrorMsg)
+					require.Contains(t, err.Error(), client.ConnectionErrorMsg)
 					return
 				}
 				require.NoError(t, err)
-			case <-time.After(tt.timeout):
+			case <-time.After(100 * time.Millisecond):
 				t.Logf("%s timed out", tt.name)
 				t.FailNow()
 			}