From dc54f4215620da74b70dbd07d0f1369e409d3e0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 8 Oct 2020 11:38:42 +0200 Subject: Log serving type used for requests This extends our structured logging with information about how the given request was served --- internal/domain/domain.go | 30 ++++---------- internal/logging/logging.go | 18 ++++---- internal/logging/logging_test.go | 86 ++++++++++++++++++++++++++++++--------- internal/serving/lookup_path.go | 1 + internal/source/disk/custom.go | 1 + internal/source/disk/group.go | 1 + internal/source/gitlab/factory.go | 3 +- 7 files changed, 90 insertions(+), 50 deletions(-) diff --git a/internal/domain/domain.go b/internal/domain/domain.go index d3daa143..deff2cc5 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -53,6 +53,10 @@ func (d *Domain) resolve(r *http.Request) *serving.Request { // 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 + } + request := d.resolve(r) if request == nil { @@ -65,10 +69,6 @@ func (d *Domain) GetLookupPath(r *http.Request) *serving.LookupPath { // 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 d.isUnconfigured() { - return false - } - if lookupPath := d.GetLookupPath(r); lookupPath != nil { return lookupPath.IsHTTPSOnly } @@ -78,10 +78,6 @@ 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 d.isUnconfigured() { - return false - } - if lookupPath := d.GetLookupPath(r); lookupPath != nil { return lookupPath.HasAccessControl } @@ -91,10 +87,6 @@ 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 d.isUnconfigured() { - return false - } - if lookupPath := d.GetLookupPath(r); lookupPath != nil { return lookupPath.IsNamespaceProject } @@ -104,10 +96,6 @@ 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 d.isUnconfigured() { - return 0 - } - if lookupPath := d.GetLookupPath(r); lookupPath != nil { return lookupPath.ProjectID } @@ -117,10 +105,6 @@ 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 d.isUnconfigured() { - return false - } - return d.GetLookupPath(r) != nil } @@ -146,7 +130,7 @@ 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) { + 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) @@ -160,7 +144,7 @@ func (d *Domain) ServeFileHTTP(w http.ResponseWriter, r *http.Request) bool { // ServeNotFoundHTTP serves the not found pages from the projects. func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { - if d.isUnconfigured() || !d.HasLookupPath(r) { + if !d.HasLookupPath(r) { httperrors.Serve404(w) return } @@ -196,7 +180,7 @@ 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.isUnconfigured() || !d.HasLookupPath(r) { + if !d.HasLookupPath(r) { httperrors.Serve404(w) return } diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 3432cdf7..6643e169 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -53,16 +53,20 @@ func getAccessLogger(format string) (*logrus.Logger, error) { // getExtraLogFields is used to inject additional fields into the // HTTP access logger middleware. func getExtraLogFields(r *http.Request) log.Fields { - var projectID uint64 - if d := request.GetDomain(r); d != nil { - projectID = d.GetProjectID(r) + logFields := log.Fields{ + "pages_https": request.IsHTTPS(r), + "pages_host": request.GetHost(r), } - return log.Fields{ - "pages_https": request.IsHTTPS(r), - "pages_host": request.GetHost(r), - "pages_project_id": projectID, + 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 + } } + + return logFields } // BasicAccessLogger configures the GitLab pages basic HTTP access logger middleware diff --git a/internal/logging/logging_test.go b/internal/logging/logging_test.go index ec8837b6..e87a8c0d 100644 --- a/internal/logging/logging_test.go +++ b/internal/logging/logging_test.go @@ -8,32 +8,78 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/request" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" ) +type lookupPathFunc func(*http.Request) *serving.LookupPath + +func (f lookupPathFunc) Resolve(r *http.Request) (*serving.Request, error) { + return &serving.Request{LookupPath: f(r)}, nil +} + func TestGetExtraLogFields(t *testing.T) { + domainWithResolver := &domain.Domain{ + Resolver: lookupPathFunc(func(*http.Request) *serving.LookupPath { + return &serving.LookupPath{ + ServingType: "file", + ProjectID: 100, + Prefix: "/prefix", + } + }), + } + tests := []struct { - name string - scheme string - host string - domain *domain.Domain + name string + scheme string + host string + domain *domain.Domain + expectedHTTPS interface{} + expectedHost interface{} + expectedProjectID interface{} + expectedProjectPrefix interface{} + expectedServingType interface{} }{ { - name: "https", - scheme: request.SchemeHTTPS, - host: "githost.io", - domain: &domain.Domain{}, + name: "https", + scheme: request.SchemeHTTPS, + host: "githost.io", + domain: domainWithResolver, + expectedHTTPS: true, + expectedHost: "githost.io", + expectedProjectID: uint64(100), + expectedProjectPrefix: "/prefix", + expectedServingType: "file", + }, + { + name: "http", + scheme: request.SchemeHTTP, + host: "githost.io", + domain: domainWithResolver, + expectedHTTPS: false, + expectedHost: "githost.io", + expectedProjectID: uint64(100), + expectedProjectPrefix: "/prefix", + expectedServingType: "file", }, { - name: "http", - scheme: request.SchemeHTTP, - host: "githost.io", - domain: &domain.Domain{}, + name: "domain_without_resolved", + scheme: request.SchemeHTTP, + host: "githost.io", + domain: &domain.Domain{}, + expectedHTTPS: false, + expectedHost: "githost.io", + expectedProjectID: nil, + expectedServingType: nil, }, { - name: "no_domain", - scheme: request.SchemeHTTP, - host: "githost.io", - domain: nil, + name: "no_domain", + scheme: request.SchemeHTTP, + host: "githost.io", + domain: nil, + expectedHTTPS: false, + expectedHost: "githost.io", + expectedProjectID: nil, + expectedServingType: nil, }, } @@ -46,9 +92,11 @@ func TestGetExtraLogFields(t *testing.T) { req = request.WithHostAndDomain(req, tt.host, tt.domain) got := getExtraLogFields(req) - require.Equal(t, got["pages_https"], tt.scheme == request.SchemeHTTPS) - require.Equal(t, got["pages_host"], tt.host) - require.Equal(t, got["pages_project_id"], uint64(0x0)) + require.Equal(t, tt.expectedHTTPS, got["pages_https"]) + require.Equal(t, tt.expectedHost, got["pages_host"]) + 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"]) }) } } diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index 4360358b..1aefe1b8 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -2,6 +2,7 @@ package serving // LookupPath holds a domain project configuration needed to handle a request type LookupPath struct { + ServingType string // Serving type being used, like `zip` Prefix string // Project prefix, for example, /my/project in group.gitlab.io/my/project/index.html Path string // Path is an internal and serving-specific location of a document IsNamespaceProject bool // IsNamespaceProject is DEPRECATED, see https://gitlab.com/gitlab-org/gitlab-pages/issues/272 diff --git a/internal/source/disk/custom.go b/internal/source/disk/custom.go index f120cb65..fb2aafcd 100644 --- a/internal/source/disk/custom.go +++ b/internal/source/disk/custom.go @@ -15,6 +15,7 @@ type customProjectResolver struct { func (p *customProjectResolver) Resolve(r *http.Request) (*serving.Request, error) { lookupPath := &serving.LookupPath{ + ServingType: "file", Prefix: "/", Path: p.path, IsNamespaceProject: false, diff --git a/internal/source/disk/group.go b/internal/source/disk/group.go index 161ffa02..b12727aa 100644 --- a/internal/source/disk/group.go +++ b/internal/source/disk/group.go @@ -88,6 +88,7 @@ func (g *Group) Resolve(r *http.Request) (*serving.Request, error) { } lookupPath := &serving.LookupPath{ + ServingType: "file", Prefix: prefix, Path: filepath.Join(g.name, projectPath, "public") + "/", IsNamespaceProject: projectConfig.NamespaceProject, diff --git a/internal/source/gitlab/factory.go b/internal/source/gitlab/factory.go index 39193a16..41f7ea56 100644 --- a/internal/source/gitlab/factory.go +++ b/internal/source/gitlab/factory.go @@ -15,8 +15,9 @@ import ( // https://gitlab.com/gitlab-org/gitlab-pages/issues/272 func fabricateLookupPath(size int, lookup api.LookupPath) *serving.LookupPath { return &serving.LookupPath{ - Prefix: lookup.Prefix, + ServingType: lookup.Source.Type, Path: lookup.Source.Path, + Prefix: lookup.Prefix, IsNamespaceProject: (lookup.Prefix == "/" && size > 1), IsHTTPSOnly: lookup.HTTPSOnly, HasAccessControl: lookup.AccessControl, -- cgit v1.2.3