Merge branch '535-handle-unauthorized-err' into 'master'

Handle unauthorized error and fallback to disk source

Closes #535

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

7 files changed, 96 insertions, 23 deletions

diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 7ed56f5a..757adb2b 100644
--- a/internal/source/gitlab/cache/cache_test.go
+++ b/internal/source/gitlab/cache/cache_test.go
@@ -13,14 +13,14 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
-type client struct {
+type clientMock struct {
 	counter uint64
 	lookups chan uint64
 	domain  chan string
 	failure error
 }
 
-func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
+func (c *clientMock) GetLookup(ctx context.Context, _ string) api.Lookup {
 	lookup := api.Lookup{}
 	if c.failure == nil {
 		lookup.Name = <-c.domain
@@ -33,11 +33,11 @@ func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	return lookup
 }
 
-func (c *client) Status() error {
+func (c *clientMock) Status() error {
 	return nil
 }
 
-func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
+func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *clientMock)) {
 	var chanSize int
 
 	if config.buffered {
@@ -46,7 +46,7 @@ func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*
 		chanSize = 0
 	}
 
-	resolver := &client{
+	resolver := &clientMock{
 		domain:  make(chan string, chanSize),
 		lookups: make(chan uint64, 100),
 		failure: config.failure,
@@ -90,7 +90,7 @@ type entryConfig struct {
 
 func TestResolve(t *testing.T) {
 	t.Run("when item is not cached", func(t *testing.T) {
-		withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *clientMock) {
 			require.Equal(t, 0, len(resolver.lookups))
 			resolver.domain <- "my.gitlab.com"
 
@@ -103,7 +103,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is not cached and accessed multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *clientMock) {
 			wg := &sync.WaitGroup{}
 			ctx := context.Background()
 
@@ -127,7 +127,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *clientMock) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -138,7 +138,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when a non-retrieved new item is in short cache", func(t *testing.T) {
-		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *clientMock) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(*Entry) {
 				lookup := make(chan *api.Lookup, 1)
 
@@ -157,7 +157,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item is in long cache only", func(t *testing.T) {
-		withTestCache(resolverConfig{buffered: false}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{buffered: false}, nil, func(cache *Cache, resolver *clientMock) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
@@ -172,7 +172,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when item in long cache is requested multiple times", func(t *testing.T) {
-		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *clientMock) {
 			cache.withTestEntry(entryConfig{expired: true, retrieved: true}, func(*Entry) {
 				cache.Resolve(context.Background(), "my.gitlab.com")
 				cache.Resolve(context.Background(), "my.gitlab.com")
@@ -192,7 +192,7 @@ func TestResolve(t *testing.T) {
 		cc.maxRetrievalInterval = 0
 		err := errors.New("500 error")
 
-		withTestCache(resolverConfig{failure: err}, &cc, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{failure: err}, &cc, func(cache *Cache, resolver *clientMock) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, 3, len(resolver.lookups))
@@ -204,7 +204,7 @@ func TestResolve(t *testing.T) {
 		cc := defaultCacheConfig
 		cc.retrievalTimeout = 0
 
-		withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *clientMock) {
 			lookup := cache.Resolve(context.Background(), "my.gitlab.com")
 
 			require.Equal(t, 0, len(resolver.lookups))
@@ -213,7 +213,7 @@ func TestResolve(t *testing.T) {
 	})
 
 	t.Run("when retrieval failed because of resolution context being canceled", func(t *testing.T) {
-		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *clientMock) {
 			cache.withTestEntry(entryConfig{expired: false, retrieved: false}, func(entry *Entry) {
 				ctx, cancel := context.WithCancel(context.Background())
 				cancel()
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index eddb02cb..865bf88b 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -9,6 +9,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 // Retriever is an utility type that performs an HTTP request with backoff in
@@ -55,8 +56,9 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domainName string) <
 
 		for i := 1; i <= r.maxRetrievalRetries; i++ {
 			lookup = r.client.GetLookup(ctx, domainName)
-			if lookup.Error == nil || errors.Is(lookup.Error, domain.ErrDomainDoesNotExist) {
-				// do not retry if the domain does not exist
+			if lookup.Error == nil || errors.Is(lookup.Error, domain.ErrDomainDoesNotExist) ||
+				errors.Is(lookup.Error, client.ErrUnauthorizedAPI) {
+				// do not retry if the domain does not exist or there is an auth error
 				break
 			}
 
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 524eb8a4..98939641 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -24,6 +24,12 @@ import (
 // or a 401 given that the credentials used are wrong
 const ConnectionErrorMsg = "failed to connect to internal Pages API"
 
+// ErrUnauthorizedAPI is returned when resolving a domain with the GitLab API
+// returns a http.StatusUnauthorized. This happens if the common secret file
+// is not synced between gitlab-pages and gitlab-rails servers.
+// See https://gitlab.com/gitlab-org/gitlab-pages/-/issues/535 for more details.
+var ErrUnauthorizedAPI = errors.New("pages endpoint unauthorized")
+
 // Client is a HTTP client to access Pages internal API
 type Client struct {
 	secretKey      []byte
@@ -161,6 +167,8 @@ func (gc *Client) get(ctx context.Context, path string, params url.Values) (*htt
 	// StatusNoContent means that a domain does not exist, it is not an error
 	if resp.StatusCode == http.StatusNoContent {
 		return nil, nil
+	} else if resp.StatusCode == http.StatusUnauthorized {
+		return nil, ErrUnauthorizedAPI
 	}
 
 	return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 153b9372..32fd8c18 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -138,7 +138,7 @@ func TestNewInvalidConfiguration(t *testing.T) {
 }
 func TestLookupForErrorResponses(t *testing.T) {
 	tests := map[int]string{
-		http.StatusUnauthorized: "HTTP status: 401",
+		http.StatusUnauthorized: ErrUnauthorizedAPI.Error(),
 		http.StatusNotFound:     "HTTP status: 404",
 	}
 
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index d164460a..1f937e5d 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -2,12 +2,14 @@ package gitlab
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"path"
 	"strings"
 	"sync"
 
 	"github.com/cenkalti/backoff/v4"
+	"gitlab.com/gitlab-org/labkit/log"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
@@ -49,6 +51,14 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 	lookup := g.client.Resolve(context.Background(), name)
 
 	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
 	}
 
diff --git a/test/acceptance/helpers_test.go b/test/acceptance/helpers_test.go
index 8d8ca8d6..5c380938 100644
--- a/test/acceptance/helpers_test.go
+++ b/test/acceptance/helpers_test.go
@@ -536,10 +536,11 @@ func waitForRoundtrips(t *testing.T, listeners []ListenSpec, timeout time.Durati
 }
 
 type stubOpts struct {
-	apiCalled        bool
-	statusReadyCount int
-	statusHandler    http.HandlerFunc
-	pagesHandler     http.HandlerFunc
+	apiCalled           bool
+	statusReadyCount    int
+	statusHandler       http.HandlerFunc
+	pagesHandler        http.HandlerFunc
+	pagesStatusResponse int
 }
 
 func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
@@ -553,6 +554,7 @@ func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 	statusHandler := func(w http.ResponseWriter, r *http.Request) {
 		if currentStatusCount < opts.statusReadyCount {
 			w.WriteHeader(http.StatusBadGateway)
+			return
 		}
 
 		w.WriteHeader(http.StatusNoContent)
@@ -565,8 +567,20 @@ func NewGitlabDomainsSourceStub(t *testing.T, opts *stubOpts) *httptest.Server {
 	mux.HandleFunc("/api/v4/internal/pages/status", statusHandler)
 
 	pagesHandler := func(w http.ResponseWriter, r *http.Request) {
-		opts.apiCalled = true
 		domain := r.URL.Query().Get("host")
+		if domain == "127.0.0.1" {
+			// shortcut for healthy checkup done by WaitUntilRequestSucceeds
+			w.WriteHeader(http.StatusNoContent)
+			return
+		}
+
+		opts.apiCalled = true
+
+		if opts.pagesStatusResponse != 0 {
+			w.WriteHeader(opts.pagesStatusResponse)
+			return
+		}
+
 		path := "../../shared/lookups/" + domain + ".json"
 
 		fixture, err := os.Open(path)
diff --git a/test/acceptance/serving_test.go b/test/acceptance/serving_test.go
index 6cc41eff..19476830 100644
--- a/test/acceptance/serving_test.go
+++ b/test/acceptance/serving_test.go
@@ -496,7 +496,7 @@ func TestDomainsSource(t *testing.T) {
 			gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t)
 
 			pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", tt.args.configSource}
-			teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...)
+			teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, []ListenSpec{httpListener}, "", []string{}, pagesArgs...)
 			defer teardown()
 
 			response, err := GetPageFromListener(t, httpListener, tt.args.domain, tt.args.urlSuffix)
@@ -516,6 +516,45 @@ 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`
+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.apiCalled, "API should be called")
+	require.Equal(t, http.StatusBadGateway, failedResponse.StatusCode, "first response should fail with 502")
+
+	// make request again
+	opts.apiCalled = false
+
+	response, err := GetPageFromListener(t, httpListener, domain, "/")
+	require.NoError(t, err)
+	defer response.Body.Close()
+
+	require.False(t, opts.apiCalled, "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) {
 	skipUnlessEnabled(t)