From f14dd2ae1ab26b78331c84968e3316e98ca14ae8 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 12 Aug 2026 22:04:56 +1000 Subject: [PATCH] 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. --- cmd/spinifex/cmd/admin.go | 35 +++++++++- .../register_imported_ami_snapshot_test.go | 66 +++++++++++++++++++ .../ec2/image/ami_source_volume_test.go | 26 ++++++++ spinifex/handlers/ec2/image/service_impl.go | 2 + .../ec2/snapshot/service_impl_test.go | 25 +++++++ 5 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 cmd/spinifex/cmd/register_imported_ami_snapshot_test.go diff --git a/cmd/spinifex/cmd/admin.go b/cmd/spinifex/cmd/admin.go index a40127a3f..3e43b1433 100644 --- a/cmd/spinifex/cmd/admin.go +++ b/cmd/spinifex/cmd/admin.go @@ -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 diff --git a/cmd/spinifex/cmd/register_imported_ami_snapshot_test.go b/cmd/spinifex/cmd/register_imported_ami_snapshot_test.go new file mode 100644 index 000000000..b1c5ec12d --- /dev/null +++ b/cmd/spinifex/cmd/register_imported_ami_snapshot_test.go @@ -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") +} diff --git a/spinifex/handlers/ec2/image/ami_source_volume_test.go b/spinifex/handlers/ec2/image/ami_source_volume_test.go index 8ed58b951..35086c495 100644 --- a/spinifex/handlers/ec2/image/ami_source_volume_test.go +++ b/spinifex/handlers/ec2/image/ami_source_volume_test.go @@ -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) { diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index 4fd70921c..f6a873ce3 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -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) { diff --git a/spinifex/handlers/ec2/snapshot/service_impl_test.go b/spinifex/handlers/ec2/snapshot/service_impl_test.go index 347364ddf..8cd89b423 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl_test.go +++ b/spinifex/handlers/ec2/snapshot/service_impl_test.go @@ -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)