diff --git a/spinifex/handlers/elbv2/service_impl.go b/spinifex/handlers/elbv2/service_impl.go index 3463b5f44..f06a54087 100644 --- a/spinifex/handlers/elbv2/service_impl.go +++ b/spinifex/handlers/elbv2/service_impl.go @@ -1315,11 +1315,22 @@ func (s *ELBv2ServiceImpl) createLoadBalancer(ctx context.Context, input *elbv2. if syncLaunch { // Drive the data-plane launch inline so the returned record carries the // allocated front-end IP. The caller is off the gateway responder, so - // 267.4 does not apply. provisionLBDataPlane leaves the record marked - // failed on launch failure for diagnosis. + // 267.4 does not apply. if err := s.provisionLBDataPlane(ctx, lc); err != nil { slog.ErrorContext(ctx, "CreateLoadBalancerSync: data-plane launch failed", "lbArn", lbArn, "err", err) - return nil, errors.New(awserrors.ErrorServerInternal) + // Unwind completely rather than leaving the failed record behind. + // A synchronous caller gets an error and no ARN, and this path's + // LBs live in the system account (the EKS managed CP VPC), so the + // record is unreachable to the cluster owner — it cannot be + // diagnosed, only leaked. The managed SG is the costly part: it + // pins its VPC against DeleteVpc with DependencyViolation, which + // no retry can clear. The async path keeps its failed record on + // purpose; the caller holds the ARN there and can reclaim it. + s.rollbackLBInfra(ctx, eniIDs, nlbManagedSGID, name, accountID) + if delErr := s.store.DeleteLoadBalancer(lbID); delErr != nil { + slog.ErrorContext(ctx, "CreateLoadBalancerSync: rollback failed to delete record", "lbId", lbID, "err", delErr) + } + return nil, lbLaunchError(err) } launched, lerr := s.store.GetLoadBalancerByArn(lbArn) if lerr != nil || launched == nil { @@ -1456,6 +1467,31 @@ func (s *ELBv2ServiceImpl) launchLBVMAsync(ctx context.Context, lc lbLaunchCtx) } } +// lbLaunchPassthroughCodes are the AWS error codes a data-plane launch failure may +// carry that name a real, actionable cause. Capacity shortfalls are the ones a caller +// can do something about (retry, free an address, pick another type); anything else is +// a defect on our side and stays ServerInternal rather than inventing a client error. +var lbLaunchPassthroughCodes = []string{ + awserrors.ErrorInsufficientAddressCapacity, + awserrors.ErrorInsufficientInstanceCapacity, + awserrors.ErrorInsufficientCapacity, + awserrors.ErrorInsufficientCapacityOnHost, +} + +// lbLaunchError maps a data-plane launch failure onto the error code that caused it, +// so the caller sees why the create failed. Flattening every launch failure to +// ServerInternal discards the one fact that makes the failure diagnosable: an address +// exhaustion reported as an internal error looks like a bug in the gateway rather than +// a capacity problem in the VPC. +func lbLaunchError(err error) error { + for _, code := range lbLaunchPassthroughCodes { + if awserrors.IsErrorCode(err, code) { + return errors.New(code) + } + } + return errors.New(awserrors.ErrorServerInternal) +} + // rollbackLBInfra tears down ENIs, the NLB managed SG, and the name claim created // by a partial CreateLoadBalancer. Each step is best-effort and idempotent. func (s *ELBv2ServiceImpl) rollbackLBInfra(ctx context.Context, eniIDs []string, nlbManagedSGID, name, accountID string) { diff --git a/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go b/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go index 3322b48a7..b80aa30d4 100644 --- a/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go +++ b/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go @@ -2,11 +2,14 @@ package handlers_elbv2 import ( "context" + "errors" + "strings" "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/mulgadc/spinifex/spinifex/awserrors" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -63,6 +66,31 @@ func managedENI(t *testing.T, vpcSvc *handlers_ec2_vpc.VPCServiceImpl) *ec2.Netw return nil } +// hasManagedNLBSG reports whether any NLB managed SG (named spinifex-nlb-) +// is still present, so a rollback can be asserted to have removed it. +func hasManagedNLBSG(t *testing.T, vpcSvc *handlers_ec2_vpc.VPCServiceImpl) bool { + t.Helper() + out, err := vpcSvc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{}, testAccountID) + require.NoError(t, err) + for _, sg := range out.SecurityGroups { + if strings.HasPrefix(aws.StringValue(sg.GroupName), "spinifex-nlb-") { + return true + } + } + return false +} + +// nlbSyncInput builds an internet-facing network LB create input for the +// synchronous-launch failure tests. +func nlbSyncInput(name, subnetID string) *elbv2.CreateLoadBalancerInput { + return &elbv2.CreateLoadBalancerInput{ + Name: aws.String(name), + Type: aws.String("network"), + Scheme: aws.String(elbv2.LoadBalancerSchemeEnumInternetFacing), + Subnets: []*string{aws.String(subnetID)}, + } +} + func createNLB(t *testing.T, svc *ELBv2ServiceImpl, name, scheme, subnetID string) *elbv2.LoadBalancer { t.Helper() in := &elbv2.CreateLoadBalancerInput{ @@ -346,3 +374,74 @@ func TestDeleteNLB_DeletesManagedSG(t *testing.T) { assert.Empty(t, out.SecurityGroups, "managed SG must be deleted with the NLB") } } + +// TestCreateLoadBalancerSync_LaunchFailure_RollsBackEverything verifies a +// synchronous create whose data-plane launch fails leaves nothing behind: no +// record, no managed ENI, no managed SG, and the name claim released. The +// leaked managed SG is what pinned the EKS control-plane VPC against DeleteVpc. +func TestCreateLoadBalancerSync_LaunchFailure_RollsBackEverything(t *testing.T) { + svc, vpcSvc, mock := setupSubnetTestService(t) + subnetID, _ := firstSubnet(t, vpcSvc) + + // Force the inline data-plane launch to fail. + mock.launchErr = errors.New("allocate public IP: " + awserrors.ErrorInsufficientAddressCapacity) + + _, err := svc.CreateLoadBalancerSync(nlbSyncInput("nlb-sync-fail", subnetID), testAccountID) + require.Error(t, err) + + lbs, err := svc.store.ListLoadBalancers() + require.NoError(t, err) + assert.Empty(t, lbs, "failed sync create must leave no LB record") + assert.Equal(t, 0, countManagedENIs(t, vpcSvc), "failed sync create must leave no managed ENI") + assert.False(t, hasManagedNLBSG(t, vpcSvc), "failed sync create must leave no managed NLB SG") + + // Name claim released: reusing the name on a later (now succeeding) create works. + mock.launchErr = nil + out, err := svc.CreateLoadBalancerSync(nlbSyncInput("nlb-sync-fail", subnetID), testAccountID) + require.NoError(t, err, "name claim must be released so the name is reusable") + require.Len(t, out.LoadBalancers, 1) +} + +// TestCreateLoadBalancerSync_LaunchFailure_SurfacesCause verifies the real +// launch-failure cause reaches the caller: a capacity shortfall surfaces its own +// code, while an unrecognised cause stays ServerInternal rather than inventing a +// client error. +func TestCreateLoadBalancerSync_LaunchFailure_SurfacesCause(t *testing.T) { + svc, vpcSvc, mock := setupSubnetTestService(t) + subnetID, _ := firstSubnet(t, vpcSvc) + + mock.launchErr = errors.New("allocate public IP for internet-facing ALB: " + awserrors.ErrorInsufficientAddressCapacity) + _, err := svc.CreateLoadBalancerSync(nlbSyncInput("nlb-cap", subnetID), testAccountID) + require.Error(t, err) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInsufficientAddressCapacity), + "capacity failure must surface InsufficientAddressCapacity, got %v", err) + + mock.launchErr = errors.New("qemu exited with code 1") + _, err = svc.CreateLoadBalancerSync(nlbSyncInput("nlb-opaque", subnetID), testAccountID) + require.Error(t, err) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorServerInternal), + "unrecognised failure must stay ServerInternal, got %v", err) +} + +// TestCreateLoadBalancer_AsyncLaunchFailure_KeepsFailedRecord guards the +// deliberate asymmetry: the async path returns the ARN to the caller before +// launching, so its failed record and managed SG are kept — the caller can +// reclaim them via DeleteLoadBalancer. Only the sync path rolls back fully. +func TestCreateLoadBalancer_AsyncLaunchFailure_KeepsFailedRecord(t *testing.T) { + svc, vpcSvc, mock := setupSubnetTestService(t) + subnetID, _ := firstSubnet(t, vpcSvc) + mock.launchErr = errors.New("allocate public IP: " + awserrors.ErrorInsufficientAddressCapacity) + + out, err := svc.CreateLoadBalancer(context.Background(), nlbSyncInput("nlb-async-fail", subnetID), testAccountID) + require.NoError(t, err, "async create returns before the launch goroutine runs") + require.Len(t, out.LoadBalancers, 1) + arn := *out.LoadBalancers[0].LoadBalancerArn + svc.WaitLaunches() + + rec, err := svc.store.GetLoadBalancerByArn(arn) + require.NoError(t, err) + require.NotNil(t, rec, "async failed record must remain for the caller to reclaim") + assert.Equal(t, StateFailed, rec.State) + assert.NotEmpty(t, rec.NLBManagedSGID, "async failed record must retain its managed SG for reclamation") + assert.True(t, hasManagedNLBSG(t, vpcSvc), "async failure must leave the managed SG in place") +}