diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index e7e9d46da..b6cd62bc2 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -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}) diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index de32a33d9..e1a1f6c5f 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -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) { diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index a2f2ffa4f..6e4266165 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -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. diff --git a/spinifex/handlers/ec2/vpc/external_ipam.go b/spinifex/handlers/ec2/vpc/external_ipam.go index b15b0a4b8..bececbc3c 100644 --- a/spinifex/handlers/ec2/vpc/external_ipam.go +++ b/spinifex/handlers/ec2/vpc/external_ipam.go @@ -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)