From 95631bf57b82d7b02cf149a6ec3415e3417aae47 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 19 Jul 2026 12:40:37 +1000 Subject: [PATCH] fix(elbv2): roll back managed SG on sync LB launch failure and surface the cause (#547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CreateLoadBalancerSync whose data-plane launch failed returned without rolling back the NLB managed security group. The orphaned SG sat in the EKS managed control-plane VPC and blocked DeleteVpc with DependencyViolation indefinitely, wedging the cluster in DELETING. The failure was also flattened to ServerInternal, discarding the only useful diagnostic. The sync path now unwinds fully on launch failure (ENIs, managed SG, name claim, record) and maps the launch error onto its real cause: capacity shortfalls pass through as their own code, anything else stays ServerInternal. The async path deliberately keeps its failed record — the caller holds the ARN and can reclaim it via DeleteLoadBalancer. --- spinifex/handlers/elbv2/service_impl.go | 42 +++++++- .../elbv2/service_impl_nlb_sg_test.go | 99 +++++++++++++++++++ 2 files changed, 138 insertions(+), 3 deletions(-) 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") +}