🐛 attempt to remove cache entry if unpack fails (#207)

(bugfix): attempt to remove cache entry if unpack fails

and add a new test case that ensures if the required label
is missing from the image, thus failing image unpacking,
an error is consistently returned

fixes #206

Signed-off-by: Bryce Palmer <everettraven@gmail.com>

internal/source/image_registry_client.go

@@ -19,6 +19,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	apimacherrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	catalogdv1alpha1 "github.com/operator-framework/catalogd/api/core/v1alpha1"
@@ -94,7 +95,7 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Ca
 	l.V(1).Info("resolved image descriptor", "digest", imgDesc.Digest.String())
 
 	unpackPath := filepath.Join(i.BaseCachePath, catalog.Name, imgDesc.Digest.Hex)
-	if _, err = os.Stat(unpackPath); errors.Is(err, os.ErrNotExist) {
+	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 {
 			return nil, fmt.Errorf("error cleaning up catalog cache: %w", err)
@@ -105,6 +106,15 @@ func (i *ImageRegistry) Unpack(ctx context.Context, catalog *catalogdv1alpha1.Ca
 		}
 
 		if err = unpackImage(ctx, imgRef, unpackPath, remoteOpts...); err != nil {
+			cleanupErr := os.RemoveAll(unpackPath)
+			if cleanupErr != nil {
+				err = apimacherrors.NewAggregate(
+					[]error{
+						err,
+						fmt.Errorf("error cleaning up unpack path after unpack failed: %w", cleanupErr),
+					},
+				)
+			}
 			return nil, wrapUnrecoverable(fmt.Errorf("error unpacking image: %w", err), isDigest)
 		}
 	} else if err != nil {

internal/source/image_registry_client_test.go

@@ -337,7 +337,8 @@ func TestImageRegistry(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			// Create context, temporary cache directory,
 			// and image registry source
-			ctx := context.Background()
+			ctx, cancel := context.WithCancel(context.Background())
+			t.Cleanup(cancel)
 			testCache := t.TempDir()
 			imgReg := &source.ImageRegistry{
 				BaseCachePath: testCache,
@@ -407,3 +408,55 @@ func TestImageRegistry(t *testing.T) {
 		})
 	}
 }
+
+// TestImageRegistryMissingLabelConsistentFailure is a test
+// case that specifically tests that multiple calls to the
+// ImageRegistry.Unpack() method return an error and is meant
+// to ensure coverage of the bug reported in
+// https://github.com/operator-framework/catalogd/issues/206
+func TestImageRegistryMissingLabelConsistentFailure(t *testing.T) {
+	// Create context, temporary cache directory,
+	// and image registry source
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	testCache := t.TempDir()
+	imgReg := &source.ImageRegistry{
+		BaseCachePath: testCache,
+	}
+
+	// Start a new server running an image registry
+	srv := httptest.NewServer(registry.New())
+	defer srv.Close()
+
+	// parse the server url so we can grab just the host
+	url, err := url.Parse(srv.URL)
+	require.NoError(t, err)
+
+	imgName, err := name.ParseReference(fmt.Sprintf("%s/%s", url.Host, "test-image:test"))
+	require.NoError(t, err)
+
+	image, err := random.Image(20, 20)
+	require.NoError(t, err)
+
+	err = remote.Write(imgName, image)
+	require.NoError(t, err)
+
+	catalog := &v1alpha1.Catalog{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test",
+		},
+		Spec: v1alpha1.CatalogSpec{
+			Source: v1alpha1.CatalogSource{
+				Type: v1alpha1.SourceTypeImage,
+				Image: &v1alpha1.ImageSource{
+					Ref: imgName.Name(),
+				},
+			},
+		},
+	}
+
+	for i := 0; i < 3; i++ {
+		_, err = imgReg.Unpack(ctx, catalog)
+		require.Error(t, err, "unpack run ", i)
+	}
+}