From aaef75842604b0940f834b0e5dcedbb11051ade5 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Mon, 29 Jun 2026 15:30:26 +1000 Subject: [PATCH] fix: cross-check live BlockDeviceMappings before DeleteVolume (#440) The daemon-side in-use gate trusts persisted volume State/AttachedInstance, which the attach/detach lifecycle can leave desynced: a running root volume was observed with State:"" and AttachedInstance:"" while the instance's BlockDeviceMappings still listed it, so DeleteVolume passed the gate and removed the durable backing under a live QEMU. Add an authoritative guard at the gateway DeleteVolume path: fan out the strict DescribeInstances survey and refuse with VolumeInUse when any non-terminated instance's BlockDeviceMappings reference the volume. Fail closed on an incomplete cluster view rather than delete against a partial survey. Terminate-time cleanup uses the daemon volume service directly and is unaffected. --- spinifex/gateway/ec2.go | 2 +- spinifex/gateway/ec2/volume/DeleteVolume.go | 62 ++++++++++++++- .../gateway/ec2/volume/DeleteVolume_test.go | 77 +++++++++++++++++++ spinifex/gateway/ec2/volume/handler_test.go | 6 +- 4 files changed, 141 insertions(+), 6 deletions(-) diff --git a/spinifex/gateway/ec2.go b/spinifex/gateway/ec2.go index da151c5cb..bcf3665d8 100644 --- a/spinifex/gateway/ec2.go +++ b/spinifex/gateway/ec2.go @@ -244,7 +244,7 @@ var ec2Actions = map[string]EC2Handler{ return gateway_ec2_volume.CreateVolume(input, gw.NATSConn, accountID) }), "DeleteVolume": ec2Handler(func(input *ec2.DeleteVolumeInput, gw *GatewayConfig, accountID string) (any, error) { - return gateway_ec2_volume.DeleteVolume(input, gw.NATSConn, accountID) + return gateway_ec2_volume.DeleteVolume(input, gw.NATSConn, gw.DiscoverActiveNodes(), accountID) }), "AttachVolume": ec2Handler(func(input *ec2.AttachVolumeInput, gw *GatewayConfig, accountID string) (any, error) { return gateway_ec2_volume.AttachVolume(input, gw.NATSConn, accountID) diff --git a/spinifex/gateway/ec2/volume/DeleteVolume.go b/spinifex/gateway/ec2/volume/DeleteVolume.go index e52be12fc..20f0fdc98 100644 --- a/spinifex/gateway/ec2/volume/DeleteVolume.go +++ b/spinifex/gateway/ec2/volume/DeleteVolume.go @@ -2,10 +2,13 @@ package gateway_ec2_volume import ( "errors" + "log/slog" "strings" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/awserrors" + gateway_ec2_instance "github.com/mulgadc/spinifex/spinifex/gateway/ec2/instance" handlers_ec2_volume "github.com/mulgadc/spinifex/spinifex/handlers/ec2/volume" "github.com/nats-io/nats.go" ) @@ -22,8 +25,41 @@ func ValidateDeleteVolumeInput(input *ec2.DeleteVolumeInput) error { return nil } -// DeleteVolume handles the DeleteVolume API call -func DeleteVolume(input *ec2.DeleteVolumeInput, natsConn *nats.Conn, accountID string) (ec2.DeleteVolumeOutput, error) { +// volumeHeldByInstance returns the id of a non-terminated instance whose +// BlockDeviceMappings reference volumeID, or "" if none does. A terminated +// instance's mappings are stale (its volumes are released), so they do not +// block a delete. This is the authoritative in-use signal: persisted volume +// State/AttachedInstance can drift out of sync with the real attachment. +func volumeHeldByInstance(reservations []*ec2.Reservation, volumeID string) string { + for _, res := range reservations { + if res == nil { + continue + } + for _, inst := range res.Instances { + if inst == nil { + continue + } + if inst.State != nil && aws.StringValue(inst.State.Name) == ec2.InstanceStateNameTerminated { + continue + } + for _, bdm := range inst.BlockDeviceMappings { + if bdm != nil && bdm.Ebs != nil && aws.StringValue(bdm.Ebs.VolumeId) == volumeID { + return aws.StringValue(inst.InstanceId) + } + } + } + } + return "" +} + +// DeleteVolume handles the DeleteVolume API call. +// +// Before destroying the backing it cross-checks live cluster-wide instance +// BlockDeviceMappings: the daemon-side gate trusts persisted volume metadata, +// which the attach/detach lifecycle can leave desynced (a running root with +// State:"" / AttachedInstance:""), so a drifted-but-live volume could be +// deleted under a running QEMU. The check fails closed on an incomplete view. +func DeleteVolume(input *ec2.DeleteVolumeInput, natsConn *nats.Conn, expectedNodes int, accountID string) (ec2.DeleteVolumeOutput, error) { var output ec2.DeleteVolumeOutput err := ValidateDeleteVolumeInput(input) @@ -31,6 +67,28 @@ func DeleteVolume(input *ec2.DeleteVolumeInput, natsConn *nats.Conn, accountID s return output, err } + volumeID := aws.StringValue(input.VolumeId) + + // Strict fan-out: complete is true only when every active node answered and + // both instance buckets queried, so a node that owns the attachment cannot be + // silently missing from the survey. Refuse rather than delete against a + // partial view of the cluster. + reservations, complete, err := gateway_ec2_instance.DescribeInstancesForReconcile( + &ec2.DescribeInstancesInput{}, natsConn, expectedNodes, accountID) + if err != nil { + slog.Error("DeleteVolume: in-use precheck fan-out failed", "volumeId", volumeID, "err", err) + return output, errors.New(awserrors.ErrorServerInternal) + } + if !complete { + slog.Error("DeleteVolume: refusing delete on incomplete instance view", "volumeId", volumeID) + return output, errors.New(awserrors.ErrorServerInternal) + } + if holder := volumeHeldByInstance(reservations, volumeID); holder != "" { + slog.Error("DeleteVolume: volume still attached to a live instance", + "volumeId", volumeID, "instanceId", holder) + return output, errors.New(awserrors.ErrorVolumeInUse) + } + volumeService := handlers_ec2_volume.NewNATSVolumeService(natsConn) result, err := volumeService.DeleteVolume(input, accountID) diff --git a/spinifex/gateway/ec2/volume/DeleteVolume_test.go b/spinifex/gateway/ec2/volume/DeleteVolume_test.go index 2ff9253b4..add54b471 100644 --- a/spinifex/gateway/ec2/volume/DeleteVolume_test.go +++ b/spinifex/gateway/ec2/volume/DeleteVolume_test.go @@ -97,3 +97,80 @@ func TestValidateDeleteVolumeInput(t *testing.T) { }) } } + +func instanceWithVolume(instanceID, state, volumeID string) *ec2.Instance { + return &ec2.Instance{ + InstanceId: aws.String(instanceID), + State: &ec2.InstanceState{Name: aws.String(state)}, + BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{ + {DeviceName: aws.String("/dev/vda"), Ebs: &ec2.EbsInstanceBlockDevice{VolumeId: aws.String(volumeID)}}, + }, + } +} + +func reservations(instances ...*ec2.Instance) []*ec2.Reservation { + return []*ec2.Reservation{{Instances: instances}} +} + +func TestVolumeHeldByInstance(t *testing.T) { + const vol = "vol-target" + + tests := []struct { + name string + reservations []*ec2.Reservation + want string + }{ + { + name: "RunningInstanceHoldsVolume", + reservations: reservations(instanceWithVolume("i-running", ec2.InstanceStateNameRunning, vol)), + want: "i-running", + }, + { + name: "StoppedInstanceHoldsVolume", + reservations: reservations(instanceWithVolume("i-stopped", ec2.InstanceStateNameStopped, vol)), + want: "i-stopped", + }, + { + name: "TerminatedInstanceIgnored", + reservations: reservations(instanceWithVolume("i-dead", ec2.InstanceStateNameTerminated, vol)), + want: "", + }, + { + name: "NoInstanceReferencesVolume", + reservations: reservations(instanceWithVolume("i-other", ec2.InstanceStateNameRunning, "vol-different")), + want: "", + }, + { + name: "RunningWinsOverTerminatedDuplicate", + reservations: reservations( + instanceWithVolume("i-dead", ec2.InstanceStateNameTerminated, vol), + instanceWithVolume("i-live", ec2.InstanceStateNameRunning, vol), + ), + want: "i-live", + }, + { + name: "EmptyReservations", + reservations: nil, + want: "", + }, + { + name: "NilSafety", + reservations: []*ec2.Reservation{ + nil, + {Instances: []*ec2.Instance{nil, {InstanceId: aws.String("i-nobdm")}}}, + {Instances: []*ec2.Instance{{ + InstanceId: aws.String("i-nilebs"), + State: &ec2.InstanceState{Name: aws.String(ec2.InstanceStateNameRunning)}, + BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{nil, {Ebs: nil}}, + }}}, + }, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, volumeHeldByInstance(tt.reservations, vol)) + }) + } +} diff --git a/spinifex/gateway/ec2/volume/handler_test.go b/spinifex/gateway/ec2/volume/handler_test.go index c8c4531d3..eea638890 100644 --- a/spinifex/gateway/ec2/volume/handler_test.go +++ b/spinifex/gateway/ec2/volume/handler_test.go @@ -28,17 +28,17 @@ func TestCreateVolume_NilNATS(t *testing.T) { } func TestDeleteVolume_ValidationErrors(t *testing.T) { - _, err := DeleteVolume(nil, nil, "") + _, err := DeleteVolume(nil, nil, 1, "") assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue) - _, err = DeleteVolume(&ec2.DeleteVolumeInput{VolumeId: aws.String("bad")}, nil, "") + _, err = DeleteVolume(&ec2.DeleteVolumeInput{VolumeId: aws.String("bad")}, nil, 1, "") assert.EqualError(t, err, awserrors.ErrorInvalidVolumeIDMalformed) } func TestDeleteVolume_NilNATS(t *testing.T) { _, err := DeleteVolume(&ec2.DeleteVolumeInput{ VolumeId: aws.String("vol-1234567890abcdef0"), - }, nil, "acct-123") + }, nil, 1, "acct-123") assert.Error(t, err) }