fix: fail a public-IP launch closed when a node has no external IPAM

A node whose NewExternalIPAM call lost the race with the JetStream meta leader logs a WARN and carries on with d.externalIPAM nil. That nil concrete pointer was handed straight to the PublicIPAllocator interface, where it reads as non-nil, so the guard in PrepareRunInstances passed and findPool derefed a nil receiver. RunInstances into a subnet with MapPublicIpOnLaunch took the daemon down with SIGSEGV, and because the ENI was already persisted and the gateway saw only a NATS timeout, nothing rolled it back: the orphan ENI later rejected DeleteSecurityGroup with DependencyViolation. The concrete value now crosses into PublicIPAllocator and PublicIPReleaser only once init succeeded, and AllocateIP/ReleaseIP tolerate a nil receiver so another caller repeating the mistake degrades instead of panicking. Making the interface genuinely nil is not enough on its own: the existing nil check would then have skipped assignment altogether and booted an instance with no public address on a subnet that promised one. The wantPublic decision now runs unconditionally and a missing allocator takes the allocation-failure path, which detaches and deletes the ENI, returns capacity and reports InsufficientAddressCapacity. Launches that do not ask for a public IP are untouched: subnets default to MapPublicIpOnLaunch only when the cluster has public pools, which is the same condition that initialises the IPAM.
2026-10-20 11:21:49 +00:00 · 2026-08-19 11:16:41 +10:00
parent bdc79108e9
commit 4672541b38
4 changed files with 150 additions and 68 deletions
@@ -1692,8 +1692,20 @@ func (d *Daemon) startCluster() error {
 		slog.Info("EIP service disabled — no external IPAM; serving empty/unsupported responses")
 	}

-	d.instanceService.SetTerminationDeps(d.volumeService, d.vpcService, d.externalIPAM, d.tagsService)
-	d.instanceService.SetRunInstancesDeps(d.imageService, d.keyService, &daemonENICreator{d: d}, d.externalIPAM)
+	// A nil *ExternalIPAM stored in an interface is not a nil interface, so hand
+	// the concrete value over only once init succeeded — otherwise the service's
+	// own nil checks pass and the allocate path derefs a nil receiver.
+	var (
+		ipAllocator handlers_ec2_instance.PublicIPAllocator
+		ipReleaser  handlers_ec2_instance.PublicIPReleaser
+	)
+	if d.externalIPAM != nil {
+		ipAllocator = d.externalIPAM
+		ipReleaser = d.externalIPAM
+	}
+
+	d.instanceService.SetTerminationDeps(d.volumeService, d.vpcService, ipReleaser, d.tagsService)
+	d.instanceService.SetRunInstancesDeps(d.imageService, d.keyService, &daemonENICreator{d: d}, ipAllocator)

 	if d.gpuManager != nil {
 		d.instanceService.SetGPUClaimer(&daemonGPUClaimer{d: d})
@@ -716,74 +716,69 @@ func (s *InstanceServiceImpl) PrepareRunInstances(ctx context.Context, input *ec
 				"mac", instance.ENIMac,
 			)

-			if s.ipAllocator != nil {
-				subnet, subErr := s.eniCreator.GetSubnet(ctx, accountID, *input.SubnetId)
-				wantPublic := subErr == nil && subnet != nil && subnet.MapPublicIpOnLaunch
-				if len(input.NetworkInterfaces) > 0 && input.NetworkInterfaces[0] != nil && input.NetworkInterfaces[0].AssociatePublicIpAddress != nil {
-					wantPublic = *input.NetworkInterfaces[0].AssociatePublicIpAddress
-				}
-				if wantPublic {
-					region := s.config.Region
-					az := s.config.AZ
-					publicIP, poolName, allocErr := s.ipAllocator.AllocateIP(ctx, region, az, handlers_ec2_vpc.PurposeENIPublic, "", *eni.NetworkInterfaceId, instance.ID)
-					if allocErr != nil {
-						// Fail rather than boot an unreachable instance;
-						// detach before delete since in-use ENIs reject deletion.
-						slog.ErrorContext(ctx, "PrepareRunInstances: public IP allocation failed — aborting launch",
-							"instanceId", instance.ID, "eniId", *eni.NetworkInterfaceId, "err", allocErr)
-						if detErr := s.eniCreator.DetachENI(ctx, accountID, *eni.NetworkInterfaceId); detErr != nil {
-							slog.WarnContext(ctx, "PrepareRunInstances: failed to detach ENI after public-IP allocation failure",
-								"eniId", *eni.NetworkInterfaceId, "err", detErr)
-						}
-						if _, delErr := s.eniDeleter.DeleteNetworkInterface(ctx, &ec2.DeleteNetworkInterfaceInput{
-							NetworkInterfaceId: eni.NetworkInterfaceId,
-						}, accountID); delErr != nil {
-							slog.WarnContext(ctx, "PrepareRunInstances: failed to delete ENI after public-IP allocation failure",
-								"eniId", *eni.NetworkInterfaceId, "err", delErr)
-						}
-						// Carry the allocator's own error rather than a flat
-						// capacity code: the gateway resolves the AWS code out
-						// of the wrap chain, so a genuinely exhausted pool still
-						// surfaces InsufficientAddressCapacity while an upstream
-						// DHCP or IPAM fault reports as itself instead of
-						// wearing a capacity code it did not earn.
-						lastRunErr = fmt.Errorf("allocate public IP for %s: %w", instance.ID, allocErr)
-						if reservationID == "" {
-							s.resourceMgr.Deallocate(instanceType)
-						} else {
-							s.resourceMgr.ReleaseToReservation(reservationID, instanceType)
-						}
-						continue
-					} else {
-						if updateErr := s.eniCreator.UpdateENIPublicIP(ctx, accountID, *eni.NetworkInterfaceId, publicIP, poolName); updateErr != nil {
-							slog.WarnContext(ctx, "PrepareRunInstances: failed to update ENI with public IP", "eniId", *eni.NetworkInterfaceId, "err", updateErr)
-						}
-						portName := topology.Port(*eni.NetworkInterfaceId)
-						if natErr := utils.AddNAT(s.natsConn, *eni.VpcId, publicIP, *eni.PrivateIpAddress, portName, *eni.MacAddress); natErr != nil {
-							slog.ErrorContext(ctx, "PrepareRunInstances: vpc.add-nat failed — rolling back public IP to avoid surfacing an unreachable address",
-								"instanceId", instance.ID, "publicIp", publicIP, "pool", poolName, "err", natErr)
-							// Neutralise before releasing in case timeout committed the rule.
-							utils.PublishNATEvent(s.natsConn, "vpc.delete-nat", *eni.VpcId, publicIP, *eni.PrivateIpAddress, portName, *eni.MacAddress)
-							s.rollbackAutoAssignedPublicIP(ctx, accountID, instance.ID, *eni.NetworkInterfaceId, publicIP, poolName)
-							lastRunErr = natErr
-							if reservationID == "" {
-								s.resourceMgr.Deallocate(instanceType)
-							} else {
-								s.resourceMgr.ReleaseToReservation(reservationID, instanceType)
-							}
-							continue
-						}
-						ec2Instance.PublicIpAddress = aws.String(publicIP)
-						instance.PublicIP = publicIP
-						instance.PublicIPPool = poolName
-						slog.InfoContext(ctx, "PrepareRunInstances: auto-assigned public IP",
-							"instanceId", instance.ID,
-							"publicIp", publicIP,
-							"privateIp", *eni.PrivateIpAddress,
-							"pool", poolName,
-						)
+			subnet, subErr := s.eniCreator.GetSubnet(ctx, accountID, *input.SubnetId)
+			wantPublic := subErr == nil && subnet != nil && subnet.MapPublicIpOnLaunch
+			if len(input.NetworkInterfaces) > 0 && input.NetworkInterfaces[0] != nil && input.NetworkInterfaces[0].AssociatePublicIpAddress != nil {
+				wantPublic = *input.NetworkInterfaces[0].AssociatePublicIpAddress
+			}
+			if wantPublic {
+				publicIP, poolName, allocErr := s.allocatePublicIP(ctx, *eni.NetworkInterfaceId, instance.ID)
+				if allocErr != nil {
+					// Fail rather than boot an unreachable instance;
+					// detach before delete since in-use ENIs reject deletion.
+					slog.ErrorContext(ctx, "PrepareRunInstances: public IP allocation failed — aborting launch",
+						"instanceId", instance.ID, "eniId", *eni.NetworkInterfaceId, "err", allocErr)
+					if detErr := s.eniCreator.DetachENI(ctx, accountID, *eni.NetworkInterfaceId); detErr != nil {
+						slog.WarnContext(ctx, "PrepareRunInstances: failed to detach ENI after public-IP allocation failure",
+							"eniId", *eni.NetworkInterfaceId, "err", detErr)
 					}
+					if _, delErr := s.eniDeleter.DeleteNetworkInterface(ctx, &ec2.DeleteNetworkInterfaceInput{
+						NetworkInterfaceId: eni.NetworkInterfaceId,
+					}, accountID); delErr != nil {
+						slog.WarnContext(ctx, "PrepareRunInstances: failed to delete ENI after public-IP allocation failure",
+							"eniId", *eni.NetworkInterfaceId, "err", delErr)
+					}
+					// Carry the allocator's own error rather than a flat
+					// capacity code: the gateway resolves the AWS code out
+					// of the wrap chain, so a genuinely exhausted pool still
+					// surfaces InsufficientAddressCapacity while an upstream
+					// DHCP or IPAM fault reports as itself instead of
+					// wearing a capacity code it did not earn.
+					lastRunErr = fmt.Errorf("allocate public IP for %s: %w", instance.ID, allocErr)
+					if reservationID == "" {
+						s.resourceMgr.Deallocate(instanceType)
+					} else {
+						s.resourceMgr.ReleaseToReservation(reservationID, instanceType)
+					}
+					continue
 				}
+				if updateErr := s.eniCreator.UpdateENIPublicIP(ctx, accountID, *eni.NetworkInterfaceId, publicIP, poolName); updateErr != nil {
+					slog.WarnContext(ctx, "PrepareRunInstances: failed to update ENI with public IP", "eniId", *eni.NetworkInterfaceId, "err", updateErr)
+				}
+				portName := topology.Port(*eni.NetworkInterfaceId)
+				if natErr := utils.AddNAT(s.natsConn, *eni.VpcId, publicIP, *eni.PrivateIpAddress, portName, *eni.MacAddress); natErr != nil {
+					slog.ErrorContext(ctx, "PrepareRunInstances: vpc.add-nat failed — rolling back public IP to avoid surfacing an unreachable address",
+						"instanceId", instance.ID, "publicIp", publicIP, "pool", poolName, "err", natErr)
+					// Neutralise before releasing in case timeout committed the rule.
+					utils.PublishNATEvent(s.natsConn, "vpc.delete-nat", *eni.VpcId, publicIP, *eni.PrivateIpAddress, portName, *eni.MacAddress)
+					s.rollbackAutoAssignedPublicIP(ctx, accountID, instance.ID, *eni.NetworkInterfaceId, publicIP, poolName)
+					lastRunErr = natErr
+					if reservationID == "" {
+						s.resourceMgr.Deallocate(instanceType)
+					} else {
+						s.resourceMgr.ReleaseToReservation(reservationID, instanceType)
+					}
+					continue
+				}
+				ec2Instance.PublicIpAddress = aws.String(publicIP)
+				instance.PublicIP = publicIP
+				instance.PublicIPPool = poolName
+				slog.InfoContext(ctx, "PrepareRunInstances: auto-assigned public IP",
+					"instanceId", instance.ID,
+					"publicIp", publicIP,
+					"privateIp", *eni.PrivateIpAddress,
+					"pool", poolName,
+				)
 			}
 		}

@@ -2508,6 +2503,22 @@ func (s *InstanceServiceImpl) deleteInstanceVolumes(ctx context.Context, instanc
 	_ = instanceID
 }

+// allocatePublicIP allocates an auto-assigned public IP for a new ENI. A node
+// whose external IPAM never initialised has no allocator wired: report no
+// capacity so the caller aborts rather than booting an unreachable instance.
+func (s *InstanceServiceImpl) allocatePublicIP(ctx context.Context, eniID, instanceID string) (string, string, error) {
+	if s.ipAllocator == nil {
+		return "", "", fmt.Errorf("no public IP allocator on this node: %w", errors.New(awserrors.ErrorInsufficientAddressCapacity))
+	}
+	region := ""
+	az := ""
+	if s.config != nil {
+		region = s.config.Region
+		az = s.config.AZ
+	}
+	return s.ipAllocator.AllocateIP(ctx, region, az, handlers_ec2_vpc.PurposeENIPublic, "", eniID, instanceID)
+}
+
 // rollbackAutoAssignedPublicIP unwinds a failed auto-assign: clears the ENI
 // public IP record, releases the IPAM lease, then detaches and deletes the ENI.
 func (s *InstanceServiceImpl) rollbackAutoAssignedPublicIP(ctx context.Context, accountID, instanceID, eniID, publicIP, poolName string) {
@@ -16,6 +16,7 @@ import (
 	"github.com/mulgadc/spinifex/spinifex/config"
 	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
 	"github.com/mulgadc/spinifex/spinifex/gpu"
+	handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc"
 	"github.com/mulgadc/spinifex/spinifex/tags"
 	"github.com/mulgadc/spinifex/spinifex/testutil"
 	spxtypes "github.com/mulgadc/spinifex/spinifex/types"
@@ -3165,6 +3166,55 @@ func TestPrepareRunInstances_PublicIPAutoAssigned(t *testing.T) {
 	}
 }

+// TestPrepareRunInstances_PublicIPWithoutAllocator covers a node whose external
+// IPAM never initialised. A nil *ExternalIPAM in the interface reads as non-nil,
+// so the launch used to deref a nil receiver and take the daemon down with the
+// ENI already persisted; both wirings must now fail closed and unwind the ENI.
+func TestPrepareRunInstances_PublicIPWithoutAllocator(t *testing.T) {
+	cases := []struct {
+		name string
+		ipam PublicIPAllocator
+	}{
+		{name: "typed_nil_external_ipam", ipam: (*handlers_ec2_vpc.ExternalIPAM)(nil)},
+		{name: "no_allocator_wired", ipam: nil},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			eni := &fakeENICreator{
+				subnet: &SubnetInfo{SubnetID: "subnet-1", VpcID: "vpc-1", MapPublicIpOnLaunch: true},
+				createOut: &ec2.CreateNetworkInterfaceOutput{
+					NetworkInterface: &ec2.NetworkInterface{
+						NetworkInterfaceId: aws.String("eni-no-ipam"),
+						MacAddress:         aws.String("aa:bb:cc:dd:ee:11"),
+						PrivateIpAddress:   aws.String("10.0.0.40"),
+						VpcId:              aws.String("vpc-1"),
+					},
+				},
+			}
+			deleter := &fakeENIDeleter{}
+			svc, prov := prepareSvcWithENI(t, eni, nil)
+			svc.eniDeleter = deleter
+			svc.SetRunInstancesDeps(svc.amiLoader, nil, eni, tc.ipam)
+
+			_, instances, _, err := svc.PrepareRunInstances(context.Background(), &ec2.RunInstancesInput{
+				InstanceType: aws.String("t3.micro"),
+				ImageId:      aws.String("ami-1"),
+				SubnetId:     aws.String("subnet-1"),
+				MinCount:     aws.Int64(1),
+				MaxCount:     aws.Int64(1),
+			}, "acc", "")
+
+			require.Error(t, err, "a public-IP launch with no allocator must fail, not boot an instance with no public address")
+			assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, err.Error())
+			assert.Empty(t, instances)
+			assert.Equal(t, 1, eni.detachCalls, "ENI must be detached before delete")
+			assert.Equal(t, []string{"eni-no-ipam"}, deleter.calls,
+				"the auto-created ENI must be deleted, otherwise it strands and blocks security group deletion")
+			assert.Len(t, prov.deallocated, 1, "capacity must be returned when the launch aborts")
+		})
+	}
+}
+
 // TestPrepareRunInstances_NATFailureRollsBackPublicIP verifies that a vpc.add-nat
 // failure drops the instance, clears the ENI public IP, releases the IPAM
 // lease, and deallocates capacity.
@@ -79,7 +79,12 @@ func (m *ExternalIPAM) EnableDHCP(client *dhcp.NATSClient) error {

 // AllocateIP allocates the next available external IP from the best pool
 // matching the given region/AZ. Returns the allocated IP and pool name.
+// Nil-receiver tolerant: a node whose IPAM never initialised reports no
+// capacity rather than panicking through a typed-nil interface.
 func (m *ExternalIPAM) AllocateIP(ctx context.Context, region, az, purpose, allocID, eniID, instanceID string) (string, string, error) {
+	if m == nil {
+		return "", "", fmt.Errorf("external IPAM unavailable on this node: %w", errors.New(awserrors.ErrorInsufficientAddressCapacity))
+	}
 	pool := m.findPool(region, az)
 	if pool == nil {
 		return "", "", fmt.Errorf("no external pool available for region=%q az=%q: %w", region, az, errors.New(awserrors.ErrorInsufficientAddressCapacity))
@@ -113,7 +118,11 @@ func (m *ExternalIPAM) AllocateFromPool(ctx context.Context, poolName, purpose,
 // ReleaseIP releases a previously allocated external IP back to its pool.
 // ownerENIID, when non-empty, scopes the release to the ENI that currently owns
 // the lease so a stale or duplicated teardown for a recycled IP is a no-op.
+// Nil-receiver tolerant for the same reason as AllocateIP.
 func (m *ExternalIPAM) ReleaseIP(ctx context.Context, poolName, ip, ownerENIID string) error {
+	if m == nil {
+		return errors.New("external IPAM unavailable on this node")
+	}
 	addr, err := netip.ParseAddr(ip)
 	if err != nil {
 		return fmt.Errorf("parse release IP %q: %w", ip, err)