diff options
author | Kamil TrzciĆski <ayufan@ayufan.eu> | 2020-10-27 06:49:07 +0300 |
---|---|---|
committer | Vladimir Shushlin <vshushlin@gitlab.com> | 2020-10-27 06:49:07 +0300 |
commit | 5be346a94902c7147f3b1571e6ebb2c1f4e98eb8 (patch) | |
tree | 1e6328dd475c3578a3910181d6e2a26f20285bef /internal | |
parent | b382faeec6491bb544d33549570610a476f597b7 (diff) |
Properly handle processing failures with `5xx`
Prior to this change ALL processing failures
unrelated to "file missing" would return `404`.
This is inaccurate.
Processing failures are failure of GitLab Pages
and `500` should be returned in such cases.
Diffstat (limited to 'internal')
-rw-r--r-- | internal/httperrors/httperrors.go | 14 | ||||
-rw-r--r-- | internal/httprange/http_reader.go | 5 | ||||
-rw-r--r-- | internal/httprange/http_reader_test.go | 8 | ||||
-rw-r--r-- | internal/httprange/resource.go | 3 | ||||
-rw-r--r-- | internal/httprange/resource_test.go | 3 | ||||
-rw-r--r-- | internal/serving/disk/local/serving_test.go | 84 | ||||
-rw-r--r-- | internal/serving/disk/reader.go | 81 | ||||
-rw-r--r-- | internal/serving/disk/serving.go | 6 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving_test.go | 32 | ||||
-rw-r--r-- | internal/vfs/errors.go | 18 | ||||
-rw-r--r-- | internal/vfs/local/vfs.go | 8 | ||||
-rw-r--r-- | internal/vfs/local/vfs_test.go | 4 | ||||
-rw-r--r-- | internal/vfs/zip/archive_test.go | 3 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 6 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 4 |
15 files changed, 221 insertions, 58 deletions
diff --git a/internal/httperrors/httperrors.go b/internal/httperrors/httperrors.go index 1ae5224b..476d270c 100644 --- a/internal/httperrors/httperrors.go +++ b/internal/httperrors/httperrors.go @@ -3,6 +3,10 @@ package httperrors import ( "fmt" "net/http" + + log "github.com/sirupsen/logrus" + + "gitlab.com/gitlab-org/labkit/errortracking" ) type content struct { @@ -177,6 +181,16 @@ func Serve500(w http.ResponseWriter) { serveErrorPage(w, content500) } +// Serve500WithRequest returns a 500 error response / HTML page to the http.ResponseWriter +func Serve500WithRequest(w http.ResponseWriter, r *http.Request, reason string, err error) { + log.WithFields(log.Fields{ + "host": r.Host, + "path": r.URL.Path, + }).WithError(err).Error(reason) + errortracking.Capture(err, errortracking.WithRequest(r)) + serveErrorPage(w, content500) +} + // Serve502 returns a 502 error response / HTML page to the http.ResponseWriter func Serve502(w http.ResponseWriter) { serveErrorPage(w, content502) diff --git a/internal/httprange/http_reader.go b/internal/httprange/http_reader.go index 44694e85..5dc0f693 100644 --- a/internal/httprange/http_reader.go +++ b/internal/httprange/http_reader.go @@ -14,6 +14,9 @@ import ( ) var ( + // ErrNotFound is returned when servers responds with 404 + ErrNotFound = errors.New("resource not found") + // ErrRangeRequestsNotSupported is returned by Seek and Read // when the remote server does not allow range requests (Accept-Ranges was not set) ErrRangeRequestsNotSupported = errors.New("range requests are not supported by the remote server") @@ -131,6 +134,8 @@ func (r *Reader) setResponse(res *http.Response) error { if r.offset > 0 || r.Resource.ETag != "" && r.Resource.ETag != res.Header.Get("ETag") { return ErrContentHasChanged } + case http.StatusNotFound: + return ErrNotFound case http.StatusPartialContent: // Requested `Range` request succeeded https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/206 break diff --git a/internal/httprange/http_reader_test.go b/internal/httprange/http_reader_test.go index 5e971575..9c8f11d0 100644 --- a/internal/httprange/http_reader_test.go +++ b/internal/httprange/http_reader_test.go @@ -226,9 +226,13 @@ func TestReaderSetResponse(t *testing.T) { status: http.StatusRequestedRangeNotSatisfiable, expectedErrMsg: ErrRangeRequestsNotSupported.Error(), }, - "unhandled_status_code": { + "not_found": { status: http.StatusNotFound, - expectedErrMsg: "httprange: read response 404:", + expectedErrMsg: ErrNotFound.Error(), + }, + "unhandled_status_code": { + status: http.StatusInternalServerError, + expectedErrMsg: "httprange: read response 500:", }, } for name, tt := range tests { diff --git a/internal/httprange/resource.go b/internal/httprange/resource.go index 7e21ef29..d2dbd340 100644 --- a/internal/httprange/resource.go +++ b/internal/httprange/resource.go @@ -71,6 +71,9 @@ func NewResource(ctx context.Context, url string) (*Resource, error) { case http.StatusRequestedRangeNotSatisfiable: return nil, ErrRangeRequestsNotSupported + case http.StatusNotFound: + return nil, ErrNotFound + default: return nil, fmt.Errorf("httprange: new resource %d: %q", res.StatusCode, res.Status) } diff --git a/internal/httprange/resource_test.go b/internal/httprange/resource_test.go index 89d15a21..ace8f92c 100644 --- a/internal/httprange/resource_test.go +++ b/internal/httprange/resource_test.go @@ -2,7 +2,6 @@ package httprange import ( "context" - "fmt" "net/http" "net/http/httptest" "testing" @@ -60,7 +59,7 @@ func TestNewResource(t *testing.T) { "not_found": { url: "/some/resource", status: http.StatusNotFound, - expectedErrMsg: fmt.Sprintf("httprange: new resource %d: %q", http.StatusNotFound, "404 Not Found"), + expectedErrMsg: ErrNotFound.Error(), }, "invalid_url": { url: "/%", diff --git a/internal/serving/disk/local/serving_test.go b/internal/serving/disk/local/serving_test.go index 60f01acd..2602451f 100644 --- a/internal/serving/disk/local/serving_test.go +++ b/internal/serving/disk/local/serving_test.go @@ -15,29 +15,79 @@ import ( func TestDisk_ServeFileHTTP(t *testing.T) { defer setUpTests(t)() - s := Instance() - w := httptest.NewRecorder() - r := httptest.NewRequest("GET", "http://group.gitlab-example.com/serving/index.html", nil) - handler := serving.Handler{ - Writer: w, - Request: r, - LookupPath: &serving.LookupPath{ - Prefix: "/serving", - Path: "group/serving/public", + tests := map[string]struct { + vfsPath string + path string + expectedStatus int + expectedBody string + }{ + "accessing /index.html": { + vfsPath: "group/serving/public", + path: "/index.html", + expectedStatus: http.StatusOK, + expectedBody: "HTML Document", + }, + "accessing /": { + vfsPath: "group/serving/public", + path: "/", + expectedStatus: http.StatusOK, + expectedBody: "HTML Document", + }, + "accessing without /": { + vfsPath: "group/serving/public", + path: "", + expectedStatus: http.StatusFound, + expectedBody: `<a href="//group.gitlab-example.com/serving/">Found</a>.`, + }, + "accessing vfs path that is missing": { + vfsPath: "group/serving/public-missing", + path: "/index.html", + // we expect the status to not be set + expectedStatus: 0, + }, + "accessing vfs path that is forbidden (like file)": { + vfsPath: "group/serving/public/index.html", + path: "/index.html", + expectedStatus: http.StatusInternalServerError, }, - SubPath: "/index.html", } - require.True(t, s.ServeFileHTTP(handler)) + s := Instance() - resp := w.Result() - defer resp.Body.Close() + for name, test := range tests { + t.Run(name, func(t *testing.T) { + w := httptest.NewRecorder() + w.Code = 0 // ensure that code is not set, and it is being set by handler + r := httptest.NewRequest("GET", "http://group.gitlab-example.com/serving"+test.path, nil) - require.Equal(t, http.StatusOK, resp.StatusCode) - body, err := ioutil.ReadAll(resp.Body) - require.NoError(t, err) + handler := serving.Handler{ + Writer: w, + Request: r, + LookupPath: &serving.LookupPath{ + Prefix: "/serving/", + Path: test.vfsPath, + }, + SubPath: test.path, + } - require.Contains(t, string(body), "HTML Document") + if test.expectedStatus == 0 { + require.False(t, s.ServeFileHTTP(handler)) + require.Zero(t, w.Code, "we expect status to not be set") + return + } + + require.True(t, s.ServeFileHTTP(handler)) + + resp := w.Result() + defer resp.Body.Close() + + require.Equal(t, test.expectedStatus, resp.StatusCode) + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + require.Contains(t, string(body), test.expectedBody) + }) + } } var chdirSet = false diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index e7f15a1e..5b0c56f0 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -11,7 +11,9 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + "gitlab.com/gitlab-org/labkit/errortracking" + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/redirects" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk/symlink" @@ -25,39 +27,51 @@ type Reader struct { } // Show the user some validation messages for their _redirects file -func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirects.Redirects) error { +func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirects.Redirects) { h.Writer.Header().Set("Content-Type", "text/plain; charset=utf-8") h.Writer.Header().Set("X-Content-Type-Options", "nosniff") h.Writer.WriteHeader(http.StatusOK) - _, err := fmt.Fprintln(h.Writer, redirects.Status()) - return err + fmt.Fprintln(h.Writer, redirects.Status()) } -func (reader *Reader) tryRedirects(h serving.Handler) error { +// tryRedirects returns true if it successfully handled request +func (reader *Reader) tryRedirects(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path) - if err != nil { - return err + if vfs.IsNotExist(err) { + return false + } else if err != nil { + httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err) + return true } r := redirects.ParseRedirects(ctx, root) rewrittenURL, status, err := r.Rewrite(h.Request.URL) if err != nil { - return err + if err != redirects.ErrNoRedirect { + // We assume that rewrite failure is not fatal + // and we only capture the error + errortracking.Capture(err, errortracking.WithRequest(h.Request)) + } + return false } http.Redirect(h.Writer, h.Request, rewrittenURL.Path, status) - - return nil + return true } -func (reader *Reader) tryFile(h serving.Handler) error { +// tryFile returns true if it successfully handled request +func (reader *Reader) tryFile(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path) - if err != nil { - return err + if vfs.IsNotExist(err) { + return false + } else if err != nil { + httperrors.Serve500WithRequest(h.Writer, h.Request, + "vfs.Root", err) + return true } fullPath, err := reader.resolvePath(ctx, root, h.SubPath) @@ -80,7 +94,7 @@ func (reader *Reader) tryFile(h serving.Handler) error { // Ensure that there's always "/" at end redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" http.Redirect(h.Writer, h.Request, redirectPath, 302) - return nil + return true } } @@ -89,7 +103,9 @@ func (reader *Reader) tryFile(h serving.Handler) error { } if err != nil { - return err + // We assume that this is mostly missing file type of the error + // and additional handlers should try to process the request + return false } // Serve status of `_redirects` under `_redirects` @@ -97,34 +113,42 @@ func (reader *Reader) tryFile(h serving.Handler) error { if fullPath == redirects.ConfigFile { if os.Getenv("FF_ENABLE_REDIRECTS") != "false" { r := redirects.ParseRedirects(ctx, root) - return reader.serveRedirectsStatus(h, r) + reader.serveRedirectsStatus(h, r) + return true } h.Writer.WriteHeader(http.StatusForbidden) - return nil + return true } return reader.serveFile(ctx, h.Writer, h.Request, root, fullPath, h.LookupPath.HasAccessControl) } -func (reader *Reader) tryNotFound(h serving.Handler) error { +func (reader *Reader) tryNotFound(h serving.Handler) bool { ctx := h.Request.Context() root, err := reader.vfs.Root(ctx, h.LookupPath.Path) - if err != nil { - return err + if vfs.IsNotExist(err) { + return false + } else if err != nil { + httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err) + return true } page404, err := reader.resolvePath(ctx, root, "404.html") if err != nil { - return err + // We assume that this is mostly missing file type of the error + // and additional handlers should try to process the request + return false } err = reader.serveCustomFile(ctx, h.Writer, h.Request, http.StatusNotFound, root, page404) if err != nil { - return err + httperrors.Serve500WithRequest(h.Writer, h.Request, "serveCustomFile", err) + return true } - return nil + + return true } // Resolve the HTTP request to a path on disk, converting requests for @@ -168,19 +192,21 @@ func (reader *Reader) resolvePath(ctx context.Context, root vfs.Root, subPath .. return fullPath, nil } -func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath string, accessControl bool) error { +func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *http.Request, root vfs.Root, origPath string, accessControl bool) bool { fullPath := reader.handleContentEncoding(ctx, w, r, root, origPath) file, err := root.Open(ctx, fullPath) if err != nil { - return err + httperrors.Serve500WithRequest(w, r, "root.Open", err) + return true } defer file.Close() fi, err := root.Lstat(ctx, fullPath) if err != nil { - return err + httperrors.Serve500WithRequest(w, r, "root.Lstat", err) + return true } if !accessControl { @@ -191,7 +217,8 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h contentType, err := reader.detectContentType(ctx, root, origPath) if err != nil { - return err + httperrors.Serve500WithRequest(w, r, "detectContentType", err) + return true } w.Header().Set("Content-Type", contentType) @@ -208,7 +235,7 @@ func (reader *Reader) serveFile(ctx context.Context, w http.ResponseWriter, r *h io.Copy(w, file) } - return nil + return true } func (reader *Reader) serveCustomFile(ctx context.Context, w http.ResponseWriter, r *http.Request, code int, root vfs.Root, origPath string) error { diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go index 11b1689e..30c821ea 100644 --- a/internal/serving/disk/serving.go +++ b/internal/serving/disk/serving.go @@ -17,12 +17,12 @@ type Disk struct { // ServeFileHTTP serves a file from disk and returns true. It returns false // when a file could not been found. func (s *Disk) ServeFileHTTP(h serving.Handler) bool { - if s.reader.tryFile(h) == nil { + if s.reader.tryFile(h) { return true } if os.Getenv("FF_ENABLE_REDIRECTS") != "false" { - if s.reader.tryRedirects(h) == nil { + if s.reader.tryRedirects(h) { return true } } @@ -32,7 +32,7 @@ func (s *Disk) ServeFileHTTP(h serving.Handler) bool { // ServeNotFoundHTTP tries to read a custom 404 page func (s *Disk) ServeNotFoundHTTP(h serving.Handler) { - if s.reader.tryNotFound(h) == nil { + if s.reader.tryNotFound(h) { return } diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go index e95432ae..bca2ae51 100644 --- a/internal/serving/disk/zip/serving_test.go +++ b/internal/serving/disk/zip/serving_test.go @@ -17,25 +17,40 @@ func TestZip_ServeFileHTTP(t *testing.T) { defer cleanup() tests := map[string]struct { + vfsPath string path string expectedStatus int expectedBody string }{ "accessing /index.html": { + vfsPath: testServerURL + "/public.zip", path: "/index.html", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing /": { + vfsPath: testServerURL + "/public.zip", path: "/", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing without /": { + vfsPath: testServerURL + "/public.zip", path: "", expectedStatus: http.StatusFound, expectedBody: `<a href="//zip.gitlab.io/zip/">Found</a>.`, }, + "accessing archive that is 404": { + vfsPath: testServerURL + "/invalid.zip", + path: "/index.html", + // we expect the status to not be set + expectedStatus: 0, + }, + "accessing archive that is 500": { + vfsPath: testServerURL + "/500", + path: "/index.html", + expectedStatus: http.StatusInternalServerError, + }, } s := Instance() @@ -43,6 +58,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { w := httptest.NewRecorder() + w.Code = 0 // ensure that code is not set, and it is being set by handler r := httptest.NewRequest("GET", "http://zip.gitlab.io/zip"+test.path, nil) handler := serving.Handler{ @@ -50,11 +66,17 @@ func TestZip_ServeFileHTTP(t *testing.T) { Request: r, LookupPath: &serving.LookupPath{ Prefix: "/zip/", - Path: testServerURL + "/public.zip", + Path: test.vfsPath, }, SubPath: test.path, } + if test.expectedStatus == 0 { + require.False(t, s.ServeFileHTTP(handler)) + require.Zero(t, w.Code, "we expect status to not be set") + return + } + require.True(t, s.ServeFileHTTP(handler)) resp := w.Result() @@ -76,9 +98,15 @@ func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) { chdir := testhelpers.ChdirInPath(t, "../../../../shared/pages", &chdirSet) - testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + m := http.NewServeMux() + m.HandleFunc("/public.zip", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, zipFilePath) })) + m.HandleFunc("/500", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + + testServer := httptest.NewServer(m) return testServer.URL, func() { chdir() diff --git a/internal/vfs/errors.go b/internal/vfs/errors.go new file mode 100644 index 00000000..32b86192 --- /dev/null +++ b/internal/vfs/errors.go @@ -0,0 +1,18 @@ +package vfs + +import ( + "fmt" +) + +type ErrNotExist struct { + Inner error +} + +func (e ErrNotExist) Error() string { + return fmt.Sprintf("not exist: %q", e.Inner) +} + +func IsNotExist(err error) bool { + _, ok := err.(*ErrNotExist) + return ok +} diff --git a/internal/vfs/local/vfs.go b/internal/vfs/local/vfs.go index ca74dfbe..bdcd5160 100644 --- a/internal/vfs/local/vfs.go +++ b/internal/vfs/local/vfs.go @@ -20,12 +20,16 @@ func (fs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { } rootPath, err = filepath.EvalSymlinks(rootPath) - if err != nil { + if os.IsNotExist(err) { + return nil, &vfs.ErrNotExist{Inner: err} + } else if err != nil { return nil, err } fi, err := os.Lstat(rootPath) - if err != nil { + if os.IsNotExist(err) { + return nil, &vfs.ErrNotExist{Inner: err} + } else if err != nil { return nil, err } diff --git a/internal/vfs/local/vfs_test.go b/internal/vfs/local/vfs_test.go index ec67d595..b678cfa7 100644 --- a/internal/vfs/local/vfs_test.go +++ b/internal/vfs/local/vfs_test.go @@ -9,6 +9,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" ) var localVFS = &VFS{} @@ -98,7 +100,7 @@ func TestVFSRoot(t *testing.T) { rootVFS, err := localVFS.Root(context.Background(), filepath.Join(tmpDir, test.path)) if test.expectedIsNotExist { - require.Equal(t, test.expectedIsNotExist, os.IsNotExist(err)) + require.Equal(t, test.expectedIsNotExist, vfs.IsNotExist(err)) return } diff --git a/internal/vfs/zip/archive_test.go b/internal/vfs/zip/archive_test.go index ef6785b5..2474d419 100644 --- a/internal/vfs/zip/archive_test.go +++ b/internal/vfs/zip/archive_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/httprange" "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) @@ -272,7 +273,7 @@ func TestReadArchiveFails(t *testing.T) { err := zip.openArchive(context.Background()) require.Error(t, err) - require.Contains(t, err.Error(), "Not Found") + require.Contains(t, err.Error(), httprange.ErrNotFound.Error()) _, err = zip.Open(context.Background(), "index.html") require.EqualError(t, err, os.ErrNotExist.Error()) diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index 78a77e1c..d3c5cf9b 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -9,6 +9,7 @@ import ( "github.com/patrickmn/go-cache" + "gitlab.com/gitlab-org/gitlab-pages/internal/httprange" "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -82,6 +83,11 @@ func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) { continue } + // If archive is not found, return a known `vfs` error + if err == httprange.ErrNotFound { + err = &vfs.ErrNotExist{Inner: err} + } + return root, err } } diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index c12e49cd..d4730524 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -9,6 +9,8 @@ import ( "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/httprange" + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -25,7 +27,7 @@ func TestVFSRoot(t *testing.T) { }, "zip_file_does_not_exist": { path: "/unknown", - expectedErrMsg: "404 Not Found", + expectedErrMsg: vfs.ErrNotExist{Inner: httprange.ErrNotFound}.Error(), }, "invalid_url": { path: "/%", |