fix: revert cache lookup changes (GoogleContainerTools#9313)

* Revert "fix(lookupRemote): fixed lookup.go lookupRemote to compare remote and cached digests (GoogleContainerTools#9278)"

This reverts commit 9ff4546.

* Revert "fix: Scope Issue with the 'entry' variable when looking up remote images and tests additions (GoogleContainerTools#9211)"

This reverts commit ffe769c.

* Revert "fix: puling images when working with a remote repository (GoogleContainerTools#9177) (GoogleContainerTools#9181)"

This reverts commit 9376dc0.

integration/build_dependencies_test.go

@@ -96,7 +96,6 @@ func TestBuildDependenciesCache(t *testing.T) {
 	// The test then triggers another build and verifies that the images in `rebuilt` were built
 	// (e.g., the changed images and their dependents), and that the other images were found in the artifact cache.
 	// It runs the profile `concurrency-0` which builds with maximum concurrency.
-	MarkIntegrationTest(t, CanRunWithoutGcp)
 	tests := []struct {
 		description string
 		change      []int
@@ -137,6 +136,7 @@ func TestBuildDependenciesCache(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
+			MarkIntegrationTest(t, CanRunWithoutGcp)
 			// modify file `foo` to invalidate cache for target artifacts
 			for _, i := range test.change {
 				Run(t, fmt.Sprintf("testdata/build-dependencies/app%d", i), "sh", "-c", fmt.Sprintf("echo %s > foo", uuid.New().String()))

pkg/skaffold/build/cache/cache.go

@@ -42,14 +42,6 @@ type ImageDetails struct {
 	ID     string `yaml:"id,omitempty"`
 }
 
-func (d ImageDetails) HasID() bool {
-	return d.ID != ""
-}
-
-func (d ImageDetails) HasDigest() bool {
-	return d.Digest != ""
-}
-
 // ArtifactCache is a map of [artifact dependencies hash : ImageDetails]
 type ArtifactCache map[string]ImageDetails
 

pkg/skaffold/build/cache/lookup.go

@@ -68,29 +68,30 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string, plat
 		return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)}
 	}
 
+	c.cacheMutex.RLock()
+	entry, cacheHit := c.artifactCache[hash]
+	c.cacheMutex.RUnlock()
+
 	pls := platforms.GetPlatforms(a.ImageName)
+	// TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images
+	// See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403
+	if !cacheHit && !pls.IsMultiPlatform() {
+		var pl v1.Platform
+		if len(pls.Platforms) == 1 {
+			pl = util.ConvertToV1Platform(pls.Platforms[0])
+		}
+		if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil {
+			log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err)
+			return needsBuilding{hash: hash}
+		}
+	}
+
 	if isLocal, err := c.isLocalImage(a.ImageName); err != nil {
 		return failed{err}
 	} else if isLocal {
-		c.cacheMutex.RLock()
-		entry, cacheHit := c.artifactCache[hash]
-		c.cacheMutex.RUnlock()
-
-		// TODO (gaghosh): allow `tryImport` when the Docker daemon starts supporting multiarch images
-		// See https://github.com/docker/buildx/issues/1220#issuecomment-1189996403
-		if !cacheHit && !pls.IsMultiPlatform() {
-			var pl v1.Platform
-			if len(pls.Platforms) == 1 {
-				pl = util.ConvertToV1Platform(pls.Platforms[0])
-			}
-			if entry, err = c.tryImport(ctx, a, tag, hash, pl); err != nil {
-				log.Entry(ctx).Debugf("Could not import artifact from Docker, building instead (%s)", err)
-				return needsBuilding{hash: hash}
-			}
-		}
 		return c.lookupLocal(ctx, hash, tag, entry)
 	}
-	return c.lookupRemote(ctx, hash, tag, pls.Platforms)
+	return c.lookupRemote(ctx, hash, tag, pls.Platforms, entry)
 }
 
 func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDetails) cacheDetails {
@@ -118,41 +119,25 @@ func (c *cache) lookupLocal(ctx context.Context, hash, tag string, entry ImageDe
 	return needsBuilding{hash: hash}
 }
 
-func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform) cacheDetails {
-	c.cacheMutex.RLock()
-	cachedEntry, cacheHit := c.artifactCache[hash]
-	c.cacheMutex.RUnlock()
-
+func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []specs.Platform, entry ImageDetails) cacheDetails {
 	if remoteDigest, err := docker.RemoteDigest(tag, c.cfg, nil); err == nil {
-		if cacheHit && remoteDigest == cachedEntry.Digest {
-			log.Entry(ctx).Debugf("Found %s remote with the same digest", tag)
+		// Image exists remotely with the same tag and digest
+		if remoteDigest == entry.Digest {
 			return found{hash: hash}
 		}
-
-		if !cacheHit {
-			log.Entry(ctx).Debugf("Added digest for %s to cache entry", tag)
-			cachedEntry.Digest = remoteDigest
-			c.cacheMutex.Lock()
-			c.artifactCache[hash] = cachedEntry
-			c.cacheMutex.Unlock()
-		}
 	}
 
-	if cachedEntry.HasDigest() {
-		// Image exists remotely with a different tag
-		fqn := tag + "@" + cachedEntry.Digest // Actual tag will be ignored but we need the registry and the digest part of it.
-		log.Entry(ctx).Debugf("Looking up %s tag with the full fqn %s", tag, cachedEntry.Digest)
-		if remoteDigest, err := docker.RemoteDigest(fqn, c.cfg, nil); err == nil {
-			log.Entry(ctx).Debugf("Found %s with the full fqn", tag)
-			if remoteDigest == cachedEntry.Digest {
-				return needsRemoteTagging{hash: hash, tag: tag, digest: cachedEntry.Digest, platforms: platforms}
-			}
+	// Image exists remotely with a different tag
+	fqn := tag + "@" + entry.Digest // Actual tag will be ignored but we need the registry and the digest part of it.
+	if remoteDigest, err := docker.RemoteDigest(fqn, c.cfg, nil); err == nil {
+		if remoteDigest == entry.Digest {
+			return needsRemoteTagging{hash: hash, tag: tag, digest: entry.Digest, platforms: platforms}
 		}
 	}
 
 	// Image exists locally
-	if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
-		return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
+	if entry.ID != "" && c.client != nil && c.client.ImageExists(ctx, entry.ID) {
+		return needsPushing{hash: hash, tag: tag, imageID: entry.ID}
 	}
 
 	return needsBuilding{hash: hash}

pkg/skaffold/build/cache/lookup_test.go

@@ -146,13 +146,18 @@ func TestLookupRemote(t *testing.T) {
 		hasher      artifactHasher
 		cache       map[string]ImageDetails
 		api         *testutil.FakeAPIClient
-		tag         string
 		expected    cacheDetails
 	}{
+		{
+			description: "miss",
+			hasher:      mockHasher{"hash"},
+			api:         &testutil.FakeAPIClient{ErrImagePull: true},
+			cache:       map[string]ImageDetails{},
+			expected:    needsBuilding{hash: "hash"},
+		},
 		{
 			description: "hash failure",
 			hasher:      failingHasher{errors.New("BUG")},
-			tag:         "tag",
 			expected:    failed{err: errors.New("getting hash for artifact \"artifact\": BUG")},
 		},
 		{
@@ -161,7 +166,6 @@ func TestLookupRemote(t *testing.T) {
 			cache: map[string]ImageDetails{
 				"hash": {Digest: "digest"},
 			},
-			tag:      "tag",
 			expected: found{hash: "hash"},
 		},
 		{
@@ -170,18 +174,16 @@ func TestLookupRemote(t *testing.T) {
 			cache: map[string]ImageDetails{
 				"hash": {Digest: "otherdigest"},
 			},
-			tag:      "fqn_tag",
-			expected: needsRemoteTagging{hash: "hash", tag: "fqn_tag", digest: "otherdigest"},
+			expected: needsRemoteTagging{hash: "hash", tag: "tag", digest: "otherdigest"},
 		},
 		{
 			description: "found locally",
 			hasher:      mockHasher{"hash"},
 			cache: map[string]ImageDetails{
 				"hash": {ID: "imageID"},
 			},
-			api:      (&testutil.FakeAPIClient{}).Add("no_remote_tag", "imageID"),
-			tag:      "no_remote_tag",
-			expected: needsPushing{hash: "hash", tag: "no_remote_tag", imageID: "imageID"},
+			api:      (&testutil.FakeAPIClient{}).Add("tag", "imageID"),
+			expected: needsPushing{hash: "hash", tag: "tag", imageID: "imageID"},
 		},
 		{
 			description: "not found",
@@ -190,7 +192,6 @@ func TestLookupRemote(t *testing.T) {
 				"hash": {ID: "imageID"},
 			},
 			api:      &testutil.FakeAPIClient{},
-			tag:      "no_remote_tag",
 			expected: needsBuilding{hash: "hash"},
 		},
 	}
@@ -200,7 +201,7 @@ func TestLookupRemote(t *testing.T) {
 				switch {
 				case identifier == "tag":
 					return "digest", nil
-				case identifier == "fqn_tag@otherdigest":
+				case identifier == "tag@otherdigest":
 					return "otherdigest", nil
 				default:
 					return "", errors.New("unknown remote tag")
@@ -215,13 +216,13 @@ func TestLookupRemote(t *testing.T) {
 				cfg:                &mockConfig{mode: config.RunModes.Build},
 			}
 			t.Override(&newArtifactHasherFunc, func(_ graph.ArtifactGraph, _ DependencyLister, _ config.RunMode) artifactHasher { return test.hasher })
-			details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": test.tag}, platform.Resolver{}, []*latest.Artifact{{
+			details := cache.lookupArtifacts(context.Background(), map[string]string{"artifact": "tag"}, platform.Resolver{}, []*latest.Artifact{{
 				ImageName: "artifact",
 			}})
 
 			// cmp.Diff cannot access unexported fields in *exec.Cmd, so use reflect.DeepEqual here directly
 			if !reflect.DeepEqual(test.expected, details[0]) {
-				t.Errorf("Expected result different from actual result. Expected: \n\"%v\", \nActual: \n\"%v\"", test.expected, details[0])
+				t.Errorf("Expected result different from actual result. Expected: \n%v, \nActual: \n%v", test.expected, details)
 			}
 		})
 	}

pkg/skaffold/build/cache/retrieve_test.go

@@ -220,27 +220,22 @@ func TestCacheBuildRemote(t *testing.T) {
 			"artifact1": {"dep1", "dep2"},
 			"artifact2": {"dep3"},
 		})
-		tagToDigest := map[string]string{
-			"artifact1:tag1": "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165",
-			"artifact2:tag2": "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539",
-		}
 
 		// Mock Docker
-		api := &testutil.FakeAPIClient{}
-		for tag, digest := range tagToDigest {
-			api = api.Add(tag, digest)
-		}
-
-		dockerDaemon := fakeLocalDaemon(api)
+		dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{})
 		t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) {
 			return dockerDaemon, nil
 		})
 		t.Override(&docker.DefaultAuthHelper, stubAuth{})
 		t.Override(&docker.RemoteDigest, func(ref string, _ docker.Config, _ []specs.Platform) (string, error) {
-			if digest, ok := tagToDigest[ref]; ok {
-				return digest, nil
+			switch ref {
+			case "artifact1:tag1":
+				return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil
+			case "artifact2:tag2":
+				return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil
+			default:
+				return "", errors.New("unknown remote tag")
 			}
-			return "", errors.New("unknown remote tag")
 		})
 
 		// Mock args builder
@@ -315,11 +310,7 @@ func TestCacheFindMissing(t *testing.T) {
 		})
 
 		// Mock Docker
-		api := &testutil.FakeAPIClient{}
-		api = api.Add("artifact1:tag1", "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165")
-		api = api.Add("artifact2:tag2", "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539")
-
-		dockerDaemon := fakeLocalDaemon(api)
+		dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{})
 		t.Override(&docker.NewAPIClient, func(context.Context, docker.Config) (docker.LocalDaemon, error) {
 			return dockerDaemon, nil
 		})
@@ -348,7 +339,7 @@ func TestCacheFindMissing(t *testing.T) {
 			pipeline:  latest.Pipeline{Build: latest.BuildConfig{BuildType: latest.BuildType{LocalBuild: &latest.LocalBuild{TryImportMissing: true}}}},
 			cacheFile: tmpDir.Path("cache"),
 		}
-		artifactCache, err := NewCache(context.Background(), cfg, func(imageName string) (bool, error) { return true, nil }, deps, graph.ToArtifactGraph(artifacts), make(mockArtifactStore))
+		artifactCache, err := NewCache(context.Background(), cfg, func(imageName string) (bool, error) { return false, nil }, deps, graph.ToArtifactGraph(artifacts), make(mockArtifactStore))
 		t.CheckNoError(err)
 
 		// Because the artifacts are in the docker registry, we expect them to be imported correctly.

pkg/skaffold/runner/new.go

@@ -229,21 +229,14 @@ func setupTrigger(triggerName string, setIntent func(bool), setAutoTrigger func(
 func isImageLocal(runCtx *runcontext.RunContext, imageName string) (bool, error) {
 	pipeline, found := runCtx.PipelineForImage(imageName)
 	if !found {
-		log.Entry(context.TODO()).Debugf("Didn't find pipeline for image %s. Using default pipeline!", imageName)
 		pipeline = runCtx.DefaultPipeline()
 	}
-	if pipeline.Build.GoogleCloudBuild != nil {
-		log.Entry(context.TODO()).Debugf("Image %s is remote because it has pipeline.Build.GoogleCloudBuild", imageName)
-		return false, nil
-	}
-	if pipeline.Build.Cluster != nil {
-		log.Entry(context.TODO()).Debugf("Image %s is remote because it has pipeline.Build.Cluster", imageName)
+	if pipeline.Build.GoogleCloudBuild != nil || pipeline.Build.Cluster != nil {
 		return false, nil
 	}
 
 	// if we're deploying to local Docker, all images must be local
 	if pipeline.Deploy.DockerDeploy != nil {
-		log.Entry(context.TODO()).Debugf("Image %s is local because it has docker deploy", imageName)
 		return true, nil
 	}