From cf69cd60078e511fd22352531e450523bfbc5c9b Mon Sep 17 00:00:00 2001 From: joshsiv-mulga Date: Tue, 19 May 2026 10:02:43 +1000 Subject: [PATCH] fix(vpcd): make handleDeleteNAT idempotent on 'already absent' (mulga-siv-107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instance termination fans out through multiple cleanup paths (instanceCleanerAdapter.ReleasePublicIP AND VPCServiceImpl.DeleteNetworkInterface), both of which publish vpc.delete-nat for the same (vpc, logical_ip). The publish is fire-and-forget (utils.PublishNATEvent uses PublishEvent for vpc.delete-nat, not RequestEvent), so callers never branch on the response. The second publisher then triggered a spurious 'ERROR vpcd: failed to delete dnat_and_snat rule ... not found' log line ms after the first delete succeeded — confusing triage signal with no actionable consequence. Add ErrNATNotFound sentinel to the OVN client interface and wrap the not-found return paths in both LiveOVNClient and MockOVNClient. handleDeleteNAT matches errors.Is(err, ErrNATNotFound) and treats that as success (logs INFO 'already absent (idempotent)'). Existing TestTopologyHandler_DeleteNAT_NotFound asserted the opposite contract; the comment claimed EIP disassociation needed the error signal, but DisassociateAddress also fire-and-forgets vpc.delete-nat, so the contract never had a real consumer. Test renamed to _Idempotent with assertSuccess. Other failure modes (router missing, OVN unreachable, transact error) still surface as ERROR + reply with the error string. Refs: mulga-siv-107 --- spinifex/services/vpcd/mock_ovn.go | 4 ++-- spinifex/services/vpcd/ovn.go | 11 +++++++++-- spinifex/services/vpcd/topology.go | 12 ++++++++++++ spinifex/services/vpcd/topology_test.go | 18 ++++++++++++------ 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/spinifex/services/vpcd/mock_ovn.go b/spinifex/services/vpcd/mock_ovn.go index d135203e8..e0a9f0899 100644 --- a/spinifex/services/vpcd/mock_ovn.go +++ b/spinifex/services/vpcd/mock_ovn.go @@ -452,7 +452,7 @@ func (m *MockOVNClient) DeleteNAT(_ context.Context, routerName string, natType, } } if foundUUID == "" { - return fmt.Errorf("NAT %s %s not found", natType, logicalIP) + return fmt.Errorf("NAT %s %s: %w", natType, logicalIP, ErrNATNotFound) } // Remove from router's NAT list for i, uuid := range lr.NAT { @@ -479,7 +479,7 @@ func (m *MockOVNClient) DeleteNATByExternalIP(_ context.Context, routerName stri } } if len(foundUUIDs) == 0 { - return fmt.Errorf("NAT %s external_ip=%s not found", natType, externalIP) + return fmt.Errorf("NAT %s external_ip=%s: %w", natType, externalIP, ErrNATNotFound) } for _, foundUUID := range foundUUIDs { for i, uuid := range lr.NAT { diff --git a/spinifex/services/vpcd/ovn.go b/spinifex/services/vpcd/ovn.go index ca4713774..78d14c3c5 100644 --- a/spinifex/services/vpcd/ovn.go +++ b/spinifex/services/vpcd/ovn.go @@ -2,6 +2,7 @@ package vpcd import ( "context" + "errors" "fmt" "log/slog" "slices" @@ -12,6 +13,12 @@ import ( "github.com/ovn-kubernetes/libovsdb/ovsdb" ) +// ErrNATNotFound is returned by DeleteNAT / DeleteNATByExternalIP when the +// rule is already absent. Callers that want idempotent semantics (e.g. the +// vpc.delete-nat NATS handler, which races with vm-cleanup paths that emit +// the same delete) match it with errors.Is and treat as success. +var ErrNATNotFound = errors.New("NAT not found") + // transactOps executes a set of OVSDB operations as a single transaction, // checking both the RPC error and individual operation results. func (c *LiveOVNClient) transactOps(ctx context.Context, ops []ovsdb.Operation) error { @@ -841,7 +848,7 @@ func (c *LiveOVNClient) DeleteNAT(ctx context.Context, routerName string, natTyp return fmt.Errorf("find NAT: %w", err) } if len(nats) == 0 { - return fmt.Errorf("NAT %s %s not found", natType, logicalIP) + return fmt.Errorf("NAT %s %s: %w", natType, logicalIP, ErrNATNotFound) } nat := &nats[0] @@ -883,7 +890,7 @@ func (c *LiveOVNClient) DeleteNATByExternalIP(ctx context.Context, routerName st return fmt.Errorf("find NAT by external IP: %w", err) } if len(nats) == 0 { - return fmt.Errorf("NAT %s external_ip=%s not found", natType, externalIP) + return fmt.Errorf("NAT %s external_ip=%s: %w", natType, externalIP, ErrNATNotFound) } lr, err := c.GetLogicalRouter(ctx, routerName) diff --git a/spinifex/services/vpcd/topology.go b/spinifex/services/vpcd/topology.go index 1e96c4325..71cf2d607 100644 --- a/spinifex/services/vpcd/topology.go +++ b/spinifex/services/vpcd/topology.go @@ -3,6 +3,7 @@ package vpcd import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "net" @@ -1380,6 +1381,17 @@ func (h *TopologyHandler) handleDeleteNAT(msg *nats.Msg) { routerName := "vpc-" + evt.VpcId if err := h.ovn.DeleteNAT(ctx, routerName, "dnat_and_snat", evt.LogicalIP); err != nil { + // vpc.delete-nat is published from multiple cleanup paths (per-VM + // ReleasePublicIP, ENI delete, VPC delete sweep), so a duplicate + // delete for the same logical_ip is expected. Treat "already gone" + // as success — the caller's intent ("ensure this NAT is removed") + // is satisfied whether we did the delete or someone else did. + if errors.Is(err, ErrNATNotFound) { + slog.Info("vpcd: dnat_and_snat rule already absent (idempotent)", + "router", routerName, "logical_ip", evt.LogicalIP) + respond(msg, nil) + return + } slog.Error("vpcd: failed to delete dnat_and_snat rule", "router", routerName, "logicalIP", evt.LogicalIP, "err", err) respond(msg, err) return diff --git a/spinifex/services/vpcd/topology_test.go b/spinifex/services/vpcd/topology_test.go index a5e6ef997..bf462482b 100644 --- a/spinifex/services/vpcd/topology_test.go +++ b/spinifex/services/vpcd/topology_test.go @@ -4015,11 +4015,17 @@ func TestTopologyHandler_DeleteNAT_Success(t *testing.T) { assert.Empty(t, lr.NAT, "NAT rule must be removed") } -// TestTopologyHandler_DeleteNAT_NotFound: an idempotent delete-by-LogicalIP -// for a NAT that doesn't exist must return a clear error, not silently -// succeed. Otherwise EIP disassociation can't tell whether it actually -// disabled the NAT or hit a stale fixture. -func TestTopologyHandler_DeleteNAT_NotFound(t *testing.T) { +// TestTopologyHandler_DeleteNAT_NotFound_Idempotent: instance termination +// fans out through multiple cleanup paths (ReleasePublicIP via +// instanceCleanerAdapter AND DeleteNetworkInterface in the VPC service), and +// both publish vpc.delete-nat for the same (vpc, logical_ip). The publish is +// fire-and-forget (utils.PublishNATEvent for vpc.delete-nat), so callers +// never branch on the response — a hard error on second delete just +// pollutes triage logs without surfacing any actionable failure. Treat +// "rule already absent" as success so the second publisher doesn't emit a +// spurious ERROR. The handler still returns an error for other failure +// modes (router missing, OVN unreachable, transact failure). +func TestTopologyHandler_DeleteNAT_NotFound_Idempotent(t *testing.T) { _, nc := startTestNATS(t) mock := NewMockOVNClient() _ = mock.Connect(context.Background()) @@ -4040,5 +4046,5 @@ func TestTopologyHandler_DeleteNAT_NotFound(t *testing.T) { data, _ := json.Marshal(delEvt) resp, err := nc.Request(TopicDeleteNAT, data, 5_000_000_000) require.NoError(t, err) - assertFailure(t, resp, "delete-nat for non-existent rule must fail") + assertSuccess(t, resp, "delete-nat for non-existent rule must be idempotent (mulga-siv-107)") }