diff options
author | feistel <6742251-feistel@users.noreply.gitlab.com> | 2021-07-26 18:30:26 +0300 |
---|---|---|
committer | feistel <6742251-feistel@users.noreply.gitlab.com> | 2021-11-08 18:24:14 +0300 |
commit | 77a733b8af393f81c1143ca27324384b1801a090 (patch) | |
tree | c4e17cbfedb872806b3b3c73959806a79d3e3a93 /internal | |
parent | 81e6fdba4fa75b3e882cde818c25a2a76f531de7 (diff) |
refactor: read and use file_sha256 as cache key for zip VFS
Now that the Rails API is serving the archive file_sha256 in the
lookup response, we can move away from using the base URL as the
key to cache the archive.
Changelog: change
Diffstat (limited to 'internal')
-rw-r--r-- | internal/serving/disk/reader.go | 6 | ||||
-rw-r--r-- | internal/serving/disk/symlink/path_test.go | 2 | ||||
-rw-r--r-- | internal/serving/disk/zip/serving_test.go | 12 | ||||
-rw-r--r-- | internal/serving/lookup_path.go | 3 | ||||
-rw-r--r-- | internal/source/gitlab/api/lookup_path.go | 5 | ||||
-rw-r--r-- | internal/source/gitlab/factory.go | 1 | ||||
-rw-r--r-- | internal/testhelpers/tmpdir.go | 2 | ||||
-rw-r--r-- | internal/vfs/local/root_test.go | 10 | ||||
-rw-r--r-- | internal/vfs/local/vfs.go | 2 | ||||
-rw-r--r-- | internal/vfs/local/vfs_test.go | 2 | ||||
-rw-r--r-- | internal/vfs/vfs.go | 6 | ||||
-rw-r--r-- | internal/vfs/zip/archive_test.go | 9 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 17 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 34 |
14 files changed, 73 insertions, 38 deletions
diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index 10af9e35..4cc0ad8b 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -37,7 +37,7 @@ func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirec // 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) + root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.Sha256) if vfs.IsNotExist(err) { return false } else if err != nil { @@ -70,7 +70,7 @@ func (reader *Reader) tryRedirects(h serving.Handler) bool { func (reader *Reader) tryFile(h serving.Handler) bool { ctx := h.Request.Context() - root, err := reader.vfs.Root(ctx, h.LookupPath.Path) + root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.Sha256) if vfs.IsNotExist(err) { return false } else if err != nil { @@ -133,7 +133,7 @@ func redirectPath(request *http.Request) string { func (reader *Reader) tryNotFound(h serving.Handler) bool { ctx := h.Request.Context() - root, err := reader.vfs.Root(ctx, h.LookupPath.Path) + root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.Sha256) if vfs.IsNotExist(err) { return false } else if err != nil { diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go index 21880efa..59cb685f 100644 --- a/internal/serving/disk/symlink/path_test.go +++ b/internal/serving/disk/symlink/path_test.go @@ -72,7 +72,7 @@ func simpleJoin(path ...string) string { } func testEvalSymlinks(t *testing.T, wd, path, want string) { - root, err := localFs.Root(context.Background(), wd) + root, err := localFs.Root(context.Background(), wd, "") require.NoError(t, err) have, err := symlink.EvalSymlinks(context.Background(), root, path) diff --git a/internal/serving/disk/zip/serving_test.go b/internal/serving/disk/zip/serving_test.go index 167c47c3..66e52046 100644 --- a/internal/serving/disk/zip/serving_test.go +++ b/internal/serving/disk/zip/serving_test.go @@ -27,49 +27,58 @@ func TestZip_ServeFileHTTP(t *testing.T) { tests := map[string]struct { vfsPath string + sha256 string path string expectedStatus int expectedBody string }{ "accessing /index.html": { vfsPath: httpURL, + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", path: "/index.html", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing /index.html from disk": { vfsPath: fileURL, + sha256: "15c5438164ec67bb2225f68d7d7a2e0b608035264e5275b7e3302641aa25a528", path: "/index.html", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing /": { vfsPath: httpURL, + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", path: "/", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing / from disk": { vfsPath: fileURL, + sha256: "15c5438164ec67bb2225f68d7d7a2e0b608035264e5275b7e3302641aa25a528", path: "/", expectedStatus: http.StatusOK, expectedBody: "zip.gitlab.io/project/index.html\n", }, "accessing without /": { vfsPath: httpURL, + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", path: "", expectedStatus: http.StatusFound, expectedBody: `<a href="//zip.gitlab.io/zip/">Found</a>.`, }, "accessing without / from disk": { vfsPath: fileURL, + sha256: "15c5438164ec67bb2225f68d7d7a2e0b608035264e5275b7e3302641aa25a528", 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", + // the sha is needed or we would get a 500 + sha256: "foo", + path: "/index.html", // we expect the status to not be set expectedStatus: 0, }, @@ -111,6 +120,7 @@ func TestZip_ServeFileHTTP(t *testing.T) { LookupPath: &serving.LookupPath{ Prefix: "/zip/", Path: test.vfsPath, + Sha256: test.sha256, }, SubPath: test.path, } diff --git a/internal/serving/lookup_path.go b/internal/serving/lookup_path.go index 1aefe1b8..f5ad2ab5 100644 --- a/internal/serving/lookup_path.go +++ b/internal/serving/lookup_path.go @@ -5,7 +5,8 @@ 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 + Sha256 string + IsNamespaceProject bool // IsNamespaceProject is DEPRECATED, see https://gitlab.com/gitlab-org/gitlab-pages/issues/272 IsHTTPSOnly bool HasAccessControl bool ProjectID uint64 diff --git a/internal/source/gitlab/api/lookup_path.go b/internal/source/gitlab/api/lookup_path.go index 0a765217..242a77c0 100644 --- a/internal/source/gitlab/api/lookup_path.go +++ b/internal/source/gitlab/api/lookup_path.go @@ -11,6 +11,7 @@ type LookupPath struct { // Source describes GitLab Page serving variant type Source struct { - Type string `json:"type,omitempty"` - Path string `json:"path,omitempty"` + Type string `json:"type,omitempty"` + Path string `json:"path,omitempty"` + Sha256 string `json:"sha256,omitempty"` } diff --git a/internal/source/gitlab/factory.go b/internal/source/gitlab/factory.go index 21572cd0..fb7a6863 100644 --- a/internal/source/gitlab/factory.go +++ b/internal/source/gitlab/factory.go @@ -25,6 +25,7 @@ func fabricateLookupPath(size int, lookup api.LookupPath) *serving.LookupPath { return &serving.LookupPath{ ServingType: lookup.Source.Type, Path: lookup.Source.Path, + Sha256: lookup.Source.Sha256, Prefix: lookup.Prefix, IsNamespaceProject: (lookup.Prefix == "/" && size > 1), IsHTTPSOnly: lookup.HTTPSOnly, diff --git a/internal/testhelpers/tmpdir.go b/internal/testhelpers/tmpdir.go index 92987199..de104a04 100644 --- a/internal/testhelpers/tmpdir.go +++ b/internal/testhelpers/tmpdir.go @@ -24,7 +24,7 @@ func TmpDir(tb testing.TB, pattern string) (vfs.Root, string) { tmpDir, err = filepath.EvalSymlinks(tmpDir) require.NoError(tb, err) - root, err := fs.Root(context.Background(), tmpDir) + root, err := fs.Root(context.Background(), tmpDir, "") require.NoError(tb, err) tb.Cleanup(func() { diff --git a/internal/vfs/local/root_test.go b/internal/vfs/local/root_test.go index a835bae3..e7bbb63f 100644 --- a/internal/vfs/local/root_test.go +++ b/internal/vfs/local/root_test.go @@ -15,7 +15,7 @@ import ( func TestValidatePath(t *testing.T) { ctx := context.Background() - rootVFS, err := localVFS.Root(ctx, ".") + rootVFS, err := localVFS.Root(ctx, ".", "") require.NoError(t, err) root := rootVFS.(*Root) @@ -64,7 +64,7 @@ func TestValidatePath(t *testing.T) { func TestReadlink(t *testing.T) { ctx := context.Background() - root, err := localVFS.Root(ctx, ".") + root, err := localVFS.Root(ctx, ".", "") require.NoError(t, err) tests := map[string]struct { @@ -140,7 +140,7 @@ func TestReadlinkAbsolutePath(t *testing.T) { err = os.Symlink(dirFilePath, symlinkPath) require.NoError(t, err) - root, err := localVFS.Root(context.Background(), dirPath) + root, err := localVFS.Root(context.Background(), dirPath, "") require.NoError(t, err) tests := map[string]struct { @@ -169,7 +169,7 @@ func TestReadlinkAbsolutePath(t *testing.T) { func TestLstat(t *testing.T) { ctx := context.Background() - root, err := localVFS.Root(ctx, ".") + root, err := localVFS.Root(ctx, ".", "") require.NoError(t, err) tests := map[string]struct { @@ -233,7 +233,7 @@ func TestLstat(t *testing.T) { func TestOpen(t *testing.T) { ctx := context.Background() - root, err := localVFS.Root(ctx, ".") + root, err := localVFS.Root(ctx, ".", "") require.NoError(t, err) tests := map[string]struct { diff --git a/internal/vfs/local/vfs.go b/internal/vfs/local/vfs.go index 5cca94fd..cb48ad1c 100644 --- a/internal/vfs/local/vfs.go +++ b/internal/vfs/local/vfs.go @@ -15,7 +15,7 @@ var errNotDirectory = errors.New("path needs to be a directory") type VFS struct{} -func (localFs VFS) Root(ctx context.Context, path string) (vfs.Root, error) { +func (localFs VFS) Root(ctx context.Context, path string, cacheKey string) (vfs.Root, error) { rootPath, err := filepath.Abs(path) if err != nil { return nil, err diff --git a/internal/vfs/local/vfs_test.go b/internal/vfs/local/vfs_test.go index 2072d110..3f84bb39 100644 --- a/internal/vfs/local/vfs_test.go +++ b/internal/vfs/local/vfs_test.go @@ -96,7 +96,7 @@ func TestVFSRoot(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - rootVFS, err := localVFS.Root(context.Background(), filepath.Join(tmpDir, test.path)) + rootVFS, err := localVFS.Root(context.Background(), filepath.Join(tmpDir, test.path), "") if test.expectedIsNotExist { require.Equal(t, test.expectedIsNotExist, vfs.IsNotExist(err)) diff --git a/internal/vfs/vfs.go b/internal/vfs/vfs.go index 40fe8bc7..a93f9e26 100644 --- a/internal/vfs/vfs.go +++ b/internal/vfs/vfs.go @@ -13,7 +13,7 @@ import ( // VFS abstracts the things Pages needs to serve a static site from disk. type VFS interface { - Root(ctx context.Context, path string) (Root, error) + Root(ctx context.Context, path string, cacheKey string) (Root, error) Name() string Reconfigure(config *config.Config) error } @@ -34,8 +34,8 @@ func (i *instrumentedVFS) log(ctx context.Context) *logrus.Entry { return log.ContextLogger(ctx).WithField("vfs", i.fs.Name()) } -func (i *instrumentedVFS) Root(ctx context.Context, path string) (Root, error) { - root, err := i.fs.Root(ctx, path) +func (i *instrumentedVFS) Root(ctx context.Context, path string, cacheKey string) (Root, error) { + root, err := i.fs.Root(ctx, path, cacheKey) i.increment("Root", err) i.log(ctx). diff --git a/internal/vfs/zip/archive_test.go b/internal/vfs/zip/archive_test.go index acd89d49..423d9eaf 100644 --- a/internal/vfs/zip/archive_test.go +++ b/internal/vfs/zip/archive_test.go @@ -97,6 +97,7 @@ func TestOpenCached(t *testing.T) { tests := []struct { name string vfsPath string + sha256 string filePath string expectedArchiveStatus archiveStatus expectedOpenErr error @@ -106,6 +107,7 @@ func TestOpenCached(t *testing.T) { { name: "open file first time", vfsPath: testServerURL + "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "index.html", // we expect five requests to: // read resource and zip metadata @@ -116,6 +118,7 @@ func TestOpenCached(t *testing.T) { { name: "open file second time", vfsPath: testServerURL + "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "index.html", // we expect one request to read file with cached data offset expectedRequests: 1, @@ -124,6 +127,7 @@ func TestOpenCached(t *testing.T) { { name: "when the URL changes", vfsPath: testServerURL + "/public.zip?new-secret", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "index.html", expectedRequests: 1, expectedArchiveStatus: archiveOpened, @@ -131,6 +135,7 @@ func TestOpenCached(t *testing.T) { { name: "when opening cached file and content changes", vfsPath: testServerURL + "/public.zip?changed-content=1", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "index.html", expectedRequests: 1, // we receive an error on `read` as `open` offset is already cached @@ -140,6 +145,7 @@ func TestOpenCached(t *testing.T) { { name: "after content change archive is reloaded", vfsPath: testServerURL + "/public.zip?new-secret", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "index.html", expectedRequests: 5, expectedArchiveStatus: archiveOpened, @@ -147,6 +153,7 @@ func TestOpenCached(t *testing.T) { { name: "when opening non-cached file and content changes", vfsPath: testServerURL + "/public.zip?changed-content=1", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", filePath: "subdir/hello.html", expectedRequests: 1, // we receive an error on `read` as `open` offset is already cached @@ -158,7 +165,7 @@ func TestOpenCached(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { start := atomic.LoadInt64(&requests) - zip, err := fs.Root(context.Background(), test.vfsPath) + zip, err := fs.Root(context.Background(), test.vfsPath, test.sha256) require.NoError(t, err) f, err := zip.Open(context.Background(), test.filePath) diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go index c90d7614..40fa617b 100644 --- a/internal/vfs/zip/vfs.go +++ b/internal/vfs/zip/vfs.go @@ -164,15 +164,20 @@ func (fs *zipVFS) keyFromPath(path string) (string, error) { // If findOrOpenArchive returns errAlreadyCached, the for loop will continue // to try and find the cached archive or return if there's an error, for example // if the context is canceled. -func (fs *zipVFS) Root(ctx context.Context, path string) (vfs.Root, error) { - key, err := fs.keyFromPath(path) - if err != nil { - return nil, err +func (fs *zipVFS) Root(ctx context.Context, path string, cacheKey string) (vfs.Root, error) { + // TODO: update acceptance tests mocked response to return a cacheKey + if cacheKey == "" { + k, err := fs.keyFromPath(path) + if err != nil { + return nil, err + } + + cacheKey = k } // we do it in loop to not use any additional locks for { - root, err := fs.findOrOpenArchive(ctx, key, path) + root, err := fs.findOrOpenArchive(ctx, cacheKey, path) if err == errAlreadyCached { continue } @@ -252,7 +257,7 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, key string) (*zipArch } // findOrOpenArchive gets archive from cache and tries to open it -func (fs *zipVFS) findOrOpenArchive(ctx context.Context, key, path string) (*zipArchive, error) { +func (fs *zipVFS) findOrOpenArchive(ctx context.Context, key string, path string) (*zipArchive, error) { zipArchive, err := fs.findOrCreateArchive(ctx, key) if err != nil { return nil, err diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index fb368a69..9343405c 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -23,17 +23,21 @@ func TestVFSRoot(t *testing.T) { tests := map[string]struct { path string + sha256 string expectedErrMsg string }{ "zip_file_exists": { - path: "/public.zip", + path: "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", }, "zip_file_does_not_exist": { path: "/unknown", + sha256: "filedoesnotexist", expectedErrMsg: vfs.ErrNotExist{Inner: httprange.ErrNotFound}.Error(), }, "invalid_url": { path: "/%", + sha256: "invalidurl", expectedErrMsg: "invalid URL", }, } @@ -42,7 +46,7 @@ func TestVFSRoot(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { - root, err := vfs.Root(context.Background(), url+tt.path) + root, err := vfs.Root(context.Background(), url+tt.path, tt.sha256) if tt.expectedErrMsg != "" { require.Error(t, err) require.Contains(t, err.Error(), tt.expectedErrMsg) @@ -77,7 +81,8 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { path := testServerURL + "/public.zip" vfs := New(&zipCfg).(*zipVFS) - root, err := vfs.Root(context.Background(), path) + key := "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75" + root, err := vfs.Root(context.Background(), path, key) require.NoError(t, err) done := make(chan struct{}) @@ -93,13 +98,13 @@ func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { default: vfs.cache.Flush() - vfs.cache.SetDefault(path, root) + vfs.cache.SetDefault(key, root) } } }() require.Eventually(t, func() bool { - _, err := vfs.findOrOpenArchive(context.Background(), path, path) + _, err := vfs.findOrOpenArchive(context.Background(), key, path) return err == errAlreadyCached }, 3*time.Second, time.Nanosecond) } @@ -113,6 +118,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { tests := map[string]struct { path string + sha256 string expirationInterval time.Duration refreshInterval time.Duration @@ -122,6 +128,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { }{ "after cache expiry of successful open a new archive is returned": { path: "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", expirationInterval: expiryInterval, expectNewArchive: true, expectOpenError: false, @@ -134,6 +141,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { }, "subsequent open during refresh interval does refresh archive": { path: "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", expirationInterval: time.Second, refreshInterval: time.Second, // refresh always expectNewArchive: false, @@ -142,6 +150,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { }, "subsequent open before refresh interval does not refresh archive": { path: "/public.zip", + sha256: "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75", expirationInterval: time.Second, refreshInterval: time.Millisecond, // very short interval should not refresh expectNewArchive: false, @@ -170,7 +179,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { path := testServerURL + test.path // create a new archive and increase counters - archive1, err1 := vfs.findOrOpenArchive(context.Background(), path, path) + archive1, err1 := vfs.findOrOpenArchive(context.Background(), test.sha256, path) if test.expectOpenError { require.Error(t, err1) require.Nil(t, archive1) @@ -178,7 +187,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { require.NoError(t, err1) } - item1, exp1, found := vfs.cache.GetWithExpiration(path) + item1, exp1, found := vfs.cache.GetWithExpiration(test.sha256) require.True(t, found) // give some time to for timeouts to fire @@ -186,7 +195,7 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { if test.expectNewArchive { // should return a new archive - archive2, err2 := vfs.findOrOpenArchive(context.Background(), path, path) + archive2, err2 := vfs.findOrOpenArchive(context.Background(), test.sha256, path) if test.expectOpenError { require.Error(t, err2) require.Nil(t, archive2) @@ -198,11 +207,11 @@ func TestVFSFindOrOpenArchiveRefresh(t *testing.T) { } // should return exactly the same archive - archive2, err2 := vfs.findOrOpenArchive(context.Background(), path, path) + archive2, err2 := vfs.findOrOpenArchive(context.Background(), test.sha256, path) require.Equal(t, archive1, archive2, "same archive is returned") require.Equal(t, err1, err2, "same error for the same archive") - item2, exp2, found := vfs.cache.GetWithExpiration(path) + item2, exp2, found := vfs.cache.GetWithExpiration(test.sha256) require.True(t, found) require.Equal(t, item1, item2, "same item is returned") @@ -224,9 +233,10 @@ func TestVFSReconfigureTransport(t *testing.T) { fileURL := testhelpers.ToFileProtocol(t, "group/zip.gitlab.io/public.zip") vfs := New(&zipCfg) + key := "d6b318b399cfe9a1c8483e49847ee49a2676d8cfd6df57ec64d971ad03640a75" // try to open a file URL without registering the file protocol - _, err := vfs.Root(context.Background(), fileURL) + _, err := vfs.Root(context.Background(), fileURL, key) require.Error(t, err) require.Contains(t, err.Error(), "unsupported protocol scheme \"file\"") @@ -237,7 +247,7 @@ func TestVFSReconfigureTransport(t *testing.T) { err = vfs.Reconfigure(&config.Config{Zip: cfg}) require.NoError(t, err) - root, err := vfs.Root(context.Background(), fileURL) + root, err := vfs.Root(context.Background(), fileURL, key) require.NoError(t, err) fi, err := root.Lstat(context.Background(), "index.html") |