fix(lookupRemote): fixed lookup.go lookupRemote caching mechanism

pkg/skaffold/build/cache/lookup.go

@@ -128,31 +128,25 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, platforms []
 			log.Entry(ctx).Debugf("Found %s remote with the same digest", tag)
 			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}
+	if cacheHit {
+		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 locally
-	if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
-		return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
+		// Image exists locally
+		if cachedEntry.HasID() && c.client != nil && c.client.ImageExists(ctx, cachedEntry.ID) {
+			return needsPushing{hash: hash, tag: tag, imageID: cachedEntry.ID}
+		}
 	}
 
 	return needsBuilding{hash: hash}

pkg/skaffold/build/cache/lookup_test.go

@@ -141,70 +141,99 @@ func TestLookupLocal(t *testing.T) {
 }
 
 func TestLookupRemote(t *testing.T) {
+	commonRemoteDigestMap := map[string]string{
+		"tag":                 "digest",
+		"fqn_tag@otherdigest": "otherdigest",
+	}
+
 	tests := []struct {
-		description string
-		hasher      artifactHasher
-		cache       map[string]ImageDetails
-		api         *testutil.FakeAPIClient
-		tag         string
-		expected    cacheDetails
+		description     string
+		hasher          artifactHasher
+		cache           map[string]ImageDetails
+		remoteDigestMap map[string]string
+		api             *testutil.FakeAPIClient
+		tag             string
+		expected        cacheDetails
 	}{
 		{
-			description: "hash failure",
-			hasher:      failingHasher{errors.New("BUG")},
-			tag:         "tag",
-			expected:    failed{err: errors.New("getting hash for artifact \"artifact\": BUG")},
+			description:     "hash failure",
+			hasher:          failingHasher{errors.New("BUG")},
+			tag:             "tag",
+			remoteDigestMap: commonRemoteDigestMap,
+			expected:        failed{err: errors.New("getting hash for artifact \"artifact\": BUG")},
 		},
 		{
-			description: "hit",
+			description: "cache miss but remote found",
+			hasher:      mockHasher{"hash"},
+			cache:       map[string]ImageDetails{},
+			remoteDigestMap: map[string]string{
+				"tag":        "digest",
+				"tag@digest": "digest",
+			},
+			tag:      "tag",
+			expected: needsBuilding{hash: "hash"},
+		},
+		{
+			description: "cache hit and digests are the same",
 			hasher:      mockHasher{"hash"},
 			cache: map[string]ImageDetails{
 				"hash": {Digest: "digest"},
 			},
-			tag:      "tag",
-			expected: found{hash: "hash"},
+			remoteDigestMap: commonRemoteDigestMap,
+			tag:             "tag",
+			expected:        found{hash: "hash"},
+		},
+		{
+			description: "cache hit but digests are not the same, no remote or locally",
+			hasher:      mockHasher{"hash"},
+			cache: map[string]ImageDetails{
+				"hash": {Digest: "otherdigest"},
+			},
+			remoteDigestMap: commonRemoteDigestMap,
+			tag:             "tag",
+			expected:        needsBuilding{hash: "hash"},
 		},
 		{
-			description: "hit with different tag",
+			description: "cache hit with different tag",
 			hasher:      mockHasher{"hash"},
 			cache: map[string]ImageDetails{
 				"hash": {Digest: "otherdigest"},
 			},
-			tag:      "fqn_tag",
-			expected: needsRemoteTagging{hash: "hash", tag: "fqn_tag", digest: "otherdigest"},
+			remoteDigestMap: commonRemoteDigestMap,
+			tag:             "fqn_tag",
+			expected:        needsRemoteTagging{hash: "hash", tag: "fqn_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"},
+			remoteDigestMap: commonRemoteDigestMap,
+			api:             (&testutil.FakeAPIClient{}).Add("no_remote_tag", "imageID"),
+			tag:             "no_remote_tag",
+			expected:        needsPushing{hash: "hash", tag: "no_remote_tag", imageID: "imageID"},
 		},
 		{
 			description: "not found",
 			hasher:      mockHasher{"hash"},
 			cache: map[string]ImageDetails{
 				"hash": {ID: "imageID"},
 			},
-			api:      &testutil.FakeAPIClient{},
-			tag:      "no_remote_tag",
-			expected: needsBuilding{hash: "hash"},
+			remoteDigestMap: commonRemoteDigestMap,
+			api:             &testutil.FakeAPIClient{},
+			tag:             "no_remote_tag",
+			expected:        needsBuilding{hash: "hash"},
 		},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			t.Override(&docker.RemoteDigest, func(identifier string, _ docker.Config, _ []specs.Platform) (string, error) {
-				switch {
-				case identifier == "tag":
-					return "digest", nil
-				case identifier == "fqn_tag@otherdigest":
-					return "otherdigest", nil
-				default:
-					return "", errors.New("unknown remote tag")
+				if digest, ok := test.remoteDigestMap[identifier]; ok {
+					return digest, nil
 				}
+
+				return "", errors.New("unknown remote tag")
 			})
 
 			cache := &cache{