diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index 32d4df2fb20..63776515eb7 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/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} diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 1358a3bdb6b..ebdf46b7ced 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -141,37 +141,67 @@ 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", @@ -179,9 +209,10 @@ func TestLookupRemote(t *testing.T) { 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", @@ -189,22 +220,20 @@ func TestLookupRemote(t *testing.T) { 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{