From 3ee412ca02bbcb699ecab57da2defd16570499e8 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 19 Aug 2026 12:18:04 +1000 Subject: [PATCH] fix: keep the RDS endpoint ENI alive across a DB instance replace (#833) --- spinifex/daemon/daemon_system_instance.go | 23 ++++++++- .../daemon/daemon_system_instance_test.go | 49 +++++++++++++++++++ spinifex/handlers/rds/launch.go | 4 ++ spinifex/handlers/rds/launch_test.go | 3 ++ spinifex/handlers/sysinstance/launcher.go | 5 ++ 5 files changed, 82 insertions(+), 2 deletions(-) diff --git a/spinifex/daemon/daemon_system_instance.go b/spinifex/daemon/daemon_system_instance.go index 25d523689..0b86df369 100644 --- a/spinifex/daemon/daemon_system_instance.go +++ b/spinifex/daemon/daemon_system_instance.go @@ -52,6 +52,25 @@ func (d *Daemon) recordENIInstanceOwner(eniAccountID, eniID, instanceOwnerID str } } +// attachExtraENI attaches one pre-created extra ENI and, when the caller asked +// for an explicit DeleteOnTermination, persists it onto the record. An ENI that +// has to outlive its VM is only safe once that flag is stored, so a failed stamp +// fails the attach rather than leaving a disposable NIC behind. +func (d *Daemon) attachExtraENI(eniAccountID string, extra sysinstance.ExtraENIInput, instanceID string, deviceIndex int64) error { + if _, err := d.vpcService.AttachENI(eniAccountID, extra.ENIID, instanceID, deviceIndex); err != nil { + return err + } + if extra.DeleteOnTermination == nil { + return nil + } + if err := d.vpcService.UpdateENI(eniAccountID, extra.ENIID, func(r *handlers_ec2_vpc.ENIRecord) { + r.DeleteOnTermination = extra.DeleteOnTermination + }); err != nil { + return fmt.Errorf("set DeleteOnTermination on ENI %s: %w", extra.ENIID, err) + } + return nil +} + // LaunchSystemInstance creates and starts a system-managed VM (ELBv2 LB). // The VM is owned by the system account (GlobalAccountID), is not visible to // customer DescribeInstances calls, and always boots via direct kernel boot @@ -160,7 +179,7 @@ func (d *Daemon) LaunchSystemInstance(input *handlers_elbv2.SystemInstanceInput) // customer-VPC ENI) lives in its own account; the ENI record is // account-keyed, so attach under extra.AccountID when set. extraAccount := resolveENIAccount(extra.AccountID, eniAccountID) - if _, attachErr := d.vpcService.AttachENI(extraAccount, extra.ENIID, instance.ID, int64(idx+1)); attachErr != nil { + if attachErr := d.attachExtraENI(extraAccount, extra, instance.ID, int64(idx+1)); attachErr != nil { slog.Error("LaunchSystemInstance: failed to attach extra ENI", "eniId", extra.ENIID, "instanceId", instance.ID, "err", attachErr) d.cleanupFailedSystemInstance(instance, instanceType) return nil, fmt.Errorf("attach extra ENI %s: %w", extra.ENIID, attachErr) @@ -488,7 +507,7 @@ func (d *Daemon) launchAMISystemInstance(input *sysinstance.SystemInstanceInput) for idx, extra := range input.ExtraENIs { if d.vpcService != nil { extraAccount := resolveENIAccount(extra.AccountID, input.AccountID) - if _, attachErr := d.vpcService.AttachENI(extraAccount, extra.ENIID, inst.ID, int64(idx+1)); attachErr != nil { + if attachErr := d.attachExtraENI(extraAccount, extra, inst.ID, int64(idx+1)); attachErr != nil { slog.Error("launchAMISystemInstance: failed to attach extra ENI", "eniId", extra.ENIID, "instanceId", inst.ID, "err", attachErr) d.vmMgr.MarkFailed(context.Background(), inst, "extra_eni_attach_failed") return nil, fmt.Errorf("attach extra ENI %s: %w", extra.ENIID, attachErr) diff --git a/spinifex/daemon/daemon_system_instance_test.go b/spinifex/daemon/daemon_system_instance_test.go index bc9ee8fcd..f65b54a0c 100644 --- a/spinifex/daemon/daemon_system_instance_test.go +++ b/spinifex/daemon/daemon_system_instance_test.go @@ -12,10 +12,12 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" "github.com/mulgadc/spinifex/spinifex/handlers/elbv2" handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + "github.com/mulgadc/spinifex/spinifex/handlers/sysinstance" "github.com/mulgadc/spinifex/spinifex/network/external" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -877,3 +879,50 @@ func TestReleaseSystemInstanceEIP_ReleasesEipServiceAllocation(t *testing.T) { assert.Empty(t, inst.PublicIPAllocID, "instance alloc ID must be cleared so externalIPAM teardown does not double-release") assert.Empty(t, inst.PublicIPAssocID) } + +// --- attachExtraENI: DeleteOnTermination --- + +// TestAttachExtraENI_DeleteOnTerminationFalseSurvivesTerminate covers the RDS +// replace: the customer/endpoint ENI is attached as an extra NIC and has to +// outlive the VM, because a replacement launch resolves the endpoint by that +// ENI ID. Without the explicit flag the terminate sweep deletes it and the +// replacement fails with InvalidNetworkInterfaceID.NotFound. +func TestAttachExtraENI_DeleteOnTerminationFalseSurvivesTerminate(t *testing.T) { + f := newENIHotPlugFixture(t) + f.vmInst.AccountID = testAccountID + + require.NoError(t, f.daemon.attachExtraENI(testAccountID, sysinstance.ExtraENIInput{ + ENIID: f.eniID, + DeleteOnTermination: aws.Bool(false), + }, f.vmInst.ID, 1)) + + rec, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err) + require.NotNil(t, rec.DeleteOnTermination, "the attach must stamp the flag, not leave the attach-time default") + assert.False(t, *rec.DeleteOnTermination) + + require.NoError(t, newInstanceCleanerAdapter(f.daemon).DetachAndDeleteENI(f.vmInst)) + + rec, err = f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err, "the endpoint ENI must survive the terminate half of a replace") + assert.Equal(t, "available", rec.Status, "it must be detached, so the replacement VM can re-attach it") + assert.Empty(t, rec.InstanceId) +} + +// TestAttachExtraENI_NilDeleteOnTerminationStaysDisposable holds the ELBv2 +// multi-subnet ALB and EKS cross-account NLB extras at their current behaviour: +// they carry no flag and are deleted with the VM. +func TestAttachExtraENI_NilDeleteOnTerminationStaysDisposable(t *testing.T) { + f := newENIHotPlugFixture(t) + f.vmInst.AccountID = testAccountID + + require.NoError(t, f.daemon.attachExtraENI(testAccountID, sysinstance.ExtraENIInput{ + ENIID: f.eniID, + }, f.vmInst.ID, 1)) + + require.NoError(t, newInstanceCleanerAdapter(f.daemon).DetachAndDeleteENI(f.vmInst)) + + _, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.Error(t, err, "an extra ENI with no explicit flag stays disposable") + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInvalidNetworkInterfaceIDNotFound)) +} diff --git a/spinifex/handlers/rds/launch.go b/spinifex/handlers/rds/launch.go index 1eb1f3740..8fe209d24 100644 --- a/spinifex/handlers/rds/launch.go +++ b/spinifex/handlers/rds/launch.go @@ -239,6 +239,10 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o ENIIP: customerENI.ip, SubnetID: in.SubnetID, AccountID: in.AccountID, + // The endpoint is the ENI's address, so it has to survive the + // terminate half of a replace the way the datadir volume does. + // A DeleteDBInstance deletes it explicitly once the VM is gone. + DeleteOnTermination: aws.Bool(false), }}, UserData: in.UserData, IamInstanceProfileArn: in.IamInstanceProfileArn, diff --git a/spinifex/handlers/rds/launch_test.go b/spinifex/handlers/rds/launch_test.go index 682742104..ef9257db2 100644 --- a/spinifex/handlers/rds/launch_test.go +++ b/spinifex/handlers/rds/launch_test.go @@ -598,6 +598,9 @@ func TestLaunchDBInstanceVMWiresBothNICs(t *testing.T) { ENIIP: out.CustomerENIIP, SubnetID: testDBSubnet, AccountID: testCustomerAccount, + // The endpoint address lives on this NIC, so it has to survive the + // terminate half of a replace the way the datadir volume does. + DeleteOnTermination: aws.Bool(false), }, in.ExtraENIs[0], "the extra NIC must carry the customer account, or the daemon updates the wrong ENI record") assert.Equal(t, "#cloud-config\n", in.UserData) assert.Equal(t, "arn:aws:iam::000000000000:instance-profile/rdsInstanceRole", in.IamInstanceProfileArn) diff --git a/spinifex/handlers/sysinstance/launcher.go b/spinifex/handlers/sysinstance/launcher.go index 113f49bd3..f3cfaa63e 100644 --- a/spinifex/handlers/sysinstance/launcher.go +++ b/spinifex/handlers/sysinstance/launcher.go @@ -100,6 +100,11 @@ type ExtraENIInput struct { // LB VM lives in the system CP VPC but fronts a customer-VPC ENI). Empty // falls back to the primary AccountID — back-compat for same-account extras. AccountID string `json:"account_id,omitempty"` + // DeleteOnTermination stamps an explicit value on the attached ENI record, + // for an ENI that has to outlive the VM it is plugged into (an RDS endpoint + // ENI across a class-change replace). Nil keeps the attach-time default, + // which deletes the ENI with the VM. + DeleteOnTermination *bool `json:"delete_on_termination,omitempty"` } // NICConfig describes a single network interface for a BootDirect microVM.