diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2020-09-09 09:23:02 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2020-09-24 03:22:45 +0300 |
commit | 3e20c84456ec3194de8a35af3cd287a37c006549 (patch) | |
tree | 537e905021f4803bbb67c570136a0c0895ed1424 | |
parent | 7802bb75e8edafe05855fcbdb72aeea7bb906ae7 (diff) |
Rebase from base branch
Add vfs.VFS implementation for zip
Fix build errors
Clean zip vfs
Add tests for Root
Add zip serving metric
Return a zip.Instance() when source == zip
Add simple acceptance test for zip serving
Use correct contents
No need to start testServer in go routine
-rw-r--r-- | acceptance_test.go | 80 | ||||
-rw-r--r-- | helpers_test.go | 28 | ||||
-rw-r--r-- | internal/serving/disk/local/serving.go | 3 | ||||
-rw-r--r-- | internal/serving/disk/serving.go | 7 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving.go | 17 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving_test.go | 59 | ||||
-rw-r--r-- | internal/source/gitlab/factory.go | 7 | ||||
-rw-r--r-- | internal/source/gitlab/gitlab.go | 12 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 79 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 58 | ||||
-rw-r--r-- | metrics/metrics.go | 10 | ||||
-rw-r--r-- | shared/lookups/zip.gitlab.io.json | 16 |
12 files changed, 368 insertions, 8 deletions
diff --git a/acceptance_test.go b/acceptance_test.go index f8c2a546..bc9e9c9e 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -21,6 +21,10 @@ import ( var pagesBinary = flag.String("gitlab-pages-binary", "./gitlab-pages", "Path to the gitlab-pages binary") +const ( + objectStorageMockServer = "127.0.0.1:37003" +) + // TODO: Use TCP port 0 everywhere to avoid conflicts. The binary could output // the actual port (and type of listener) for us to read in place of the // hardcoded values below. @@ -1925,3 +1929,79 @@ func TestDomainsSource(t *testing.T) { }) } } + +func TestZipServing(t *testing.T) { + skipUnlessEnabled(t) + + var apiCalled bool + source := NewGitlabDomainsSourceStub(t, &apiCalled) + defer source.Close() + + gitLabAPISecretKey := CreateGitLabAPISecretKeyFixtureFile(t) + + pagesArgs := []string{"-gitlab-server", source.URL, "-api-secret-key", gitLabAPISecretKey, "-domain-config-source", "gitlab"} + teardown := RunPagesProcessWithEnvs(t, true, *pagesBinary, listeners, "", []string{}, pagesArgs...) + defer teardown() + + _, cleanup := newZipFileServerURL(t, "shared/pages/group/zip.gitlab.io/public.zip") + defer cleanup() + + tests := map[string]struct { + urlSuffix string + expectedStatusCode int + expectedContent string + }{ + "base_domain_no_suffix": { + urlSuffix: "/", + expectedStatusCode: http.StatusOK, + expectedContent: "zip.gitlab.io/project/index.html\n", + }, + "file_exists": { + urlSuffix: "/index.html", + expectedStatusCode: http.StatusOK, + expectedContent: "zip.gitlab.io/project/index.html\n", + }, + "file_exists_in_subdir": { + urlSuffix: "/subdir/hello.html", + expectedStatusCode: http.StatusOK, + expectedContent: "zip.gitlab.io/project/subdir/hello.html\n", + }, + "file_exists_symlink": { + urlSuffix: "/symlink.html", + expectedStatusCode: http.StatusOK, + expectedContent: "symlink.html->subdir/linked.html\n", + }, + "dir": { + urlSuffix: "/subdir/", + expectedStatusCode: http.StatusNotFound, + expectedContent: "zip.gitlab.io/project/404.html\n", + }, + "file_does_not_exist": { + urlSuffix: "/unknown.html", + expectedStatusCode: http.StatusNotFound, + expectedContent: "zip.gitlab.io/project/404.html\n", + }, + "bad_symlink": { + urlSuffix: "/bad-symlink.html", + expectedStatusCode: http.StatusNotFound, + expectedContent: "zip.gitlab.io/project/404.html\n", + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + response, err := GetPageFromListener(t, httpListener, "zip.gitlab.io", tt.urlSuffix) + require.NoError(t, err) + + require.Equal(t, tt.expectedStatusCode, response.StatusCode) + if tt.expectedStatusCode == http.StatusOK || tt.expectedStatusCode == http.StatusNotFound { + defer response.Body.Close() + + body, err := ioutil.ReadAll(response.Body) + require.NoError(t, err) + + require.Equal(t, tt.expectedContent, string(body), "content mismatch") + } + }) + } +} diff --git a/helpers_test.go b/helpers_test.go index d08869fd..6ee11b86 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -522,3 +522,31 @@ func copyFile(dest, src string) error { _, err = io.Copy(destFile, srcFile) return err } + +func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) { + t.Helper() + + m := http.NewServeMux() + m.HandleFunc("/public.zip", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.ServeFile(w, r, zipFilePath) + })) + + // create a listener with the desired port. + l, err := net.Listen("tcp", objectStorageMockServer) + require.NoError(t, err) + + testServer := httptest.NewUnstartedServer(m) + + // NewUnstartedServer creates a listener. Close that listener and replace + // with the one we created. + testServer.Listener.Close() + testServer.Listener = l + + // Start the server. + testServer.Start() + + return testServer.URL, func() { + // Cleanup. + testServer.Close() + } +} diff --git a/internal/serving/disk/local/serving.go b/internal/serving/disk/local/serving.go index 230a71da..d4a882bc 100644 --- a/internal/serving/disk/local/serving.go +++ b/internal/serving/disk/local/serving.go @@ -5,9 +5,10 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/local" + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) -var instance = disk.New(vfs.Instrumented(&local.VFS{}, "local")) +var instance = disk.New(vfs.Instrumented(&local.VFS{}, "local"), metrics.DiskServingFileSize) // Instance returns a serving instance that is capable of reading files // from the disk diff --git a/internal/serving/disk/serving.go b/internal/serving/disk/serving.go index bb9b40d2..94ea3f23 100644 --- a/internal/serving/disk/serving.go +++ b/internal/serving/disk/serving.go @@ -3,10 +3,11 @@ package disk import ( "os" + "github.com/prometheus/client_golang/prometheus" + "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" - "gitlab.com/gitlab-org/gitlab-pages/metrics" ) // Disk describes a disk access serving @@ -42,10 +43,10 @@ func (s *Disk) ServeNotFoundHTTP(h serving.Handler) { // New returns a serving instance that is capable of reading files // from the VFS -func New(vfs vfs.VFS) serving.Serving { +func New(vfs vfs.VFS, fileSizeMetric prometheus.Histogram) serving.Serving { return &Disk{ reader: Reader{ - fileSizeMetric: metrics.DiskServingFileSize, + fileSizeMetric: fileSizeMetric, vfs: vfs, }, } diff --git a/internal/serving/disk/zip/serving.go b/internal/serving/disk/zip/serving.go new file mode 100644 index 00000000..42cc8f0c --- /dev/null +++ b/internal/serving/disk/zip/serving.go @@ -0,0 +1,17 @@ +package zip + +import ( + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving/disk" + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs/zip" + "gitlab.com/gitlab-org/gitlab-pages/metrics" +) + +var instance = disk.New(vfs.Instrumented(zip.New(), "zip"), metrics.ZipServingFileSize) + +// Instance returns a serving instance that is capable of reading files +// from a zip archives opened from a URL, most likely stored in object storage +func Instance() serving.Serving { + return instance +} diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go new file mode 100644 index 00000000..14fdabdf --- /dev/null +++ b/internal/serving/disk/zip/serving_test.go @@ -0,0 +1,59 @@ +package zip + +import ( + "io/ioutil" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" +) + +func TestZip_ServeFileHTTP(t *testing.T) { + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + defer cleanup() + + s := Instance() + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "http://zip.gitlab.io/zip/index.html", nil) + handler := serving.Handler{ + Writer: w, + Request: r, + LookupPath: &serving.LookupPath{ + Prefix: "", + Path: testServerURL + "/public.zip", + }, + SubPath: "/index.html", + } + + require.True(t, s.ServeFileHTTP(handler)) + + resp := w.Result() + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode) + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + require.Contains(t, string(body), "zip.gitlab.io/project/index.html\n") +} + +var chdirSet = false + +func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) { + t.Helper() + + chdir := testhelpers.ChdirInPath(t, "../../../../shared/pages", &chdirSet) + + testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.ServeFile(w, r, zipFilePath) + })) + + return testServer.URL, func() { + chdir() + testServer.Close() + } +} diff --git a/internal/source/gitlab/factory.go b/internal/source/gitlab/factory.go index b5ab367b..39193a16 100644 --- a/internal/source/gitlab/factory.go +++ b/internal/source/gitlab/factory.go @@ -1,12 +1,11 @@ package gitlab import ( - "strings" - log "github.com/sirupsen/logrus" "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/serving/disk/zip" "gitlab.com/gitlab-org/gitlab-pages/internal/serving/serverless" "gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab/api" ) @@ -17,7 +16,7 @@ import ( func fabricateLookupPath(size int, lookup api.LookupPath) *serving.LookupPath { return &serving.LookupPath{ Prefix: lookup.Prefix, - Path: strings.TrimPrefix(lookup.Source.Path, "/"), + Path: lookup.Source.Path, IsNamespaceProject: (lookup.Prefix == "/" && size > 1), IsHTTPSOnly: lookup.HTTPSOnly, HasAccessControl: lookup.AccessControl, @@ -32,6 +31,8 @@ func fabricateServing(lookup api.LookupPath) serving.Serving { switch source.Type { case "file": return local.Instance() + case "zip": + return zip.Instance() case "serverless": serving, err := serverless.NewFromAPISource(source.Serverless) if err != nil { diff --git a/internal/source/gitlab/gitlab.go b/internal/source/gitlab/gitlab.go index 67c4eb6b..fb15a07a 100644 --- a/internal/source/gitlab/gitlab.go +++ b/internal/source/gitlab/gitlab.go @@ -4,6 +4,7 @@ import ( "context" "errors" "net/http" + "net/url" "path" "strings" "sync" @@ -94,6 +95,17 @@ func (g *Gitlab) Resolve(r *http.Request) (*serving.Request, error) { subPath = strings.TrimPrefix(urlPath, lookup.Prefix) } + if lookup.Source.Type == "zip" { + urlPath, err := url.Parse(lookup.Source.Path) + if err != nil { + return nil, err + } + + lookup.Source.Path = urlPath.String() + } else { + lookup.Source.Path = strings.TrimPrefix(lookup.Source.Path, "/") + } + return &serving.Request{ Serving: fabricateServing(lookup), LookupPath: fabricateLookupPath(size, lookup), diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go new file mode 100644 index 00000000..ccce8b3c --- /dev/null +++ b/internal/vfs/zip/vfs.go @@ -0,0 +1,79 @@ +package zip + +import ( + "context" + "errors" + "time" + + "github.com/patrickmn/go-cache" + + "gitlab.com/gitlab-org/gitlab-pages/internal/vfs" +) + +const ( + // TODO: should we make zip root cache configurable? + cacheExpirationInterval = time.Minute + cacheRefreshInterval = time.Minute / 2 +) + +var ( + errNotZipArchive = errors.New("cached item is not a zip archive") +) + +// zipVFS is a simple cached implementation of the vfs.VFS interface +type zipVFS struct { + cache *cache.Cache +} + +// Root opens an archive given a URL path and returns an instance of zipArchive +// that implements the vfs.Root. +// To avoid using locks, the function runs in a for loop until an archive is +// either found or created and saved. If there's an error while saving the +// archive to the given path because it already exists, the for loop will +// continue to try and find the already saved archive again. +func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) { + // we do it in loop to not use any additional locks + for { + archive, expiry, found := fs.cache.GetWithExpiration(path) + if found { + if time.Until(expiry) < cacheRefreshInterval { + // refresh item + fs.cache.Set(path, archive, cache.DefaultExpiration) + } + } else { + archive = newArchive(path, DefaultOpenTimeout) + + // if it errors, it means that it is already added + // retry again to get it + // if adding the archive to the cache fails it means it's already been added before, retry getting archive + if fs.cache.Add(path, archive, cache.DefaultExpiration) != nil { + continue + } + } + + zipDir, ok := archive.(*zipArchive) + if !ok { + // fail if the found archive in cache is not a zipArchive + return nil, errNotZipArchive + } + + err := zipDir.openArchive(ctx) + return zipDir, err + } +} + +// New creates a zipVFS instance that can be used by a serving request +func New() vfs.VFS { + zipVFS := &zipVFS{ + cache: cache.New(cacheExpirationInterval, cacheRefreshInterval), + } + + zipVFS.cache.OnEvicted(func(path string, object interface{}) { + // TODO: Add and update zip metric on eviction https://gitlab.com/gitlab-org/gitlab-pages/-/issues/423 + if archive, ok := object.(*zipArchive); archive != nil && ok { + archive.close() + } + }) + + return zipVFS +} diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go new file mode 100644 index 00000000..5e1e8b65 --- /dev/null +++ b/internal/vfs/zip/vfs_test.go @@ -0,0 +1,58 @@ +package zip + +import ( + "context" + "io/ioutil" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestVFSRoot(t *testing.T) { + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + defer cleanup() + + tests := map[string]struct { + path string + expectedErrMsg string + }{ + "zip_file_exists": { + path: "/public.zip", + }, + "zip_file_does_not_exist": { + path: "/unknown", + expectedErrMsg: "404 Not Found", + }, + } + + testZipVFS := New() + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + rootVFS, err := testZipVFS.Root(context.Background(), testServerURL+tt.path) + if tt.expectedErrMsg != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tt.expectedErrMsg) + return + } + + require.NoError(t, err) + require.IsType(t, &zipArchive{}, rootVFS) + + f, err := rootVFS.Open(context.Background(), "index.html") + require.NoError(t, err) + + content, err := ioutil.ReadAll(f) + require.NoError(t, err) + require.Equal(t, "zip.gitlab.io/project/index.html\n", string(content)) + + fi, err := rootVFS.Lstat(context.Background(), "index.html") + require.NoError(t, err) + require.Equal(t, "index.html", fi.Name()) + + link, err := rootVFS.Readlink(context.Background(), "symlink.html") + require.NoError(t, err) + require.Equal(t, "subdir/linked.html", link) + }) + } +} diff --git a/metrics/metrics.go b/metrics/metrics.go index cb4287c8..0f532144 100644 --- a/metrics/metrics.go +++ b/metrics/metrics.go @@ -78,7 +78,7 @@ var ( Help: "The time (in seconds) it takes to get a response from the GitLab domains API", }, []string{"status_code"}) - // DiskServingFileSize metric for file size serving. serving_types: disk and object_storage + // DiskServingFileSize metric for file size serving. DiskServingFileSize = prometheus.NewHistogram(prometheus.HistogramOpts{ Name: "gitlab_pages_disk_serving_file_size_bytes", Help: "The size in bytes for each file that has been served", @@ -86,6 +86,14 @@ var ( Buckets: prometheus.ExponentialBuckets(1.0, 10.0, 9), }) + // ZipServingFileSize metric for file size serving. + ZipServingFileSize = prometheus.NewHistogram(prometheus.HistogramOpts{ + Name: "gitlab_pages_zip_serving_file_size_bytes", + Help: "The size in bytes for each file that has been served", + // From 1B to 100MB in *10 increments (1 10 100 1,000 10,000 100,000 1'000,000 10'000,000 100'000,000) + Buckets: prometheus.ExponentialBuckets(1.0, 10.0, 9), + }) + // ServingTime metric for time taken to find a file serving it or not found. ServingTime = prometheus.NewHistogram(prometheus.HistogramOpts{ Name: "gitlab_pages_serving_time_seconds", diff --git a/shared/lookups/zip.gitlab.io.json b/shared/lookups/zip.gitlab.io.json new file mode 100644 index 00000000..cf755a58 --- /dev/null +++ b/shared/lookups/zip.gitlab.io.json @@ -0,0 +1,16 @@ +{ + "certificate": "", + "key": "", + "lookup_paths": [ + { + "access_control": false, + "https_only": false, + "prefix": "/", + "project_id": 123, + "source": { + "path": "http://127.0.0.1:37003/public.zip", + "type": "zip" + } + } + ] +} |