diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2021-11-17 02:35:13 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2021-11-17 02:35:13 +0300 |
commit | 5ce207307401844547296c82d45570a0bedc3008 (patch) | |
tree | 6845038eb52271dab4a03f7976627fd1fe2e78ef | |
parent | 99abcab290104090f69b94ced029ca2658da4c00 (diff) | |
parent | dff59f10c72bd1d0c23fa0486150257e29ee9ae0 (diff) |
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
-rw-r--r-- | internal/serving/disk/reader.go | 8 | ||||
-rw-r--r-- | internal/vfs/errors.go | 18 | ||||
-rw-r--r-- | internal/vfs/local/vfs.go | 12 | ||||
-rw-r--r-- | internal/vfs/local/vfs_test.go | 6 | ||||
-rw-r--r-- | internal/vfs/zip/vfs.go | 67 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 5 |
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: "/%", |