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

7 files changed, 51 insertions, 29 deletions

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")