diff options
-rw-r--r-- | app.go | 7 | ||||
-rw-r--r-- | internal/domain/domain.go | 35 | ||||
-rw-r--r-- | internal/serving/disk/reader.go | 1 | ||||
-rw-r--r-- | internal/source/disk/custom.go | 4 | ||||
-rw-r--r-- | internal/source/domains.go | 6 | ||||
-rw-r--r-- | internal/source/gitlab/cache/retriever.go | 13 | ||||
-rw-r--r-- | internal/source/gitlab/client/client.go | 49 | ||||
-rw-r--r-- | internal/source/gitlab/client/client_test.go | 4 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 5 |
9 files changed, 52 insertions, 72 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 33828616..f70c4fea 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 12223bad..df6b6d94 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 da30c3d5..037abfee 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 cbec8d94..cf81fab2 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 136fc206..8b3e4723 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 2042b8e3..524eb8a4 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 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 28d9cac8..e7cd76e7 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 |