From a69ed27dc4ff2f8de28e76fcb06708a267b97361 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 6 Oct 2026 12:01:13 +1100 Subject: [PATCH] fix(spx): extract image imports into a disk-backed scratch dir (#1171) --- cmd/spinifex/cmd/admin.go | 76 +++++++----- cmd/spinifex/cmd/admin_images_import_test.go | 117 +++++++++++++++++++ docs/COMMANDS.md | 2 +- 3 files changed, 168 insertions(+), 27 deletions(-) create mode 100644 cmd/spinifex/cmd/admin_images_import_test.go diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index 62c21092f..268a0cec3 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -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: /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 diff --git a/cmd/spinifex/cmd/admin_images_import_test.go b/cmd/spinifex/cmd/admin_images_import_test.go new file mode 100644 index 000000000..14e89e5fd --- /dev/null +++ b/cmd/spinifex/cmd/admin_images_import_test.go @@ -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") + }) + } +} diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index f16e61d42..112fcf89e 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -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 ` is passed. `--checksum` takes the sums file the image was published with (`SHA256SUMS`, `SHA512SUMS`, `.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 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: `/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 ` is passed. `--checksum` takes the sums file the image was published with (`SHA256SUMS`, `SHA512SUMS`, `.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 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-/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. |