sync: fix inconsistent test, used inject error fw for hard to reach test cases

Signed-off-by: Petu Eusebiu <peusebiu@cisco.com>

Makefile

@@ -49,8 +49,8 @@ test: check-skopeo $(NOTATION)
 	go test -tags extended,containers_image_openpgp -v -trimpath -race -timeout 15m -cover -coverpkg ./... -coverprofile=coverage-extended.txt -covermode=atomic ./...
 	go test -tags minimal,containers_image_openpgp -v -trimpath -race -cover -coverpkg ./... -coverprofile=coverage-minimal.txt -covermode=atomic ./...
 	# development-mode unit tests possibly using failure injection
-	go test -tags dev,extended,containers_image_openpgp -v -trimpath -race -timeout 15m -cover -coverpkg ./... -coverprofile=coverage-dev-extended.txt -covermode=atomic ./pkg/test/... ./pkg/storage/...
+	go test -tags dev,extended,containers_image_openpgp -v -trimpath -race -timeout 15m -cover -coverpkg ./... -coverprofile=coverage-dev-extended.txt -covermode=atomic ./pkg/test/... ./pkg/storage/... ./pkg/extensions/sync/... -run ^TestInject
-	go test -tags dev,minimal,containers_image_openpgp -v -trimpath -race -cover -coverpkg ./... -coverprofile=coverage-dev-minimal.txt -covermode=atomic ./pkg/test/... ./pkg/storage/...
+	go test -tags dev,minimal,containers_image_openpgp -v -trimpath -race -cover -coverpkg ./... -coverprofile=coverage-dev-minimal.txt -covermode=atomic ./pkg/test/... ./pkg/storage/... ./pkg/extensions/sync/... -run ^TestInject
 
 .PHONY: run-bench
 run-bench: binary bench

go.sum

@@ -1862,6 +1862,8 @@ github.com/onsi/gomega v1.15.0/go.mod h1:cIuvLEne0aoVhAgh/O6ac0Op8WWw9H6eYCriF+t
 github.com/onsi/gomega v1.16.0 h1:6gjqkI8iiRHMvdccRJM8rVKjCWk6ZIm6FTm3ddIe4/c=
 github.com/onsi/gomega v1.16.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAlGdZY=
 github.com/op/go-logging v0.0.0-20160315200505-970db520ece7/go.mod h1:HzydrMdWErDVzsI23lYNej1Htcns9BCg93Dk0bBINWk=
+github.com/open-policy-agent/opa v0.32.0/go.mod h1:5sJdtc+1/U8zy/j30njpQl6u9rM4MzTOhG9EW1uOmsY=
+github.com/open-policy-agent/opa v0.34.1/go.mod h1:buysXn+6zB/b+6JgLkP4WgKZ9+UgUtFAgtemYGrL9Ik=
 github.com/open-policy-agent/opa v0.37.0 h1:OUXB+RAcxQpmXeNW2BN1wYzQQvVCPF1T9zv+QXGr9Wg=
 github.com/open-policy-agent/opa v0.37.0/go.mod h1:xX3NUCZuXK8f0CNhFQvhm4495mZLptf94pIkWRLaFqo=
 github.com/opencontainers/distribution-spec/specs-go v0.0.0-20211026153258-b3f631f25f1a h1:cZbrj1PTlynrvx4pIVbrI3rvncbxtMG5U4EYM6ScQ/A=

pkg/extensions/sync/on_demand.go

@@ -194,16 +194,7 @@ func syncOneImage(imageChannel chan error, cfg Config, storeController storage.S
 				return
 			}
 
-			localCachePath, err := getLocalCachePath(imageStore, repo)
+			localImageRef, localCachePath, err := getLocalImageRef(imageStore, repo, tag)
-			if err != nil {
-				log.Error().Err(err).Str("dir", localCachePath).Msg("couldn't create temporary dir")
-
-				imageChannel <- err
-
-				return
-			}
-
-			localImageRef, err := getLocalImageRef(localCachePath, repo, tag)
 			if err != nil {
 				log.Error().Err(err).Msgf("couldn't obtain a valid image reference for reference %s/%s:%s",
 					localCachePath, repo, tag)

pkg/extensions/sync/sync.go

@@ -22,6 +22,7 @@ import (
 	"zotregistry.io/zot/errors"
 	"zotregistry.io/zot/pkg/log"
 	"zotregistry.io/zot/pkg/storage"
+	"zotregistry.io/zot/pkg/test"
 )
 
 const (
@@ -102,7 +103,8 @@ func getUpstreamCatalog(client *resty.Client, upstreamURL string, log log.Logger
 // It returns a string slice of tags and any error encountered.
 func getImageTags(ctx context.Context, sysCtx *types.SystemContext, repoRef reference.Named) ([]string, error) {
 	dockerRef, err := docker.NewReference(reference.TagNameOnly(repoRef))
-	if err != nil {
+	// hard to reach test case, injected error, see pkg/test/dev.go
+	if err = test.Error(err); err != nil {
 		return nil, err // Should never happen for a reference with tag and no digest
 	}
 
@@ -364,16 +366,7 @@ func syncRegistry(regCfg RegistryConfig, upstreamURL string, storeController sto
 			continue
 		}
 
-		localCachePath, err := getLocalCachePath(imageStore, repo)
+		localImageRef, localCachePath, err := getLocalImageRef(imageStore, repo, tag)
-		if err != nil {
-			log.Error().Err(err).Str("dir", localCachePath).Msg("couldn't create temporary dir")
-
-			return err
-		}
-
-		defer os.RemoveAll(localCachePath)
-
-		localImageRef, err := getLocalImageRef(localCachePath, repo, tag)
 		if err != nil {
 			log.Error().Err(err).Msgf("couldn't obtain a valid image reference for reference %s/%s:%s",
 				localCachePath, repo, tag)
@@ -381,6 +374,8 @@ func syncRegistry(regCfg RegistryConfig, upstreamURL string, storeController sto
 			return err
 		}
 
+		defer os.RemoveAll(localCachePath)
+
 		log.Info().Msgf("copying image %s to %s", upstreamImageRef.DockerReference(), localCachePath)
 
 		if err = retry.RetryIfNecessary(context.Background(), func() error {
@@ -431,7 +426,7 @@ func getLocalContexts(log log.Logger) (*types.SystemContext, *signature.PolicyCo
 	policy = &signature.Policy{Default: []signature.PolicyRequirement{signature.NewPRInsecureAcceptAnything()}}
 
 	policyContext, err := signature.NewPolicyContext(policy)
-	if err != nil {
+	if err := test.Error(err); err != nil {
 		log.Error().Err(err).Msg("couldn't create policy context")
 
 		return &types.SystemContext{}, &signature.PolicyContext{}, err

pkg/extensions/sync/sync_internal_test.go

@@ -34,6 +34,52 @@ const (
 	host = "127.0.0.1:45117"
 )
 
+func TestInjectSyncUtils(t *testing.T) {
+	Convey("Inject errors in utils functions", t, func() {
+		repositoryReference := fmt.Sprintf("%s/%s", host, testImage)
+		ref, err := parseRepositoryReference(repositoryReference)
+		So(err, ShouldBeNil)
+		So(ref.Name(), ShouldEqual, repositoryReference)
+
+		taggedRef, err := reference.WithTag(ref, "tag")
+		So(err, ShouldBeNil)
+
+		injected := test.InjectFailure(0)
+		if injected {
+			_, err = getImageTags(context.Background(), &types.SystemContext{}, taggedRef)
+			So(err, ShouldNotBeNil)
+		}
+
+		injected = test.InjectFailure(0)
+		_, _, err = getLocalContexts(log.NewLogger("debug", ""))
+		if injected {
+			So(err, ShouldNotBeNil)
+		} else {
+			So(err, ShouldBeNil)
+		}
+
+		storageDir, err := ioutil.TempDir("", "oci-dest-repo-test")
+		if err != nil {
+			panic(err)
+		}
+
+		defer os.RemoveAll(storageDir)
+
+		log := log.Logger{Logger: zerolog.New(os.Stdout)}
+		metrics := monitoring.NewMetricsServer(false, log)
+
+		imageStore := storage.NewImageStore(storageDir, false, storage.DefaultGCDelay, false, false, log, metrics)
+
+		injected = test.InjectFailure(0)
+		_, _, err = getLocalImageRef(imageStore, testImage, testImageTag)
+		if injected {
+			So(err, ShouldNotBeNil)
+		} else {
+			So(err, ShouldBeNil)
+		}
+	})
+}
+
 func TestSyncInternal(t *testing.T) {
 	Convey("Verify parseRepositoryReference func", t, func() {
 		repositoryReference := fmt.Sprintf("%s/%s", host, testImage)
@@ -82,8 +128,6 @@ func TestSyncInternal(t *testing.T) {
 		dockerRef, err := docker.NewReference(taggedRef)
 		So(err, ShouldBeNil)
 
-		// tag := getTagFromRef(dockerRef, log.NewLogger("", ""))
-
 		So(getTagFromRef(dockerRef, log.NewLogger("debug", "")), ShouldNotBeNil)
 
 		var tlsVerify bool
@@ -110,6 +154,32 @@ func TestSyncInternal(t *testing.T) {
 		So(err, ShouldNotBeNil)
 	})
 
+	Convey("Verify getLocalImageRef()", t, func() {
+		storageDir, err := ioutil.TempDir("", "oci-dest-repo-test")
+		if err != nil {
+			panic(err)
+		}
+
+		defer os.RemoveAll(storageDir)
+
+		log := log.Logger{Logger: zerolog.New(os.Stdout)}
+		metrics := monitoring.NewMetricsServer(false, log)
+
+		imageStore := storage.NewImageStore(storageDir, false, storage.DefaultGCDelay, false, false, log, metrics)
+
+		err = os.Chmod(imageStore.RootDir(), 0o000)
+		So(err, ShouldBeNil)
+
+		_, _, err = getLocalImageRef(imageStore, testImage, testImageTag)
+		So(err, ShouldNotBeNil)
+
+		err = os.Chmod(imageStore.RootDir(), 0o755)
+		So(err, ShouldBeNil)
+
+		_, _, err = getLocalImageRef(imageStore, "zot][]321", "tag_tag][]")
+		So(err, ShouldNotBeNil)
+	})
+
 	Convey("Test getUpstreamCatalog() with missing certs", t, func() {
 		var tlsVerify bool
 		updateDuration := time.Microsecond
@@ -191,10 +261,6 @@ func TestSyncInternal(t *testing.T) {
 		httpClient, err = getHTTPClient(&syncRegistryConfig, baseSecureURL, Credentials{}, log.NewLogger("debug", ""))
 		So(err, ShouldNotBeNil)
 		So(httpClient, ShouldBeNil)
-
-		httpClient, err = getHTTPClient(&syncRegistryConfig, "invalidUrl]", Credentials{}, log.NewLogger("debug", ""))
-		So(err, ShouldNotBeNil)
-		So(httpClient, ShouldBeNil)
 	})
 
 	Convey("Test imagesToCopyFromUpstream()", t, func() {

pkg/extensions/sync/sync_test.go

@@ -626,14 +626,13 @@ func TestOnDemandPermsDenied(t *testing.T) {
 			}
 		}()
 
-		for {
+		syncSubDir := path.Join(destDir, testImage, sync.SyncBlobUploadDir)
-			err = os.Chmod(path.Join(destDir, testImage, sync.SyncBlobUploadDir), 0o000)
-			if err != nil {
-				continue
-			}
 
-			break
+		err = os.MkdirAll(syncSubDir, 0o755)
-		}
+		So(err, ShouldBeNil)
+
+		err = os.Chmod(syncSubDir, 0o000)
+		So(err, ShouldBeNil)
 
 		// wait till ready
 		for {
@@ -649,108 +648,13 @@ func TestOnDemandPermsDenied(t *testing.T) {
 		So(err, ShouldBeNil)
 		So(resp.StatusCode(), ShouldEqual, 404)
 
-		err = os.Chmod(path.Join(destDir, testImage, sync.SyncBlobUploadDir), 0o755)
+		err = os.Chmod(syncSubDir, 0o755)
 		if err != nil {
 			panic(err)
 		}
 	})
 }
 
-func TestPeriodicallyPermsDenied(t *testing.T) {
-	Convey("Verify periodically sync feature without perm on sync cache", t, func() {
-		updateDuration, _ := time.ParseDuration("30m")
-
-		sctlr, srcBaseURL, srcDir, _, _ := startUpstreamServer(false, false)
-		defer os.RemoveAll(srcDir)
-
-		defer func() {
-			sctlr.Shutdown()
-		}()
-
-		regex := ".*"
-		semver := true
-		var tlsVerify bool
-
-		syncRegistryConfig := sync.RegistryConfig{
-			Content: []sync.Content{
-				{
-					Prefix: testImage,
-					Tags: &sync.Tags{
-						Regex:  &regex,
-						Semver: &semver,
-					},
-				},
-			},
-			URLs:         []string{srcBaseURL},
-			PollInterval: updateDuration,
-			TLSVerify:    &tlsVerify,
-			CertDir:      "",
-			OnDemand:     true,
-		}
-
-		defaultVal := true
-		syncConfig := &sync.Config{
-			Enable:     &defaultVal,
-			Registries: []sync.RegistryConfig{syncRegistryConfig},
-		}
-
-		destPort := test.GetFreePort()
-		destConfig := config.New()
-		destBaseURL := test.GetBaseURL(destPort)
-
-		destConfig.HTTP.Port = destPort
-
-		destDir, err := ioutil.TempDir("", "oci-dest-repo-test")
-		if err != nil {
-			panic(err)
-		}
-
-		defer os.RemoveAll(destDir)
-
-		destConfig.Storage.RootDirectory = destDir
-
-		destConfig.Extensions = &extconf.ExtensionConfig{}
-		destConfig.Extensions.Search = nil
-		destConfig.Extensions.Sync = syncConfig
-
-		dctlr := api.NewController(destConfig)
-
-		go func() {
-			// this blocks
-			if err := dctlr.Run(); err != nil {
-				return
-			}
-		}()
-
-		for {
-			err = os.Chmod(path.Join(destDir, testImage, sync.SyncBlobUploadDir), 0o000)
-			if err != nil {
-				continue
-			}
-
-			break
-		}
-
-		// wait till ready
-		for {
-			_, err := resty.R().Get(destBaseURL)
-			if err == nil {
-				break
-			}
-
-			time.Sleep(100 * time.Millisecond)
-		}
-
-		defer func() {
-			dctlr.Shutdown()
-			err := os.Chmod(path.Join(destDir, testImage, sync.SyncBlobUploadDir), 0o755)
-			if err != nil {
-				panic(err)
-			}
-		}()
-	})
-}
-
 func TestBadTLS(t *testing.T) {
 	Convey("Verify sync TLS feature", t, func() {
 		updateDuration, _ := time.ParseDuration("30m")
@@ -2895,7 +2799,10 @@ func TestError(t *testing.T) {
 
 		// give permission denied on pushSyncedLocalImage()
 		localRepoPath := path.Join(destDir, testImage, "blobs")
-		err := os.MkdirAll(localRepoPath, 0o000)
+		err := os.MkdirAll(localRepoPath, 0o755)
+		So(err, ShouldBeNil)
+
+		err = os.Chmod(localRepoPath, 0o000)
 		So(err, ShouldBeNil)
 
 		resp, err := client.R().Get(destBaseURL + "/v2/" + testImage + "/manifests/" + testImageTag)

pkg/extensions/sync/utils.go

@@ -28,6 +28,7 @@ import (
 	"zotregistry.io/zot/pkg/extensions/monitoring"
 	"zotregistry.io/zot/pkg/log"
 	"zotregistry.io/zot/pkg/storage"
+	"zotregistry.io/zot/pkg/test"
 )
 
 type ReferenceList struct {
@@ -527,22 +528,6 @@ func StripRegistryTransport(url string) string {
 	return strings.Replace(strings.Replace(url, "http://", "", 1), "https://", "", 1)
 }
 
-// get a .sync subdir used for temporary store one synced image.
-func getLocalCachePath(imageStore storage.ImageStore, repo string) (string, error) {
-	uuid, err := guuid.NewV4()
-	if err != nil {
-		return "", err
-	}
-
-	localCachePath := path.Join(imageStore.RootDir(), repo, SyncBlobUploadDir, uuid.String())
-
-	if err = os.MkdirAll(path.Join(localCachePath, repo), storage.DefaultDirPerms); err != nil {
-		return "", err
-	}
-
-	return localCachePath, nil
-}
-
 // get an ImageReference given the registry, repo and tag.
 func getImageRef(registryDomain, repo, tag string) (types.ImageReference, error) {
 	repoRef, err := parseRepositoryReference(fmt.Sprintf("%s/%s", registryDomain, repo))
@@ -564,16 +549,28 @@ func getImageRef(registryDomain, repo, tag string) (types.ImageReference, error)
 }
 
 // get a local ImageReference used to temporary store one synced image.
-func getLocalImageRef(localCachePath, repo, tag string) (types.ImageReference, error) {
+func getLocalImageRef(imageStore storage.ImageStore, repo, tag string) (types.ImageReference, string, error) {
+	uuid, err := guuid.NewV4()
+	// hard to reach test case, injected error, see pkg/test/dev.go
+	if err := test.Error(err); err != nil {
+		return nil, "", err
+	}
+
+	localCachePath := path.Join(imageStore.RootDir(), repo, SyncBlobUploadDir, uuid.String())
+
+	if err = os.MkdirAll(path.Join(localCachePath, repo), storage.DefaultDirPerms); err != nil {
+		return nil, "", err
+	}
+
 	localRepo := path.Join(localCachePath, repo)
 	localTaggedRepo := fmt.Sprintf("%s:%s", localRepo, tag)
 
 	localImageRef, err := layout.ParseReference(localTaggedRepo)
 	if err != nil {
-		return nil, err
+		return nil, "", err
 	}
 
-	return localImageRef, nil
+	return localImageRef, localCachePath, nil
 }
 
 // canSkipImage returns whether or not the image can be skipped from syncing.

pkg/storage/storage_fs_test.go

@@ -793,7 +793,7 @@ func TestHardLink(t *testing.T) {
 	})
 }
 
-func TestWriteFile(t *testing.T) {
+func TestInjectWriteFile(t *testing.T) {
 	Convey("writeFile with commit", t, func() {
 		dir, err := ioutil.TempDir("", "oci-repo-test")
 		So(err, ShouldBeNil)