Respond with 502 if a domain can not be retrieved from a source

8 files changed, 43 insertions, 59 deletions

diff --git a/app.go b/app.go
index 7c927dfe..f64421a4 100644
--- a/app.go
+++ b/app.go
@@ -61,7 +61,7 @@ func (a *theApp) ServeTLS(ch *tls.ClientHelloInfo) (*tls.Certificate, error) {
 		return nil, nil
 	}
 
-	if domain := a.domain(ch.ServerName); domain != nil {
+	if domain, _ := a.domain(ch.ServerName); domain != nil {
 		tls, _ := domain.EnsureCertificate()
 		return tls, nil
 	}
@@ -86,16 +86,18 @@ func (a *theApp) redirectToHTTPS(w http.ResponseWriter, r *http.Request, statusC
 	http.Redirect(w, r, u.String(), statusCode)
 }
 
-func (a *theApp) getHostAndDomain(r *http.Request) (host string, domain *domain.Domain) {
+func (a *theApp) getHostAndDomain(r *http.Request) (string, *domain.Domain, error) {
 	host, _, err := net.SplitHostPort(r.Host)
 	if err != nil {
 		host = r.Host
 	}
 
-	return host, a.domain(host)
+	domain, err := a.domain(host)
+
+	return host, domain, err
 }
 
-func (a *theApp) domain(host string) *domain.Domain {
+func (a *theApp) domain(host string) (*domain.Domain, error) {
 	return a.domains.GetDomain(host)
 }
 
@@ -163,7 +165,15 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht
 // downstream middlewares to use
 func (a *theApp) routingMiddleware(handler http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		host, domain := a.getHostAndDomain(r)
+		// 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 {
+			log.WithError(err).Error("could not fetch domain information from a source")
+
+			httperrors.Serve502(w)
+			return
+		}
 
 		r = request.WithHostAndDomain(r, host, domain)
 
@@ -289,7 +299,7 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler {
 }
 
 func (a *theApp) buildHandlerPipeline() (http.Handler, error) {
-	// Handlers should be applied in reverse order
+	// Handlers should be applied in a reverse order
 	handler := a.serveFileOrNotFoundHandler()
 	if !a.DisableCrossOriginRequests {
 		handler = corsHandler.Handler(handler)
diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index 320dcb11..d84fc690 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -198,14 +198,17 @@ func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.Res
 	http.Redirect(w, r, redirectURI, 302)
 }
 
-func (a *Auth) domainAllowed(domain string, domains *source.Domains) bool {
-	domainConfigured := (domain == a.pagesDomain) || strings.HasSuffix("."+domain, a.pagesDomain)
+func (a *Auth) domainAllowed(name string, domains *source.Domains) bool {
+	isConfigured := (name == a.pagesDomain) || strings.HasSuffix("."+name, a.pagesDomain)
 
-	if domainConfigured {
+	if isConfigured {
 		return true
 	}
 
-	return domains.HasDomain(domain)
+	domain, err := domains.GetDomain(name)
+
+	// domain exists and there is no error
+	return (domain != nil && err == nil)
 }
 
 func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWriter, r *http.Request, domains *source.Domains) bool {
diff --git a/internal/domain/domain.go b/internal/domain/domain.go
index f7eba5ca..28eb3196 100644
--- a/internal/domain/domain.go
+++ b/internal/domain/domain.go
@@ -162,10 +162,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 {
 	if d.isUnconfigured() || !d.HasLookupPath(r) {
-		// TODO: this seems to be wrong:
-		// as we should rather return false,
-		// and fallback to `ServeNotFoundHTTP`
-		// to handle this case
+		// TODO: this seems to be wrong: as we should rather return false, and
+		// fallback to `ServeNotFoundHTTP` to handle this case
 		httperrors.Serve404(w)
 		return true
 	}
diff --git a/internal/source/disk/disk.go b/internal/source/disk/disk.go
index a3ae8901..b79d222d 100644
--- a/internal/source/disk/disk.go
+++ b/internal/source/disk/disk.go
@@ -24,25 +24,16 @@ func New() *Disk {
 	}
 }
 
-// GetDomain returns a domain from the domains map
-func (d *Disk) GetDomain(host string) *domain.Domain {
+// GetDomain returns a domain from the domains map if it exists
+func (d *Disk) GetDomain(host string) (*domain.Domain, error) {
 	host = strings.ToLower(host)
-	d.lock.RLock()
-	defer d.lock.RUnlock()
-	domain, _ := d.dm[host]
 
-	return domain
-}
-
-// HasDomain checks for presence of a domain in the domains map
-func (d *Disk) HasDomain(host string) bool {
 	d.lock.RLock()
 	defer d.lock.RUnlock()
 
-	host = strings.ToLower(host)
-	_, isPresent := d.dm[host]
+	domain, _ := d.dm[host]
 
-	return isPresent
+	return domain, nil
 }
 
 // IsReady checks if the domains source is ready for work. The disk source is
diff --git a/internal/source/domains.go b/internal/source/domains.go
index 92bab663..f9dadef8 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -34,16 +34,10 @@ func NewDomains() *Domains {
 // sources here because it allows us to switch behavior and the domain source
 // 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 {
+func (d *Domains) GetDomain(name string) (*domain.Domain, error) {
 	return d.source(name).GetDomain(name)
 }
 
-// HasDomain checks if domain exists. It is using new and the legacy domains
-// source.
-func (d *Domains) HasDomain(name string) bool {
-	return d.source(name).HasDomain(name)
-}
-
 // Start starts the disk domain source. It is DEPRECATED, because we want to
 // remove it entirely when disk source gets removed.
 func (d *Domains) Read(rootDomain string) {
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index 1da50db8..64ec5f8d 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
@@ -14,27 +15,20 @@ type mockSource struct {
 	mock.Mock
 }
 
-func (m mockSource) GetDomain(name string) *domain.Domain {
+func (m mockSource) GetDomain(name string) (*domain.Domain, error) {
 	args := m.Called(name)
 
-	return args.Get(0).(*domain.Domain)
+	return args.Get(0).(*domain.Domain), args.Error(1)
 }
 
-func (m mockSource) HasDomain(name string) bool {
-	args := m.Called(name)
-
-	return args.Bool(0)
-}
-
-func TestSourceTransition(t *testing.T) {
+func TestHasDomain(t *testing.T) {
 	testDomain := newSourceDomains[0]
 
 	t.Run("when requesting a test domain", func(t *testing.T) {
 		newSource := new(mockSource)
 		newSource.On("GetDomain", testDomain).
-			Return(&domain.Domain{Name: testDomain}).
+			Return(&domain.Domain{Name: testDomain}, nil).
 			Once()
-		newSource.On("HasDomain", testDomain).Return(true).Once()
 		defer newSource.AssertExpectations(t)
 
 		domains := &Domains{
@@ -43,7 +37,6 @@ func TestSourceTransition(t *testing.T) {
 		}
 
 		domains.GetDomain(testDomain)
-		domains.HasDomain(testDomain)
 	})
 
 	t.Run("when requesting a non-test domain", func(t *testing.T) {
@@ -55,7 +48,9 @@ func TestSourceTransition(t *testing.T) {
 			gitlab: newSource,
 		}
 
-		assert.Nil(t, domains.GetDomain("some.test.io"))
-		assert.False(t, domains.HasDomain("some.test.io"))
+		domain, err := domains.GetDomain("domain.test.io")
+
+		require.NoError(t, err)
+		assert.Nil(t, domain)
 	})
 }
diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go
index b0efb01d..09cd06b9 100644
--- a/internal/source/gitlab/gitlab.go
+++ b/internal/source/gitlab/gitlab.go
@@ -1,6 +1,7 @@
 package gitlab
 
 import (
+	"errors"
 	"net/http"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
@@ -21,15 +22,8 @@ func New() *Gitlab {
 
 // GetDomain return a representation of a domain that we have fetched from
 // GitLab
-// It should return source.Lookup TODO
-func (g *Gitlab) GetDomain(name string) *domain.Domain {
-	return nil
-}
-
-// HasDomain checks if a domain is known to GitLab
-// TODO lookup status code etc.
-func (g *Gitlab) HasDomain(name string) bool {
-	return g.GetDomain(name) != nil
+func (g *Gitlab) GetDomain(name string) (*domain.Domain, error) {
+	return nil, errors.New("not implemented")
 }
 
 // Resolve is supposed to get the serving lookup path based on the request from
diff --git a/internal/source/source.go b/internal/source/source.go
index da63cbcc..4b43b8f4 100644
--- a/internal/source/source.go
+++ b/internal/source/source.go
@@ -4,6 +4,5 @@ import "gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 
 // Source represents an abstract interface of a domains configuration source.
 type Source interface {
-	GetDomain(string) *domain.Domain
-	HasDomain(string) bool
+	GetDomain(string) (*domain.Domain, error)
 }