fix: release only system-tagged addresses in the terminate backstop

An internet-facing ALB's address is now tagged managed-by at allocation, and
ReleaseAddressByInstanceID leaves any untagged record alone with a warning, so
a wrong instance ID can no longer release a customer's own address.

spinifex/domains/ec2/eip/service_impl.go

@@ -879,7 +879,8 @@ func (s *EIPServiceImpl) AssociatedPublicIPForInstance(ctx context.Context, acco
 // against instanceID, across all accounts. Idempotent backstop for system-VM
 // teardown paths that lose the cached allocation ID — when the VM is already
 // gone from the manager the per-instance release never runs, so an
-// internet-facing ALB's EIP would otherwise orphan. No-op when nothing matches.
+// internet-facing ALB's EIP would otherwise orphan. It touches only addresses
+// tagged as system-managed, and is a no-op when nothing matches.
 func (s *EIPServiceImpl) ReleaseAddressByInstanceID(instanceID string) error {
 	ctx := context.Background()
 	if instanceID == "" {
@@ -908,6 +909,13 @@ func (s *EIPServiceImpl) ReleaseAddressByInstanceID(instanceID string) error {
 		if record.InstanceId != instanceID || record.AllocationId == "" {
 			continue
 		}
+		// Only an address a system VM launch allocated, so a wrong instance ID
+		// can never release a customer's own address.
+		if !awstags.IsSystemManaged(record.Tags[awstags.ManagedByKey]) {
+			slog.Warn("ReleaseAddressByInstanceID: address is not system-managed, left alone",
+				"instanceId", instanceID, "allocationId", record.AllocationId)
+			continue
+		}
 		// Key is "{accountID}.{allocID}"; the account scopes the release calls.
 		accountID, _, found := strings.Cut(k, ".")
 		if !found {

spinifex/domains/ec2/eip/service_impl_test.go

@@ -179,7 +179,12 @@ func TestEIP_ReleaseWhileAssociated(t *testing.T) {
 func TestEIP_ReleaseByInstanceID_ReclaimsAssociatedEIP(t *testing.T) {
 	svc, _, _ := setupTestEIP(t)
 
-	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{
+		TagSpecifications: []*ec2.TagSpecification{{
+			ResourceType: aws.String("elastic-ip"),
+			Tags:         []*ec2.Tag{{Key: aws.String("spinifex:managed-by"), Value: aws.String("elbv2")}},
+		}},
+	}, testAccountID)
 	require.NoError(t, err)
 	allocID := *out.AllocationId
 
@@ -861,3 +866,26 @@ func TestEIP_DisassociateByENI_ClearsEveryEIPOnTheInterface(t *testing.T) {
 	require.NoError(t, err)
 	assert.False(t, found)
 }
+
+func TestEIP_ReleaseByInstanceID_LeavesUntaggedAddress(t *testing.T) {
+	svc, _, _ := setupTestEIP(t)
+
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	require.NoError(t, err)
+	key := testAccountID + "." + *out.AllocationId
+	entry, err := svc.eipKV.Get(t.Context(), key)
+	require.NoError(t, err)
+	var record EIPRecord
+	require.NoError(t, json.Unmarshal(entry.Value(), &record))
+	record.State = "associated"
+	record.InstanceId = "i-customer"
+	data, err := json.Marshal(record)
+	require.NoError(t, err)
+	_, err = svc.eipKV.Update(t.Context(), key, data, entry.Revision())
+	require.NoError(t, err)
+
+	require.NoError(t, svc.ReleaseAddressByInstanceID("i-customer"))
+
+	_, err = svc.eipKV.Get(t.Context(), key)
+	assert.NoError(t, err, "an address no system launch allocated is never released by instance ID")
+}

spinifex/runtime/daemon/daemon_system_instance.go

@@ -245,7 +245,12 @@ func (d *Daemon) launchDirectSystemInstance(input *elbv2domain.SystemInstanceInp
 	publicIP := ""
 	if input.Scheme == elbv2domain.SchemeInternetFacing && d.vpcService != nil && instance.ENIId != "" {
 		if d.eipService != nil {
-			allocOut, allocErr := d.eipService.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, eniAccountID)
+			allocOut, allocErr := d.eipService.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{
+				TagSpecifications: []*ec2.TagSpecification{{
+					ResourceType: aws.String("elastic-ip"),
+					Tags:         []*ec2.Tag{{Key: aws.String(tags.ManagedByKey), Value: aws.String(managedBy)}},
+				}},
+			}, eniAccountID)
 			if allocErr != nil {
 				slog.Error("LaunchSystemInstance: EIP AllocateAddress failed", "instanceId", instance.ID, "err", allocErr)
 				d.cleanupFailedSystemInstance(instance, instanceType)
@@ -636,7 +641,10 @@ func (d *Daemon) attachSystemMgmtNIC(inst *vm.VM) error {
 // and the teardown deletes a still-attached ENI (InvalidNetworkInterface.InUse).
 func (d *Daemon) TerminateSystemInstance(instanceID string) error {
 	var termErr error
-	if _, exists := d.vmMgr.Get(instanceID); exists {
+	if inst, exists := d.vmMgr.Get(instanceID); exists {
+		if !isSystemVM(inst) {
+			return refuseNonSystemTerminate(instanceID)
+		}
 		termErr = d.terminateSystemInstanceLocal(instanceID)
 	} else {
 		termErr = d.terminateSystemInstanceRemote(instanceID)
@@ -663,12 +671,28 @@ func (d *Daemon) reclaimSystemInstanceEIP(instanceID string) {
 	}
 }
 
-// terminateSystemInstanceLocal stops a VM owned by this node.
+// isSystemVM reports whether inst is a platform-owned VM: a known system
+// manager launched it into the system account.
+func isSystemVM(inst *vm.VM) bool {
+	return tags.IsSystemManaged(inst.ManagedBy) && inst.AccountID == awsidentifiers.GlobalAccountID
+}
+
+// refuseNonSystemTerminate answers a system terminate aimed at a customer
+// instance as if the system instance were gone, so no caller can end one.
+func refuseNonSystemTerminate(instanceID string) error {
+	slog.Warn("TerminateSystemInstance: refused, not a system instance", "instanceId", instanceID)
+	return fmt.Errorf("%w: %s", systeminstance.ErrSystemInstanceNotFound, instanceID)
+}
+
+// terminateSystemInstanceLocal stops a system VM owned by this node.
 func (d *Daemon) terminateSystemInstanceLocal(instanceID string) error {
 	instance, exists := d.vmMgr.Get(instanceID)
 	if !exists {
 		return fmt.Errorf("%w: %s", systeminstance.ErrSystemInstanceNotFound, instanceID)
 	}
+	if !isSystemVM(instance) {
+		return refuseNonSystemTerminate(instanceID)
+	}
 
 	// Release EIP through the EIP service for system VMs whose public IP was
 	// allocated via AllocateAddress (internet-facing ALBs). releaseSystemInstanceEIP

spinifex/runtime/daemon/daemon_system_instance_coverage_test.go

@@ -731,7 +731,7 @@ func TestTerminateSystemInstance(t *testing.T) {
 	t.Run("local terminate releases the EIP", func(t *testing.T) {
 		eip := &sysInstReclaimingEIP{fakeEIPService: fakeEIPService{disassociateErr: errInjected}}
 		d.eipService = eip
-		inst := &vm.VM{ID: "i-term-ok", Status: vm.StateRunning, AccountID: testAccountID,
+		inst := &vm.VM{ID: "i-term-ok", Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID, ManagedBy: tags.ManagedByELBv2,
 			PublicIP: "203.0.113.7", PublicIPAllocID: "eipalloc-t", PublicIPAssocID: "eipassoc-t"}
 		d.vmMgr.Insert(inst)
 
@@ -746,7 +746,7 @@ func TestTerminateSystemInstance(t *testing.T) {
 	t.Run("local terminate failure", func(t *testing.T) {
 		d.eipService = nil
 		d.vmMgr.SetDeps(vm.Deps{NodeID: d.node, TransitionState: func(*vm.VM, vm.InstanceState) error { return errInjected }})
-		d.vmMgr.Insert(&vm.VM{ID: "i-term-fail", Status: vm.StateRunning})
+		d.vmMgr.Insert(&vm.VM{ID: "i-term-fail", Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID, ManagedBy: tags.ManagedByELBv2})
 
 		err := d.TerminateSystemInstance("i-term-fail")
 		require.Error(t, err)

spinifex/runtime/daemon/daemon_system_terminate_guard_test.go

@@ -0,0 +1,181 @@
+package daemon
+
+import (
+	"encoding/json"
+	"fmt"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	testutil "github.com/mulgadc/spinifex/internal/testkit"
+	ec2eip "github.com/mulgadc/spinifex/spinifex/domains/ec2/eip"
+	"github.com/mulgadc/spinifex/spinifex/domains/ec2/systeminstance"
+	ec2vpc "github.com/mulgadc/spinifex/spinifex/domains/ec2/vpc"
+	"github.com/mulgadc/spinifex/spinifex/domains/network/external"
+	awsidentifiers "github.com/mulgadc/spinifex/spinifex/foundation/aws/identifiers"
+	"github.com/mulgadc/spinifex/spinifex/foundation/aws/tags"
+	"github.com/mulgadc/spinifex/spinifex/runtime/compute/vm"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// countTerminations makes every state transition observable, so a test can prove
+// vmMgr.Terminate was never reached.
+func countTerminations(d *Daemon) *atomic.Int32 {
+	var n atomic.Int32
+	d.vmMgr.SetDeps(vm.Deps{NodeID: d.node, TransitionState: func(*vm.VM, vm.InstanceState) error {
+		n.Add(1)
+		return nil
+	}})
+	return &n
+}
+
+func TestS7_TerminateSystemInstanceRefusesCustomerInstance(t *testing.T) {
+	customers := map[string]*vm.VM{
+		"customer":                 {ID: "i-cust-plain", Status: vm.StateRunning, AccountID: testAccountID},
+		"system manager, customer": {ID: "i-cust-managed", Status: vm.StateRunning, AccountID: testAccountID, ManagedBy: tags.ManagedByEKS},
+		"system account, no owner": {ID: "i-sys-unmanaged", Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID},
+		"unknown manager":          {ID: "i-unknown-mgr", Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID, ManagedBy: "somebody"},
+	}
+
+	t.Run("local", func(t *testing.T) {
+		d := createTestDaemon(t, sharedNATSURL)
+		t.Cleanup(d.vmMgr.WaitForBackgroundWork)
+		terminated := countTerminations(d)
+		eip := &sysInstReclaimingEIP{}
+		d.eipService = eip
+		for name, inst := range customers {
+			d.vmMgr.Insert(inst)
+			err := d.terminateSystemInstanceLocal(inst.ID)
+			require.ErrorIs(t, err, systeminstance.ErrSystemInstanceNotFound, name)
+			err = d.TerminateSystemInstance(inst.ID)
+			require.ErrorIs(t, err, systeminstance.ErrSystemInstanceNotFound, name)
+			assert.Equal(t, vm.StateRunning, d.vmMgr.Status(inst), name)
+		}
+		assert.Zero(t, terminated.Load(), "vmMgr.Terminate is never reached for a customer instance")
+		assert.Empty(t, eip.reclaimed, "a refused terminate must not release the instance's address")
+	})
+
+	t.Run("routed", func(t *testing.T) {
+		d := createTestDaemon(t, sharedNATSURL)
+		t.Cleanup(d.vmMgr.WaitForBackgroundWork)
+		terminated := countTerminations(d)
+		for name, inst := range customers {
+			d.vmMgr.Insert(inst)
+			require.NoError(t, d.subscribeSystemTerminate(inst.ID))
+			reply, err := d.natsConn.Request("system.TerminateInstance."+inst.ID, nil, 5*time.Second)
+			require.NoError(t, err, name)
+			var env systemInstanceTerminateEnvelope
+			require.NoError(t, json.Unmarshal(reply.Data, &env))
+			assert.Contains(t, env.Error, systeminstance.ErrSystemInstanceNotFound.Error(), name)
+			assert.Equal(t, vm.StateRunning, d.vmMgr.Status(inst), name)
+		}
+		assert.Zero(t, terminated.Load(), "vmMgr.Terminate is never reached for a customer instance")
+	})
+}
+
+func TestS7_TerminateSystemInstanceStillTerminatesEverySystemKind(t *testing.T) {
+	for _, managedBy := range []string{tags.ManagedByELBv2, tags.ManagedByEKS, tags.ManagedByRDS, tags.ManagedByBedrock, tags.ManagedBySQS} {
+		t.Run(managedBy, func(t *testing.T) {
+			d := createTestDaemon(t, sharedNATSURL)
+			t.Cleanup(d.vmMgr.WaitForBackgroundWork)
+			local := &vm.VM{ID: "i-sys-local-" + managedBy, Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID, ManagedBy: managedBy}
+			d.vmMgr.Insert(local)
+			require.NoError(t, d.TerminateSystemInstance(local.ID))
+			assert.Equal(t, vm.StateTerminated, d.vmMgr.Status(local))
+
+			routed := &vm.VM{ID: "i-sys-routed-" + managedBy, Status: vm.StateRunning, AccountID: awsidentifiers.GlobalAccountID, ManagedBy: managedBy}
+			d.vmMgr.Insert(routed)
+			require.NoError(t, d.subscribeSystemTerminate(routed.ID))
+			reply, err := d.natsConn.Request("system.TerminateInstance."+routed.ID, nil, 5*time.Second)
+			require.NoError(t, err)
+			var env systemInstanceTerminateEnvelope
+			require.NoError(t, json.Unmarshal(reply.Data, &env))
+			assert.Empty(t, env.Error)
+			assert.Equal(t, vm.StateTerminated, d.vmMgr.Status(routed))
+		})
+	}
+}
+
+// bindRealEIP gives d a real EIP service and allocates an address, optionally
+// tagged as system-managed, then records instanceID against it.
+func bindRealEIP(t *testing.T, d *Daemon, instanceID, managedBy string) (allocID, publicIP string, svc *ec2eip.EIPServiceImpl) {
+	t.Helper()
+	_, nc, js := testutil.StartTestJetStream(t)
+	ipam, err := ec2vpc.NewExternalIPAM(t.Context(), js, []external.ExternalPoolConfig{
+		{Name: "wan-test", RangeStart: "203.0.113.10", RangeEnd: "203.0.113.20", Gateway: "203.0.113.1", PrefixLen: 24},
+	})
+	require.NoError(t, err)
+	d.externalIPAM = ipam
+	svc, err = ec2eip.NewEIPServiceImpl(t.Context(), nc, ipam, d.vpcService)
+	require.NoError(t, err)
+	d.eipService = svc
+
+	in := &ec2.AllocateAddressInput{}
+	if managedBy != "" {
+		in.TagSpecifications = []*ec2.TagSpecification{{
+			ResourceType: aws.String("elastic-ip"),
+			Tags:         []*ec2.Tag{{Key: aws.String(tags.ManagedByKey), Value: aws.String(managedBy)}},
+		}}
+	}
+	out, err := svc.AllocateAddress(t.Context(), in, testAccountID)
+	require.NoError(t, err)
+
+	kv, err := js.KeyValue(t.Context(), ec2eip.KVBucketEIPs)
+	require.NoError(t, err)
+	key := testAccountID + "." + aws.StringValue(out.AllocationId)
+	entry, err := kv.Get(t.Context(), key)
+	require.NoError(t, err)
+	var rec ec2eip.EIPRecord
+	require.NoError(t, json.Unmarshal(entry.Value(), &rec))
+	rec.InstanceId = instanceID
+	data, err := json.Marshal(rec)
+	require.NoError(t, err)
+	_, err = kv.Update(t.Context(), key, data, entry.Revision())
+	require.NoError(t, err)
+	return aws.StringValue(out.AllocationId), aws.StringValue(out.PublicIp), svc
+}
+
+func addressKept(t *testing.T, svc *ec2eip.EIPServiceImpl, allocID string) bool {
+	t.Helper()
+	out, err := svc.DescribeAddresses(t.Context(), &ec2.DescribeAddressesInput{AllocationIds: []*string{aws.String(allocID)}}, testAccountID)
+	return err == nil && len(out.Addresses) == 1
+}
+
+func TestS7_TerminateSystemInstanceNeverReleasesCustomerEIP(t *testing.T) {
+	t.Run("remote owner refuses", func(t *testing.T) {
+		owner := createTestDaemon(t, sharedNATSURL)
+		t.Cleanup(owner.vmMgr.WaitForBackgroundWork)
+		cust := &vm.VM{ID: "i-cust-remote-eip", Status: vm.StateRunning, AccountID: testAccountID}
+		owner.vmMgr.Insert(cust)
+		require.NoError(t, owner.subscribeSystemTerminate(cust.ID))
+
+		caller := createVPCTestDaemon(t)
+		allocID, _, svc := bindRealEIP(t, caller, cust.ID, "")
+
+		err := caller.TerminateSystemInstance(cust.ID)
+		require.ErrorIs(t, err, systeminstance.ErrSystemInstanceNotFound)
+		assert.Equal(t, vm.StateRunning, owner.vmMgr.Status(cust))
+		assert.True(t, addressKept(t, svc, allocID), "a customer address survives a refused terminate")
+	})
+
+	t.Run("no owner, stopped customer instance", func(t *testing.T) {
+		d := createVPCTestDaemon(t)
+		id := fmt.Sprintf("i-cust-stopped-%d", time.Now().UnixNano())
+		allocID, _, svc := bindRealEIP(t, d, id, "")
+
+		_ = d.TerminateSystemInstance(id)
+		assert.True(t, addressKept(t, svc, allocID), "a customer address survives a wrong system instance ID")
+	})
+
+	t.Run("system address is still released on the no-owner path", func(t *testing.T) {
+		d := createVPCTestDaemon(t)
+		id := fmt.Sprintf("i-alb-gone-%d", time.Now().UnixNano())
+		allocID, _, svc := bindRealEIP(t, d, id, tags.ManagedByELBv2)
+
+		_ = d.TerminateSystemInstance(id)
+		assert.False(t, addressKept(t, svc, allocID), "an internet-facing ALB address is released when no node owns the VM")
+	})
+}