fix(spx): extract image imports into a disk-backed scratch dir (#1171)

2026-10-15 09:01:31 +00:00 · 2026-10-06 12:01:13 +11:00
parent bcbe415288
commit a69ed27dc4
3 changed files with 168 additions and 27 deletions
@@ -425,7 +425,7 @@ func init() {
 	adminJoinCmd.MarkFlagRequired("host")
 	adminJoinCmd.MarkFlagRequired("token")

-	imagesImportCmd.Flags().String("tmp-dir", os.TempDir(), "Temporary directory for image import processing")
+	imagesImportCmd.Flags().String("tmp-dir", "", "Scratch directory for extracting the image (default: <spinifex-dir>/images); must be disk-backed, not tmpfs")

 	imagesImportCmd.Flags().String("name", "", "Import specified image by name")
 	imagesImportCmd.Flags().String("ami-name", "", "Override the registered AMI name (DescribeImages name). Defaults to ami-{distro}-{version}-{arch}. Use for locally-built appliances (e.g. --ami-name spinifex-eks-node).")
@@ -585,7 +585,6 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 	var image utils.Images

 	var imageFile string
-	var imageStat os.FileInfo
 	var err error

 	forceCmd, _ := cmd.Flags().GetBool("force")
@@ -753,25 +752,59 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 		os.Exit(1)
 	}

-	// Next, validate if the image is raw, tar, gz, xv, etc. We need to upload the raw image
-	tmpDir, err := os.MkdirTemp(ostmpDir, "spinifex-image-tmp-*")
+	// An empty --tmp-dir scratches beside the catalog: disk-backed, unlike a
+	// tmpfs /tmp, and already root:spinifex 0770.
+	scratchRoot := ostmpDir
+	if scratchRoot == "" {
+		scratchRoot = imageDir
+	}

+	extractedImagePath, cleanupScratch, err := extractToScratch(imageFile, scratchRoot)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not create temp dir: %v\n", err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
 		os.Exit(1)
 	}

-	extractedImagePath, err := utils.ExtractDiskImageFromFile(imageFile, imagePath)
+	// os.Exit skips deferred calls, so scratch is removed before any exit.
+	volumeId, err := registerExtractedImage(image, amiNameOverride, imagePath, extractedImagePath, sourceDigest)
+	cleanupScratch()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not extract image: %v\n", err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
 		os.Exit(1)
 	}

-	imageStat, err = os.Stat(extractedImagePath)
+	fmt.Printf("✅ Image import complete. Image-ID (AMI): %s\n", volumeId)
+	fmt.Printf("✅ Source digest: %s:%s (%s)\n", sourceDigest.Algorithm, sourceDigest.Value, sourceDigest.Verification)
+}

+// extractToScratch extracts imageFile into a fresh hidden directory under
+// scratchRoot and returns the extracted path with a cleanup that removes that
+// directory. On error the directory is already removed.
+func extractToScratch(imageFile, scratchRoot string) (string, func(), error) {
+	scratchDir, err := os.MkdirTemp(scratchRoot, ".import-*")
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not stat image: %v\n", err)
-		os.Exit(1)
+		return "", nil, fmt.Errorf("create scratch dir under %s: %w", scratchRoot, err)
+	}
+	cleanup := func() {
+		if err := os.RemoveAll(scratchDir); err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: could not remove scratch dir %s: %v\n", scratchDir, err)
+		}
+	}
+
+	extracted, err := utils.ExtractDiskImageFromFile(imageFile, scratchDir)
+	if err != nil {
+		cleanup()
+		return "", nil, fmt.Errorf("extract image: %w", err)
+	}
+	return extracted, cleanup, nil
+}
+
+// registerExtractedImage writes the AMI manifest into imagePath, imports
+// extractedImagePath into storage and registers the AMI, returning its ID.
+func registerExtractedImage(image utils.Images, amiNameOverride, imagePath, extractedImagePath string, sourceDigest ebsmetadata.ImageDigest) (string, error) {
+	imageStat, err := os.Stat(extractedImagePath)
+	if err != nil {
+		return "", fmt.Errorf("stat image: %w", err)
 	}

 	// Describe the image/AMI as the control-plane document it will become.
@@ -810,24 +843,19 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 	// Save as JSON
 	jsonData, err := json.Marshal(ami)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not marshal manifest: %v\n", err)
-		os.Exit(1)
+		return "", fmt.Errorf("marshal manifest: %w", err)
 	}

 	manifestFilename := fmt.Sprintf("%s/%s.json", imagePath, ami.Name)
 	// Write to file
 	err = os.WriteFile(manifestFilename, jsonData, 0600)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not write manifest: %v\n", err)
-		os.Exit(1)
+		return "", fmt.Errorf("write manifest: %w", err)
 	}

-	defer os.RemoveAll(tmpDir)
-
 	appConfig, nc, err := loadConfigAndConnect()
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not connect to the cluster: %v\n", err)
-		os.Exit(1)
+		return "", fmt.Errorf("connect to the cluster: %w", err)
 	}
 	defer nc.Close()
 	node := appConfig.Nodes[appConfig.Node]
@@ -844,8 +872,7 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 		Progress:         os.Stdout,
 	})
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Could not import image: %v\n", err)
-		os.Exit(1)
+		return "", fmt.Errorf("import image: %w", err)
 	}

 	// admin.ImportImage only wrote the provider's half of the snapshot (its
@@ -853,8 +880,7 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 	// DescribeSnapshots and GetAMISourceVolumeID cannot resolve the import.
 	metaStore := objectstore.NewS3ObjectStoreFromConfig(node.Predastore.Host, node.Predastore.Region, node.Predastore.AccessKey, node.Predastore.SecretKey)
 	if err := registerImportedAMISnapshot(metaStore, node.Predastore.Bucket, ami, node.AZ, node.Viperblock.EncryptionKeyFile != ""); err != nil {
-		fmt.Fprintf(os.Stderr, "Imported %s but could not register its snapshot: %v\n", volumeId, err)
-		os.Exit(1)
+		return "", fmt.Errorf("imported %s but could not register its snapshot: %w", volumeId, err)
 	}

 	// The document is what DescribeImages enumerates, so it is written last:
@@ -862,12 +888,10 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 	// partway leaves no half-registered image behind.
 	ami.State = "available"
 	if err := ebsmetadata.NewStore(metaStore, node.Predastore.Bucket).PutAMI(context.Background(), ami); err != nil {
-		fmt.Fprintf(os.Stderr, "Imported %s but could not register it: %v\n", volumeId, err)
-		os.Exit(1)
+		return "", fmt.Errorf("imported %s but could not register it: %w", volumeId, err)
 	}

-	fmt.Printf("✅ Image import complete. Image-ID (AMI): %s\n", volumeId)
-	fmt.Printf("✅ Source digest: %s:%s (%s)\n", sourceDigest.Algorithm, sourceDigest.Value, sourceDigest.Verification)
+	return volumeId, nil
 }

 // registerImportedAMISnapshot writes the EC2 control plane's snapshot document
@@ -0,0 +1,117 @@
+package cmd
+
+import (
+	"bytes"
+	"compress/gzip"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func readRawDiskFixture(t *testing.T) []byte {
+	t.Helper()
+	data, err := os.ReadFile(filepath.Join("..", "..", "..", "tests", "unit-test-disk-image.raw"))
+	require.NoError(t, err)
+	return data
+}
+
+func gzipData(t *testing.T, data []byte) []byte {
+	t.Helper()
+	var buf bytes.Buffer
+	gw := gzip.NewWriter(&buf)
+	_, err := gw.Write(data)
+	require.NoError(t, err)
+	require.NoError(t, gw.Close())
+	return buf.Bytes()
+}
+
+func qcow2Data(t *testing.T, raw []byte) []byte {
+	t.Helper()
+	if _, err := exec.LookPath("qemu-img"); err != nil {
+		t.Skip("qemu-img not found")
+	}
+	dir := t.TempDir()
+	rawPath := filepath.Join(dir, "src.raw")
+	qcowPath := filepath.Join(dir, "src.qcow2")
+	require.NoError(t, os.WriteFile(rawPath, raw, 0o600))
+	out, err := exec.Command("qemu-img", "convert", "-f", "raw", "-O", "qcow2", rawPath, qcowPath).CombinedOutput()
+	require.NoError(t, err, string(out))
+	data, err := os.ReadFile(qcowPath)
+	require.NoError(t, err)
+	return data
+}
+
+func dirNames(t *testing.T, dir string) []string {
+	t.Helper()
+	entries, err := os.ReadDir(dir)
+	require.NoError(t, err)
+	names := make([]string, 0, len(entries))
+	for _, e := range entries {
+		names = append(names, e.Name())
+	}
+	sort.Strings(names)
+	return names
+}
+
+// The catalog dir doubles as the download cache, so the source lives there
+// as it does for a --name import, with the images dir as the scratch root.
+func TestExtractToScratch_LeavesNoIntermediateInCatalog(t *testing.T) {
+	raw := readRawDiskFixture(t)
+
+	cases := []struct {
+		name    string
+		source  string
+		payload func(t *testing.T) []byte
+		wantErr string
+	}{
+		{
+			name:    "gzip raw is decompressed into scratch",
+			source:  "disk.raw.gz",
+			payload: func(t *testing.T) []byte { return gzipData(t, raw) },
+		},
+		{
+			name:    "qcow2 is converted into scratch",
+			source:  "disk.qcow2",
+			payload: func(t *testing.T) []byte { return qcow2Data(t, raw) },
+		},
+		{
+			name:    "intermediate that fails the disk sniff is removed",
+			source:  "disk.raw.gz",
+			payload: func(t *testing.T) []byte { return gzipData(t, []byte("not a disk image")) },
+			wantErr: "extract image",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			imagesDir := t.TempDir()
+			catalogDir := filepath.Join(imagesDir, "debian", "13", "x86_64")
+			require.NoError(t, os.MkdirAll(catalogDir, 0o770))
+			source := filepath.Join(catalogDir, tc.source)
+			require.NoError(t, os.WriteFile(source, tc.payload(t), 0o600))
+
+			extracted, cleanup, err := extractToScratch(source, imagesDir)
+
+			if tc.wantErr != "" {
+				require.ErrorContains(t, err, tc.wantErr)
+				assert.Nil(t, cleanup)
+			} else {
+				require.NoError(t, err)
+				scratchDir := filepath.Dir(extracted)
+				assert.Equal(t, imagesDir, filepath.Dir(scratchDir))
+				assert.True(t, strings.HasPrefix(filepath.Base(scratchDir), ".import-"), "extracted into %s", extracted)
+				assert.FileExists(t, extracted)
+				cleanup()
+			}
+
+			assert.Equal(t, []string{"debian"}, dirNames(t, imagesDir), "scratch dir must be removed")
+			assert.Equal(t, []string{tc.source}, dirNames(t, catalogDir), "catalog must hold only the source")
+		})
+	}
+}
@@ -166,7 +166,7 @@ Operational commands for inspecting cluster state. These fan out NATS requests t

 | Command | Flags | Description |
 |---------|-------|-------------|
-| `spx admin images import` | `--name`, `--file`, `--distro`, `--version`, `--arch`, `--platform`, `--boot-mode` (bios/uefi/uefi-preferred), `--tag`, `--force`, `--skip-verify`, `--checksum`, `--ami-name` (override the registered `DescribeImages` name, which otherwise defaults to `ami-{distro}-{version}-{arch}`; used for locally-built appliances such as `--ami-name spinifex-eks-node`), `--tmp-dir` (default: `/tmp`) | Catalog imports (`--name`) download the image, fetch the catalog `Checksum` URL, verify the SHA-256/SHA-512 digest, and inherit `BootMode` from the catalog entry. `--boot-mode` overrides the catalog value when set. Mismatch fails closed; the cached file is left on disk and `--force` re-downloads. `--file` imports require an explicit `--boot-mode` because there is no catalog metadata to inherit from, and are verified only when `--checksum <sums-file>` is passed. `--checksum` takes the sums file the image was published with (`SHA256SUMS`, `SHA512SUMS`, `<image>.sha256`/`.sha512`, coreutils, BSD or bare-digest format), looks up the entry for the image's basename, and infers sha256 or sha512 from the digest length. A mismatch exits 1 before anything is written to storage and leaves the file in place. `--checksum` proves only that the image matches the sums file supplied with it, not that either is authentic; verify the sums file's vendor signature separately. `--checksum` is refused without `--file` or with `--skip-verify`. A `--file` import without `--checksum` prints `Importing <file> without checksum validation` to stderr and proceeds; `--name --file` does not fall back to the catalog digest. Every import records the source artifact's digest on the AMI with who verified it (`operator`, `catalog` or `unverified`, the unverified cases hashed as sha256), prints it on success, and shows it in `spx admin images describe`. The digest is of the file as supplied, not of the stored volume. `--skip-verify` bypasses verification for catalog imports and prints a warning to stderr; use only for debugging or when upstream mirrors are confirmed-broken. Import never modifies the image: the bytes written to storage are the bytes that were verified. A guest that needs to trust the deployment CA for in-guest SDK-over-TLS calls fetches it at runtime from `http://169.254.169.254/spinifex/ca.pem` (see the IMDS section), so a CA rotation reaches guests without re-importing anything. |
+| `spx admin images import` | `--name`, `--file`, `--distro`, `--version`, `--arch`, `--platform`, `--boot-mode` (bios/uefi/uefi-preferred), `--tag`, `--force`, `--skip-verify`, `--checksum`, `--ami-name` (override the registered `DescribeImages` name, which otherwise defaults to `ami-{distro}-{version}-{arch}`; used for locally-built appliances such as `--ami-name spinifex-eks-node`), `--tmp-dir` (default: `<spinifex-dir>/images`) | Catalog imports (`--name`) download the image, fetch the catalog `Checksum` URL, verify the SHA-256/SHA-512 digest, and inherit `BootMode` from the catalog entry. `--boot-mode` overrides the catalog value when set. Mismatch fails closed; the cached file is left on disk and `--force` re-downloads. `--file` imports require an explicit `--boot-mode` because there is no catalog metadata to inherit from, and are verified only when `--checksum <sums-file>` is passed. `--checksum` takes the sums file the image was published with (`SHA256SUMS`, `SHA512SUMS`, `<image>.sha256`/`.sha512`, coreutils, BSD or bare-digest format), looks up the entry for the image's basename, and infers sha256 or sha512 from the digest length. A mismatch exits 1 before anything is written to storage and leaves the file in place. `--checksum` proves only that the image matches the sums file supplied with it, not that either is authentic; verify the sums file's vendor signature separately. `--checksum` is refused without `--file` or with `--skip-verify`. A `--file` import without `--checksum` prints `Importing <file> without checksum validation` to stderr and proceeds; `--name --file` does not fall back to the catalog digest. Every import records the source artifact's digest on the AMI with who verified it (`operator`, `catalog` or `unverified`, the unverified cases hashed as sha256), prints it on success, and shows it in `spx admin images describe`. The digest is of the file as supplied, not of the stored volume. `--skip-verify` bypasses verification for catalog imports and prints a warning to stderr; use only for debugging or when upstream mirrors are confirmed-broken. Import never modifies the image: the bytes written to storage are the bytes that were verified. Compressed and qcow2 images are extracted into a hidden `.import-*` scratch directory under `--tmp-dir`, which is removed when the import succeeds or fails, so the catalog directory keeps only the downloaded source (for `--name`) and the AMI manifest. Point `--tmp-dir` at a disk-backed filesystem with room for the full raw image, never at a tmpfs such as `/tmp`. A guest that needs to trust the deployment CA for in-guest SDK-over-TLS calls fetches it at runtime from `http://169.254.169.254/spinifex/ca.pem` (see the IMDS section), so a CA rotation reaches guests without re-importing anything. |
 | `spx admin images list` | — | Lists available OS images that can be imported or downloaded |
 | `spx admin images describe` | `--image-id` (required) | Read-only. Prints the AMI's name, owner, creation date, boot mode and architecture, then its source digest: algorithm and value, verification (`operator` with the sums file name, `catalog` with the sums URL, or `unverified`), and the source file name. AMIs imported before digest recording print `Source digest:  not recorded`. The digest is not exposed through `DescribeImages`. |
 | `spx admin images promote` | `--image-id` (required), `--yes` | Reads `ami-<id>/config.json`, validates the AMI is account-owned, then rewrites `ImageOwnerAlias` to `"system"` in-place. No block data is copied. The change takes effect immediately — the AMI becomes visible to all accounts via `DescribeImages`. Prompts for confirmation (skipped with `--yes`). Already-system AMIs are refused. |