Fix FS cache for image refs with tags (#394)

Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>

internal/controllers/core/clustercatalog_controller.go

@@ -263,6 +263,8 @@ func (r *ClusterCatalogReconciler) needsUnpacking(catalog *v1alpha1.ClusterCatal
 		return false
 	}
 	// if the spec.Source.Image.Ref was changed, unpack the new ref
+	// NOTE: we must compare image reference WITHOUT sha hash here
+	// otherwise we will always be unpacking image even when poll interval not lapsed
 	if catalog.Spec.Source.Image.Ref != catalog.Status.ResolvedSource.Image.Ref {
 		return true
 	}

internal/source/image_registry_client.go

@@ -92,16 +92,6 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
 		remoteOpts = append(remoteOpts, remote.WithTransport(tlsTransport))
 	}
 
-	digest, isDigest := imgRef.(name.Digest)
-	if isDigest {
-		hexVal := strings.TrimPrefix(digest.DigestStr(), "sha256:")
-		unpackPath := filepath.Join(i.BaseCachePath, catalog.Name, hexVal)
-		if stat, err := os.Stat(unpackPath); err == nil && stat.IsDir() {
-			l.V(1).Info("found image in filesystem cache", "digest", hexVal)
-			return unpackedResult(os.DirFS(unpackPath), catalog, digest.String(), metav1.Time{Time: time.Now()}), nil
-		}
-	}
-
 	// always fetch the hash
 	imgDesc, err := remote.Head(imgRef, remoteOpts...)
 	if err != nil {
@@ -110,6 +100,13 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
 	l.V(1).Info("resolved image descriptor", "digest", imgDesc.Digest.String())
 
 	unpackPath := filepath.Join(i.BaseCachePath, catalog.Name, imgDesc.Digest.Hex)
+	resolvedRef := fmt.Sprintf("%s@sha256:%s", imgRef.Context().Name(), imgDesc.Digest.Hex)
+	if stat, err := os.Stat(unpackPath); err == nil && stat.IsDir() {
+		l.V(1).Info("found image in filesystem cache", "digest", imgDesc.Digest.Hex)
+		// TODO: https://github.com/operator-framework/catalogd/issues/389
+		return unpackedResult(os.DirFS(unpackPath), catalog, resolvedRef, metav1.Time{Time: time.Now()}), nil
+	}
+
 	if _, err = os.Stat(unpackPath); errors.Is(err, os.ErrNotExist) { //nolint: nestif
 		// Ensure any previous unpacked catalog is cleaned up before unpacking the new catalog.
 		if err := i.Cleanup(ctx, catalog); err != nil {
@@ -130,13 +127,13 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl
 					},
 				)
 			}
+			_, isDigest := imgRef.(name.Digest)
 			return nil, wrapUnrecoverable(fmt.Errorf("error unpacking image: %w", err), isDigest)
 		}
 	} else if err != nil {
 		return nil, fmt.Errorf("error checking if image is in filesystem cache: %w", err)
 	}
 
-	resolvedRef := fmt.Sprintf("%s@sha256:%s", imgRef.Context().Name(), imgDesc.Digest.Hex)
 	return unpackedResult(os.DirFS(unpackPath), catalog, resolvedRef, metav1.Time{Time: time.Now()}), nil
 }
 

internal/source/image_registry_client_test.go

@@ -1,6 +1,7 @@
 package source_test
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -10,6 +11,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/go-logr/logr/funcr"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/registry"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -19,6 +21,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/operator-framework/catalogd/api/core/v1alpha1"
 	catalogderrors "github.com/operator-framework/catalogd/internal/errors"
@@ -344,6 +347,16 @@ func TestImageRegistry(t *testing.T) {
 				BaseCachePath: testCache,
 			}
 
+			// Create a logger with a simple function-based LogSink that writes to the buffer
+			var buf bytes.Buffer
+			logger := funcr.New(func(prefix, args string) {
+				buf.WriteString(fmt.Sprintf("%s %s\n", prefix, args))
+			}, funcr.Options{Verbosity: 1})
+
+			// Add the logger into the context which will later be used
+			// in the Unpack function to get the logger
+			ctx = log.IntoContext(ctx, logger)
+
 			// Start a new server running an image registry
 			srv := httptest.NewServer(registry.New())
 			defer srv.Close()
@@ -399,6 +412,10 @@ func TestImageRegistry(t *testing.T) {
 				entries, err := os.ReadDir(filepath.Join(testCache, tt.catalog.Name))
 				require.NoError(t, err)
 				assert.Len(t, entries, 1)
+				// If the digest should already exist check that we actually hit it
+				if tt.digestAlreadyExists {
+					require.Contains(t, buf.String(), "found image in filesystem cache")
+				}
 			} else {
 				assert.Error(t, err)
 				var unrecov *catalogderrors.Unrecoverable