fix: register EC2 snapshot metadata for provider-backed AMI imports (#774)

The catalog image-import command wrote a volume's blocks through the EBS provider but never wrote the EC2 control plane's {snapshotID}/metadata.json, so the imported AMI's snapshot was invisible to DescribeSnapshots and any resolution path without the system-AMI fallback. Register it the same way the ochre weights import already does, and log when the system-AMI fallback masks a missing document instead of silently succeeding.
2026-10-15 17:11:30 +00:00 · 2026-08-12 22:04:56 +10:00
parent 1e1d00cda1
commit f14dd2ae1a
5 changed files with 153 additions and 1 deletions
@@ -34,6 +34,7 @@ import (
 	"github.com/mulgadc/spinifex/spinifex/ebsprovider"
 	"github.com/mulgadc/spinifex/spinifex/formation"
 	"github.com/mulgadc/spinifex/spinifex/gpu"
+	handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
 	handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc"
 	handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam"
 	"github.com/mulgadc/spinifex/spinifex/hostdns"
@@ -709,11 +710,19 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 		os.Exit(1)
 	}

+	// admin.ImportImage only wrote the provider's half of the snapshot (its
+	// blocks); this writes the EC2 control plane's, without which
+	// 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)
+	}
+
 	// The document is what DescribeImages enumerates, so it is written last:
 	// until it exists the AMI is not launchable, and an import that failed
 	// partway leaves no half-registered image behind.
 	ami.State = "available"
-	metaStore := objectstore.NewS3ObjectStoreFromConfig(node.Predastore.Host, node.Predastore.Region, node.Predastore.AccessKey, node.Predastore.SecretKey)
 	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)
@@ -722,6 +731,30 @@ func runimagesImportCmd(cmd *cobra.Command, args []string) {
 	fmt.Printf("✅ Image import complete. Image-ID (AMI): %s\n", volumeId)
 }

+// registerImportedAMISnapshot writes the EC2 control plane's snapshot document
+// for a catalog-imported AMI. The provider-backed import path only writes the
+// storage backend's blocks under ami.SnapshotID, so without this a launch or
+// DescribeSnapshots call cannot resolve it. The volume ID matches ami.ImageID:
+// admin.ImportImage creates the volume under the AMI's own ID.
+func registerImportedAMISnapshot(store objectstore.ObjectStore, bucket string, ami ebsmetadata.AMI, az string, encrypted bool) error {
+	cfg := &handlers_ec2_snapshot.SnapshotConfig{
+		SnapshotID:       ami.SnapshotID,
+		VolumeID:         ami.ImageID,
+		VolumeSize:       utils.SafeUint64ToInt64(ami.VolumeSizeGiB),
+		State:            "completed",
+		Progress:         "100%",
+		StartTime:        time.Now(),
+		Description:      fmt.Sprintf("Imported AMI volume for %s", ami.Name),
+		Encrypted:        encrypted,
+		OwnerID:          utils.GlobalAccountID,
+		AvailabilityZone: az,
+	}
+	if err := handlers_ec2_snapshot.WriteSnapshotConfig(store, bucket, ami.SnapshotID, cfg); err != nil {
+		return fmt.Errorf("register snapshot metadata: %w", err)
+	}
+	return nil
+}
+
 // caBakeRunCommand installs the uploaded CA into the guest trust store across
 // the distro families, then removes the staged copy. Each branch is gated on its
 // updater existing so only the matching distro's branch writes anything (install
@@ -0,0 +1,66 @@
+package cmd
+
+import (
+	"testing"
+
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
+	handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
+	"github.com/mulgadc/spinifex/spinifex/objectstore"
+	"github.com/mulgadc/spinifex/spinifex/utils"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The provider-backed import path (admin.ImportImage) only writes the
+// storage backend's blocks, not the EC2 control plane's document, so what it
+// writes must be readable by DescribeSnapshots and GetAMISourceVolumeID the
+// same way CreateImageFromInstance's and CopyImage's snapshots are.
+func TestRegisterImportedAMISnapshot_WritesEC2ReadableMetadata(t *testing.T) {
+	store := objectstore.NewMemoryObjectStore()
+	const bucket = "predastore"
+	ami := ebsmetadata.AMI{
+		ImageID:         "ami-import01",
+		SnapshotID:      "snap-ami-import01",
+		Name:            "debian-13-x86_64",
+		VolumeSizeGiB:   8,
+		ImageOwnerAlias: "system",
+	}
+
+	require.NoError(t, registerImportedAMISnapshot(store, bucket, ami, "ap-southeast-2a", true))
+
+	cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(store, bucket, "snap-ami-import01")
+	require.NoError(t, err)
+	assert.Equal(t, "snap-ami-import01", cfg.SnapshotID)
+	assert.Equal(t, "ami-import01", cfg.VolumeID, "the import path creates the volume under the AMI's own ID")
+	assert.Equal(t, int64(8), cfg.VolumeSize)
+	assert.Equal(t, "completed", cfg.State)
+	assert.Equal(t, "100%", cfg.Progress)
+	assert.Equal(t, "ap-southeast-2a", cfg.AvailabilityZone)
+	assert.True(t, cfg.Encrypted)
+	assert.Equal(t, utils.GlobalAccountID, cfg.OwnerID, "a system-catalog import has no tenant account to own its snapshot")
+	assert.Contains(t, cfg.Description, ami.Name)
+}
+
+// An unencrypted volume must not be advertised as encrypted, which would let
+// a consumer skip a key it actually needs.
+func TestRegisterImportedAMISnapshot_UnencryptedVolume(t *testing.T) {
+	store := objectstore.NewMemoryObjectStore()
+	ami := ebsmetadata.AMI{ImageID: "ami-plain01", SnapshotID: "snap-ami-plain01", VolumeSizeGiB: 4}
+
+	require.NoError(t, registerImportedAMISnapshot(store, "predastore", ami, "ap-southeast-2a", false))
+
+	cfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(store, "predastore", "snap-ami-plain01")
+	require.NoError(t, err)
+	assert.False(t, cfg.Encrypted)
+}
+
+// A failed metadata write must surface: silently continuing would leave the
+// AMI document referencing a snapshot ID DescribeSnapshots can never resolve.
+func TestRegisterImportedAMISnapshot_WriteFailureSurfaces(t *testing.T) {
+	store := &failingObjectStore{MemoryObjectStore: objectstore.NewMemoryObjectStore(), failPutObject: true}
+	ami := ebsmetadata.AMI{ImageID: "ami-fail01", SnapshotID: "snap-ami-fail01", VolumeSizeGiB: 4}
+
+	err := registerImportedAMISnapshot(store, "predastore", ami, "ap-southeast-2a", false)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "register snapshot metadata")
+}
@@ -2,6 +2,8 @@ package handlers_ec2_image

 import (
 	"context"
+	"log/slog"
+	"strings"
 	"testing"

 	"github.com/mulgadc/spinifex/spinifex/awserrors"
@@ -50,6 +52,30 @@ func TestGetAMISourceVolumeID_BundledSystemAMI(t *testing.T) {
 	assert.Equal(t, "ami-sys01", got, "the bundled AMI's snapshot reads chunks from a volume named after the AMI")
 }

+// TestGetAMISourceVolumeID_BundledSystemAMI_LogsFallbackWarning locks that the
+// bundled-system fallback is observable: it masks a missing control-plane
+// document, so a caller relying on it (e.g. a stale catalog import predating
+// this fix) must be able to spot it in the logs rather than launch silently.
+func TestGetAMISourceVolumeID_BundledSystemAMI_LogsFallbackWarning(t *testing.T) {
+	svc, store := setupProviderImageService(t)
+	putSourceVolumeAMI(t, svc, store, ebsmetadata.AMI{
+		ImageID: "ami-sys02", SnapshotID: "snap-ami-sys02", ImageOwnerAlias: "system",
+	}, "")
+
+	var buf strings.Builder
+	prev := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})))
+	t.Cleanup(func() { slog.SetDefault(prev) })
+
+	got, err := svc.GetAMISourceVolumeID(context.Background(), "ami-sys02")
+	require.NoError(t, err)
+	assert.Equal(t, "ami-sys02", got)
+
+	logs := buf.String()
+	assert.Contains(t, logs, "level=WARN", "the fallback masking a missing snapshot document must be visible")
+	assert.Contains(t, logs, "ami-sys02")
+}
+
 // TestGetAMISourceVolumeID_AccountAMIMissingSnapshotMetadata locks that the
 // bundled fallback does not apply to account-owned AMIs.
 func TestGetAMISourceVolumeID_AccountAMIMissingSnapshotMetadata(t *testing.T) {
@@ -552,6 +552,8 @@ func (s *ImageServiceImpl) GetAMISourceVolumeID(ctx context.Context, imageID str
 	snapCfg, err := handlers_ec2_snapshot.ReadSnapshotConfig(s.store, s.bucketName, meta.SnapshotID)
 	if err != nil {
 		if objectstore.IsNoSuchKeyError(err) && meta.ImageOwnerAlias != "" && !utils.IsAccountID(meta.ImageOwnerAlias) {
+			slog.WarnContext(ctx, "GetAMISourceVolumeID: system AMI has no snapshot metadata document, falling back to imageID as volume ID",
+				"imageId", imageID, "snapshotId", meta.SnapshotID, "imageOwnerAlias", meta.ImageOwnerAlias)
 			return imageID, nil
 		}
 		if objectstore.IsNoSuchKeyError(err) || errors.Is(err, handlers_ec2_snapshot.ErrCorruptSnapshotMetadata) {
@@ -309,6 +309,31 @@ func TestDescribeSnapshots_Empty(t *testing.T) {
 	assert.Empty(t, result.Snapshots)
 }

+// TestDescribeSnapshots_ImportedAMISnapshotVisible locks that a snapshot
+// registered directly via WriteSnapshotConfig -- the way a provider-backed
+// AMI import registers its snapshot, without ever calling CreateSnapshot --
+// is still listed. Before that registration exists, ListObjectsV2 finds the
+// provider-written "snap-.../" prefix but getSnapshotConfig 404s on it, and
+// DescribeSnapshots silently skips it.
+func TestDescribeSnapshots_ImportedAMISnapshotVisible(t *testing.T) {
+	svc, store := setupTestSnapshotService(t)
+
+	require.NoError(t, WriteSnapshotConfig(store, "test-bucket", "snap-ami-import01", &SnapshotConfig{
+		SnapshotID: "snap-ami-import01",
+		VolumeID:   "ami-import01",
+		VolumeSize: 8,
+		State:      "completed",
+		Progress:   "100%",
+		OwnerID:    testAccountID,
+	}))
+
+	result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{}, testAccountID)
+	require.NoError(t, err)
+	require.Len(t, result.Snapshots, 1)
+	assert.Equal(t, "snap-ami-import01", *result.Snapshots[0].SnapshotId)
+	assert.Equal(t, "ami-import01", *result.Snapshots[0].VolumeId)
+}
+
 // TestDescribeSnapshots_AccountScoping tests that account A cannot see account B's snapshots.
 func TestDescribeSnapshots_AccountScoping(t *testing.T) {
 	svc, store := setupTestSnapshotService(t)