Merge branch 'remove/api-polling' into 'master'

refactor: remove GitLab API internal polling

Closes #449

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

14 files changed, 2 insertions, 399 deletions

diff --git a/go.mod b/go.mod
index 1dac1973..302dbfea 100644
--- a/go.mod
+++ b/go.mod
@@ -4,7 +4,6 @@ go 1.15
 
 require (
 	github.com/andybalholm/brotli v1.0.3
-	github.com/cenkalti/backoff/v4 v4.0.2
 	github.com/golang-jwt/jwt/v4 v4.0.0
 	github.com/golang/mock v1.3.1
 	github.com/golangci/golangci-lint v1.27.0
diff --git a/go.sum b/go.sum
index 5c60e5ee..a226d319 100644
--- a/go.sum
+++ b/go.sum
@@ -46,8 +46,6 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/bombsimon/wsl/v3 v3.0.0 h1:w9f49xQatuaeTJFaNP4SpiWSR5vfT6IstPtM62JjcqA=
 github.com/bombsimon/wsl/v3 v3.0.0/go.mod h1:st10JtZYLE4D5sC7b8xV4zTKZwAQjCH/Hy2Pm1FNZIc=
-github.com/cenkalti/backoff/v4 v4.0.2 h1:JIufpQLbh4DkbQoii76ItQIUFzevQSqOLZca4eamEDs=
-github.com/cenkalti/backoff/v4 v4.0.2/go.mod h1:eEew/i+1Q6OrCDZh3WiXYv3+nJwBASZ8Bog/87DQnVg=
 github.com/certifi/gocertifi v0.0.0-20180905225744-ee1a9a0726d2/go.mod h1:GJKEexRPVJrBSOjoqN5VNOIKJ5Q3RViH6eu3puDRwx4=
 github.com/cespare/xxhash v1.1.0 h1:a6HrQnmkObjyL+Gs60czilIUGqrzKutQD6XZog3p+ko=
 github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc=
diff --git a/internal/source/gitlab/api/client.go b/internal/source/gitlab/api/client.go
index 181c580b..923f7c03 100644
--- a/internal/source/gitlab/api/client.go
+++ b/internal/source/gitlab/api/client.go
@@ -8,7 +8,4 @@ import (
 type Client interface {
 	// Resolve retrieves an VirtualDomain from the GitLab API and wraps it into a Lookup
 	GetLookup(ctx context.Context, domain string) Lookup
-
-	// Status checks the connectivity with the GitLab API
-	Status() error
 }
diff --git a/internal/source/gitlab/api/resolver.go b/internal/source/gitlab/api/resolver.go
index 738278e2..b1e9404f 100644
--- a/internal/source/gitlab/api/resolver.go
+++ b/internal/source/gitlab/api/resolver.go
@@ -9,7 +9,4 @@ import (
 type Resolver interface {
 	// Resolve retrieves an VirtualDomain from the GitLab API and wraps it into a Lookup
 	Resolve(ctx context.Context, domain string) *Lookup
-
-	// Status checks the connectivity with the GitLab API
-	Status() error
 }
diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go
index 71b8e745..cb12c088 100644
--- a/internal/source/gitlab/cache/cache.go
+++ b/internal/source/gitlab/cache/cache.go
@@ -89,8 +89,3 @@ func (c *Cache) Resolve(ctx context.Context, domain string) *api.Lookup {
 	metrics.DomainsSourceCacheMiss.Inc()
 	return entry.Retrieve(ctx)
 }
-
-// Status calls the client Status to check connectivity with the API
-func (c *Cache) Status() error {
-	return c.client.Status()
-}
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 31071fa8..30185143 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -128,22 +128,6 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	return lookup
 }
 
-// Status checks that Pages can reach the rails internal Pages API
-// for source domain configuration.
-// Timeout is the same as -gitlab-client-http-timeout
-func (gc *Client) Status() error {
-	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
-}
-
 func (gc *Client) get(ctx context.Context, path string, params url.Values) (*http.Response, error) {
 	endpoint, err := gc.endpoint(path, params)
 	if err != nil {
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 12bc026e..131ddd9d 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -234,90 +234,6 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) {
 	require.Equal(t, "mygroup/myproject/public/", lookupPath.Source.Path)
 }
 
-func TestClientStatus(t *testing.T) {
-	tests := []struct {
-		name    string
-		status  int
-		wantErr bool
-	}{
-		{
-			name:   "api_enabled",
-			status: http.StatusNoContent,
-		},
-		{
-			name:    "api_unauthorized",
-			status:  http.StatusUnauthorized,
-			wantErr: true,
-		},
-		{
-			name:    "server_error",
-			status:  http.StatusInternalServerError,
-			wantErr: true,
-		},
-		{
-			name:    "gateway_timeout",
-			status:  http.StatusGatewayTimeout,
-			wantErr: true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			mux := http.NewServeMux()
-			mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) {
-				w.WriteHeader(tt.status)
-			})
-
-			server := httptest.NewServer(mux)
-			defer server.Close()
-
-			client := defaultClient(t, server.URL)
-
-			err := client.Status()
-			if tt.wantErr {
-				require.Error(t, err)
-				require.Contains(t, err.Error(), ConnectionErrorMsg)
-				return
-			}
-
-			require.NoError(t, err)
-		})
-	}
-}
-
-func TestClientStatusClientTimeout(t *testing.T) {
-	timeout := 20 * time.Millisecond
-
-	mux := http.NewServeMux()
-	mux.HandleFunc("/api/v4/internal/pages/status", func(w http.ResponseWriter, r *http.Request) {
-		time.Sleep(timeout * 3)
-
-		w.WriteHeader(http.StatusOK)
-	})
-
-	server := httptest.NewServer(mux)
-	defer server.Close()
-
-	client := defaultClient(t, server.URL)
-	client.httpClient.Timeout = timeout
-
-	err := client.Status()
-	require.Error(t, err)
-	// we can receive any of these messages
-	// - context deadline exceeded (Client.Timeout exceeded while awaiting headers)
-	// - net/http: request canceled (Client.Timeout exceeded while awaiting headers)
-	// - context deadline exceeded
-	require.Contains(t, err.Error(), "exceeded")
-}
-
-func TestClientStatusConnectionRefused(t *testing.T) {
-	client := defaultClient(t, "http://127.0.0.1:1234")
-
-	err := client.Status()
-	require.Error(t, err)
-	require.Contains(t, err.Error(), "connection refused")
-}
-
 func validateToken(t *testing.T, tokenString string) {
 	t.Helper()
 	token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) {
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index a295b753..b79b434f 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -7,9 +7,7 @@ import (
 	"path"
 	"sort"
 	"strings"
-	"sync"
 
-	"github.com/cenkalti/backoff/v4"
 	"gitlab.com/gitlab-org/labkit/log"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/config"
@@ -25,8 +23,6 @@ import (
 // information about domains from GitLab instance.
 type Gitlab struct {
 	client     api.Resolver
-	mu         sync.RWMutex
-	isReady    bool
 	enableDisk bool
 }
 
@@ -42,8 +38,6 @@ func New(cfg *config.GitLab) (*Gitlab, error) {
 		enableDisk: cfg.EnableDisk,
 	}
 
-	go g.poll(backoff.DefaultInitialInterval, maxPollingTime)
-
 	return g, nil
 }
 
@@ -55,10 +49,6 @@ func (g *Gitlab) GetDomain(ctx context.Context, name string) (*domain.Domain, er
 	if lookup.Error != nil {
 		if errors.Is(lookup.Error, client.ErrUnauthorizedAPI) {
 			log.WithError(lookup.Error).Error("Pages cannot communicate with an instance of the GitLab API. Please sync your gitlab-secrets.json file: https://docs.gitlab.com/ee/administration/pages/#pages-cannot-communicate-with-an-instance-of-the-gitlab-api")
-
-			g.mu.Lock()
-			g.isReady = false
-			g.mu.Unlock()
 		}
 
 		return nil, lookup.Error
@@ -123,8 +113,5 @@ func sortLookupsByPrefixLengthDesc(lookups []api.LookupPath) {
 
 // IsReady returns the value of Gitlab `isReady` which is updated by `Poll`.
 func (g *Gitlab) IsReady() bool {
-	g.mu.RLock()
-	defer g.mu.RUnlock()
-
-	return g.isReady
+	return true
 }
diff --git a/internal/source/gitlab/gitlab_poll.go b/internal/source/gitlab/gitlab_poll.go
deleted file mode 100644
index 110eb829..00000000
--- a/internal/source/gitlab/gitlab_poll.go
+++ /dev/null
@@ -1,45 +0,0 @@
-package gitlab
-
-import (
-	"time"
-
-	"github.com/cenkalti/backoff/v4"
-	"gitlab.com/gitlab-org/labkit/log"
-)
-
-const (
-	// maxPollingTime is the maximum duration to try to call the Status API
-	maxPollingTime = 60 * time.Minute
-)
-
-// Poll tries to call the /internal/pages/status API endpoint once plus
-// for `maxElapsedTime`
-// 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
-	backOff.MaxElapsedTime = maxElapsedTime
-
-	operation := func() error {
-		log.Info("Checking GitLab internal API availability")
-
-		err := g.client.Status()
-		if err != nil {
-			log.WithError(err).Warn("attempted to connect to the API")
-		}
-
-		return err
-	}
-
-	err := backoff.Retry(operation, backOff)
-	if err != nil {
-		log.WithError(err).Errorf("failed to connect to the internal GitLab API after %.2fs", maxElapsedTime.Seconds())
-		return
-	}
-
-	g.mu.Lock()
-	g.isReady = true
-	g.mu.Unlock()
-
-	log.Info("GitLab internal pages status API connected successfully")
-}
diff --git a/internal/source/gitlab/gitlab_poll_test.go b/internal/source/gitlab/gitlab_poll_test.go
deleted file mode 100644
index 33f9f6a4..00000000
--- a/internal/source/gitlab/gitlab_poll_test.go
+++ /dev/null
@@ -1,79 +0,0 @@
-package gitlab
-
-import (
-	"fmt"
-	"testing"
-	"time"
-
-	"github.com/sirupsen/logrus/hooks/test"
-	"github.com/stretchr/testify/require"
-
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
-)
-
-func TestClient_Poll(t *testing.T) {
-	hook := test.NewGlobal()
-	tests := []struct {
-		name         string
-		retries      int
-		maxTime      time.Duration
-		expectedFail bool
-	}{
-		{
-			name:         "success_with_no_retry",
-			retries:      0,
-			maxTime:      10 * time.Millisecond,
-			expectedFail: false,
-		},
-		{
-			name:         "success_after_N_retries",
-			retries:      3,
-			maxTime:      30 * time.Millisecond,
-			expectedFail: false,
-		},
-		{
-			name:         "fail_with_no_retries",
-			retries:      0,
-			maxTime:      10 * time.Millisecond,
-			expectedFail: true,
-		},
-		{
-			name:         "fail_after_N_retries",
-			retries:      3,
-			maxTime:      30 * time.Millisecond,
-			expectedFail: true,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			defer hook.Reset()
-			var counter int
-			client := client.StubClient{StatusErr: func() error {
-				if tt.expectedFail {
-					return fmt.Errorf(client.ConnectionErrorMsg)
-				}
-
-				if counter < tt.retries {
-					counter++
-					return fmt.Errorf(client.ConnectionErrorMsg)
-				}
-
-				return nil
-			}}
-
-			glClient := Gitlab{client: client}
-
-			glClient.poll(3*time.Millisecond, tt.maxTime)
-			if tt.expectedFail {
-				require.False(t, glClient.isReady)
-
-				s := fmt.Sprintf("failed to connect to the internal GitLab API after %.2fs", tt.maxTime.Seconds())
-				require.Equal(t, s, hook.LastEntry().Message)
-				return
-			}
-
-			require.True(t, glClient.isReady)
-			require.Equal(t, "GitLab internal pages status API connected successfully", hook.LastEntry().Message)
-		})
-	}
-}
diff --git a/internal/source/gitlab/gitlab_test.go b/internal/source/gitlab/gitlab_test.go
index d2a8e267..d1ef2a95 100644
--- a/internal/source/gitlab/gitlab_test.go
+++ b/internal/source/gitlab/gitlab_test.go
@@ -38,7 +38,6 @@ func TestGetDomain(t *testing.T) {
 
 		_, err := source.GetDomain(context.Background(), "test")
 		require.EqualError(t, err, client.ErrUnauthorizedAPI.Error())
-		require.False(t, source.IsReady())
 	})
 }
 
diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go
index c5fd8319..c1074230 100644
--- a/test/acceptance/helpers_test.go
+++ b/test/acceptance/helpers_test.go
@@ -514,41 +514,11 @@ func ClientWithConfig(tlsConfig *tls.Config) (*http.Client, func()) {
 	return client, tr.CloseIdleConnections
 }
 
-func waitForRoundtrips(t *testing.T, listeners []ListenSpec, timeout time.Duration) {
-	nListening := 0
-	start := time.Now()
-	for _, spec := range listeners {
-		for time.Since(start) < timeout {
-			req, err := http.NewRequest("GET", spec.URL("/"), nil)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			client := QuickTimeoutHTTPSClient
-			if spec.Type == "https-proxyv2" {
-				client = QuickTimeoutProxyv2Client
-			}
-
-			if response, err := client.Transport.RoundTrip(req); err == nil {
-				nListening++
-				response.Body.Close()
-				break
-			}
-
-			time.Sleep(100 * time.Millisecond)
-		}
-	}
-
-	require.Equal(t, len(listeners), nListening, "all listeners must be accepting TCP connections")
-}
-
 type stubOpts struct {
 	m                   sync.RWMutex
 	apiCalled           bool
-	statusReadyCount    int
 	authHandler         http.HandlerFunc
 	userHandler         http.HandlerFunc
-	statusHandler       http.HandlerFunc
 	pagesHandler        http.HandlerFunc
 	pagesStatusResponse int
 	pagesRoot           string
@@ -558,23 +528,7 @@ func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 	t.Helper()
 	require.NotNil(t, opts)
 
-	currentStatusCount := 0
-
 	router := mux.NewRouter()
-	statusHandler := func(w http.ResponseWriter, r *http.Request) {
-		if currentStatusCount < opts.statusReadyCount {
-			w.WriteHeader(http.StatusBadGateway)
-			return
-		}
-
-		w.WriteHeader(http.StatusNoContent)
-	}
-
-	if opts.statusHandler != nil {
-		statusHandler = opts.statusHandler
-	}
-
-	router.HandleFunc("/api/v4/internal/pages/status", statusHandler)
 
 	pagesHandler := defaultAPIHandler(t, opts)
 	if opts.pagesHandler != nil {
diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go
index fb73e03e..a4f3085b 100644
--- a/test/acceptance/serving_test.go
+++ b/test/acceptance/serving_test.go
@@ -387,7 +387,6 @@ func TestDomainsSource(t *testing.T) {
 		configSource string
 		domain       string
 		urlSuffix    string
-		readyCount   int
 	}
 	type want struct {
 		statusCode int
@@ -459,41 +458,12 @@ func TestDomainsSource(t *testing.T) {
 				apiCalled:  false,
 			},
 		},
-		{
-			name: "auto_source_gitlab_is_not_ready",
-			args: args{
-				configSource: "auto",
-				domain:       "test.domain.com",
-				urlSuffix:    "/",
-				readyCount:   100, // big number to ensure the API is in bad state for a while
-			},
-			want: want{
-				statusCode: http.StatusOK,
-				content:    "main-dir\n",
-				apiCalled:  false,
-			},
-		},
-		{
-			name: "auto_source_gitlab_is_ready",
-			args: args{
-				configSource: "auto",
-				domain:       "new-source-test.gitlab.io",
-				urlSuffix:    "/my/pages/project/",
-				readyCount:   0,
-			},
-			want: want{
-				statusCode: http.StatusOK,
-				content:    "New Pages GitLab Source TEST OK\n",
-				apiCalled:  true,
-			},
-		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			opts := &stubOpts{
-				apiCalled:        false,
-				statusReadyCount: tt.args.readyCount,
+				apiCalled: false,
 			}
 
 			source := NewGitlabDomainsSourceStub(t, opts)
@@ -522,46 +492,6 @@ func TestDomainsSource(t *testing.T) {
 	}
 }
 
-// TestGitLabSourceBecomesUnauthorized proves workaround for https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535
-// The first request will fail and display an error but subsequent requests will
-// serve from disk source when `domain-config-source=auto`
-// TODO: remove with domain-source-config https://gitlab.com/gitlab-org/gitlab-pages/-/issues/382
-func TestGitLabSourceBecomesUnauthorized(t *testing.T) {
-	opts := &stubOpts{
-		// edge case https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535
-		pagesStatusResponse: http.StatusUnauthorized,
-	}
-	source := NewGitlabDomainsSourceStub(t, opts)
-	defer source.Close()
-
-	gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t)
-
-	pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "auto"}
-	teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, []ListenSpec{httpListener}, "", []string{}, pagesArgs...)
-	defer teardown()
-
-	domain := "test.domain.com"
-	failedResponse, err := GetPageFromListener(t, httpListener, domain, "/")
-	require.NoError(t, err)
-
-	require.True(t, opts.getAPICalled(), "API should be called")
-	require.Equal(t, http.StatusBadGateway, failedResponse.StatusCode, "first response should fail with 502")
-
-	// make request again
-	opts.setAPICalled(false)
-
-	response, err := GetPageFromListener(t, httpListener, domain, "/")
-	require.NoError(t, err)
-	defer response.Body.Close()
-
-	require.False(t, opts.getAPICalled(), "API should not be called after the first failure")
-	require.Equal(t, http.StatusOK, response.StatusCode, "second response should succeed")
-
-	body, err := ioutil.ReadAll(response.Body)
-	require.NoError(t, err)
-	require.Equal(t, "main-dir\n", string(body), "content mismatch")
-}
-
 func TestKnownHostInReverseProxySetupReturns200(t *testing.T) {
 	RunPagesProcess(t,
 		withListeners([]ListenSpec{proxyListener}),
diff --git a/test/acceptance/status_test.go b/test/acceptance/status_test.go
index 59e7bc17..ef01c692 100644
--- a/test/acceptance/status_test.go
+++ b/test/acceptance/status_test.go
@@ -3,7 +3,6 @@ package acceptance_test
 import (
 	"net/http"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 )
@@ -19,31 +18,3 @@ func TestStatusPage(t *testing.T) {
 	defer rsp.Body.Close()
 	require.Equal(t, http.StatusOK, rsp.StatusCode)
 }
-
-func TestStatusNotYetReady(t *testing.T) {
-	listeners := supportedListeners()
-
-	RunPagesProcess(t,
-		withoutWait,
-		withExtraArgument("pages-status", "/@statuscheck"),
-		withExtraArgument("pages-root", "../../shared/invalid-pages"),
-		withStubOptions(&stubOpts{
-			statusReadyCount: 100,
-		}),
-	)
-
-	waitForRoundtrips(t, listeners, time.Duration(len(listeners))*time.Second)
-
-	// test status on all supported listeners
-	for _, spec := range listeners {
-		rsp, err := GetPageFromListener(t, spec, "group.gitlab-example.com", "@statuscheck")
-		require.NoError(t, err)
-		defer rsp.Body.Close()
-		require.Equal(t, http.StatusServiceUnavailable, rsp.StatusCode)
-
-		rsp2, err2 := GetPageFromListener(t, httpListener, "group.gitlab-example.com", "index.html")
-		require.NoError(t, err2)
-		defer rsp2.Body.Close()
-		require.Equal(t, http.StatusServiceUnavailable, rsp2.StatusCode, "page should not be served")
-	}
-}