diff --git a/spinifex/domains/ec2/image/account_termination_current_behaviour_test.go b/spinifex/domains/ec2/image/account_termination_current_behaviour_test.go index a15307d6e..d208e8348 100644 --- a/spinifex/domains/ec2/image/account_termination_current_behaviour_test.go +++ b/spinifex/domains/ec2/image/account_termination_current_behaviour_test.go @@ -124,9 +124,10 @@ func TestCurrentBehaviour_AccountTermination_CreateImageSnapshotDoesNotPinSource assert.Equal(t, "vol-imaged", source, "a launch from the image still resolves the deleted volume") } -// DeleteSnapshot consults volumes cloned from the snapshot, not images -// registered on it, so the snapshot under a live image deletes. -func TestCurrentBehaviour_AccountTermination_DeleteSnapshotIgnoresRegisteredImage(t *testing.T) { +// DeleteSnapshot consults AMI documents as well as volumes cloned from the +// snapshot, so the snapshot under a live image refuses to delete until the +// image is deregistered. +func TestAccountTermination_DeleteSnapshotRefusesARegisteredImage(t *testing.T) { s := newStorageServices(t) ctx := context.Background() @@ -146,16 +147,18 @@ func TestCurrentBehaviour_AccountTermination_DeleteSnapshotIgnoresRegisteredImag require.NoError(t, err) _, err = s.snapshots.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String(snapshotID)}, ctAccountID) - require.NoError(t, err, "the snapshot deletes while an image is registered on it") + assert.EqualError(t, err, awserrors.ErrorInvalidSnapshotInUse, "the snapshot refuses to delete while an image is registered on it") - images, err := s.images.DescribeImages(ctx, &ec2.DescribeImagesInput{ImageIds: []*string{registered.ImageId}}, ctAccountID) + _, err = s.metadata.GetSnapshot(ctx, ctAccountID, snapshotID) + assert.NoError(t, err, "the snapshot document is still present") + _, held := s.provider.Snapshot(snapshotID) + assert.True(t, held, "the provider still holds the snapshot") + + _, err = s.images.DeregisterImage(ctx, &ec2.DeregisterImageInput{ImageId: registered.ImageId}, ctAccountID) require.NoError(t, err) - require.Len(t, images.Images, 1) - assert.Equal(t, ec2.ImageStateAvailable, aws.StringValue(images.Images[0].State), "the image still reports available") - _, err = s.images.GetAMISourceVolumeID(ctx, aws.StringValue(registered.ImageId)) - assert.EqualError(t, err, awserrors.ErrorInvalidSnapshotNotFound, "a launch from the image can no longer resolve its snapshot") - require.NoError(t, s.deleteVolume("vol-registered"), "with the snapshot gone, the source volume deletes too") + _, err = s.snapshots.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String(snapshotID)}, ctAccountID) + require.NoError(t, err, "the snapshot deletes once the image is deregistered") } // deleteDuringClone runs a hook once, after the provider has accepted a clone diff --git a/spinifex/domains/ec2/snapshot/service_impl.go b/spinifex/domains/ec2/snapshot/service_impl.go index c1b871b8c..dda1b24d5 100644 --- a/spinifex/domains/ec2/snapshot/service_impl.go +++ b/spinifex/domains/ec2/snapshot/service_impl.go @@ -685,6 +685,25 @@ func (s *SnapshotServiceImpl) snapshotInUseByVolumes(ctx context.Context, snapsh return false, nil } +// amiInUseBySnapshot checks if any registered AMI names the given snapshot as +// its root device. AWS refuses DeleteSnapshot while a registered AMI +// references the snapshot, the same way it refuses one backing a volume. +func (s *SnapshotServiceImpl) amiInUseBySnapshot(ctx context.Context, snapshotID string) (bool, error) { + // Whole-cluster and strict for the same reason as the volume check above: + // a system AMI can be registered by an account other than the snapshot's + // owner, and an AMI this cannot read is not evidence that none exists. + amis, err := s.metadata.ListAMIsStrict(ctx) + if err != nil { + return false, fmt.Errorf("amiInUseBySnapshot: failed to list AMIs: %w", err) + } + for _, ami := range amis { + if ami.SnapshotID == snapshotID { + return true, nil + } + } + return false, nil +} + // DeleteSnapshot deletes a snapshot after verifying the caller owns it. func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.DeleteSnapshotInput, accountID string) (*ec2.DeleteSnapshotOutput, error) { if input == nil || input.SnapshotId == nil { @@ -724,6 +743,17 @@ func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.Del slog.InfoContext(ctx, "DeleteSnapshot blocked: snapshot in use by volume", "snapshotId", snapshotID) return nil, errors.New(awserrors.ErrorInvalidSnapshotInUse) } + + // Check if any registered AMI names this snapshot as its root device + amiInUse, err := s.amiInUseBySnapshot(ctx, snapshotID) + if err != nil { + slog.ErrorContext(ctx, "DeleteSnapshot failed to check AMI usage", "snapshotId", snapshotID, "err", err) + return nil, errors.New(awserrors.ErrorServerInternal) + } + if amiInUse { + slog.InfoContext(ctx, "DeleteSnapshot blocked: snapshot in use by registered AMI", "snapshotId", snapshotID) + return nil, errors.New(awserrors.ErrorInvalidSnapshotInUse) + } if s.provider == nil { slog.ErrorContext(ctx, "no EBS provider configured", "op", "DeleteSnapshot") return nil, errors.New(awserrors.ErrorServerInternal) diff --git a/spinifex/domains/ec2/snapshot/service_impl_test.go b/spinifex/domains/ec2/snapshot/service_impl_test.go index 61ecb158a..78415f200 100644 --- a/spinifex/domains/ec2/snapshot/service_impl_test.go +++ b/spinifex/domains/ec2/snapshot/service_impl_test.go @@ -490,6 +490,101 @@ func TestDeleteSnapshot_InUseByVolume(t *testing.T) { assert.Len(t, result.Snapshots, 1) } +// TestDeleteSnapshot_InUseByRegisteredAMI tests that deleting a snapshot +// fails while an AMI's own CreateImage snapshot still names it, matching AWS. +func TestDeleteSnapshot_InUseByRegisteredAMI(t *testing.T) { + svc, store := setupTestSnapshotService(t) + + createTestVolume(t, svc, store, "vol-source", 50) + snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{ + VolumeId: aws.String("vol-source"), + }, testAccountID) + require.NoError(t, err) + + require.NoError(t, svc.metadata.PutAMI(context.Background(), ebsmetadata.AMI{ + ImageID: "ami-registered", Name: "ct-ami", SnapshotID: *snap.SnapshotId, + })) + + _, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{ + SnapshotId: snap.SnapshotId, + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotInUse) + + // The provider and control-plane document both survive the refusal. + result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: []*string{snap.SnapshotId}, + }, testAccountID) + require.NoError(t, err) + assert.Len(t, result.Snapshots, 1) + + require.NoError(t, svc.metadata.DeleteAMI(context.Background(), "ami-registered")) + _, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{ + SnapshotId: snap.SnapshotId, + }, testAccountID) + require.NoError(t, err, "once the AMI is gone, the delete succeeds") +} + +// TestDeleteSnapshot_InUseByRegisteredAMI_AnotherAccount tests that the AMI +// check is whole-cluster: a system AMI registered by a different account than +// the snapshot's owner still blocks the delete. +func TestDeleteSnapshot_InUseByRegisteredAMI_AnotherAccount(t *testing.T) { + svc, store := setupTestSnapshotService(t) + + createTestVolume(t, svc, store, "vol-source", 50) + snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{ + VolumeId: aws.String("vol-source"), + }, testAccountID) + require.NoError(t, err) + + require.NoError(t, svc.metadata.PutAMI(context.Background(), ebsmetadata.AMI{ + ImageID: "ami-other-account", Name: "ct-system-ami", + ImageOwnerAlias: otherAccountID, SnapshotID: *snap.SnapshotId, + })) + + _, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{ + SnapshotId: snap.SnapshotId, + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotInUse) +} + +// TestDeleteSnapshot_AMIReadFailureFailsClosed tests that an AMI document +// this cannot decode aborts the delete rather than being treated as absent: +// a partial AMI listing is not evidence that no AMI names the snapshot. +func TestDeleteSnapshot_AMIReadFailureFailsClosed(t *testing.T) { + svc, store := setupTestSnapshotService(t) + + createTestVolume(t, svc, store, "vol-source", 50) + snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{ + VolumeId: aws.String("vol-source"), + }, testAccountID) + require.NoError(t, err) + + key, err := ebsmetadata.AMIKey("ami-corrupt") + require.NoError(t, err) + _, err = store.PutObject(context.Background(), &s3.PutObjectInput{ + Bucket: aws.String("test-bucket"), + Key: aws.String(key), + Body: strings.NewReader("not-json"), + }) + require.NoError(t, err) + + _, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{ + SnapshotId: snap.SnapshotId, + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorServerInternal) + + // Nothing was deleted: the snapshot survives in both the provider and the + // control-plane document. + result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{ + SnapshotIds: []*string{snap.SnapshotId}, + }, testAccountID) + require.NoError(t, err) + assert.Len(t, result.Snapshots, 1) +} + // TestDeleteSnapshot_NotFound tests deleting a non-existent snapshot. func TestDeleteSnapshot_NotFound(t *testing.T) { svc, _ := setupTestSnapshotService(t) diff --git a/tests/e2e/harness/sweep.go b/tests/e2e/harness/sweep.go index b3e9b5b8a..32cf44c5e 100644 --- a/tests/e2e/harness/sweep.go +++ b/tests/e2e/harness/sweep.go @@ -78,8 +78,10 @@ func (r *SweepReport) errf(format string, args ...any) { } // SweepRunResources deletes every EC2/ELBv2 resource tagged e2e:run=, -// in dependency order (load balancers → instances → NAT gateways → snapshots → -// volumes → images → security groups → subnets → key pairs). It is idempotent +// in dependency order (load balancers → instances → NAT gateways → images → +// snapshots → volumes → security groups → subnets → key pairs). Images come +// before snapshots: DeleteSnapshot now refuses while a registered AMI names +// the snapshot, so its AMI has to be deregistered first. It is idempotent // and best-effort; callers inspect SweepReport but a non-empty Errors list is // not necessarily a run failure (resources may already be gone, or still // settling). A blank runID returns an empty report without touching anything. @@ -92,9 +94,9 @@ func SweepRunResources(ec2c ec2iface.EC2API, elbc elbv2iface.ELBV2API, runID str sweepLoadBalancers(elbc, runID, rep) sweepInstances(ec2c, runID, rep) sweepNatGateways(ec2c, runID, rep) + sweepImages(ec2c, runID, rep) sweepSnapshots(ec2c, runID, rep) sweepVolumes(ec2c, runID, rep) - sweepImages(ec2c, runID, rep) sweepSecurityGroups(ec2c, runID, rep) sweepSubnets(ec2c, runID, rep) sweepKeyPairs(ec2c, runID, rep)