diff options
-rw-r--r-- | app.go | 3 | ||||
-rw-r--r-- | internal/domain/domain.go | 54 | ||||
-rw-r--r-- | internal/domain/domain_test.go | 20 | ||||
-rw-r--r-- | internal/logging/logging.go | 1 | ||||
-rw-r--r-- | internal/logging/logging_test.go | 23 | ||||
-rw-r--r-- | internal/source/disk/custom.go | 4 | ||||
-rw-r--r-- | internal/source/disk/domain_test.go | 6 | ||||
-rw-r--r-- | internal/source/disk/group.go | 5 | ||||
-rw-r--r-- | internal/source/gitlab/cache/cache_test.go | 28 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 4 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 7 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_test.go | 3 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 11 |
13 files changed, 69 insertions, 100 deletions
@@ -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 5f32df0b..636b1bbd 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 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 43fe65e6..4dcf66d2 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 2ed2b6ec..9079cc9d 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 93717028..037abfee 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 323b45e5..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, }, 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/gitlab/cache/cache_test.go b/internal/source/gitlab/cache/cache_test.go index 757adb2b..7ed56f5a 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 953c42e9..eddb02cb 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 eb956d33..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" @@ -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 ee5a5ab3..153b9372 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 c31d9466..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,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`. |