Merge branch 'fix/native-fs-error' into 'master'

refactor: remove vfs.ErrNotExist and switch to go native fs error

See merge request gitlab-org/gitlab-pages!569

6 files changed, 48 insertions, 68 deletions

diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go
index 06ff67fb..5a6122df 100644
--- a/internal/serving/disk/reader.go
+++ b/internal/serving/disk/reader.go
@@ -2,8 +2,10 @@ package disk
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net/http"
 	"os"
 	"strconv"
@@ -39,7 +41,7 @@ func (reader *Reader) serveRedirectsStatus(h serving.Handler, redirects *redirec
 func (reader *Reader) tryRedirects(h serving.Handler) bool {
 	ctx := h.Request.Context()
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256)
-	if vfs.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return false
 	} else if err != nil {
 		httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err)
@@ -72,7 +74,7 @@ func (reader *Reader) tryFile(h serving.Handler) bool {
 	ctx := h.Request.Context()
 
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256)
-	if vfs.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return false
 	} else if err != nil {
 		httperrors.Serve500WithRequest(h.Writer, h.Request,
@@ -135,7 +137,7 @@ func (reader *Reader) tryNotFound(h serving.Handler) bool {
 	ctx := h.Request.Context()
 
 	root, err := reader.vfs.Root(ctx, h.LookupPath.Path, h.LookupPath.SHA256)
-	if vfs.IsNotExist(err) {
+	if errors.Is(err, fs.ErrNotExist) {
 		return false
 	} else if err != nil {
 		httperrors.Serve500WithRequest(h.Writer, h.Request, "vfs.Root", err)
diff --git a/internal/vfs/errors.go b/internal/vfs/errors.go
deleted file mode 100644
index 32b86192..00000000
--- a/internal/vfs/errors.go
+++ /dev/null
@@ -1,18 +0,0 @@
-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 cb48ad1c..ab757ed6 100644
--- a/internal/vfs/local/vfs.go
+++ b/internal/vfs/local/vfs.go
@@ -3,7 +3,7 @@ package local
 import (
 	"context"
 	"errors"
-	"io/fs"
+	"fmt"
 	"os"
 	"path/filepath"
 
@@ -22,16 +22,12 @@ func (localFs VFS) Root(ctx context.Context, path string, cacheKey string) (vfs.
 	}
 
 	rootPath, err = filepath.EvalSymlinks(rootPath)
-	if errors.Is(err, fs.ErrNotExist) {
-		return nil, &vfs.ErrNotExist{Inner: err}
-	} else if err != nil {
-		return nil, err
+	if err != nil {
+		return nil, fmt.Errorf("could not evaluate symlinks: %w", err)
 	}
 
 	fi, err := os.Lstat(rootPath)
-	if errors.Is(err, fs.ErrNotExist) {
-		return nil, &vfs.ErrNotExist{Inner: err}
-	} else if err != nil {
+	if err != nil {
 		return nil, err
 	}
 
diff --git a/internal/vfs/local/vfs_test.go b/internal/vfs/local/vfs_test.go
index 3f84bb39..e9b3fe71 100644
--- a/internal/vfs/local/vfs_test.go
+++ b/internal/vfs/local/vfs_test.go
@@ -2,14 +2,14 @@ package local
 
 import (
 	"context"
+	"errors"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"gitlab.com/gitlab-org/gitlab-pages/internal/vfs"
 )
 
 var localVFS = &VFS{}
@@ -99,7 +99,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, vfs.IsNotExist(err))
+				require.Equal(t, test.expectedIsNotExist, errors.Is(err, fs.ErrNotExist))
 				return
 			}
 
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index 090391fb..07afa691 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -3,6 +3,7 @@ package zip
 import (
 	"context"
 	"errors"
+	"io/fs"
 	"net/http"
 	"net/url"
 	"sync"
@@ -104,46 +105,46 @@ func New(cfg *config.ZipServing) vfs.VFS {
 
 // Reconfigure will update the zipVFS configuration values and will reset the
 // cache
-func (fs *zipVFS) Reconfigure(cfg *config.Config) error {
-	fs.cacheLock.Lock()
-	defer fs.cacheLock.Unlock()
+func (zfs *zipVFS) Reconfigure(cfg *config.Config) error {
+	zfs.cacheLock.Lock()
+	defer zfs.cacheLock.Unlock()
 
-	fs.openTimeout = cfg.Zip.OpenTimeout
-	fs.cacheExpirationInterval = cfg.Zip.ExpirationInterval
-	fs.cacheRefreshInterval = cfg.Zip.RefreshInterval
-	fs.cacheCleanupInterval = cfg.Zip.CleanupInterval
+	zfs.openTimeout = cfg.Zip.OpenTimeout
+	zfs.cacheExpirationInterval = cfg.Zip.ExpirationInterval
+	zfs.cacheRefreshInterval = cfg.Zip.RefreshInterval
+	zfs.cacheCleanupInterval = cfg.Zip.CleanupInterval
 
-	if err := fs.reconfigureTransport(cfg); err != nil {
+	if err := zfs.reconfigureTransport(cfg); err != nil {
 		return err
 	}
 
-	fs.resetCache()
+	zfs.resetCache()
 
 	return nil
 }
 
-func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error {
+func (zfs *zipVFS) reconfigureTransport(cfg *config.Config) error {
 	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths)
 	if err != nil {
 		return err
 	}
 
-	fs.httpClient.Transport.(httptransport.Transport).
+	zfs.httpClient.Transport.(httptransport.Transport).
 		RegisterProtocol("file", http.NewFileTransport(fsTransport))
 
 	return nil
 }
 
-func (fs *zipVFS) resetCache() {
-	fs.cache = cache.New(fs.cacheExpirationInterval, fs.cacheCleanupInterval)
-	fs.cache.OnEvicted(func(s string, i interface{}) {
+func (zfs *zipVFS) resetCache() {
+	zfs.cache = cache.New(zfs.cacheExpirationInterval, zfs.cacheCleanupInterval)
+	zfs.cache.OnEvicted(func(s string, i interface{}) {
 		metrics.ZipCachedEntries.WithLabelValues("archive").Dec()
 
 		i.(*zipArchive).onEvicted()
 	})
 }
 
-func (fs *zipVFS) keyFromPath(path string) (string, error) {
+func (zfs *zipVFS) keyFromPath(path string) (string, error) {
 	// We assume that our URL is https://.../artifacts.zip?content-sign=aaa
 	// our caching key is `https://.../artifacts.zip`
 	// TODO: replace caching key with file_sha256
@@ -164,10 +165,10 @@ 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, cacheKey string) (vfs.Root, error) {
+func (zfs *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)
+		k, err := zfs.keyFromPath(path)
 		if err != nil {
 			return nil, err
 		}
@@ -177,21 +178,21 @@ func (fs *zipVFS) Root(ctx context.Context, path string, cacheKey string) (vfs.R
 
 	// we do it in loop to not use any additional locks
 	for {
-		root, err := fs.findOrOpenArchive(ctx, cacheKey, path)
-		if err == errAlreadyCached {
+		root, err := zfs.findOrOpenArchive(ctx, cacheKey, path)
+		if errors.Is(err, errAlreadyCached) {
 			continue
 		}
 
 		// If archive is not found, return a known `vfs` error
-		if err == httprange.ErrNotFound {
-			err = &vfs.ErrNotExist{Inner: err}
+		if errors.Is(err, httprange.ErrNotFound) {
+			return nil, fs.ErrNotExist
 		}
 
 		return root, err
 	}
 }
 
-func (fs *zipVFS) Name() string {
+func (zfs *zipVFS) Name() string {
 	return "zip"
 }
 
@@ -199,14 +200,14 @@ func (fs *zipVFS) Name() string {
 // otherwise creates the archive entry in a cache and try to save it,
 // if saving fails it's because the archive has already been cached
 // (e.g. by another concurrent request)
-func (fs *zipVFS) findOrCreateArchive(ctx context.Context, key string) (*zipArchive, error) {
+func (zfs *zipVFS) findOrCreateArchive(ctx context.Context, key string) (*zipArchive, error) {
 	// This needs to happen in lock to ensure that
 	// concurrent access will not remove it
 	// it is needed due to the bug https://github.com/patrickmn/go-cache/issues/48
-	fs.cacheLock.Lock()
-	defer fs.cacheLock.Unlock()
+	zfs.cacheLock.Lock()
+	defer zfs.cacheLock.Unlock()
 
-	archive, expiry, found := fs.cache.GetWithExpiration(key)
+	archive, expiry, found := zfs.cache.GetWithExpiration(key)
 	if found {
 		status, _ := archive.(*zipArchive).openStatus()
 		switch status {
@@ -219,8 +220,8 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, key string) (*zipArch
 			metrics.ZipCacheRequests.WithLabelValues("archive", "hit-open-error").Inc()
 
 		case archiveOpened:
-			if time.Until(expiry) < fs.cacheRefreshInterval {
-				fs.cache.SetDefault(key, archive)
+			if time.Until(expiry) < zfs.cacheRefreshInterval {
+				zfs.cache.SetDefault(key, archive)
 				metrics.ZipCacheRequests.WithLabelValues("archive", "hit-refresh").Inc()
 			} else {
 				metrics.ZipCacheRequests.WithLabelValues("archive", "hit").Inc()
@@ -235,16 +236,16 @@ func (fs *zipVFS) findOrCreateArchive(ctx context.Context, key string) (*zipArch
 	}
 
 	if archive == nil {
-		archive = newArchive(fs, fs.openTimeout)
+		archive = newArchive(zfs, zfs.openTimeout)
 
 		// We call delete to ensure that expired item
 		// is properly evicted as there's a bug in a cache library:
 		// https://github.com/patrickmn/go-cache/issues/48
-		fs.cache.Delete(key)
+		zfs.cache.Delete(key)
 
 		// if adding the archive to the cache fails it means it's already been added before
 		// this is done to find concurrent additions.
-		if fs.cache.Add(key, archive, fs.cacheExpirationInterval) != nil {
+		if zfs.cache.Add(key, archive, zfs.cacheExpirationInterval) != nil {
 			metrics.ZipCacheRequests.WithLabelValues("archive", "already-cached").Inc()
 			return nil, errAlreadyCached
 		}
@@ -257,8 +258,8 @@ 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) {
-	zipArchive, err := fs.findOrCreateArchive(ctx, key)
+func (zfs *zipVFS) findOrOpenArchive(ctx context.Context, key, path string) (*zipArchive, error) {
+	zipArchive, err := zfs.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 9343405c..eaa2352d 100644
--- a/internal/vfs/zip/vfs_test.go
+++ b/internal/vfs/zip/vfs_test.go
@@ -3,6 +3,7 @@ package zip
 import (
 	"context"
 	"io"
+	"io/fs"
 	"testing"
 	"time"
 
@@ -12,8 +13,6 @@ 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"
 )
 
@@ -33,7 +32,7 @@ func TestVFSRoot(t *testing.T) {
 		"zip_file_does_not_exist": {
 			path:           "/unknown",
 			sha256:         "filedoesnotexist",
-			expectedErrMsg: vfs.ErrNotExist{Inner: httprange.ErrNotFound}.Error(),
+			expectedErrMsg: fs.ErrNotExist.Error(),
 		},
 		"invalid_url": {
 			path:           "/%",