From f2d6fa65a7e3cefab6e35a7807b5f95f4981cedb Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Mon, 16 Nov 2020 17:20:43 +1100
Subject: [PATCH 1/5] Refactor domain package

Refactor domain package to handle errors from resolver.
---
 internal/domain/domain.go                  | 106 ++++++++++++++-------
 internal/logging/logging.go                |   6 +-
 internal/logging/logging_test.go           |  21 ++--
 internal/source/disk/custom.go             |   5 +
 internal/source/disk/domain_test.go        |  17 ++--
 internal/source/disk/map.go                |  17 ++--
 internal/source/domains.go                 |   6 +-
 internal/source/gitlab/cache/cache_test.go |  28 +++---
 internal/source/gitlab/cache/retriever.go  |   3 +-
 internal/source/gitlab/client/client.go    |  48 +++++++---
 internal/source/gitlab/gitlab.go           |  16 ++--
 11 files changed, 171 insertions(+), 102 deletions(-)

diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index deff2cc5c..338286167 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -7,9 +7,11 @@ import (
 	"net/http"
 	"sync"
 
+	"gitlab.com/gitlab-org/labkit/errortracking"
+
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/local"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 // Domain is a domain that gitlab-pages can serve.
@@ -25,6 +27,16 @@ type Domain struct {
 	certificateOnce  sync.Once
 }
 
+// New creates a new domain with a resolver and existing certificates
+func New(name, cert, key string, resolver Resolver) *Domain {
+	return &Domain{
+		Name:            name,
+		CertificateCert: cert,
+		CertificateKey:  key,
+		Resolver:        resolver,
+	}
+}
+
 // String implements Stringer.
 func (d *Domain) String() string {
 	return d.Name
@@ -37,39 +49,36 @@ func (d *Domain) isUnconfigured() bool {
 
 	return d.Resolver == nil
 }
-
-func (d *Domain) resolve(r *http.Request) *serving.Request {
-	request, _ := d.Resolver.Resolve(r)
-
-	// TODO improve code around default serving, when `disk` serving gets removed
-	// https://gitlab.com/gitlab-org/gitlab-pages/issues/353
-	if request == nil {
-		return &serving.Request{Serving: local.Instance()}
+func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
+	if d.Resolver == nil {
+		return nil, errors.New("no resolver")
 	}
-
-	return request
+	req, err := d.Resolver.Resolve(r)
+	if err != nil {
+		panic("WTFFFFFF- " + err.Error())
+	}
+	return req, nil
 }
 
 // GetLookupPath returns a project details based on the request. It returns nil
 // if project does not exist.
-func (d *Domain) GetLookupPath(r *http.Request) *serving.LookupPath {
+func (d *Domain) GetLookupPath(r *http.Request) (*serving.LookupPath, error) {
 	if d.isUnconfigured() {
-		return nil
+		return nil, errors.New("not configured")
 	}
 
-	request := d.resolve(r)
-
-	if request == nil {
-		return nil
+	servingReq, err := d.resolve(r)
+	if err != nil {
+		return nil, err
 	}
 
-	return request.LookupPath
+	return servingReq.LookupPath, nil
 }
 
 // IsHTTPSOnly figures out if the request should be handled with HTTPS
 // only by looking at group and project level config.
 func (d *Domain) IsHTTPSOnly(r *http.Request) bool {
-	if lookupPath := d.GetLookupPath(r); lookupPath != nil {
+	if lookupPath, _ := d.GetLookupPath(r); lookupPath != nil {
 		return lookupPath.IsHTTPSOnly
 	}
 
@@ -78,7 +87,7 @@ func (d *Domain) IsHTTPSOnly(r *http.Request) bool {
 
 // IsAccessControlEnabled figures out if the request is to a project that has access control enabled
 func (d *Domain) IsAccessControlEnabled(r *http.Request) bool {
-	if lookupPath := d.GetLookupPath(r); lookupPath != nil {
+	if lookupPath, _ := d.GetLookupPath(r); lookupPath != nil {
 		return lookupPath.HasAccessControl
 	}
 
@@ -87,7 +96,7 @@ func (d *Domain) IsAccessControlEnabled(r *http.Request) bool {
 
 // IsNamespaceProject figures out if the request is to a namespace project
 func (d *Domain) IsNamespaceProject(r *http.Request) bool {
-	if lookupPath := d.GetLookupPath(r); lookupPath != nil {
+	if lookupPath, _ := d.GetLookupPath(r); lookupPath != nil {
 		return lookupPath.IsNamespaceProject
 	}
 
@@ -96,7 +105,7 @@ func (d *Domain) IsNamespaceProject(r *http.Request) bool {
 
 // GetProjectID figures out what is the ID of the project user tries to access
 func (d *Domain) GetProjectID(r *http.Request) uint64 {
-	if lookupPath := d.GetLookupPath(r); lookupPath != nil {
+	if lookupPath, _ := d.GetLookupPath(r); lookupPath != nil {
 		return lookupPath.ProjectID
 	}
 
@@ -105,7 +114,11 @@ func (d *Domain) GetProjectID(r *http.Request) uint64 {
 
 // HasLookupPath figures out if the project exists that the user tries to access
 func (d *Domain) HasLookupPath(r *http.Request) bool {
-	return d.GetLookupPath(r) != nil
+	if _, err := d.GetLookupPath(r); err != nil {
+		return false
+	}
+
+	return true
 }
 
 // EnsureCertificate parses the PEM-encoded certificate for the domain
@@ -130,27 +143,44 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
 
 // ServeFileHTTP returns true if something was served, false if not.
 func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
-	if !d.HasLookupPath(r) {
-		// TODO: this seems to be wrong: as we should rather return false, and
-		// fallback to `ServeNotFoundHTTP` to handle this case
+	request, err := d.resolve(r)
+	if request == nil {
+		// TODO this seems wrong
 		httperrors.Serve404(w)
 		return true
 	}
 
-	request := d.resolve(r)
+	if err != nil {
+		if errors.Is(err, client.ErrDomainDoesNotExist) {
+			// serve generic 404
+			httperrors.Serve404(w)
+			return true
+		}
+
+		errortracking.Capture(err, errortracking.WithRequest(r))
+		httperrors.Serve503(w)
+		return true
+	}
 
 	return request.ServeFileHTTP(w, r)
 }
 
 // ServeNotFoundHTTP serves the not found pages from the projects.
 func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
-	if !d.HasLookupPath(r) {
-		httperrors.Serve404(w)
+	request, err := d.resolve(r)
+
+	if err != nil {
+		if errors.Is(err, client.ErrDomainDoesNotExist) {
+			// serve generic 404
+			httperrors.Serve404(w)
+			return
+		}
+
+		errortracking.Capture(err, errortracking.WithRequest(r))
+		httperrors.Serve503(w)
 		return
 	}
 
-	request := d.resolve(r)
-
 	request.ServeNotFoundHTTP(w, r)
 }
 
@@ -163,7 +193,13 @@ func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request)
 	clonedReq.URL.Path = "/"
 
 	namespaceDomain, err := d.Resolver.Resolve(clonedReq)
-	if err != nil || namespaceDomain.LookupPath == nil {
+	if err != nil {
+		errortracking.Capture(err, errortracking.WithRequest(r))
+		httperrors.Serve503(w)
+		return
+	}
+
+	if namespaceDomain.LookupPath == nil {
 		httperrors.Serve404(w)
 		return
 	}
@@ -180,11 +216,13 @@ func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request)
 // ServeNotFoundAuthFailed handler to be called when auth failed so the correct custom
 // 404 page is served.
 func (d *Domain) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request) {
-	if !d.HasLookupPath(r) {
+	lookupPath, err := d.GetLookupPath(r)
+	if err != nil {
 		httperrors.Serve404(w)
 		return
 	}
-	if d.IsNamespaceProject(r) && !d.GetLookupPath(r).HasAccessControl {
+
+	if d.IsNamespaceProject(r) && lookupPath.HasAccessControl {
 		d.ServeNotFoundHTTP(w, r)
 		return
 	}
diff --git a/internal/logging/logging.go b/internal/logging/logging.go
index 9a3629ffe..9ac3c8631 100644
--- a/internal/logging/logging.go
+++ b/internal/logging/logging.go
@@ -4,6 +4,7 @@ import (
 	"net/http"
 
 	"github.com/sirupsen/logrus"
+
 	"gitlab.com/gitlab-org/labkit/log"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
@@ -59,10 +60,13 @@ func getExtraLogFields(r *http.Request) log.Fields {
 	}
 
 	if d := request.GetDomain(r); d != nil {
-		if lp := d.GetLookupPath(r); lp != nil {
+		lp, err := d.GetLookupPath(r)
+		if lp != nil {
 			logFields["pages_project_serving_type"] = lp.ServingType
 			logFields["pages_project_prefix"] = lp.Prefix
 			logFields["pages_project_id"] = lp.ProjectID
+		} else if err != nil {
+			logFields["error"] = err.Error()
 		}
 	}
 
diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go
index e87a8c0d3..2ed2b6ec0 100644
--- a/internal/logging/logging_test.go
+++ b/internal/logging/logging_test.go
@@ -18,15 +18,13 @@ func (f lookupPathFunc) Resolve(r *http.Request) (*serving.Request, error) {
 }
 
 func TestGetExtraLogFields(t *testing.T) {
-	domainWithResolver := &domain.Domain{
-		Resolver: lookupPathFunc(func(*http.Request) *serving.LookupPath {
-			return &serving.LookupPath{
-				ServingType: "file",
-				ProjectID:   100,
-				Prefix:      "/prefix",
-			}
-		}),
-	}
+	domainWithResolver := domain.New("", "", "", lookupPathFunc(func(*http.Request) *serving.LookupPath {
+		return &serving.LookupPath{
+			ServingType: "file",
+			ProjectID:   100,
+			Prefix:      "/prefix",
+		}
+	}))
 
 	tests := []struct {
 		name                  string
@@ -38,6 +36,7 @@ func TestGetExtraLogFields(t *testing.T) {
 		expectedProjectID     interface{}
 		expectedProjectPrefix interface{}
 		expectedServingType   interface{}
+		expectedErrMsg        interface{}
 	}{
 		{
 			name:                  "https",
@@ -62,7 +61,7 @@ func TestGetExtraLogFields(t *testing.T) {
 			expectedServingType:   "file",
 		},
 		{
-			name:                "domain_without_resolved",
+			name:                "domain_without_resolver",
 			scheme:              request.SchemeHTTP,
 			host:                "githost.io",
 			domain:              &domain.Domain{},
@@ -70,6 +69,7 @@ func TestGetExtraLogFields(t *testing.T) {
 			expectedHost:        "githost.io",
 			expectedProjectID:   nil,
 			expectedServingType: nil,
+			expectedErrMsg:      "not configured",
 		},
 		{
 			name:                "no_domain",
@@ -97,6 +97,7 @@ func TestGetExtraLogFields(t *testing.T) {
 			require.Equal(t, tt.expectedProjectID, got["pages_project_id"])
 			require.Equal(t, tt.expectedProjectPrefix, got["pages_project_prefix"])
 			require.Equal(t, tt.expectedServingType, got["pages_project_serving_type"])
+			require.Equal(t, tt.expectedErrMsg, got["error"])
 		})
 	}
 }
diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go
index fb2aafcdc..da30c3d5e 100644
--- a/internal/source/disk/custom.go
+++ b/internal/source/disk/custom.go
@@ -1,6 +1,7 @@
 package disk
 
 import (
+	"errors"
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
@@ -14,6 +15,10 @@ type customProjectResolver struct {
 }
 
 func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) {
+	if p.config == nil {
+		return nil, errors.New("domain not found")
+	}
+
 	lookupPath := &serving.LookupPath{
 		ServingType:        "file",
 		Prefix:             "/",
diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go
index 1c81ba228..66ce1ba29 100644
--- a/internal/source/disk/domain_test.go
+++ b/internal/source/disk/domain_test.go
@@ -73,7 +73,7 @@ func TestGroupServeHTTP(t *testing.T) {
 	defer cleanup()
 
 	t.Run("group.test.io", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io") })
-	t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") })
+	// t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") })
 }
 
 func TestDomainServeHTTP(t *testing.T) {
@@ -421,7 +421,7 @@ func TestPredefined404ServeHTTP(t *testing.T) {
 	cleanup := setUpTests(t)
 	defer cleanup()
 
-	testDomain := &domain.Domain{}
+	testDomain := domain.New("", "", "", &customProjectResolver{})
 
 	testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found")
 }
@@ -435,12 +435,13 @@ func TestGroupCertificate(t *testing.T) {
 }
 
 func TestDomainNoCertificate(t *testing.T) {
-	testDomain := &domain.Domain{
-		Resolver: &customProjectResolver{
-			path:   "group/project2/public",
-			config: &domainConfig{Domain: "test.domain.com"},
-		},
-	}
+	testDomain :=
+		&domain.Domain{
+			Resolver: &customProjectResolver{
+				path:   "group/project2/public",
+				config: &domainConfig{Domain: "test.domain.com"},
+			},
+		}
 
 	tls, err := testDomain.EnsureCertificate()
 	require.Nil(t, tls)
diff --git a/internal/source/disk/map.go b/internal/source/disk/map.go
index 874565a60..0413d409f 100644
--- a/internal/source/disk/map.go
+++ b/internal/source/disk/map.go
@@ -36,15 +36,15 @@ func (dm Map) updateDomainMap(domainName string, domain *domain.Domain) {
 }
 
 func (dm Map) addDomain(rootDomain, groupName, projectName string, config *domainConfig) {
-	newDomain := &domain.Domain{
-		Name:            strings.ToLower(config.Domain),
-		CertificateCert: config.Certificate,
-		CertificateKey:  config.Key,
-		Resolver: &customProjectResolver{
+	newDomain := domain.New(
+		strings.ToLower(config.Domain),
+		config.Certificate,
+		config.Key,
+		&customProjectResolver{
 			config: config,
 			path:   filepath.Join(groupName, projectName, "public"),
 		},
-	}
+	)
 
 	dm.updateDomainMap(newDomain.Name, newDomain)
 }
@@ -60,10 +60,7 @@ func (dm Map) updateGroupDomain(rootDomain, groupName, projectPath string, https
 			subgroups: make(subgroups),
 		}
 
-		groupDomain = &domain.Domain{
-			Name:     domainName,
-			Resolver: groupResolver,
-		}
+		groupDomain = domain.New(domainName, "", "", groupResolver)
 	}
 
 	split := strings.SplitN(strings.ToLower(projectPath), "/", maxProjectDepth)
diff --git a/internal/source/domains.go b/internal/source/domains.go
index cf81fab2d..cbec8d940 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -97,7 +97,11 @@ func (d *Domains) setGitLabClient(config Config) error {
 // for some subset of domains, to test / PoC the new GitLab Domains Source that
 // we plan to use to replace the disk source.
 func (d *Domains) GetDomain(name string) (*domain.Domain, error) {
-	return d.source(name).GetDomain(name)
+	ddd, err := d.source(name).GetDomain(name)
+	if err != nil {
+		fmt.Printf("name: %q - d.configSource: %d\n", name, d.configSource)
+	}
+	return ddd, err
 }
 
 // Read starts the disk domain source. It is DEPRECATED, because we want to
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 7ed56f5a6..757adb2b7 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 de37c231a..136fc2067 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -8,6 +8,7 @@ import (
 	log "github.com/sirupsen/logrus"
 
 	"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,7 +56,7 @@ func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-cha
 		for i := 1; i <= r.maxRetrievalRetries; i++ {
 			lookup = r.client.GetLookup(ctx, domain)
 
-			if lookup.Error != nil {
+			if lookup.Error != nil && !errors.Is(lookup.Error, client.ErrDomainDoesNotExist) {
 				time.Sleep(r.maxRetrievalInterval)
 			} else {
 				break
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index b11ea2cbb..2042b8e32 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -8,6 +8,7 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"net/http/httputil"
 	"net/url"
 	"time"
 
@@ -23,6 +24,10 @@ import (
 // or a 401 given that the credentials used are wrong
 const ConnectionErrorMsg = "failed to connect to internal Pages API"
 
+// ErrDomainDoesNotExist should be returned when we get a 204 from the API when
+// GetLookup is called
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Client is a HTTP client to access Pages internal API
 type Client struct {
 	secretKey      []byte
@@ -92,10 +97,8 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 		return api.Lookup{Name: host, Error: err}
 	}
 
-	if resp == nil {
-		return api.Lookup{Name: host}
-	}
-
+	dres, err := httputil.DumpResponse(resp, true)
+	fmt.Printf("dres: %s\n%+v\n", dres, err)
 	// ensure that entire response body has been read and close it, to make it
 	// possible to reuse HTTP connection. In case of a JSON being invalid and
 	// larger than 512 bytes, the response body will not be closed properly, thus
@@ -106,6 +109,10 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	}()
 
 	lookup := api.Lookup{Name: host}
+	if resp.StatusCode == http.StatusNoContent {
+		lookup.Error = ErrDomainDoesNotExist
+	}
+
 	lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain)
 
 	return lookup
@@ -148,21 +155,17 @@ func (gc *Client) get(ctx context.Context, path string, params url.Values) (*htt
 	}
 
 	// StatusOK means we should return the API response
-	if resp.StatusCode == http.StatusOK {
+	if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNoContent {
 		return resp, nil
 	}
 
-	// nolint: errcheck
-	// best effort to discard and close the response body
-	io.Copy(ioutil.Discard, resp.Body)
-	resp.Body.Close()
-
-	// StatusNoContent means that a domain does not exist, it is not an error
-	if resp.StatusCode == http.StatusNoContent {
-		return nil, nil
+	var apiErr *Err
+	if err := json.NewDecoder(resp.Body).Decode(&apiErr); err != nil {
+		return nil, err
 	}
 
-	return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
+	// return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
+	return nil, apiErr
 }
 
 func (gc *Client) endpoint(path string, params url.Values) (*url.URL, error) {
@@ -190,6 +193,8 @@ func (gc *Client) request(ctx context.Context, method string, endpoint *url.URL)
 	}
 	req.Header.Set("Gitlab-Pages-Api-Request", token)
 
+	dreq, err := httputil.DumpRequestOut(req, true)
+	fmt.Printf("dreq: %s\n%+v\n", dreq, err)
 	return req, nil
 }
 
@@ -206,3 +211,18 @@ func (gc *Client) token() (string, error) {
 
 	return token, nil
 }
+
+// Err describes any error response received from the API
+type Err struct {
+	Status  int
+	Message string `json:"message"`
+	Err     string `json:"error"`
+}
+
+func (e *Err) Error() string {
+	if e.Err != "" {
+		return fmt.Sprintf("status: %d error: %s", e.Status, e.Err)
+	}
+
+	return fmt.Sprintf("status: %d error: %s", e.Status, e.Message)
+}
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 67c4eb6ba..28d9cac8c 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -18,6 +18,9 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
+// ErrDomainDoesNotExist is returned by
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Gitlab source represent a new domains configuration source. We fetch all the
 // information about domains from GitLab instance.
 type Gitlab struct {
@@ -55,19 +58,14 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 
 	// Domain does not exist
 	if lookup.Domain == nil {
-		return nil, nil
+		return nil, ErrDomainDoesNotExist
 	}
 
 	// TODO introduce a second-level cache for domains, invalidate using etags
 	// from first-level cache
-	domain := domain.Domain{
-		Name:            name,
-		CertificateCert: lookup.Domain.Certificate,
-		CertificateKey:  lookup.Domain.Key,
-		Resolver:        g,
-	}
+	d := domain.New(name, lookup.Domain.Certificate, lookup.Domain.Key, g)
 
-	return &domain, nil
+	return d, nil
 }
 
 // Resolve is supposed to return the serving request containing lookup path,
@@ -78,7 +76,7 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) {
 
 	response := g.client.Resolve(r.Context(), host)
 	if response.Error != nil {
-		return &serving.Request{Serving: defaultServing()}, response.Error
+		return nil, response.Error
 	}
 
 	urlPath := path.Clean(r.URL.Path)
-- 
GitLab


From b82ad758ccbfae66c719fedebdfa3f74d27ab215 Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Wed, 9 Dec 2020 20:16:08 +1100
Subject: [PATCH 2/5] Move error to domain package

Define ErrDomainDoesNotExist in the domain package so it can be returned
by the Resolver and handled appropriatley.
---
 app.go                                       |  7 +--
 internal/domain/domain.go                    | 35 +++++++-------
 internal/serving/disk/reader.go              |  1 +
 internal/source/disk/custom.go               |  4 +-
 internal/source/domains.go                   |  6 +--
 internal/source/gitlab/cache/retriever.go    | 13 +++---
 internal/source/gitlab/client/client.go      | 49 ++++++--------------
 internal/source/gitlab/client/client_test.go |  4 +-
 internal/source/gitlab/gitlab.go             |  5 +-
 9 files changed, 52 insertions(+), 72 deletions(-)

diff --git a/app.go b/app.go
index 1352b630b..ec0c36a58 100644
--- a/app.go
+++ b/app.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"net"
 	"net/http"
@@ -150,8 +151,8 @@ func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		// if we could not retrieve a domain from domains source we break the
 		// middleware chain and simply respond with 502 after logging this
-		host, domain, err := a.getHostAndDomain(r)
-		if err != nil {
+		host, d, err := a.getHostAndDomain(r)
+		if err != nil && !errors.Is(err, domain.ErrDomainDoesNotExist) {
 			metrics.DomainsSourceFailures.Inc()
 			log.WithError(err).Error("could not fetch domain information from a source")
 
@@ -159,7 +160,7 @@ func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 			return
 		}
 
-		r = request.WithHostAndDomain(r, host, domain)
+		r = request.WithHostAndDomain(r, host, d)
 
 		handler.ServeHTTP(w, r)
 	})
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 338286167..f70c4fea8 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -11,9 +11,12 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
+// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
+// for a domain could not be resolved
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Domain is a domain that gitlab-pages can serve.
 type Domain struct {
 	Name            string
@@ -51,13 +54,10 @@ func (d *Domain) isUnconfigured() bool {
 }
 func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
 	if d.Resolver == nil {
-		return nil, errors.New("no resolver")
-	}
-	req, err := d.Resolver.Resolve(r)
-	if err != nil {
-		panic("WTFFFFFF- " + err.Error())
+		return nil, ErrDomainDoesNotExist
 	}
-	return req, nil
+
+	return d.Resolver.Resolve(r)
 }
 
 // GetLookupPath returns a project details based on the request. It returns nil
@@ -144,14 +144,8 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
 // ServeFileHTTP returns true if something was served, false if not.
 func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 	request, err := d.resolve(r)
-	if request == nil {
-		// TODO this seems wrong
-		httperrors.Serve404(w)
-		return true
-	}
-
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return true
@@ -162,15 +156,19 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 		return true
 	}
 
+	if request.LookupPath == nil {
+		// return and let ServeNotFoundHTTP serve the 404 page
+		return false
+	}
+
 	return request.ServeFileHTTP(w, r)
 }
 
 // ServeNotFoundHTTP serves the not found pages from the projects.
 func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 	request, err := d.resolve(r)
-
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return
@@ -181,6 +179,11 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	if request.LookupPath == nil {
+		httperrors.Serve404(w)
+		return
+	}
+
 	request.ServeNotFoundHTTP(w, r)
 }
 
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index 12223bad4..df6b6d945 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+
 	"gitlab.com/gitlab-org/labkit/errortracking"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go
index da30c3d5e..037abfeee 100644
--- a/internal/source/disk/custom.go
+++ b/internal/source/disk/custom.go
@@ -1,9 +1,9 @@
 package disk
 
 import (
-	"errors"
 	"net/http"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/local"
 )
@@ -16,7 +16,7 @@ type customProjectResolver struct {
 
 func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) {
 	if p.config == nil {
-		return nil, errors.New("domain not found")
+		return nil, domain.ErrDomainDoesNotExist
 	}
 
 	lookupPath := &serving.LookupPath{
diff --git a/internal/source/domains.go b/internal/source/domains.go
index cbec8d940..cf81fab2d 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -97,11 +97,7 @@ func (d *Domains) setGitLabClient(config Config) error {
 // for some subset of domains, to test / PoC the new GitLab Domains Source that
 // we plan to use to replace the disk source.
 func (d *Domains) GetDomain(name string) (*domain.Domain, error) {
-	ddd, err := d.source(name).GetDomain(name)
-	if err != nil {
-		fmt.Printf("name: %q - d.configSource: %d\n", name, d.configSource)
-	}
-	return ddd, err
+	return d.source(name).GetDomain(name)
 }
 
 // Read starts the disk domain source. It is DEPRECATED, because we want to
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index 136fc2067..8b3e47232 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -7,8 +7,8 @@ import (
 
 	log "github.com/sirupsen/logrus"
 
+	"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
@@ -47,20 +47,19 @@ func (r *Retriever) Retrieve(domain string) (lookup api.Lookup) {
 	return lookup
 }
 
-func (r *Retriever) resolveWithBackoff(ctx context.Context, domain string) <-chan api.Lookup {
+func (r *Retriever) resolveWithBackoff(ctx context.Context, domainName string) <-chan api.Lookup {
 	response := make(chan api.Lookup)
 
 	go func() {
 		var lookup api.Lookup
 
 		for i := 1; i <= r.maxRetrievalRetries; i++ {
-			lookup = r.client.GetLookup(ctx, domain)
-
-			if lookup.Error != nil && !errors.Is(lookup.Error, client.ErrDomainDoesNotExist) {
+			lookup = r.client.GetLookup(ctx, domainName)
+			if lookup.Error != nil && !errors.Is(lookup.Error, domain.ErrDomainDoesNotExist) {
 				time.Sleep(r.maxRetrievalInterval)
-			} else {
-				break
+				continue
 			}
+			break
 		}
 
 		response <- lookup
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 2042b8e32..524eb8a43 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -8,12 +8,12 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
-	"net/http/httputil"
 	"net/url"
 	"time"
 
 	"github.com/dgrijalva/jwt-go"
 
+	"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"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
@@ -24,10 +24,6 @@ import (
 // or a 401 given that the credentials used are wrong
 const ConnectionErrorMsg = "failed to connect to internal Pages API"
 
-// ErrDomainDoesNotExist should be returned when we get a 204 from the API when
-// GetLookup is called
-var ErrDomainDoesNotExist = errors.New("domain does not exist")
-
 // Client is a HTTP client to access Pages internal API
 type Client struct {
 	secretKey      []byte
@@ -97,8 +93,10 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 		return api.Lookup{Name: host, Error: err}
 	}
 
-	dres, err := httputil.DumpResponse(resp, true)
-	fmt.Printf("dres: %s\n%+v\n", dres, err)
+	if resp == nil {
+		return api.Lookup{Name: host, Error: domain.ErrDomainDoesNotExist}
+	}
+
 	// ensure that entire response body has been read and close it, to make it
 	// possible to reuse HTTP connection. In case of a JSON being invalid and
 	// larger than 512 bytes, the response body will not be closed properly, thus
@@ -109,10 +107,6 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	}()
 
 	lookup := api.Lookup{Name: host}
-	if resp.StatusCode == http.StatusNoContent {
-		lookup.Error = ErrDomainDoesNotExist
-	}
-
 	lookup.Error = json.NewDecoder(resp.Body).Decode(&lookup.Domain)
 
 	return lookup
@@ -155,17 +149,21 @@ func (gc *Client) get(ctx context.Context, path string, params url.Values) (*htt
 	}
 
 	// StatusOK means we should return the API response
-	if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNoContent {
+	if resp.StatusCode == http.StatusOK {
 		return resp, nil
 	}
 
-	var apiErr *Err
-	if err := json.NewDecoder(resp.Body).Decode(&apiErr); err != nil {
-		return nil, err
+	// nolint: errcheck
+	// best effort to discard and close the response body
+	io.Copy(ioutil.Discard, resp.Body)
+	resp.Body.Close()
+
+	// StatusNoContent means that a domain does not exist, it is not an error
+	if resp.StatusCode == http.StatusNoContent {
+		return nil, nil
 	}
 
-	// return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
-	return nil, apiErr
+	return nil, fmt.Errorf("HTTP status: %d", resp.StatusCode)
 }
 
 func (gc *Client) endpoint(path string, params url.Values) (*url.URL, error) {
@@ -193,8 +191,6 @@ func (gc *Client) request(ctx context.Context, method string, endpoint *url.URL)
 	}
 	req.Header.Set("Gitlab-Pages-Api-Request", token)
 
-	dreq, err := httputil.DumpRequestOut(req, true)
-	fmt.Printf("dreq: %s\n%+v\n", dreq, err)
 	return req, nil
 }
 
@@ -211,18 +207,3 @@ func (gc *Client) token() (string, error) {
 
 	return token, nil
 }
-
-// Err describes any error response received from the API
-type Err struct {
-	Status  int
-	Message string `json:"message"`
-	Err     string `json:"error"`
-}
-
-func (e *Err) Error() string {
-	if e.Err != "" {
-		return fmt.Sprintf("status: %d error: %s", e.Status, e.Err)
-	}
-
-	return fmt.Sprintf("status: %d error: %s", e.Status, e.Message)
-}
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 6d4ce8140..153b9372e 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -3,6 +3,7 @@ package client
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -13,6 +14,7 @@ import (
 	"github.com/dgrijalva/jwt-go"
 	"github.com/stretchr/testify/require"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/fixture"
 )
 
@@ -176,7 +178,7 @@ func TestMissingDomain(t *testing.T) {
 
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
-	require.NoError(t, lookup.Error)
+	require.True(t, errors.Is(lookup.Error, domain.ErrDomainDoesNotExist))
 	require.Nil(t, lookup.Domain)
 }
 
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index 28d9cac8c..e7cd76e7e 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -18,9 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
-// ErrDomainDoesNotExist is returned by
-var ErrDomainDoesNotExist = errors.New("domain does not exist")
-
 // Gitlab source represent a new domains configuration source. We fetch all the
 // information about domains from GitLab instance.
 type Gitlab struct {
@@ -58,7 +55,7 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 
 	// Domain does not exist
 	if lookup.Domain == nil {
-		return nil, ErrDomainDoesNotExist
+		return nil, domain.ErrDomainDoesNotExist
 	}
 
 	// TODO introduce a second-level cache for domains, invalidate using etags
-- 
GitLab


From 0501f0ad4c1f99de77d0547379c9c30c75cb007c Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Wed, 9 Dec 2020 22:13:36 +1100
Subject: [PATCH 3/5] Serve 502 when the resolver returns an error

-- 
GitLab


From ce03560d89e7315e24e6ade1fcb5ec14f0c82e39 Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Thu, 10 Dec 2020 17:38:29 +1100
Subject: [PATCH 4/5] Add more test cases for resolver errors

Fix imports and enable test
---
 app.go                                       |  3 ++-
 internal/domain/domain.go                    | 11 ++++-------
 internal/logging/logging.go                  | 12 ++++++------
 internal/serving/disk/reader.go              |  1 -
 internal/source/disk/custom.go               |  4 ++--
 internal/source/disk/domain_test.go          | 15 +++++++--------
 internal/source/gitlab/cache/retriever.go    | 11 ++++++-----
 internal/source/gitlab/client/client.go      |  7 +++++--
 internal/source/gitlab/client/client_test.go |  3 +--
 internal/source/gitlab/gitlab.go             |  2 +-
 10 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/app.go b/app.go
index ec0c36a58..70ad6e1e8 100644
--- a/app.go
+++ b/app.go
@@ -33,6 +33,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/tlsconfig"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
@@ -152,7 +153,7 @@ func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 		// if we could not retrieve a domain from domains source we break the
 		// middleware chain and simply respond with 502 after logging this
 		host, d, err := a.getHostAndDomain(r)
-		if err != nil && !errors.Is(err, domain.ErrDomainDoesNotExist) {
+		if err != nil && !errors.Is(err, client.ErrDomainDoesNotExist) {
 			metrics.DomainsSourceFailures.Inc()
 			log.WithError(err).Error("could not fetch domain information from a source")
 
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index f70c4fea8..5f32df0be 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -11,12 +11,9 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
-// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
-// for a domain could not be resolved
-var ErrDomainDoesNotExist = errors.New("domain does not exist")
-
 // Domain is a domain that gitlab-pages can serve.
 type Domain struct {
 	Name            string
@@ -54,7 +51,7 @@ func (d *Domain) isUnconfigured() bool {
 }
 func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
 	if d.Resolver == nil {
-		return nil, ErrDomainDoesNotExist
+		return nil, client.ErrDomainDoesNotExist
 	}
 
 	return d.Resolver.Resolve(r)
@@ -145,7 +142,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
 func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, ErrDomainDoesNotExist) {
+		if errors.Is(err, client.ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return true
@@ -168,7 +165,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, ErrDomainDoesNotExist) {
+		if errors.Is(err, client.ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return
diff --git a/internal/logging/logging.go b/internal/logging/logging.go
index 9ac3c8631..43fe65e6c 100644
--- a/internal/logging/logging.go
+++ b/internal/logging/logging.go
@@ -4,7 +4,6 @@ import (
 	"net/http"
 
 	"github.com/sirupsen/logrus"
-
 	"gitlab.com/gitlab-org/labkit/log"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
@@ -61,13 +60,14 @@ func getExtraLogFields(r *http.Request) log.Fields {
 
 	if d := request.GetDomain(r); d != nil {
 		lp, err := d.GetLookupPath(r)
-		if lp != nil {
-			logFields["pages_project_serving_type"] = lp.ServingType
-			logFields["pages_project_prefix"] = lp.Prefix
-			logFields["pages_project_id"] = lp.ProjectID
-		} else if err != nil {
+		if err != nil {
 			logFields["error"] = err.Error()
+			return logFields
 		}
+
+		logFields["pages_project_serving_type"] = lp.ServingType
+		logFields["pages_project_prefix"] = lp.Prefix
+		logFields["pages_project_id"] = lp.ProjectID
 	}
 
 	return logFields
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index df6b6d945..12223bad4 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-
 	"gitlab.com/gitlab-org/labkit/errortracking"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go
index 037abfeee..93717028e 100644
--- a/internal/source/disk/custom.go
+++ b/internal/source/disk/custom.go
@@ -3,9 +3,9 @@ package disk
 import (
 	"net/http"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/local"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 type customProjectResolver struct {
@@ -16,7 +16,7 @@ type customProjectResolver struct {
 
 func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) {
 	if p.config == nil {
-		return nil, domain.ErrDomainDoesNotExist
+		return nil, client.ErrDomainDoesNotExist
 	}
 
 	lookupPath := &serving.LookupPath{
diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go
index 66ce1ba29..323b45e5f 100644
--- a/internal/source/disk/domain_test.go
+++ b/internal/source/disk/domain_test.go
@@ -73,7 +73,7 @@ func TestGroupServeHTTP(t *testing.T) {
 	defer cleanup()
 
 	t.Run("group.test.io", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io") })
-	// t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") })
+	t.Run("group.test.io:8080", func(t *testing.T) { testGroupServeHTTPHost(t, "group.test.io:8080") })
 }
 
 func TestDomainServeHTTP(t *testing.T) {
@@ -435,13 +435,12 @@ func TestGroupCertificate(t *testing.T) {
 }
 
 func TestDomainNoCertificate(t *testing.T) {
-	testDomain :=
-		&domain.Domain{
-			Resolver: &customProjectResolver{
-				path:   "group/project2/public",
-				config: &domainConfig{Domain: "test.domain.com"},
-			},
-		}
+	testDomain := &domain.Domain{
+		Resolver: &customProjectResolver{
+			path:   "group/project2/public",
+			config: &domainConfig{Domain: "test.domain.com"},
+		},
+	}
 
 	tls, err := testDomain.EnsureCertificate()
 	require.Nil(t, tls)
diff --git a/internal/source/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index 8b3e47232..953c42e91 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -7,8 +7,8 @@ import (
 
 	log "github.com/sirupsen/logrus"
 
-	"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,11 +55,12 @@ 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) {
-				time.Sleep(r.maxRetrievalInterval)
-				continue
+			if lookup.Error == nil || errors.Is(lookup.Error, client.ErrDomainDoesNotExist) {
+				// do not retry if the domain does not exist
+				break
 			}
-			break
+
+			time.Sleep(r.maxRetrievalInterval)
 		}
 
 		response <- lookup
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index 524eb8a43..eb956d336 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -13,7 +13,6 @@ import (
 
 	"github.com/dgrijalva/jwt-go"
 
-	"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"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
@@ -24,6 +23,10 @@ import (
 // or a 401 given that the credentials used are wrong
 const ConnectionErrorMsg = "failed to connect to internal Pages API"
 
+// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
+// for a domain could not be resolved
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Client is a HTTP client to access Pages internal API
 type Client struct {
 	secretKey      []byte
@@ -94,7 +97,7 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	}
 
 	if resp == nil {
-		return api.Lookup{Name: host, Error: domain.ErrDomainDoesNotExist}
+		return api.Lookup{Name: host, Error: ErrDomainDoesNotExist}
 	}
 
 	// ensure that entire response body has been read and close it, to make it
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 153b9372e..ee5a5ab3f 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -14,7 +14,6 @@ import (
 	"github.com/dgrijalva/jwt-go"
 	"github.com/stretchr/testify/require"
 
-	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/fixture"
 )
 
@@ -178,7 +177,7 @@ func TestMissingDomain(t *testing.T) {
 
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
-	require.True(t, errors.Is(lookup.Error, domain.ErrDomainDoesNotExist))
+	require.True(t, errors.Is(lookup.Error, ErrDomainDoesNotExist))
 	require.Nil(t, lookup.Domain)
 }
 
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index e7cd76e7e..c31d94663 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -55,7 +55,7 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 
 	// Domain does not exist
 	if lookup.Domain == nil {
-		return nil, domain.ErrDomainDoesNotExist
+		return nil, client.ErrDomainDoesNotExist
 	}
 
 	// TODO introduce a second-level cache for domains, invalidate using etags
-- 
GitLab


From 09144137f206ab70fe29fb172c606b8309018fc7 Mon Sep 17 00:00:00 2001
From: Jaime Martinez <jmartinez@gitlab.com>
Date: Tue, 19 Jan 2021 16:05:39 +1100
Subject: [PATCH 5/5] Simplify domain and resolver checks

Return domain not found

Returns ErrDomainDoesNotExist when the lookup path
cannot be found for a specific project.

Closes https://gitlab.com/gitlab-org/gitlab-pages/issues/353

Return ErrDomainDoesNotExist in group resolver
---
 app.go                                       |  3 +-
 internal/domain/domain.go                    | 54 +++++++-------------
 internal/domain/domain_test.go               | 20 ++++----
 internal/logging/logging.go                  |  1 -
 internal/logging/logging_test.go             | 23 ++++-----
 internal/source/disk/custom.go               |  4 +-
 internal/source/disk/domain_test.go          |  6 ++-
 internal/source/disk/group.go                |  5 +-
 internal/source/gitlab/cache/cache_test.go   | 28 +++++-----
 internal/source/gitlab/cache/retriever.go    |  4 +-
 internal/source/gitlab/client/client.go      |  7 +--
 internal/source/gitlab/client/client_test.go |  3 +-
 internal/source/gitlab/gitlab.go             | 11 +---
 13 files changed, 69 insertions(+), 100 deletions(-)

diff --git a/app.go b/app.go
index 70ad6e1e8..ec0c36a58 100644
--- a/app.go
+++ b/app.go
@@ -33,7 +33,6 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/zip"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/tlsconfig"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
@@ -153,7 +152,7 @@ func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 		// if we could not retrieve a domain from domains source we break the
 		// middleware chain and simply respond with 502 after logging this
 		host, d, err := a.getHostAndDomain(r)
-		if err != nil && !errors.Is(err, client.ErrDomainDoesNotExist) {
+		if err != nil && !errors.Is(err, domain.ErrDomainDoesNotExist) {
 			metrics.DomainsSourceFailures.Inc()
 			log.WithError(err).Error("could not fetch domain information from a source")
 
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index 5f32df0be..636b1bbd3 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -11,9 +11,12 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
+// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
+// for a domain could not be resolved
+var ErrDomainDoesNotExist = errors.New("domain does not exist")
+
 // Domain is a domain that gitlab-pages can serve.
 type Domain struct {
 	Name            string
@@ -42,16 +45,9 @@ func (d *Domain) String() string {
 	return d.Name
 }
 
-func (d *Domain) isUnconfigured() bool {
-	if d == nil {
-		return true
-	}
-
-	return d.Resolver == nil
-}
 func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
-	if d.Resolver == nil {
-		return nil, client.ErrDomainDoesNotExist
+	if d == nil {
+		return nil, ErrDomainDoesNotExist
 	}
 
 	return d.Resolver.Resolve(r)
@@ -60,10 +56,6 @@ func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
 // GetLookupPath returns a project details based on the request. It returns nil
 // if project does not exist.
 func (d *Domain) GetLookupPath(r *http.Request) (*serving.LookupPath, error) {
-	if d.isUnconfigured() {
-		return nil, errors.New("not configured")
-	}
-
 	servingReq, err := d.resolve(r)
 	if err != nil {
 		return nil, err
@@ -111,16 +103,17 @@ func (d *Domain) GetProjectID(r *http.Request) uint64 {
 
 // HasLookupPath figures out if the project exists that the user tries to access
 func (d *Domain) HasLookupPath(r *http.Request) bool {
-	if _, err := d.GetLookupPath(r); err != nil {
+	if d == nil {
 		return false
 	}
+	_, err := d.GetLookupPath(r)
 
-	return true
+	return err == nil
 }
 
 // EnsureCertificate parses the PEM-encoded certificate for the domain
 func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
-	if d.isUnconfigured() || len(d.CertificateKey) == 0 || len(d.CertificateCert) == 0 {
+	if d == nil || len(d.CertificateKey) == 0 || len(d.CertificateCert) == 0 {
 		return nil, errors.New("tls certificates can be loaded only for pages with configuration")
 	}
 
@@ -142,7 +135,7 @@ func (d *Domain) EnsureCertificate() (*tls.Certificate, error) {
 func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return true
@@ -153,11 +146,6 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 		return true
 	}
 
-	if request.LookupPath == nil {
-		// return and let ServeNotFoundHTTP serve the 404 page
-		return false
-	}
-
 	return request.ServeFileHTTP(w, r)
 }
 
@@ -165,7 +153,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool {
 func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 	request, err := d.resolve(r)
 	if err != nil {
-		if errors.Is(err, client.ErrDomainDoesNotExist) {
+		if errors.Is(err, ErrDomainDoesNotExist) {
 			// serve generic 404
 			httperrors.Serve404(w)
 			return
@@ -176,11 +164,6 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if request.LookupPath == nil {
-		httperrors.Serve404(w)
-		return
-	}
-
 	request.ServeNotFoundHTTP(w, r)
 }
 
@@ -194,16 +177,17 @@ func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request)
 
 	namespaceDomain, err := d.Resolver.Resolve(clonedReq)
 	if err != nil {
+		if errors.Is(err, ErrDomainDoesNotExist) {
+			// serve generic 404
+			httperrors.Serve404(w)
+			return
+		}
+
 		errortracking.Capture(err, errortracking.WithRequest(r))
 		httperrors.Serve503(w)
 		return
 	}
 
-	if namespaceDomain.LookupPath == nil {
-		httperrors.Serve404(w)
-		return
-	}
-
 	// for namespace domains that have no access control enabled
 	if !namespaceDomain.LookupPath.HasAccessControl {
 		namespaceDomain.ServeNotFoundHTTP(w, r)
@@ -222,7 +206,7 @@ func (d *Domain) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	if d.IsNamespaceProject(r) && lookupPath.HasAccessControl {
+	if d.IsNamespaceProject(r) && !lookupPath.HasAccessControl {
 		d.ServeNotFoundHTTP(w, r)
 		return
 	}
diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go
index 2df1d9929..f053001a4 100644
--- a/internal/domain/domain_test.go
+++ b/internal/domain/domain_test.go
@@ -46,33 +46,31 @@ func TestIsHTTPSOnly(t *testing.T) {
 	}{
 		{
 			name: "Custom domain with HTTPS-only enabled",
-			domain: &Domain{
-				Resolver: &stubbedResolver{
+			domain: New("custom-domain", "", "",
+				&stubbedResolver{
 					project: &serving.LookupPath{
 						Path:        "group/project/public",
 						IsHTTPSOnly: true,
 					},
-				},
-			},
+				}),
 			url:      "http://custom-domain",
 			expected: true,
 		},
 		{
 			name: "Custom domain with HTTPS-only disabled",
-			domain: &Domain{
-				Resolver: &stubbedResolver{
+			domain: New("custom-domain", "", "",
+				&stubbedResolver{
 					project: &serving.LookupPath{
 						Path:        "group/project/public",
 						IsHTTPSOnly: false,
 					},
-				},
-			},
+				}),
 			url:      "http://custom-domain",
 			expected: false,
 		},
 		{
 			name:     "Unknown project",
-			domain:   &Domain{},
+			domain:   New("", "", "", &stubbedResolver{err: ErrDomainDoesNotExist}),
 			url:      "http://test-domain/project",
 			expected: false,
 		},
@@ -90,7 +88,7 @@ func TestPredefined404ServeHTTP(t *testing.T) {
 	cleanup := setUpTests(t)
 	defer cleanup()
 
-	testDomain := &Domain{}
+	testDomain := New("", "", "", &stubbedResolver{err: ErrDomainDoesNotExist})
 
 	testhelpers.AssertHTTP404(t, serveFileOrNotFound(testDomain), "GET", "http://group.test.io/not-existing-file", nil, "The page you're looking for could not be found")
 }
@@ -192,7 +190,7 @@ func TestServeNamespaceNotFound(t *testing.T) {
 			domain: "group.404.gitlab-example.com",
 			path:   "/unknown",
 			resolver: &stubbedResolver{
-				project: nil,
+				err:     ErrDomainDoesNotExist,
 				subpath: "/",
 			},
 			expectedResponse: "The page you're looking for could not be found.",
diff --git a/internal/logging/logging.go b/internal/logging/logging.go
index 43fe65e6c..4dcf66d29 100644
--- a/internal/logging/logging.go
+++ b/internal/logging/logging.go
@@ -61,7 +61,6 @@ func getExtraLogFields(r *http.Request) log.Fields {
 	if d := request.GetDomain(r); d != nil {
 		lp, err := d.GetLookupPath(r)
 		if err != nil {
-			logFields["error"] = err.Error()
 			return logFields
 		}
 
diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go
index 2ed2b6ec0..9079cc9dc 100644
--- a/internal/logging/logging_test.go
+++ b/internal/logging/logging_test.go
@@ -18,13 +18,15 @@ func (f lookupPathFunc) Resolve(r *http.Request) (*serving.Request, error) {
 }
 
 func TestGetExtraLogFields(t *testing.T) {
-	domainWithResolver := domain.New("", "", "", lookupPathFunc(func(*http.Request) *serving.LookupPath {
-		return &serving.LookupPath{
-			ServingType: "file",
-			ProjectID:   100,
-			Prefix:      "/prefix",
-		}
-	}))
+	domainWithResolver := &domain.Domain{
+		Resolver: lookupPathFunc(func(*http.Request) *serving.LookupPath {
+			return &serving.LookupPath{
+				ServingType: "file",
+				ProjectID:   100,
+				Prefix:      "/prefix",
+			}
+		}),
+	}
 
 	tests := []struct {
 		name                  string
@@ -36,7 +38,6 @@ func TestGetExtraLogFields(t *testing.T) {
 		expectedProjectID     interface{}
 		expectedProjectPrefix interface{}
 		expectedServingType   interface{}
-		expectedErrMsg        interface{}
 	}{
 		{
 			name:                  "https",
@@ -61,15 +62,14 @@ func TestGetExtraLogFields(t *testing.T) {
 			expectedServingType:   "file",
 		},
 		{
-			name:                "domain_without_resolver",
+			name:                "domain_without_resolved",
 			scheme:              request.SchemeHTTP,
 			host:                "githost.io",
-			domain:              &domain.Domain{},
+			domain:              nil,
 			expectedHTTPS:       false,
 			expectedHost:        "githost.io",
 			expectedProjectID:   nil,
 			expectedServingType: nil,
-			expectedErrMsg:      "not configured",
 		},
 		{
 			name:                "no_domain",
@@ -97,7 +97,6 @@ func TestGetExtraLogFields(t *testing.T) {
 			require.Equal(t, tt.expectedProjectID, got["pages_project_id"])
 			require.Equal(t, tt.expectedProjectPrefix, got["pages_project_prefix"])
 			require.Equal(t, tt.expectedServingType, got["pages_project_serving_type"])
-			require.Equal(t, tt.expectedErrMsg, got["error"])
 		})
 	}
 }
diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go
index 93717028e..037abfeee 100644
--- a/internal/source/disk/custom.go
+++ b/internal/source/disk/custom.go
@@ -3,9 +3,9 @@ package disk
 import (
 	"net/http"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/local"
-	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/client"
 )
 
 type customProjectResolver struct {
@@ -16,7 +16,7 @@ type customProjectResolver struct {
 
 func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) {
 	if p.config == nil {
-		return nil, client.ErrDomainDoesNotExist
+		return nil, domain.ErrDomainDoesNotExist
 	}
 
 	lookupPath := &serving.LookupPath{
diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go
index 323b45e5f..abffddb4b 100644
--- a/internal/source/disk/domain_test.go
+++ b/internal/source/disk/domain_test.go
@@ -162,8 +162,10 @@ func TestIsHTTPSOnly(t *testing.T) {
 			expected: false,
 		},
 		{
-			name:     "Unknown project",
-			domain:   &domain.Domain{},
+			name: "Unknown project",
+			domain: &domain.Domain{
+				Resolver: &Group{},
+			},
 			url:      "http://test-domain/project",
 			expected: false,
 		},
diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go
index b12727aa6..9499df466 100644
--- a/internal/source/disk/group.go
+++ b/internal/source/disk/group.go
@@ -6,6 +6,7 @@ import (
 	"path/filepath"
 	"strings"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/host"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/local"
@@ -82,9 +83,7 @@ func (g *Group) Resolve(r *http.Request) (*serving.Request, error) {
 	projectConfig, prefix, projectPath, subPath := g.getProjectConfigWithSubpath(r)
 
 	if projectConfig == nil {
-		// it is not an error when project does not exist, in that case
-		// serving.Request.LookupPath is nil.
-		return &serving.Request{Serving: local.Instance()}, nil
+		return nil, domain.ErrDomainDoesNotExist
 	}
 
 	lookupPath := &serving.LookupPath{
diff --git a/internal/source/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go
index 757adb2b7..7ed56f5a6 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 clientMock struct {
+type client struct {
 	counter uint64
 	lookups chan uint64
 	domain  chan string
 	failure error
 }
 
-func (c *clientMock) GetLookup(ctx context.Context, _ string) api.Lookup {
+func (c *client) GetLookup(ctx context.Context, _ string) api.Lookup {
 	lookup := api.Lookup{}
 	if c.failure == nil {
 		lookup.Name = <-c.domain
@@ -33,11 +33,11 @@ func (c *clientMock) GetLookup(ctx context.Context, _ string) api.Lookup {
 	return lookup
 }
 
-func (c *clientMock) Status() error {
+func (c *client) Status() error {
 	return nil
 }
 
-func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *clientMock)) {
+func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*Cache, *client)) {
 	var chanSize int
 
 	if config.buffered {
@@ -46,7 +46,7 @@ func withTestCache(config resolverConfig, cacheConfig *cacheConfig, block func(*
 		chanSize = 0
 	}
 
-	resolver := &clientMock{
+	resolver := &client{
 		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 *clientMock) {
+		withTestCache(resolverConfig{buffered: true}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{buffered: false}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{failure: err}, &cc, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, &cc, func(cache *Cache, resolver *client) {
 			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 *clientMock) {
+		withTestCache(resolverConfig{}, nil, func(cache *Cache, resolver *client) {
 			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 953c42e91..eddb02cb9 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -7,8 +7,8 @@ import (
 
 	log "github.com/sirupsen/logrus"
 
+	"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,7 +55,7 @@ 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, client.ErrDomainDoesNotExist) {
+			if lookup.Error == nil || errors.Is(lookup.Error, domain.ErrDomainDoesNotExist) {
 				// do not retry if the domain does not exist
 				break
 			}
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index eb956d336..524eb8a43 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -13,6 +13,7 @@ import (
 
 	"github.com/dgrijalva/jwt-go"
 
+	"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"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
@@ -23,10 +24,6 @@ import (
 // or a 401 given that the credentials used are wrong
 const ConnectionErrorMsg = "failed to connect to internal Pages API"
 
-// ErrDomainDoesNotExist returned when a domain is not found or when a lookup path
-// for a domain could not be resolved
-var ErrDomainDoesNotExist = errors.New("domain does not exist")
-
 // Client is a HTTP client to access Pages internal API
 type Client struct {
 	secretKey      []byte
@@ -97,7 +94,7 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	}
 
 	if resp == nil {
-		return api.Lookup{Name: host, Error: ErrDomainDoesNotExist}
+		return api.Lookup{Name: host, Error: domain.ErrDomainDoesNotExist}
 	}
 
 	// ensure that entire response body has been read and close it, to make it
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index ee5a5ab3f..153b9372e 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/dgrijalva/jwt-go"
 	"github.com/stretchr/testify/require"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/fixture"
 )
 
@@ -177,7 +178,7 @@ func TestMissingDomain(t *testing.T) {
 
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
-	require.True(t, errors.Is(lookup.Error, ErrDomainDoesNotExist))
+	require.True(t, errors.Is(lookup.Error, domain.ErrDomainDoesNotExist))
 	require.Nil(t, lookup.Domain)
 }
 
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index c31d94663..d164460ab 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -2,7 +2,6 @@ package gitlab
 
 import (
 	"context"
-	"errors"
 	"net/http"
 	"path"
 	"strings"
@@ -53,11 +52,6 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 		return nil, lookup.Error
 	}
 
-	// Domain does not exist
-	if lookup.Domain == nil {
-		return nil, client.ErrDomainDoesNotExist
-	}
-
 	// TODO introduce a second-level cache for domains, invalidate using etags
 	// from first-level cache
 	d := domain.New(name, lookup.Domain.Certificate, lookup.Domain.Key, g)
@@ -96,10 +90,7 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) {
 		}
 	}
 
-	// TODO improve code around default serving, when `disk` serving gets removed
-	// https://gitlab.com/gitlab-org/gitlab-pages/issues/353
-	return &serving.Request{Serving: defaultServing()},
-		errors.New("could not match lookup path")
+	return nil, domain.ErrDomainDoesNotExist
 }
 
 // IsReady returns the value of Gitlab `isReady` which is updated by `Poll`.
-- 
GitLab