fix(npe): handle case where os.Stat returns different error types in DirExists (#2253)

See https://github.com/project-zot/zot/actions/runs/7905369535/job/21577848110

Also add tests to fix some of the coverage fluctuations.

Signed-off-by: Andrei Aaron <aaaron@luxoft.com>

pkg/storage/common/common_test.go

@@ -25,6 +25,8 @@ import (
 	"zotregistry.dev/zot/pkg/test/mocks"
 )
 
+var ErrTestError = errors.New("TestError")
+
 func TestValidateManifest(t *testing.T) {
 	Convey("Make manifest", t, func(c C) {
 		dir := t.TempDir()
@@ -510,3 +512,32 @@ func TestIsSignature(t *testing.T) {
 		So(isSingature, ShouldBeFalse)
 	})
 }
+
+func TestDedupeGeneratorErrors(t *testing.T) {
+	log := log.Logger{Logger: zerolog.New(os.Stdout)}
+
+	// Ideally this would be covered by the end-to-end test,
+	// but the coverage for the error is unpredictable, prone to race conditions
+	Convey("GetNextDigestWithBlobPaths errors", t, func(c C) {
+		imgStore := &mocks.MockedImageStore{
+			GetRepositoriesFn: func() ([]string, error) {
+				return []string{"repo1", "repo2"}, nil
+			},
+			GetNextDigestWithBlobPathsFn: func(repos []string, lastDigests []godigest.Digest) (
+				godigest.Digest, []string, error,
+			) {
+				return "sha256:123", []string{}, ErrTestError
+			},
+		}
+
+		generator := &common.DedupeTaskGenerator{
+			ImgStore: imgStore,
+			Dedupe:   true,
+			Log:      log,
+		}
+
+		task, err := generator.Next()
+		So(err, ShouldNotBeNil)
+		So(task, ShouldBeNil)
+	})
+}

pkg/storage/local/driver.go

@@ -5,11 +5,9 @@ import (
 	"bytes"
 	"errors"
 	"io"
-	"io/fs"
 	"os"
 	"path"
 	"sort"
-	"syscall"
 	"time"
 	"unicode/utf8"
 
@@ -45,13 +43,10 @@ func (driver *Driver) DirExists(path string) bool {
 
 	fileInfo, err := os.Stat(path)
 	if err != nil {
-		if e, ok := err.(*fs.PathError); ok && errors.Is(e.Err, syscall.ENAMETOOLONG) || //nolint: errorlint
+		// if os.Stat returns any error, fileInfo will be nil
-			errors.Is(e.Err, syscall.EINVAL) {
+		// we can't check if the path is a directory using fileInfo if we received an error
-			return false
+		// let's assume the directory doesn't exist in all error cases
-		}
+		// see possible errors http://man.he.net/man2/newfstatat
-	}
-
-	if err != nil && os.IsNotExist(err) {
 		return false
 	}
 

pkg/storage/local/driver_test.go

@@ -0,0 +1,115 @@
+package local_test
+
+import (
+	"os"
+	"path"
+	"strings"
+	"testing"
+
+	storagedriver "github.com/docker/distribution/registry/storage/driver"
+	. "github.com/smartystreets/goconvey/convey"
+
+	storageConstants "zotregistry.dev/zot/pkg/storage/constants"
+	"zotregistry.dev/zot/pkg/storage/local"
+)
+
+func TestStorageDriver(t *testing.T) {
+	driver := local.New(true)
+
+	Convey("Test DirExists", t, func() {
+		rootDir := t.TempDir()
+
+		// Folder exists
+		result := driver.DirExists(rootDir)
+		So(result, ShouldBeTrue)
+
+		// Folder name triggering ENAMETOOLONG
+		result = driver.DirExists(path.Join(rootDir, strings.Repeat("1234567890", 1000)))
+		So(result, ShouldBeFalse)
+
+		// Folder which does not exist
+		result = driver.DirExists(path.Join(rootDir, "someName"))
+		So(result, ShouldBeFalse)
+
+		// Path is actually a file
+		fileName := "testFile"
+		_, err := os.Create(path.Join(rootDir, fileName))
+		So(err, ShouldBeNil)
+		result = driver.DirExists(path.Join(rootDir, fileName))
+		So(result, ShouldBeFalse)
+
+		// Folder name triggering ENOTDIR a one of the parents is not a folder
+		result = driver.DirExists(path.Join(rootDir, fileName, "someName"))
+		So(result, ShouldBeFalse)
+
+		// New folder created by driver
+		repoName := "testRepo"
+		err = driver.EnsureDir(path.Join(rootDir, repoName))
+		So(err, ShouldBeNil)
+
+		result = driver.DirExists(path.Join(rootDir, repoName))
+		So(result, ShouldBeTrue)
+
+		// Folder without permissions
+		err = os.Chmod(path.Join(rootDir, repoName), 0o000)
+		So(err, ShouldBeNil)
+		defer os.Chmod(path.Join(rootDir, repoName), storageConstants.DefaultDirPerms) //nolint:errcheck
+
+		result = driver.DirExists(path.Join(rootDir, repoName))
+		So(result, ShouldBeTrue)
+	})
+
+	Convey("Test Walk", t, func() {
+		Convey("Test all folders are walked and files are identified correctly", func() {
+			rootDir := t.TempDir()
+			err := driver.EnsureDir(path.Join(rootDir, "d1", "d11"))
+			So(err, ShouldBeNil)
+			err = driver.EnsureDir(path.Join(rootDir, "d1", "d12"))
+			So(err, ShouldBeNil)
+			err = driver.EnsureDir(path.Join(rootDir, "d2"))
+			So(err, ShouldBeNil)
+			_, err = os.Create(path.Join(rootDir, "d1", "d11", "f111"))
+			So(err, ShouldBeNil)
+			_, err = os.Create(path.Join(rootDir, "d2", "f21"))
+			So(err, ShouldBeNil)
+
+			fileList := []string{}
+			folderList := []string{}
+
+			err = driver.Walk(rootDir, func(fileInfo storagedriver.FileInfo) error {
+				if fileInfo.IsDir() {
+					folderList = append(folderList, fileInfo.Path())
+				} else {
+					fileList = append(fileList, fileInfo.Path())
+				}
+
+				return nil
+			})
+			So(err, ShouldBeNil)
+
+			So(len(fileList), ShouldEqual, 2)
+			So(fileList, ShouldContain, path.Join(rootDir, "d1", "d11", "f111"))
+			So(fileList, ShouldContain, path.Join(rootDir, "d2", "f21"))
+			So(len(folderList), ShouldEqual, 4)
+			So(folderList, ShouldContain, path.Join(rootDir, "d1"))
+			So(folderList, ShouldContain, path.Join(rootDir, "d1", "d11"))
+			So(folderList, ShouldContain, path.Join(rootDir, "d1", "d12"))
+			So(folderList, ShouldContain, path.Join(rootDir, "d2"))
+		})
+
+		Convey("Test deleting folders while walking raises doesn't raise errors", func() {
+			rootDir := t.TempDir()
+			err := driver.EnsureDir(path.Join(rootDir, "d1"))
+			So(err, ShouldBeNil)
+			err = driver.EnsureDir(path.Join(rootDir, "d2"))
+			So(err, ShouldBeNil)
+
+			// List/Sort d1 and d2, delete d2 while d1 is walked
+			// While d2 is walked the PathNotFoundError should be ignored
+			err = driver.Walk(rootDir, func(fileInfo storagedriver.FileInfo) error {
+				return driver.Delete(path.Join(rootDir, "d2"))
+			})
+			So(err, ShouldBeNil)
+		})
+	})
+}

pkg/storage/s3/s3_test.go

@@ -2261,9 +2261,29 @@ func TestRebuildDedupeIndex(t *testing.T) {
 	})
 }
 
-func TestRebuildDedupeMockStoreDriver(t *testing.T) {
+func TestNextRepositoryMockStoreDriver(t *testing.T) {
-	tskip.SkipS3(t)
+	testDir := t.TempDir()
+	tdir := t.TempDir()
 
+	// some s3 implementations (eg, digitalocean spaces) will return pathnotfounderror for walk but not list
+	// This code cannot be reliably covered by end to end tests
+	Convey("Trigger PathNotFound error when Walk() is called in GetNextRepository()", t, func() {
+		imgStore := createMockStorage(testDir, tdir, false, &StorageDriverMock{
+			ListFn: func(ctx context.Context, path string) ([]string, error) {
+				return []string{}, nil
+			},
+			WalkFn: func(ctx context.Context, path string, walkFn driver.WalkFn) error {
+				return driver.PathNotFoundError{}
+			},
+		})
+
+		nextRepository, err := imgStore.GetNextRepository("testRepo")
+		So(err, ShouldBeNil)
+		So(nextRepository, ShouldEqual, "")
+	})
+}
+
+func TestRebuildDedupeMockStoreDriver(t *testing.T) {
 	uuid, err := guuid.NewV4()
 	if err != nil {
 		panic(err)