diff options
author | Vladimir Shushlin <vshushlin@gitlab.com> | 2021-01-22 15:53:17 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2021-01-22 15:53:17 +0300 |
commit | 98564f93e34e9cf74b2db8b3cd03f6addf60d005 (patch) | |
tree | 061a87b6271c8efc57cb3dadf38cff258749c1b9 | |
parent | 4191fb807f4350a07cfd9b8328fbf56dad4bf13f (diff) | |
parent | 09144137f206ab70fe29fb172c606b8309018fc7 (diff) |
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
-rw-r--r-- | app.go | 7 | ||||
-rw-r--r-- | internal/domain/domain.go | 114 | ||||
-rw-r--r-- | internal/domain/domain_test.go | 20 | ||||
-rw-r--r-- | internal/logging/logging.go | 11 | ||||
-rw-r--r-- | internal/logging/logging_test.go | 2 | ||||
-rw-r--r-- | internal/source/disk/custom.go | 5 | ||||
-rw-r--r-- | internal/source/disk/domain_test.go | 8 | ||||
-rw-r--r-- | internal/source/disk/group.go | 5 | ||||
-rw-r--r-- | internal/source/disk/map.go | 17 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 13 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 3 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_test.go | 4 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 22 |
13 files changed, 124 insertions, 107 deletions
@@ -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`. |