Clear cache on startup, use tempDir for unpacking

oceanc80 · Per Goncalves da Silva · commit 4f95510de825 · 2025-01-30T14:09:12.000+01:00
Signed-off-by: Per Goncalves da Silva &lt;pegoncal@redhat.com&gt;
diff --git a/catalogd/cmd/catalogd/main.go b/catalogd/cmd/catalogd/main.go
@@ -263,6 +263,10 @@ func main() {
 	}
 
 	unpackCacheBasePath := filepath.Join(cacheDir, source.UnpackCacheDir)
+	if err := os.RemoveAll(unpackCacheBasePath); err != nil {
+		setupLog.Error(err, "unable to clear cache directory for unpacking on startup")
+		os.Exit(1)
+	}
 	if err := os.MkdirAll(unpackCacheBasePath, 0770); err != nil {
 		setupLog.Error(err, "unable to create cache directory for unpacking")
 		os.Exit(1)
@@ -290,6 +294,10 @@ func main() {
 	metrics.Registry.MustRegister(catalogdmetrics.RequestDurationMetric)
 
 	storeDir := filepath.Join(cacheDir, storageDir)
+	if err := os.RemoveAll(storeDir); err != nil {
+		setupLog.Error(err, "unable to clear storage directory for unpacking on startup")
+		os.Exit(1)
+	}
 	if err := os.MkdirAll(storeDir, 0700); err != nil {
 		setupLog.Error(err, "unable to create storage directory for catalogs")
 		os.Exit(1)
diff --git a/catalogd/internal/source/containers_image.go b/catalogd/internal/source/containers_image.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
@@ -80,8 +81,14 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
 		if !unpackStat.IsDir() {
 			panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath))
 		}
-		l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
-		return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil
+
+		// check unpack directory is read-only
+		// this should only happen once all the contents have been unpacked and is a good
+		// indication that unpack completed successfully
+		if unpackStat.Mode().Perm()&0200 == 0 {
+			l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
+			return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil
+		}
 	}
 
 	//////////////////////////////////////////////////////
@@ -296,11 +303,17 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 		return wrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
 	}
 
-	if err := os.MkdirAll(unpackPath, 0700); err != nil {
-		return fmt.Errorf("error creating unpack directory: %w", err)
-	}
 	l := log.FromContext(ctx)
-	l.Info("unpacking image", "path", unpackPath)
+	tempUnpackPath, err := os.MkdirTemp("", "unpack-*")
+	if err != nil {
+		return fmt.Errorf("error creating temporary unpack directory: %w", err)
+	}
+	defer func() {
+		if err := os.RemoveAll(tempUnpackPath); err != nil {
+			l.Error(err, "error removing temporary unpack directory")
+		}
+	}()
+	l.Info("unpacking image", "path", unpackPath, "temp path", tempUnpackPath)
 	for i, layerInfo := range img.LayerInfos() {
 		if err := func() error {
 			layerReader, _, err := layoutSrc.GetBlob(ctx, layerInfo, none.NoCache)
@@ -309,21 +322,76 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 			}
 			defer layerReader.Close()
 
-			if err := applyLayer(ctx, unpackPath, dirToUnpack, layerReader); err != nil {
+			if err := applyLayer(ctx, tempUnpackPath, dirToUnpack, layerReader); err != nil {
 				return fmt.Errorf("error applying layer[%d]: %w", i, err)
 			}
 			l.Info("applied layer", "layer", i)
 			return nil
 		}(); err != nil {
-			return errors.Join(err, deleteRecursive(unpackPath))
+			return errors.Join(err, deleteRecursive(tempUnpackPath))
 		}
 	}
+
+	// ensure unpack path is empty
+	if err := os.RemoveAll(unpackPath); err != nil {
+		return fmt.Errorf("error removing unpack path: %w", err)
+	}
+	if err := copyRecursively(tempUnpackPath, unpackPath, 0700); err != nil {
+		return fmt.Errorf("error moving temporary unpack directory to final unpack directory: %w", err)
+	}
 	if err := setReadOnlyRecursive(unpackPath); err != nil {
 		return fmt.Errorf("error making unpack directory read-only: %w", err)
 	}
 	return nil
 }
 
+func copyRecursively(srcPath string, destPath string, perm os.FileMode) error {
+	// ensure destination path not exist
+	if _, err := os.Stat(destPath); err == nil {
+		return fmt.Errorf("destination path %q already exists", destPath)
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("error checking destination path: %w", err)
+	}
+	return filepath.WalkDir(srcPath, func(path string, d fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		relPath, err := filepath.Rel(srcPath, path)
+		if err != nil {
+			return err
+		}
+		destFullPath := filepath.Join(destPath, relPath)
+		if d.IsDir() {
+			return os.MkdirAll(destFullPath, perm)
+		}
+		if d.Type()&os.ModeSymlink != 0 {
+			linkDest, err := os.Readlink(path)
+			if err != nil {
+				return err
+			}
+			return os.Symlink(linkDest, destFullPath)
+		}
+		return copyFile(path, destFullPath)
+	})
+}
+
+func copyFile(src, dest string) error {
+	in, err := os.Open(src)
+	if err != nil {
+		return err
+	}
+	defer in.Close()
+
+	out, err := os.Create(dest)
+	if err != nil {
+		return err
+	}
+	defer out.Close()
+
+	_, err = io.Copy(out, in)
+	return err
+}
+
 func applyLayer(ctx context.Context, destPath string, srcPath string, layer io.ReadCloser) error {
 	decompressed, _, err := compression.AutoDecompress(layer)
 	if err != nil {
diff --git a/catalogd/internal/source/containers_image_test.go b/catalogd/internal/source/containers_image_test.go
@@ -38,12 +38,13 @@ func TestImageRegistry(t *testing.T) {
 		// if the Catalog.Spec.Source.Image.Ref field is empty,
 		// one is injected during test runtime to ensure it
 		// points to the registry created for the test
-		catalog             *catalogdv1.ClusterCatalog
-		wantErr             bool
-		terminal            bool
-		image               v1.Image
-		digestAlreadyExists bool
-		oldDigestExists     bool
+		catalog          *catalogdv1.ClusterCatalog
+		wantErr          bool
+		terminal         bool
+		image            v1.Image
+		unpackPathExists bool
+		unfinishedUnpack bool
+		oldDigestExists  bool
 		// refType is the type of image ref this test
 		// is using. Should be one of "tag","digest"
 		refType string
@@ -176,8 +177,8 @@ func TestImageRegistry(t *testing.T) {
 				}
 				return img
 			}(),
-			digestAlreadyExists: true,
-			refType:             "tag",
+			unpackPathExists: true,
+			refType:          "tag",
 		},
 		{
 			name: "digest based image, digest already exists in cache",
@@ -194,9 +195,9 @@ func TestImageRegistry(t *testing.T) {
 					},
 				},
 			},
-			wantErr:             false,
-			digestAlreadyExists: true,
-			refType:             "digest",
+			wantErr:          false,
+			unpackPathExists: true,
+			refType:          "digest",
 			image: func() v1.Image {
 				img, err := random.Image(20, 3)
 				if err != nil {
@@ -239,6 +240,41 @@ func TestImageRegistry(t *testing.T) {
 				return img
 			}(),
 		},
+		{
+			name: "tag ref, unpack path exists but is not read-only (possibly uncaught unpack issue)",
+			catalog: &catalogdv1.ClusterCatalog{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+				Spec: catalogdv1.ClusterCatalogSpec{
+					Source: catalogdv1.CatalogSource{
+						Type: catalogdv1.SourceTypeImage,
+						Image: &catalogdv1.ImageSource{
+							Ref: "",
+						},
+					},
+				},
+			},
+			wantErr:          false,
+			refType:          "tag",
+			unpackPathExists: true,
+			unfinishedUnpack: true,
+			image: func() v1.Image {
+				img, err := random.Image(20, 3)
+				if err != nil {
+					panic(err)
+				}
+				img, err = mutate.Config(img, v1.Config{
+					Labels: map[string]string{
+						source.ConfigDirLabel: "/configs",
+					},
+				})
+				if err != nil {
+					panic(err)
+				}
+				return img
+			}(),
+		},
 		{
 			name: "tag ref, happy path",
 			catalog: &catalogdv1.ClusterCatalog{
@@ -272,6 +308,41 @@ func TestImageRegistry(t *testing.T) {
 				return img
 			}(),
 		},
+		{
+			name: "digest ref, unpack path exists but is not read-only (possibly uncaught unpack issue)",
+			catalog: &catalogdv1.ClusterCatalog{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+				Spec: catalogdv1.ClusterCatalogSpec{
+					Source: catalogdv1.CatalogSource{
+						Type: catalogdv1.SourceTypeImage,
+						Image: &catalogdv1.ImageSource{
+							Ref: "",
+						},
+					},
+				},
+			},
+			wantErr:          false,
+			refType:          "digest",
+			unpackPathExists: true,
+			unfinishedUnpack: true,
+			image: func() v1.Image {
+				img, err := random.Image(20, 3)
+				if err != nil {
+					panic(err)
+				}
+				img, err = mutate.Config(img, v1.Config{
+					Labels: map[string]string{
+						source.ConfigDirLabel: "/configs",
+					},
+				})
+				if err != nil {
+					panic(err)
+				}
+				return img
+			}(),
+		},
 		{
 			name: "digest ref, happy path",
 			catalog: &catalogdv1.ClusterCatalog{
@@ -362,9 +433,12 @@ func TestImageRegistry(t *testing.T) {
 				require.NoError(t, err)
 
 				// if the digest should already exist in the cache, create it
-				if tt.digestAlreadyExists {
-					err = os.MkdirAll(filepath.Join(testCache, tt.catalog.Name, digest.String()), os.ModePerm)
-					require.NoError(t, err)
+				if tt.unpackPathExists {
+					unpackPath := filepath.Join(testCache, tt.catalog.Name, digest.String())
+					require.NoError(t, os.MkdirAll(unpackPath, os.ModePerm))
+					if !tt.unfinishedUnpack {
+						require.NoError(t, os.Chmod(unpackPath, 0500))
+					}
 				}
 
 				err = remote.Write(imgName, tt.image)
@@ -397,7 +471,7 @@ func TestImageRegistry(t *testing.T) {
 				require.NoError(t, err)
 				assert.Len(t, entries, 1)
 				// If the digest should already exist check that we actually hit it
-				if tt.digestAlreadyExists {
+				if tt.unpackPathExists && !tt.unfinishedUnpack {
 					assert.Contains(t, buf.String(), "image already unpacked")
 					assert.Equal(t, rs.UnpackTime, unpackDirStat.ModTime().Truncate(time.Second))
 				} else if tt.oldDigestExists {
