diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go index 926ce55f..e1f6ee4a 100644 --- a/internal/controllers/core/clustercatalog_controller.go +++ b/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 } diff --git a/internal/source/image_registry_client.go b/internal/source/image_registry_client.go index 60fd3f77..e9152fe8 100644 --- a/internal/source/image_registry_client.go +++ b/internal/source/image_registry_client.go @@ -77,16 +77,6 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Cl remoteOpts = append(remoteOpts, remote.WithTransport(insecureTransport)) } - 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 { @@ -95,6 +85,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 { @@ -115,13 +112,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 } diff --git a/internal/source/image_registry_client_test.go b/internal/source/image_registry_client_test.go index c2d20042..1d75feea 100644 --- a/internal/source/image_registry_client_test.go +++ b/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