fix: keep the RDS endpoint ENI alive across a DB instance replace (#833)

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)

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))
+}

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,

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)

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.