From 4457b813aeedc397143e844843f7879b227d455e Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 10 Jun 2026 13:37:40 +1000 Subject: [PATCH] fix: make EKS DeleteCluster egress-EIP release idempotent (#336) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A retried DeleteCluster (or the egress-delete cascade) can release the hidden-pool allocation before the release step runs, so ReleaseAddress returns InvalidAllocationID.NotFound. That was treated as a fatal teardown error, so the cluster wedged in DELETING forever and its KV meta plus downstream resources leaked. Swallow the NotFound variants as idempotent success — mirroring the 165.17 ENI-teardown fix — so the SG + KV sweep proceeds. A genuine release failure still wedges DELETING for retry. --- spinifex/handlers/eks/delete_cluster_test.go | 37 +++++++++++++++++++- spinifex/handlers/eks/service_impl.go | 13 ++++++- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/spinifex/handlers/eks/delete_cluster_test.go b/spinifex/handlers/eks/delete_cluster_test.go index 01af55477..8b50de921 100644 --- a/spinifex/handlers/eks/delete_cluster_test.go +++ b/spinifex/handlers/eks/delete_cluster_test.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/eks" "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" @@ -89,6 +90,7 @@ type deleteClusterFixture struct { nlb *fakeNLBProvisioner inst *fakeK3sInst vpc *fakeK3sVPC + eip *fakeEIPProvisioner } func newDeleteClusterFixture(t *testing.T, clusterName string) *deleteClusterFixture { @@ -103,13 +105,15 @@ func newDeleteClusterFixture(t *testing.T, clusterName string) *deleteClusterFix meta.ControlPlaneInstanceID = "i-aaa111" meta.ControlPlaneENIID = "eni-aaa111" meta.ResourcesVpcConfig.VpcId = "vpc-aaa" + meta.EgressEIPPublicIP = "203.0.113.50" + meta.EgressEIPAllocationID = "eipalloc-fake01" require.NoError(t, PutClusterMeta(kv, meta)) // Real OIDC key so ZeroizeClusterOIDCKey has material to wipe. _, _, err := GenerateClusterOIDCKeypair(kv, clusterName, bootstrapTestMasterKey) require.NoError(t, err) - return &deleteClusterFixture{svc: f.svc, kv: kv, nlb: f.nlb, inst: f.inst, vpc: f.vpc} + return &deleteClusterFixture{svc: f.svc, kv: kv, nlb: f.nlb, inst: f.inst, vpc: f.vpc, eip: f.eip} } func TestDeleteCluster_AllTeardownSucceedsSweepsKV(t *testing.T) { @@ -156,3 +160,34 @@ func TestDeleteCluster_VMTerminateFailureLeavesMeta(t *testing.T) { require.NoError(t, getErr) assert.Equal(t, ClusterStatusDeleting, meta.Status) } + +// A prior retry (or the egress-delete cascade) already released the allocation, +// so ReleaseAddress returns InvalidAllocationID.NotFound. That is idempotent +// success — teardown must complete and sweep the KV, not wedge in DELETING. +func TestDeleteCluster_EgressEIPAlreadyReleasedSweepsKV(t *testing.T) { + f := newDeleteClusterFixture(t, "alpha") + f.eip.releaseErr = errors.New(awserrors.ErrorInvalidAllocationIDNotFound) + + out, err := f.svc.DeleteCluster(deleteInput("alpha"), testAccountID) + require.NoError(t, err) + require.NotNil(t, out) + + require.Len(t, f.eip.releaseCalls, 1, "release must still be attempted") + _, getErr := GetClusterMeta(f.kv, "alpha") + assert.ErrorIs(t, getErr, ErrClusterNotFound, "NotFound release must not block the KV sweep") +} + +// A genuine release failure (not a NotFound) is retryable and must still wedge +// the cluster in DELETING so the billable EIP is not orphaned. +func TestDeleteCluster_EgressEIPReleaseRealErrorLeavesMeta(t *testing.T) { + f := newDeleteClusterFixture(t, "alpha") + f.eip.releaseErr = errors.New("AddressLimitExceeded") + + _, err := f.svc.DeleteCluster(deleteInput("alpha"), testAccountID) + require.Error(t, err) + + meta, getErr := GetClusterMeta(f.kv, "alpha") + require.NoError(t, getErr) + assert.Equal(t, ClusterStatusDeleting, meta.Status) + assert.NotEmpty(t, meta.EgressEIPAllocationID, "alloc ID must remain for retry") +} diff --git a/spinifex/handlers/eks/service_impl.go b/spinifex/handlers/eks/service_impl.go index 8aca862fe..983cfde5d 100644 --- a/spinifex/handlers/eks/service_impl.go +++ b/spinifex/handlers/eks/service_impl.go @@ -764,7 +764,18 @@ func (s *EKSServiceImpl) purgeClusterInfra(accountID, name string, meta *Cluster if _, err := s.deps.EIP.ReleaseAddress(&ec2.ReleaseAddressInput{ AllocationId: aws.String(meta.EgressEIPAllocationID), }, accountID); err != nil { - teardownErrs = append(teardownErrs, fmt.Errorf("release egress EIP: %w", err)) + switch { + case awserrors.IsErrorCode(err, awserrors.ErrorInvalidAllocationIDNotFound), + awserrors.IsErrorCode(err, awserrors.ErrorInvalidAddressIDNotFound), + awserrors.IsErrorCode(err, awserrors.ErrorInvalidAddressNotFound): + // A prior retry (or the egress-delete cascade) already released + // the allocation. Idempotent success — must NOT block the SG + + // KV sweep, or the cluster wedges in DELETING permanently. + slog.Debug("purgeClusterInfra: egress EIP already released", + "cluster", name, "allocationId", meta.EgressEIPAllocationID) + default: + teardownErrs = append(teardownErrs, fmt.Errorf("release egress EIP: %w", err)) + } } } }