From e8bd99386ea53f82daaf457c8c0535d9c970adce Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Thu, 11 Jun 2026 09:47:40 +1000 Subject: [PATCH] fix(ec2): fail RunInstances when public-IP auto-assign fails (mulga-siv-275) (#345) * fix(ec2): fail RunInstances when public-IP auto-assign fails PrepareRunInstances swallowed an AllocateIP failure for a MapPublicIpOnLaunch subnet (logged WARN, fell through), so the instance booted running with no public IP. With no qemu-hostfwd fallback such an instance is unreachable. Mirror the adjacent AddNAT-failure path: detach and delete the auto-created ENI, deallocate capacity, drop the instance, and surface InsufficientAddressCapacity. * test(daemon): deterministically pick a fitting 2-vCPU type in capacity filter test CapacityFilterRespectsAllocation selected the first 2-vCPU type by Go map-iteration order, intermittently picking a type near the schedulable-memory boundary the allocator then rejected (~3/5 failures in isolation on small hosts). Select the smallest-memory 2-vCPU type from the capacity-aware set so it always fits. --- spinifex/daemon/daemon_test.go | 19 ++++---- .../handlers/ec2/instance/service_impl.go | 22 ++++++++- .../ec2/instance/service_impl_test.go | 45 +++++++++++++++++++ 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/spinifex/daemon/daemon_test.go b/spinifex/daemon/daemon_test.go index b41ad7670..64d9d1a78 100644 --- a/spinifex/daemon/daemon_test.go +++ b/spinifex/daemon/daemon_test.go @@ -539,16 +539,19 @@ func TestHandleEC2DescribeInstanceTypes(t *testing.T) { capMsgData, err := json.Marshal(capInput) require.NoError(t, err) - // Pick a 2-vCPU type from the capacity-aware set so we know the - // host has room for it — the raw instanceTypes map contains - // entries (e.g. r5.large at 16 GiB) that exceed schedulable - // memory on small test hosts. + // Pick the smallest-memory 2-vCPU type from the capacity-aware set + // so it is guaranteed to fit the host. Selecting the first match by + // map-iteration order is non-deterministic and intermittently picks a + // type near the schedulable-memory boundary that the allocator then + // rejects, flaking the test on small hosts. var instanceType2CPU *ec2.InstanceTypeInfo - for _, it := range daemon.resourceMgr.GetAvailableInstanceTypeInfos(false) { - if it.VCpuInfo != nil && it.VCpuInfo.DefaultVCpus != nil && *it.VCpuInfo.DefaultVCpus == 2 && - it.MemoryInfo != nil && it.MemoryInfo.SizeInMiB != nil { + for _, it := range daemon.resourceMgr.GetAvailableInstanceTypeInfos(true) { + if it.VCpuInfo == nil || it.VCpuInfo.DefaultVCpus == nil || *it.VCpuInfo.DefaultVCpus != 2 || + it.MemoryInfo == nil || it.MemoryInfo.SizeInMiB == nil { + continue + } + if instanceType2CPU == nil || *it.MemoryInfo.SizeInMiB < *instanceType2CPU.MemoryInfo.SizeInMiB { instanceType2CPU = it - break } } require.NotNil(t, instanceType2CPU, "Should find a 2-vCPU type that fits the host") diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 548e8d6a1..3f31e4814 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -813,7 +813,27 @@ func (s *InstanceServiceImpl) PrepareRunInstances(input *ec2.RunInstancesInput, az := s.config.AZ publicIP, poolName, allocErr := s.ipAllocator.AllocateIP(region, az, handlers_ec2_vpc.PurposeENIPublic, "", *eni.NetworkInterfaceId, instance.ID) if allocErr != nil { - slog.Warn("PrepareRunInstances: failed to allocate public IP", "instanceId", instance.ID, "err", allocErr) + // No qemu-hostfwd fallback exists: an instance on a + // MapPublicIpOnLaunch subnet with no public IP is + // unreachable, so fail the launch instead of booting a + // running-but-unreachable instance. The ENI is attached + // with a primary IP — detach before delete (in-use ENIs + // reject deletion). + slog.Error("PrepareRunInstances: public IP allocation failed — aborting launch", + "instanceId", instance.ID, "eniId", *eni.NetworkInterfaceId, "err", allocErr) + if detErr := s.eniCreator.DetachENI(accountID, *eni.NetworkInterfaceId); detErr != nil { + slog.Warn("PrepareRunInstances: failed to detach ENI after public-IP allocation failure", + "eniId", *eni.NetworkInterfaceId, "err", detErr) + } + if _, delErr := s.eniDeleter.DeleteNetworkInterface(&ec2.DeleteNetworkInterfaceInput{ + NetworkInterfaceId: eni.NetworkInterfaceId, + }, accountID); delErr != nil { + slog.Warn("PrepareRunInstances: failed to delete ENI after public-IP allocation failure", + "eniId", *eni.NetworkInterfaceId, "err", delErr) + } + lastRunErr = errors.New(awserrors.ErrorInsufficientAddressCapacity) + s.resourceMgr.Deallocate(instanceType) + continue } else { if updateErr := s.eniCreator.UpdateENIPublicIP(accountID, *eni.NetworkInterfaceId, publicIP, poolName); updateErr != nil { slog.Warn("PrepareRunInstances: failed to update ENI with public IP", "eniId", *eni.NetworkInterfaceId, "err", updateErr) diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index aeb2df4ee..8bc15687b 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -2824,6 +2824,51 @@ func TestPrepareRunInstances_NATFailureRollsBackPublicIP(t *testing.T) { } } +// TestPrepareRunInstances_PublicIPAllocFailureAbortsLaunch regresses the +// silent failure where a failed public-IP allocation (pool exhausted) was +// logged at WARN and ignored, so the instance booted running with no public +// IP. There is no qemu-hostfwd fallback, so such an instance is unreachable. +// The fix must fail the launch: detach + delete the auto-created ENI, +// deallocate capacity, drop the instance, and surface InsufficientAddressCapacity. +func TestPrepareRunInstances_PublicIPAllocFailureAbortsLaunch(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-pubip-fail"), + MacAddress: aws.String("aa:bb:cc:dd:ee:02"), + PrivateIpAddress: aws.String("10.0.0.50"), + VpcId: aws.String("vpc-1"), + }, + }, + } + ipam := &fakeIPAllocator{err: errors.New("InsufficientAddressCapacity: pool wan exhausted")} + deleter := &fakeENIDeleter{} + svc, prov := prepareSvcWithENI(t, eni, ipam) + svc.eniDeleter = deleter + + _, instances, _, err := svc.PrepareRunInstances(&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) + assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, err.Error(), + "pool exhaustion must surface as InsufficientAddressCapacity, not a silent IP-less boot") + assert.Empty(t, instances, "instance with no public IP must not be returned") + + // The ENI is attached with a primary IP; rollback must detach before + // delete (in-use ENIs reject deletion) so no eniKV record leaks. + assert.Equal(t, 1, eni.detachCalls, "rollback must detach the ENI before delete") + assert.Equal(t, []string{"eni-pubip-fail"}, deleter.calls, "rollback must delete the auto-created ENI") + assert.Equal(t, 0, eni.updateCalls, "no public IP was allocated, so the ENI must never be updated with one") + + require.Len(t, prov.deallocated, 1, "public-IP allocation failure must trigger Deallocate") +} + // natWirePayload mirrors utils.natEvent for test-side decoding. type natWirePayload struct { VpcId string `json:"vpc_id"`