From 580777d8671841081af8139c4a96eac2fcf0bee2 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 31 Aug 2026 16:44:14 +1000 Subject: [PATCH] fix: release an ENI's EIP association and reap ENIs whose instance is gone (#955) --- spinifex/daemon/eni_orphan_reaper.go | 234 ++++++++++++++++- spinifex/daemon/eni_orphan_reaper_test.go | 240 ++++++++++++++++++ spinifex/daemon/vm_adapters.go | 28 ++ spinifex/daemon/vm_adapters_test.go | 86 +++++++ spinifex/handlers/ec2/eip/service_impl.go | 102 +++++++- .../handlers/ec2/eip/service_impl_test.go | 117 +++++++++ spinifex/handlers/ec2/vpc/eni.go | 35 ++- .../handlers/ec2/vpc/eni_abandoned_test.go | 53 ++++ 8 files changed, 874 insertions(+), 21 deletions(-) diff --git a/spinifex/daemon/eni_orphan_reaper.go b/spinifex/daemon/eni_orphan_reaper.go index 2b6643100..58115bc88 100644 --- a/spinifex/daemon/eni_orphan_reaper.go +++ b/spinifex/daemon/eni_orphan_reaper.go @@ -2,6 +2,7 @@ package daemon import ( "context" + "fmt" "log/slog" "time" @@ -17,35 +18,86 @@ import ( // this leaves a wide margin past the point where a launch could still attach. const eniOrphanMinAge = 15 * time.Minute -// eniOrphanReaper deletes ENI records left behind by a launch that created the -// interface and then died before attaching it. The record outlives the launch, -// and because the security group dependency check counts an ENI that lists the -// group, the SG can never be deleted afterwards. +// instanceIndex answers whether the control plane still knows an instance. Both +// listings together cover an instance's whole life: the shared record space +// holds it while it runs and while it is stopped, and the terminated bucket +// holds it for the hour after it goes away. +type instanceIndex interface { + ListInstanceRecords() ([]*vm.InstanceRecord, error) + ListTerminatedInstanceRecords() ([]*vm.InstanceRecord, error) +} + +// eipDisassociator releases the association an Elastic IP holds on an ENI, +// leaving the allocation itself in place. +type eipDisassociator interface { + DisassociateByENI(ctx context.Context, accountID, eniID string) (bool, error) +} + +// eniOrphanReaper deletes ENI records no instance can still be using. Two shapes +// qualify, and neither has anything else that would ever remove it: // -// Cluster-wide rather than node-local: the record has no instance, so no node -// owns it, and nothing local would ever look at it again. +// A launch that created the interface and then died before attaching it leaves a +// record with no instance at all. Because the security group dependency check +// counts an ENI that lists the group, the SG can never be deleted afterwards. +// +// A teardown that never finished leaves the opposite: a record still naming an +// instance that exists nowhere. The node-local teardown reaper would have +// re-driven it, but only from the terminated bucket, which expires after an +// hour — past that the ENI, its logical switch port and its EIP association are +// permanent, and the network reconciler keeps driving toward a port that can +// never bind. +// +// Cluster-wide rather than node-local: neither shape has a live instance, so no +// node owns it, and nothing local would ever look at it again. type eniOrphanReaper struct { - vpc *handlers_ec2_vpc.VPCServiceImpl - minAge time.Duration + vpc *handlers_ec2_vpc.VPCServiceImpl + instances instanceIndex + eip eipDisassociator + minAge time.Duration + // missing holds the ENIs whose instance was absent last sweep. An instance + // record is written after its ENI is attached, so a single absent reading + // can be a launch mid-flight rather than a zombie; only an ENI missing from + // two consecutive sweeps is reaped. + missing map[string]bool } var _ vm.Reaper = (*eniOrphanReaper)(nil) // newENIOrphanReaper builds the sweep over the daemon's VPC service, or nil -// when there is no control plane on this node to sweep with. +// when there is no control plane on this node to sweep with. A nil jsManager or +// EIP service degrades the sweep rather than disabling it: see Sweep. func (d *Daemon) newENIOrphanReaper() *eniOrphanReaper { if d.vpcService == nil { return nil } - return &eniOrphanReaper{vpc: d.vpcService, minAge: eniOrphanMinAge} + r := &eniOrphanReaper{vpc: d.vpcService, minAge: eniOrphanMinAge} + if d.jsManager != nil { + r.instances = d.jsManager + } + if eip, ok := d.eipService.(eipDisassociator); ok { + r.eip = eip + } + return r } func (r *eniOrphanReaper) Class() string { return "eni-orphan" } func (r *eniOrphanReaper) Scope() vm.ReaperScope { return vm.ScopeClusterWide } -// Sweep deletes every abandoned instance ENI. A per-record failure is logged -// and skipped so one bad record cannot stall the rest of the sweep. +// Sweep deletes every abandoned launch ENI and every ENI whose instance is gone. +// A per-record failure is logged and skipped so one bad record cannot stall the +// rest of the sweep. func (r *eniOrphanReaper) Sweep(ctx context.Context) (int, error) { + reaped, err := r.sweepAbandonedLaunches(ctx) + if err != nil { + return reaped, err + } + stale, err := r.sweepStaleAttachments(ctx) + return reaped + stale, err +} + +// sweepAbandonedLaunches deletes ENIs that never reached an attachment. A plain +// delete suffices: with no attachment there is no in-use guard to clear. +func (r *eniOrphanReaper) sweepAbandonedLaunches(ctx context.Context) (int, error) { orphans, err := r.vpc.ListAbandonedInstanceENIs(ctx, r.minAge) if err != nil { return 0, err @@ -78,3 +130,161 @@ func (r *eniOrphanReaper) Sweep(ctx context.Context) (int, error) { } return reaped, nil } + +// sweepStaleAttachments deletes ENIs still naming an instance the control plane +// no longer holds anywhere. The delete uses the force path, which exists for +// exactly this owner-driven teardown; the in-use guard is left alone, since it +// is right to reject a delete aimed at another live instance's interface. +func (r *eniOrphanReaper) sweepStaleAttachments(ctx context.Context) (int, error) { + if r.instances == nil { + return 0, nil + } + + // Candidates before the keep set, so the set is never older than the + // listing it judges, and so a cluster with nothing to sweep pays for + // neither instance listing. + attached, err := r.vpc.ListAttachedInstanceENIs(ctx, r.minAge) + if err != nil { + return 0, err + } + if len(attached) == 0 { + r.missing = nil + return 0, nil + } + + known, err := r.knownInstanceIDs() + if err != nil { + // Fail closed. A short listing reads as "the instance is gone" for every + // instance missing from it, which would reap live guests' interfaces. + slog.WarnContext(ctx, "eni-orphan: instance listing failed; skipping stale-attachment sweep", "err", err) + r.missing = nil + return 0, nil + } + if len(known) == 0 { + // The record space is recreated when missing and republished only on a + // node's next state change, so an empty listing beside live ENIs is far + // likelier to be an emptied bucket than a cluster with no instances. + slog.WarnContext(ctx, "eni-orphan: no instance records at all beside attached ENIs; declining the stale-attachment sweep", + "candidates", len(attached)) + r.missing = nil + return 0, nil + } + + reaped := 0 + stillMissing := make(map[string]bool, len(r.missing)) + for _, candidate := range attached { + if ctx.Err() != nil { + r.missing = stillMissing + return reaped, ctx.Err() + } + if known[candidate.Record.InstanceId] { + continue + } + + eniID := candidate.Record.NetworkInterfaceId + if !r.missing[eniID] { + stillMissing[eniID] = true + slog.InfoContext(ctx, "eni-orphan: ENI names an instance the control plane does not hold; deferring to the next sweep", + "eniId", eniID, "accountId", candidate.AccountID, "instanceId", candidate.Record.InstanceId) + continue + } + if r.reapStale(ctx, candidate) { + reaped++ + continue + } + stillMissing[eniID] = true + } + r.missing = stillMissing + return reaped, nil +} + +// reapStale frees one zombie ENI, reporting whether the record is settled. An +// interface the owner asked to keep is detached rather than deleted, matching +// what terminate does with the same flag. +func (r *eniOrphanReaper) reapStale(ctx context.Context, candidate handlers_ec2_vpc.AccountENI) bool { + if keep := candidate.Record.DeleteOnTermination; keep != nil && !*keep { + return r.detachStale(ctx, candidate) + } + + eniID := candidate.Record.NetworkInterfaceId + slog.InfoContext(ctx, "eni-orphan: deleting ENI whose instance no longer exists", + "eniId", eniID, + "accountId", candidate.AccountID, + "instanceId", candidate.Record.InstanceId, + "privateIp", candidate.Record.PrivateIpAddress, + "createdAt", candidate.Record.CreatedAt) + + if err := r.vpc.ForceDeleteInstanceENI(ctx, candidate.AccountID, eniID); err != nil { + slog.WarnContext(ctx, "eni-orphan: stale ENI delete failed", + "eniId", eniID, "accountId", candidate.AccountID, "err", err) + return false + } + + // The EIP goes after the interface. The delete decides whether the ENI's + // public address may return to the pool by looking for an EIP that names + // the ENI, so clearing the association first would hand a still-allocated + // address back to IPAM. + r.releaseEIP(ctx, candidate.AccountID, eniID) + return true +} + +// detachStale clears a dead instance off an ENI that was asked to outlive it — +// an RDS endpoint, a customer's reserved address. Detaching takes the record +// out of the sweep's reach and leaves the interface, and its EIP, in place. +func (r *eniOrphanReaper) detachStale(ctx context.Context, candidate handlers_ec2_vpc.AccountENI) bool { + eniID := candidate.Record.NetworkInterfaceId + slog.InfoContext(ctx, "eni-orphan: detaching keep-on-terminate ENI whose instance no longer exists", + "eniId", eniID, + "accountId", candidate.AccountID, + "instanceId", candidate.Record.InstanceId, + "privateIp", candidate.Record.PrivateIpAddress) + + if err := r.vpc.DetachENI(ctx, candidate.AccountID, eniID); err != nil { + slog.WarnContext(ctx, "eni-orphan: stale ENI detach failed", + "eniId", eniID, "accountId", candidate.AccountID, "err", err) + return false + } + return true +} + +// releaseEIP drops any EIP association held on eniID, after the interface is +// gone. Nothing retries a failure here: the ENI record the sweep finds these +// through no longer exists, so the association is stranded until an operator +// disassociates the allocation by hand. +func (r *eniOrphanReaper) releaseEIP(ctx context.Context, accountID, eniID string) { + if r.eip == nil { + return + } + found, err := r.eip.DisassociateByENI(ctx, accountID, eniID) + if err != nil { + slog.ErrorContext(ctx, "eni-orphan: EIP association stranded on a deleted ENI; recover with DisassociateAddress", + "eniId", eniID, "accountId", accountID, "err", err) + return + } + if found { + slog.InfoContext(ctx, "eni-orphan: released EIP association from a stale ENI", + "eniId", eniID, "accountId", accountID) + } +} + +// knownInstanceIDs is every instance the control plane still holds a record for, +// running, stopped or recently terminated. +func (r *eniOrphanReaper) knownInstanceIDs() (map[string]bool, error) { + live, err := r.instances.ListInstanceRecords() + if err != nil { + return nil, fmt.Errorf("list instance records: %w", err) + } + terminated, err := r.instances.ListTerminatedInstanceRecords() + if err != nil { + return nil, fmt.Errorf("list terminated instance records: %w", err) + } + + known := make(map[string]bool, len(live)+len(terminated)) + for _, record := range live { + known[record.Metadata.Name] = true + } + for _, record := range terminated { + known[record.Metadata.Name] = true + } + return known, nil +} diff --git a/spinifex/daemon/eni_orphan_reaper_test.go b/spinifex/daemon/eni_orphan_reaper_test.go index 50fa3fc89..1265b3a5f 100644 --- a/spinifex/daemon/eni_orphan_reaper_test.go +++ b/spinifex/daemon/eni_orphan_reaper_test.go @@ -2,12 +2,14 @@ package daemon import ( "context" + "errors" "testing" "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + "github.com/mulgadc/spinifex/spinifex/resource" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/mulgadc/spinifex/spinifex/vm" "github.com/stretchr/testify/assert" @@ -66,3 +68,241 @@ func TestENIOrphanReaperSweep(t *testing.T) { require.NoError(t, err) assert.Zero(t, reaped) } + +// fakeInstanceIndex answers the instance-existence question from fixed sets. +type fakeInstanceIndex struct { + live []string + terminated []string + liveErr error + termErr error +} + +func instanceRecords(ids []string) []*vm.InstanceRecord { + out := make([]*vm.InstanceRecord, 0, len(ids)) + for _, id := range ids { + out = append(out, &vm.InstanceRecord{Metadata: resource.Metadata{Name: id}}) + } + return out +} + +func (f *fakeInstanceIndex) ListInstanceRecords() ([]*vm.InstanceRecord, error) { + return instanceRecords(f.live), f.liveErr +} + +func (f *fakeInstanceIndex) ListTerminatedInstanceRecords() ([]*vm.InstanceRecord, error) { + return instanceRecords(f.terminated), f.termErr +} + +// fakeEIPDisassociator records which ENIs were asked to give up their EIP. +type fakeEIPDisassociator struct { + associated map[string]bool + calls []string + err error + // onCall observes the world as the disassociation sees it, so a test can + // pin the ordering the release depends on. + onCall func(eniID string) +} + +func (f *fakeEIPDisassociator) DisassociateByENI(_ context.Context, _, eniID string) (bool, error) { + if f.onCall != nil { + f.onCall(eniID) + } + f.calls = append(f.calls, eniID) + if f.err != nil { + return false, f.err + } + return f.associated[eniID], nil +} + +// staleReaperFixture builds a VPC with one ENI attached to instanceID, aged past +// the sweep's guard, plus the reaper under test. +func staleReaperFixture(t *testing.T, instanceID string, index instanceIndex, eip eipDisassociator) (*eniOrphanReaper, *handlers_ec2_vpc.VPCServiceImpl, string) { + t.Helper() + _, nc, _ := testutil.StartTestJetStream(t) + vpcSvc, err := handlers_ec2_vpc.NewVPCServiceImplWithNATS(t.Context(), nil, nc) + require.NoError(t, err) + testutil.StubVpcdSGResponder(t, nc) + + ctx := t.Context() + vpcOut, err := vpcSvc.CreateVpc(ctx, &ec2.CreateVpcInput{CidrBlock: aws.String("10.0.0.0/16")}, reaperTestAccountID) + require.NoError(t, err) + subnetOut, err := vpcSvc.CreateSubnet(ctx, &ec2.CreateSubnetInput{ + VpcId: vpcOut.Vpc.VpcId, + CidrBlock: aws.String("10.0.1.0/24"), + }, reaperTestAccountID) + require.NoError(t, err) + + eniOut, err := vpcSvc.CreateNetworkInterface(ctx, &ec2.CreateNetworkInterfaceInput{ + SubnetId: subnetOut.Subnet.SubnetId, + Description: aws.String(handlers_ec2_vpc.AutoENIDescriptionPrefix + instanceID), + }, reaperTestAccountID) + require.NoError(t, err) + eniID := *eniOut.NetworkInterface.NetworkInterfaceId + + _, err = vpcSvc.AttachENI(reaperTestAccountID, eniID, instanceID, 0) + require.NoError(t, err) + require.NoError(t, vpcSvc.UpdateENI(reaperTestAccountID, eniID, func(rec *handlers_ec2_vpc.ENIRecord) { + rec.CreatedAt = time.Now().Add(-time.Hour) + })) + + return &eniOrphanReaper{vpc: vpcSvc, instances: index, eip: eip, minAge: 15 * time.Minute}, vpcSvc, eniID +} + +// TestENIOrphanReaperReapsStaleAttachment is the zombie case: the ENI still +// names an instance, but that instance is in neither the live nor the terminated +// space, so nothing else will ever delete the record — and until it goes, its +// logical switch port stays unbindable and its EIP stays associated. +func TestENIOrphanReaperReapsStaleAttachment(t *testing.T) { + index := &fakeInstanceIndex{live: []string{"i-somebody-else"}, terminated: []string{"i-recent"}} + eip := &fakeEIPDisassociator{associated: map[string]bool{}} + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip) + eip.associated[eniID] = true + // The delete decides whether the ENI's public address may go back to the + // pool by looking for an EIP that names the ENI, so the association has to + // still be there while it runs. + eip.onCall = func(id string) { + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, id) + assert.Error(t, err, "the EIP must be released after the interface, never before") + } + + // First sighting only defers: an instance record is written after its ENI + // is attached, so one absent reading can be a launch mid-flight. + reaped, err := r.Sweep(t.Context()) + require.NoError(t, err) + assert.Zero(t, reaped) + _, err = vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + require.NoError(t, err, "a single absent reading is not evidence enough to delete an interface") + assert.Empty(t, eip.calls) + + reaped, err = r.Sweep(t.Context()) + require.NoError(t, err) + assert.Equal(t, 1, reaped) + + _, err = vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + require.Error(t, err, "the record must be gone, or the reconciler keeps driving toward a port that cannot bind") + assert.Equal(t, []string{eniID}, eip.calls, "the EIP association must be released with the interface") + + reaped, err = r.Sweep(t.Context()) + require.NoError(t, err) + assert.Zero(t, reaped) +} + +// TestENIOrphanReaperDeclinesOnEmptyInstanceListing pins the other half of the +// fail-closed rule. The record space is recreated when missing and republished +// only on a node's next state change, so an empty listing beside live ENIs is +// an emptied bucket far more often than a cluster with no instances — and +// reaping against it would cut every guest in the cluster off the network. +func TestENIOrphanReaperDeclinesOnEmptyInstanceListing(t *testing.T) { + eip := &fakeEIPDisassociator{associated: map[string]bool{}} + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", &fakeInstanceIndex{}, eip) + + for range 2 { + reaped, err := r.Sweep(t.Context()) + require.NoError(t, err) + assert.Zero(t, reaped) + } + + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + assert.NoError(t, err) + assert.Empty(t, eip.calls) +} + +// TestENIOrphanReaperDetachesKeepOnTerminateENI pins that the sweep honours the +// flag terminate honours. DeleteOnTermination=false means the interface outlives +// its instance — an RDS endpoint holds its address this way — so a teardown that +// never finished must not become the thing that destroys it. +func TestENIOrphanReaperDetachesKeepOnTerminateENI(t *testing.T) { + index := &fakeInstanceIndex{live: []string{"i-somebody-else"}} + eip := &fakeEIPDisassociator{associated: map[string]bool{}} + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip) + require.NoError(t, vpcSvc.UpdateENI(reaperTestAccountID, eniID, func(rec *handlers_ec2_vpc.ENIRecord) { + rec.DeleteOnTermination = aws.Bool(false) + })) + + require.NoError(t, sweepTwice(t, r)) + + rec, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + require.NoError(t, err, "the interface must survive; only its dead instance goes") + assert.Empty(t, rec.InstanceId) + assert.Equal(t, "available", rec.Status) + assert.Empty(t, eip.calls, "the EIP belongs to an interface that is still there") +} + +// sweepTwice drives the two sightings a stale ENI needs before it is acted on. +func sweepTwice(t *testing.T, r *eniOrphanReaper) error { + t.Helper() + if _, err := r.Sweep(t.Context()); err != nil { + return err + } + _, err := r.Sweep(t.Context()) + return err +} + +// TestENIOrphanReaperSparesKnownInstances pins the guard that matters most: an +// ENI whose instance is running, stopped, or recently terminated is in use, and +// deleting it would cut a live guest off the network. +func TestENIOrphanReaperSparesKnownInstances(t *testing.T) { + for name, index := range map[string]*fakeInstanceIndex{ + "running or stopped": {live: []string{"i-known"}}, + "terminated": {terminated: []string{"i-known"}}, + } { + t.Run(name, func(t *testing.T) { + eip := &fakeEIPDisassociator{associated: map[string]bool{}} + r, vpcSvc, eniID := staleReaperFixture(t, "i-known", index, eip) + + require.NoError(t, sweepTwice(t, r)) + + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + assert.NoError(t, err) + assert.Empty(t, eip.calls) + }) + } +} + +// TestENIOrphanReaperSkipsStaleSweepOnListingError pins the fail-closed rule. A +// listing that errors part-way reads as "the instance is gone" for everything +// missing from it, so the sweep must decline rather than reap against it. +func TestENIOrphanReaperSkipsStaleSweepOnListingError(t *testing.T) { + for name, index := range map[string]*fakeInstanceIndex{ + "live listing failed": {liveErr: errors.New("kv unavailable")}, + "terminated listing failed": {live: []string{"i-other"}, termErr: errors.New("kv unavailable")}, + } { + t.Run(name, func(t *testing.T) { + eip := &fakeEIPDisassociator{associated: map[string]bool{}} + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip) + + require.NoError(t, sweepTwice(t, r), "a listing fault is not the sweep's failure; it just cannot decide") + + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + assert.NoError(t, err) + }) + } +} + +// TestENIOrphanReaperWithoutInstanceIndexSkipsStaleSweep pins that a node with +// no instance-record source (no JetStream manager) still runs the +// abandoned-launch sweep and simply declines the staleness question. +func TestENIOrphanReaperWithoutInstanceIndexSkipsStaleSweep(t *testing.T) { + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", nil, nil) + + require.NoError(t, sweepTwice(t, r)) + + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + assert.NoError(t, err) +} + +// TestENIOrphanReaperDeletesENIDespiteEIPFailure pins that a stuck EIP +// disassociation cannot pin the ENI. Leaving the interface behind is the worse +// outcome: the zombie port stays unbindable, while the stranded association is +// at least logged and recoverable by hand. +func TestENIOrphanReaperDeletesENIDespiteEIPFailure(t *testing.T) { + index := &fakeInstanceIndex{live: []string{"i-somebody-else"}} + eip := &fakeEIPDisassociator{err: errors.New("kv unavailable")} + r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip) + + require.NoError(t, sweepTwice(t, r)) + + _, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID) + assert.Error(t, err) + assert.Equal(t, []string{eniID}, eip.calls) +} diff --git a/spinifex/daemon/vm_adapters.go b/spinifex/daemon/vm_adapters.go index e44ab6375..9bb8745fc 100644 --- a/spinifex/daemon/vm_adapters.go +++ b/spinifex/daemon/vm_adapters.go @@ -832,6 +832,9 @@ func (a *instanceCleanerAdapter) DetachAndDeleteENI(instance *vm.VM) error { slog.Info("ENI already absent on termination", "eni", instance.ENIId, "instanceId", instance.ID) } + if err == nil { + a.disassociateENIEIP(instance.AccountID, instance.ENIId) + } } a.releaseAttachedENIs(instance) @@ -876,6 +879,7 @@ func (a *instanceCleanerAdapter) releaseAttachedENIs(instance *vm.VM) { "eni", eniId, "instanceId", instance.ID, "err", err) continue } + a.disassociateENIEIP(instance.AccountID, eniId) if deleted { slog.Info("Deleted attached ENI on termination", "eni", eniId, "instanceId", instance.ID) @@ -886,6 +890,30 @@ func (a *instanceCleanerAdapter) releaseAttachedENIs(instance *vm.VM) { } } +// disassociateENIEIP releases any Elastic IP associated with an ENI that has +// just been deleted. Nothing else did: the ENI delete deliberately leaves an +// EIP-owned public address alone, since the allocation outlives the interface, +// but the association is the interface's and has to go with it. Left behind, the +// EIP record stays "associated" and the network reconciler keeps re-asserting a +// NAT rule for a guest that no longer exists. +// +// Called after the delete, never before: the delete decides whether the ENI's +// public address may return to the pool by looking for an EIP that names it. +func (a *instanceCleanerAdapter) disassociateENIEIP(accountID, eniID string) { + eip, ok := a.d.eipService.(eipDisassociator) + if !ok { + return + } + found, err := eip.DisassociateByENI(context.Background(), accountID, eniID) + if err != nil { + slog.Warn("Failed to disassociate EIP on termination", "eni", eniID, "accountId", accountID, "err", err) + return + } + if found { + slog.Info("Disassociated EIP on termination", "eni", eniID, "accountId", accountID) + } +} + // RemoveFromPlacementGroup unbinds the instance from its placement group // if one is set. No-op for ungrouped instances and when the placement // group service is not configured. diff --git a/spinifex/daemon/vm_adapters_test.go b/spinifex/daemon/vm_adapters_test.go index fa3042e3e..aa6a52329 100644 --- a/spinifex/daemon/vm_adapters_test.go +++ b/spinifex/daemon/vm_adapters_test.go @@ -11,6 +11,7 @@ import ( "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/config" "github.com/mulgadc/spinifex/spinifex/gpu" + handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip" handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/mulgadc/spinifex/spinifex/testutil" @@ -447,6 +448,8 @@ func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesPostLaunchAttach(t *t func TestInstanceCleanerAdapter_DetachAndDeleteENI_DeleteOnTerminationFalseDetachesOnly(t *testing.T) { f := newENIHotPlugFixture(t) f.vmInst.AccountID = testAccountID + eip := newStubEIPDisassociator(f.eniID) + f.daemon.eipService = eip _, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 1) require.NoError(t, err) @@ -461,6 +464,89 @@ func TestInstanceCleanerAdapter_DetachAndDeleteENI_DeleteOnTerminationFalseDetac require.NoError(t, err, "DeleteOnTermination=false must detach, not delete") assert.Equal(t, "available", rec.Status) assert.Empty(t, rec.InstanceId) + assert.Empty(t, eip.calls, + "an EIP belongs to the interface, and the interface survives, so the association must too") +} + +// stubEIPDisassociator embeds the EIP service interface so only the one method +// terminate reaches for has to be real; anything else panics rather than +// silently answering. +type stubEIPDisassociator struct { + handlers_ec2_eip.EIPService + + associated map[string]bool + calls []string + // onCall observes the world as the disassociation sees it, so a test can + // pin the ordering the release depends on. + onCall func(eniID string) +} + +func newStubEIPDisassociator(associatedENIs ...string) *stubEIPDisassociator { + s := &stubEIPDisassociator{associated: make(map[string]bool, len(associatedENIs))} + for _, eniID := range associatedENIs { + s.associated[eniID] = true + } + return s +} + +func (s *stubEIPDisassociator) DisassociateByENI(_ context.Context, _, eniID string) (bool, error) { + if s.onCall != nil { + s.onCall(eniID) + } + s.calls = append(s.calls, eniID) + return s.associated[eniID], nil +} + +// TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPAssociation pins the +// half of the zombie the ENI delete never covered. releaseENISideEffects +// deliberately leaves an EIP-owned public address alone, because the allocation +// outlives the interface — but nothing released the association, so the EIP +// record stayed "associated" against a guest that no longer exists and the +// network reconciler kept re-asserting NAT for a port that could never bind. +func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPAssociation(t *testing.T) { + f := newENIHotPlugFixture(t) + f.vmInst.AccountID = testAccountID + f.vmInst.ENIId = f.eniID + eip := newStubEIPDisassociator(f.eniID) + f.daemon.eipService = eip + // The ENI delete decides whether the interface's public address may return + // to the IPAM pool by looking for an EIP that names it, so the association + // has to outlast the delete. + eip.onCall = func(id string) { + _, err := f.daemon.vpcService.GetENIRecord(testAccountID, id) + assert.Error(t, err, "the EIP must be released after the interface, never before") + } + + _, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 0) + require.NoError(t, err) + + cleaner := newInstanceCleanerAdapter(f.daemon) + require.NoError(t, cleaner.DetachAndDeleteENI(f.vmInst)) + + assert.Equal(t, []string{f.eniID}, eip.calls, + "the association must go with the interface it belongs to") + _, err = f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInvalidNetworkInterfaceIDNotFound)) +} + +// TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPOnPostLaunchAttach +// extends the release to the KV enumeration sweep. A hot-plug attach never sets +// instance.ENIId, so covering only the primary path would leave every EIP on a +// post-launch interface associated to a dead guest. +func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPOnPostLaunchAttach(t *testing.T) { + f := newENIHotPlugFixture(t) + f.vmInst.AccountID = testAccountID + eip := newStubEIPDisassociator(f.eniID) + f.daemon.eipService = eip + + _, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 1) + require.NoError(t, err) + require.Empty(t, f.vmInst.ENIId, "precondition: launch-time scalar must stay unset") + + cleaner := newInstanceCleanerAdapter(f.daemon) + require.NoError(t, cleaner.DetachAndDeleteENI(f.vmInst)) + + assert.Equal(t, []string{f.eniID}, eip.calls) } // TestInstanceCleanerAdapter_DetachAndDeleteENI_PrimaryENIReleased covers the diff --git a/spinifex/handlers/ec2/eip/service_impl.go b/spinifex/handlers/ec2/eip/service_impl.go index f9123f757..db976bcfd 100644 --- a/spinifex/handlers/ec2/eip/service_impl.go +++ b/spinifex/handlers/ec2/eip/service_impl.go @@ -299,14 +299,7 @@ func (s *EIPServiceImpl) DisassociateAddress(ctx context.Context, input *ec2.Dis s.publishNATEvent("vpc.delete-nat", record.VpcId, record.PublicIp, record.PrivateIp, record.ENIId, macAddr) } - // Clear association fields, revert to "allocated" state. - record.AssociationId = "" - record.ENIId = "" - record.InstanceId = "" - record.PrivateIp = "" - record.VpcId = "" - record.MacAddress = "" - record.State = "allocated" + clearAssociation(record) data, err := json.Marshal(record) if err != nil { @@ -321,6 +314,99 @@ func (s *EIPServiceImpl) DisassociateAddress(ctx context.Context, input *ec2.Dis return &ec2.DisassociateAddressOutput{}, nil } +// clearAssociation reverts a record to the unassociated "allocated" state. The +// allocation itself survives: disassociating never hands the address back. +func clearAssociation(record *EIPRecord) { + record.AssociationId = "" + record.ENIId = "" + record.InstanceId = "" + record.PrivateIp = "" + record.VpcId = "" + record.MacAddress = "" + record.State = "allocated" +} + +// DisassociateByENI releases the association an EIP holds on eniID, reporting +// whether one was found. Instance teardown deletes the ENI but nothing released +// the EIP behind it, so the record stayed "associated" against a guest that no +// longer exists and the reconciler kept re-asserting its dnat_and_snat and +// probing a port that could never bind. No association is success, so a re-run +// of terminate converges. The allocation is left intact, matching AWS: an EIP +// survives the instance it was attached to. +func (s *EIPServiceImpl) DisassociateByENI(ctx context.Context, accountID, eniID string) (bool, error) { + if eniID == "" { + return false, nil + } + record, key, revision, err := s.findByENI(ctx, accountID, eniID) + if err != nil { + return false, err + } + if record == nil { + return false, nil + } + + // The MAC comes off the record rather than a fresh ENI lookup: this runs as + // part of tearing that ENI down, so the interface may already be gone. + s.publishNATEvent("vpc.delete-nat", record.VpcId, record.PublicIp, record.PrivateIp, eniID, record.MacAddress) + + publicIP, allocationID := record.PublicIp, record.AllocationId + clearAssociation(record) + + data, err := json.Marshal(record) + if err != nil { + return false, fmt.Errorf("marshal EIP record: %w", err) + } + if _, err := s.eipKV.Update(ctx, key, data, revision); err != nil { + return false, errors.New(awserrors.ErrorServerInternal) + } + + slog.InfoContext(ctx, "DisassociateByENI completed", + "eniId", eniID, "publicIp", publicIP, "allocationId", allocationID, "accountID", accountID) + return true, nil +} + +// findByENI returns the associated EIP record holding eniID, or a nil record +// when the account has none. Unlike findByAssociationID an absent match is not +// an error: teardown asks this of every ENI, and most carry no EIP. +func (s *EIPServiceImpl) findByENI(ctx context.Context, accountID, eniID string) (*EIPRecord, string, uint64, error) { + prefix := accountID + "." + keys, err := s.eipKV.Keys(ctx) + if err != nil { + if errors.Is(err, jetstream.ErrNoKeysFound) { + return nil, "", 0, nil + } + return nil, "", 0, errors.New(awserrors.ErrorServerInternal) + } + + for _, k := range keys { + if err := ctx.Err(); err != nil { + return nil, "", 0, err + } + if k == utils.VersionKey || !strings.HasPrefix(k, prefix) { + continue + } + entry, err := s.eipKV.Get(ctx, k) + if err != nil { + if errors.Is(err, jetstream.ErrKeyNotFound) { + continue + } + // An unreadable record must not read as "this ENI has no EIP": + // the caller deletes the interface on that answer, and the + // association would be stranded with nothing left to find it. + return nil, "", 0, fmt.Errorf("eipKV.Get(%s): %w", k, err) + } + var record EIPRecord + if err := json.Unmarshal(entry.Value(), &record); err != nil { + slog.WarnContext(ctx, "findByENI: malformed EIP record", "key", k, "err", err) + continue + } + if record.ENIId == eniID { + return &record, k, entry.Revision(), nil + } + } + return nil, "", 0, nil +} + // describeAddressesValidFilters defines the set of filter names accepted by DescribeAddresses. var describeAddressesValidFilters = map[string]bool{ "allocation-id": true, diff --git a/spinifex/handlers/ec2/eip/service_impl_test.go b/spinifex/handlers/ec2/eip/service_impl_test.go index ee8960f28..41d5cbb9d 100644 --- a/spinifex/handlers/ec2/eip/service_impl_test.go +++ b/spinifex/handlers/ec2/eip/service_impl_test.go @@ -200,6 +200,123 @@ func TestEIP_ReleaseByInstanceID_ReclaimsAssociatedEIP(t *testing.T) { assert.Contains(t, err.Error(), "InvalidAllocationID.NotFound") } +// associateToENI marks an allocated EIP as associated with eniID the way +// AssociateAddress would, without needing a real VPC service behind it. +func associateToENI(t *testing.T, svc *EIPServiceImpl, allocID, eniID string) { + t.Helper() + key := testAccountID + "." + allocID + entry, err := svc.eipKV.Get(t.Context(), key) + require.NoError(t, err) + var record EIPRecord + require.NoError(t, json.Unmarshal(entry.Value(), &record)) + record.State = "associated" + record.AssociationId = "eipassoc-" + eniID + record.ENIId = eniID + record.InstanceId = "i-owner" + record.PrivateIp = "172.31.0.8" + record.VpcId = "vpc-test" + record.MacAddress = "02:52:f5:7f:9d:0e" + data, err := json.Marshal(record) + require.NoError(t, err) + _, err = svc.eipKV.Update(t.Context(), key, data, entry.Revision()) + require.NoError(t, err) +} + +// TestEIP_DisassociateByENI is the teardown path the zombie ENIs needed: the +// association goes with the interface, and the allocation stays behind. +func TestEIP_DisassociateByENI(t *testing.T) { + svc, _ := setupTestEIP(t) + + out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID) + require.NoError(t, err) + allocID, publicIP := *out.AllocationId, *out.PublicIp + associateToENI(t, svc, allocID, "eni-zombie") + + found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie") + require.NoError(t, err) + assert.True(t, found) + + entry, err := svc.eipKV.Get(t.Context(), testAccountID+"."+allocID) + require.NoError(t, err, "the allocation survives: an EIP outlives the instance it was attached to") + var record EIPRecord + require.NoError(t, json.Unmarshal(entry.Value(), &record)) + assert.Equal(t, "allocated", record.State) + assert.Equal(t, publicIP, record.PublicIp) + assert.Empty(t, record.ENIId) + assert.Empty(t, record.AssociationId) + assert.Empty(t, record.InstanceId) + assert.Empty(t, record.PrivateIp) + assert.Empty(t, record.VpcId) + assert.Empty(t, record.MacAddress) + + // The record no longer names the ENI, so a re-run finds nothing and still + // succeeds — terminate and the orphan sweep both retry this. + found, err = svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie") + require.NoError(t, err) + assert.False(t, found) +} + +// TestEIP_DisassociateByENI_NoMatchIsNoOp pins that an ENI carrying no EIP — the +// overwhelmingly common case on terminate — leaves other accounts' associations +// alone and reports no error. +func TestEIP_DisassociateByENI_NoMatchIsNoOp(t *testing.T) { + svc, _ := setupTestEIP(t) + + out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID) + require.NoError(t, err) + allocID := *out.AllocationId + associateToENI(t, svc, allocID, "eni-other") + + found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-unrelated") + require.NoError(t, err) + assert.False(t, found) + + found, err = svc.DisassociateByENI(t.Context(), testAccountID, "") + require.NoError(t, err) + assert.False(t, found) + + entry, err := svc.eipKV.Get(t.Context(), testAccountID+"."+allocID) + require.NoError(t, err) + var record EIPRecord + require.NoError(t, json.Unmarshal(entry.Value(), &record)) + assert.Equal(t, "associated", record.State) + assert.Equal(t, "eni-other", record.ENIId) +} + +// TestEIP_DisassociateByENI_ScopedToAccount pins that the scan honours the +// account prefix: a cluster-wide sweep passes the account the key was found +// under, and matching across accounts would disassociate a stranger's address. +func TestEIP_DisassociateByENI_ScopedToAccount(t *testing.T) { + svc, _ := setupTestEIP(t) + + out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID) + require.NoError(t, err) + associateToENI(t, svc, *out.AllocationId, "eni-zombie") + + found, err := svc.DisassociateByENI(t.Context(), "210987654321", "eni-zombie") + require.NoError(t, err) + assert.False(t, found) +} + +// TestEIP_DisassociateByENI_SkipsMalformedRecord pins that one undecodable +// entry cannot hide a real association. Teardown deletes the interface on a +// "no EIP" answer, and the association would then be stranded with nothing +// left that could find it. +func TestEIP_DisassociateByENI_SkipsMalformedRecord(t *testing.T) { + svc, _ := setupTestEIP(t) + + _, err := svc.eipKV.Put(t.Context(), testAccountID+".eipalloc-corrupt", []byte("{")) + require.NoError(t, err) + + out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID) + require.NoError(t, err) + associateToENI(t, svc, *out.AllocationId, "eni-zombie") + + found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie") + require.NoError(t, err) + assert.True(t, found) +} + func TestEIP_ReleaseByInstanceID_NoMatchIsNoOp(t *testing.T) { svc, _ := setupTestEIP(t) diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index f433a9f9e..f99a2eb21 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -795,6 +795,25 @@ type AccountENI struct { // the residue of a launch that died in between. Such a record keeps its // security group undeletable forever, since the SG dependency check counts it. func (s *VPCServiceImpl) ListAbandonedInstanceENIs(ctx context.Context, minAge time.Duration) ([]AccountENI, error) { + return s.scanENIs(ctx, func(record *ENIRecord) bool { + return eniIsAbandonedLaunch(record, minAge) + }) +} + +// ListAttachedInstanceENIs returns every ENI older than minAge that still names +// an instance. The caller decides which of them are stale by asking whether the +// instance still exists; this only narrows the bucket to the records where that +// question is worth asking. +func (s *VPCServiceImpl) ListAttachedInstanceENIs(ctx context.Context, minAge time.Duration) ([]AccountENI, error) { + return s.scanENIs(ctx, func(record *ENIRecord) bool { + return eniNamesInstance(record, minAge) + }) +} + +// scanENIs walks every account's ENI records and returns those match accepts. +// An unreadable or undecodable record is skipped: one bad entry must not stop a +// cluster-wide sweep. +func (s *VPCServiceImpl) scanENIs(ctx context.Context, match func(*ENIRecord) bool) ([]AccountENI, error) { keys, err := s.eniKV.Keys(ctx) if err != nil { if errors.Is(err, jetstream.ErrNoKeysFound) { @@ -820,7 +839,7 @@ func (s *VPCServiceImpl) ListAbandonedInstanceENIs(ctx context.Context, minAge t if err := json.Unmarshal(entry.Value(), &record); err != nil { continue } - if !eniIsAbandonedLaunch(&record, minAge) { + if !match(&record) { continue } out = append(out, AccountENI{AccountID: accountID, Record: record}) @@ -844,6 +863,20 @@ func eniIsAbandonedLaunch(record *ENIRecord, minAge time.Duration) bool { return time.Since(record.CreatedAt) >= minAge } +// eniNamesInstance reports whether a record claims an instance and is old enough +// that a launch could no longer still be wiring it up. A zero CreatedAt is +// excluded for the same reason it is in eniIsAbandonedLaunch: age is the only +// guard against acting on an ENI a launch is still using. +func eniNamesInstance(record *ENIRecord, minAge time.Duration) bool { + if record.InstanceId == "" { + return false + } + if record.CreatedAt.IsZero() { + return false + } + return time.Since(record.CreatedAt) >= minAge +} + // FindENIByAttachment scans the ENI bucket for the record with the given // AttachmentId. Used by DetachNetworkInterface which identifies by attachment ID. func (s *VPCServiceImpl) FindENIByAttachment(accountID, attachmentId string) (ENIRecord, error) { diff --git a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go index 151fd827f..7559c7330 100644 --- a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go +++ b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go @@ -65,6 +65,59 @@ func TestListAbandonedInstanceENIs(t *testing.T) { "only the aged, unattached, launch-created ENI is residue; the others are live or deliberate") } +// TestListAttachedInstanceENIs pins the complement of the abandoned listing: the +// records that do name an instance, which is where a zombie hides. The two +// listings must not overlap, or a sweep would act on the same record twice. +func TestListAttachedInstanceENIs(t *testing.T) { + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24") + + attached := createAutoENI(t, svc, subnetID, "i-gone", nil, time.Hour) + _, err := svc.AttachENI(testAccountID, attached, "i-gone", 0) + require.NoError(t, err) + + // Attached but still inside the age guard: a launch this young may still be + // wiring the instance up, so its ENI is not a candidate for anything. + fresh := createAutoENI(t, svc, subnetID, "i-launching", nil, time.Minute) + _, err = svc.AttachENI(testAccountID, fresh, "i-launching", 0) + require.NoError(t, err) + + // Aged but never attached — the abandoned-launch listing owns this one. + createAutoENI(t, svc, subnetID, "i-abandoned", nil, time.Hour) + + got, err := svc.ListAttachedInstanceENIs(context.Background(), 15*time.Minute) + require.NoError(t, err) + + ids := make([]string, 0, len(got)) + for _, o := range got { + assert.Equal(t, testAccountID, o.AccountID) + ids = append(ids, o.Record.NetworkInterfaceId) + } + assert.Equal(t, []string{attached}, ids, + "only the aged ENI that names an instance is a candidate for the staleness check") +} + +// TestListAttachedInstanceENIsIgnoresZeroCreatedAt pins the age guard's fallback. +// A record written before CreatedAt existed carries no age, and age is the only +// thing standing between this listing and an ENI a launch is still using. +func TestListAttachedInstanceENIsIgnoresZeroCreatedAt(t *testing.T) { + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24") + + eniID := createAutoENI(t, svc, subnetID, "i-undated", nil, time.Hour) + _, err := svc.AttachENI(testAccountID, eniID, "i-undated", 0) + require.NoError(t, err) + require.NoError(t, svc.UpdateENI(testAccountID, eniID, func(rec *ENIRecord) { + rec.CreatedAt = time.Time{} + })) + + got, err := svc.ListAttachedInstanceENIs(context.Background(), 15*time.Minute) + require.NoError(t, err) + assert.Empty(t, got) +} + // TestAbandonedENIBlocksSecurityGroupDelete pins the consequence that makes this // worth sweeping: the SG dependency check counts an abandoned ENI, so the group // stays undeletable for as long as the record survives.