Merge branch '494-handle-resolver-error' into 'master'

Handle Resolver errors when serving content

Closes #353 and #494

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

13 files changed, 124 insertions, 107 deletions

diff --git a/app.go b/app.go
index 1352b630..ec0c36a5 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 deff2cc5..636b1bbd 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -7,11 +7,16 @@ 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"
 )
 
+// 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
@@ -25,51 +30,44 @@ 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
 }
 
-func (d *Domain) isUnconfigured() bool {
+func (d *Domain) resolve(r *http.Request) (*serving.Request, error) {
 	if d == nil {
-		return true
+		return nil, ErrDomainDoesNotExist
 	}
 
-	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()}
-	}
-
-	return request
+	return d.Resolver.Resolve(r)
 }
 
 // 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 {
-	if d.isUnconfigured() {
-		return nil
+func (d *Domain) GetLookupPath(r *http.Request) (*serving.LookupPath, error) {
+	servingReq, err := d.resolve(r)
+	if err != nil {
+		return nil, err
 	}
 
-	request := d.resolve(r)
-
-	if request == nil {
-		return nil
-	}
-
-	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 +76,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 +85,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 +94,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,12 +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 {
-	return d.GetLookupPath(r) != nil
+	if d == nil {
+		return false
+	}
+	_, err := d.GetLookupPath(r)
+
+	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")
 	}
 
@@ -130,27 +133,37 @@ 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
-		httperrors.Serve404(w)
+	request, err := d.resolve(r)
+	if err != nil {
+		if errors.Is(err, ErrDomainDoesNotExist) {
+			// serve generic 404
+			httperrors.Serve404(w)
+			return true
+		}
+
+		errortracking.Capture(err, errortracking.WithRequest(r))
+		httperrors.Serve503(w)
 		return true
 	}
 
-	request := d.resolve(r)
-
 	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, 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,8 +176,15 @@ 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 {
-		httperrors.Serve404(w)
+	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
 	}
 
@@ -180,11 +200,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/domain/domain_test.go b/internal/domain/domain_test.go
index 2df1d992..f053001a 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 9a3629ff..4dcf66d2 100644
--- a/internal/logging/logging.go
+++ b/internal/logging/logging.go
@@ -59,11 +59,14 @@ func getExtraLogFields(r *http.Request) log.Fields {
 	}
 
 	if d := request.GetDomain(r); d != nil {
-		if lp := d.GetLookupPath(r); lp != nil {
-			logFields["pages_project_serving_type"] = lp.ServingType
-			logFields["pages_project_prefix"] = lp.Prefix
-			logFields["pages_project_id"] = lp.ProjectID
+		lp, err := d.GetLookupPath(r)
+		if err != nil {
+			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/logging/logging_test.go b/internal/logging/logging_test.go
index e87a8c0d..9079cc9d 100644
--- a/internal/logging/logging_test.go
+++ b/internal/logging/logging_test.go
@@ -65,7 +65,7 @@ func TestGetExtraLogFields(t *testing.T) {
 			name:                "domain_without_resolved",
 			scheme:              request.SchemeHTTP,
 			host:                "githost.io",
-			domain:              &domain.Domain{},
+			domain:              nil,
 			expectedHTTPS:       false,
 			expectedHost:        "githost.io",
 			expectedProjectID:   nil,
diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go
index fb2aafcd..037abfee 100644
--- a/internal/source/disk/custom.go
+++ b/internal/source/disk/custom.go
@@ -3,6 +3,7 @@ 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"
 )
@@ -14,6 +15,10 @@ type customProjectResolver struct {
 }
 
 func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) {
+	if p.config == nil {
+		return nil, domain.ErrDomainDoesNotExist
+	}
+
 	lookupPath := &serving.LookupPath{
 		ServingType:        "file",
 		Prefix:             "/",
diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go
index 1c81ba22..abffddb4 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,
 		},
@@ -421,7 +423,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")
 }
diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go
index b12727aa..9499df46 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/disk/map.go b/internal/source/disk/map.go
index 874565a6..0413d409 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/gitlab/cache/retriever.go b/internal/source/gitlab/cache/retriever.go
index de37c231..eddb02cb 100644
--- a/internal/source/gitlab/cache/retriever.go
+++ b/internal/source/gitlab/cache/retriever.go
@@ -7,6 +7,7 @@ import (
 
 	log "github.com/sirupsen/logrus"
 
+	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api"
 )
 
@@ -46,20 +47,20 @@ 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 {
-				time.Sleep(r.maxRetrievalInterval)
-			} else {
+			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
 				break
 			}
+
+			time.Sleep(r.maxRetrievalInterval)
 		}
 
 		response <- lookup
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index b11ea2cb..524eb8a4 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"
@@ -93,7 +94,7 @@ func (gc *Client) GetLookup(ctx context.Context, host string) api.Lookup {
 	}
 
 	if resp == nil {
-		return api.Lookup{Name: host}
+		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 6d4ce814..153b9372 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 67c4eb6b..d164460a 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,21 +52,11 @@ func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
 		return nil, lookup.Error
 	}
 
-	// Domain does not exist
-	if lookup.Domain == nil {
-		return nil, nil
-	}
-
 	// 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 +67,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)
@@ -101,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`.