From 7433c79c6bf4ac3cf5fffe7ac2e7f303b7a0a5f0 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Wed, 11 Nov 2026 20:59:50 -0600 Subject: [PATCH] fix(ingress-sidecar): remove a redirect route only via its own VRF The main-table /128 redirect for a backend was deleted by destination alone. When two VPCs carry the same backend address, as when IPAM hands a deleted Network's subnet to the next one, the live VPC's RouteReplace takes over the route and the deleted VPC's teardown then removes it. Nothing reinstalls it, so connects to the live backend fail until the slice or datapath changes. Name the VRF device in the delete so the kernel removes the route only while it still points at the tearing-down VPC, and treat a missing VRF as nothing left to remove. Refs datum-cloud/engineering#467 --- internal/ingresssidecar/backend.go | 2 +- internal/ingresssidecar/ebpfdatapath.go | 39 +++++-- .../redirectroute_integration_test.go | 107 ++++++++++++++++++ .../routeinventory_integration_test.go | 2 +- 4 files changed, 139 insertions(+), 11 deletions(-) create mode 100644 internal/ingresssidecar/redirectroute_integration_test.go diff --git a/internal/ingresssidecar/backend.go b/internal/ingresssidecar/backend.go index 39cae467..97c96b42 100644 --- a/internal/ingresssidecar/backend.go +++ b/internal/ingresssidecar/backend.go @@ -153,7 +153,7 @@ func (kernelBackend) EnsureRoute(prefix *net.IPNet, sid net.IP, tableID uint32) } func (kernelBackend) RemoveRoute(prefix *net.IPNet, tableID uint32) error { - if err := removeRedirectRoute(prefix); err != nil { + if err := removeRedirectRoute(prefix, tableID); err != nil { return fmt.Errorf("remove main-table redirect route for %s: %w", prefix, err) } if err := srv6.RouteEgressDel(prefix, tableID); err != nil { diff --git a/internal/ingresssidecar/ebpfdatapath.go b/internal/ingresssidecar/ebpfdatapath.go index e8eee53b..027aa9b0 100644 --- a/internal/ingresssidecar/ebpfdatapath.go +++ b/internal/ingresssidecar/ebpfdatapath.go @@ -101,9 +101,13 @@ func vrfLinkForTable(tableID uint32) (*netlink.Vrf, error) { return link, nil } } - return nil, fmt.Errorf("no VRF interface found for table %d", tableID) + return nil, fmt.Errorf("table %d: %w", tableID, errNoVRFForTable) } +// errNoVRFForTable is vrfLinkForTable's error when no VRF device uses the +// table. +var errNoVRFForTable = errors.New("no VRF interface found") + // egressVethNames derives a VPC's veth pair names from tableID alone, since the // callers carry only a tableID forward. Collision-free because table IDs are // already unique per VPC in this pod, and comfortably inside IFNAMSIZ, since a @@ -564,17 +568,34 @@ func ensureRedirectRoute(prefix *net.IPNet, tableID uint32) error { } // removeRedirectRoute removes the main-table route ensureRedirectRoute -// installed for prefix. Deleting by destination and table alone is enough to -// identify it. +// installed for prefix toward tableID's VRF device, and only that route. +// +// The main table holds one route per destination, and ensureRedirectRoute +// replaces it. Two VPCs can carry the same backend address, as when IPAM hands +// a deleted Network's subnet to the next one, so by the time a deleted VPC's +// route is torn down the destination may already point at the live VPC's +// VRF. The delete therefore names the device as well: the kernel then removes +// the route only while it still points at this VPC's VRF. +// +// A missing VRF device means the kernel already dropped every route through +// it, so there is nothing of this VPC's left to remove, and any route still +// held for prefix belongs to another VPC. // // An absent route is the outcome a removal asks for, so ESRCH counts as // success. Reporting failure instead retries a teardown that can never -// complete, and keeps the entry alive to collide with the next tenant given -// this address. -func removeRedirectRoute(prefix *net.IPNet) error { - err := netlink.RouteDel(&netlink.Route{ - Dst: prefix, - Table: unix.RT_TABLE_MAIN, +// complete. +func removeRedirectRoute(prefix *net.IPNet, tableID uint32) error { + link, err := vrfLinkForTable(tableID) + if errors.Is(err, errNoVRFForTable) { + return nil + } + if err != nil { + return err + } + err = netlink.RouteDel(&netlink.Route{ + Dst: prefix, + Table: unix.RT_TABLE_MAIN, + LinkIndex: link.Attrs().Index, }) if errors.Is(err, unix.ESRCH) || errors.Is(err, unix.ENOENT) { return nil diff --git a/internal/ingresssidecar/redirectroute_integration_test.go b/internal/ingresssidecar/redirectroute_integration_test.go new file mode 100644 index 00000000..514611f7 --- /dev/null +++ b/internal/ingresssidecar/redirectroute_integration_test.go @@ -0,0 +1,107 @@ +// Copyright 2026 Datum Cloud, Inc. +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +package ingresssidecar + +import ( + "fmt" + "net" + "testing" + + "github.com/containernetworking/plugins/pkg/ns" + "github.com/vishvananda/netlink" + "golang.org/x/sys/unix" + + "go.datum.net/galactic/internal/plumbing/vrf" +) + +// mainTableRedirect returns the ifindex the main-table route for prefix leaves +// through, or 0 when there is none. +func mainTableRedirect(prefix *net.IPNet) (int, error) { + routes, err := netlink.RouteListFiltered(netlink.FAMILY_V6, + &netlink.Route{Dst: prefix, Table: unix.RT_TABLE_MAIN}, + netlink.RT_FILTER_DST|netlink.RT_FILTER_TABLE) + if err != nil { + return 0, err + } + for _, r := range routes { + if r.Dst != nil && r.Dst.String() == prefix.String() { + return r.LinkIndex, nil + } + } + return 0, nil +} + +// TestRemoveRedirectRoute_KeepsAnotherVPCsRoute: two VPCs carry the same +// backend address, as when IPAM hands a deleted Network's subnet to the next +// one. The live VPC's redirect replaced the deleted VPC's, so tearing down the +// deleted VPC's route must leave the live one alone. Before the fix, the delete +// matched the destination only and removed the live VPC's route. +func TestRemoveRedirectRoute_KeepsAnotherVPCsRoute(t *testing.T) { + requireRoot(t) + + nsObj, err := ns.TempNetNS() + if err != nil { + t.Fatalf("create test netns: %v", err) + } + t.Cleanup(func() { _ = nsObj.Close() }) + + const ( + oldTable = vrf.SidecarTableIDBase + 12 + newTable = vrf.SidecarTableIDBase + 13 + gone = vrf.SidecarTableIDBase + 14 // no VRF device uses it + ) + backend := mustPrefix(t, "fd20:0:1e::1:0:0") + + err = nsObj.Do(func(_ ns.NetNS) error { + var newIndex int + for _, v := range []struct { + name string + table uint32 + }{{"ivstestold", oldTable}, {"ivstestnew", newTable}} { + link := &netlink.Vrf{LinkAttrs: netlink.LinkAttrs{Name: v.name}, Table: v.table} + if err := netlink.LinkAdd(link); err != nil { + return fmt.Errorf("add VRF %s: %w", v.name, err) + } + if err := netlink.LinkSetUp(link); err != nil { + return fmt.Errorf("set VRF %s up: %w", v.name, err) + } + newIndex = link.Attrs().Index + } + + if err := ensureRedirectRoute(backend, oldTable); err != nil { + return err + } + if err := ensureRedirectRoute(backend, newTable); err != nil { + return err + } + + if err := removeRedirectRoute(backend, oldTable); err != nil { + return fmt.Errorf("remove deleted VPC's route: %w", err) + } + if got, err := mainTableRedirect(backend); err != nil || got != newIndex { + return fmt.Errorf("after removing the deleted VPC's route: redirect via ifindex %d (err %w), want live VPC's %d", + got, err, newIndex) + } + + if err := removeRedirectRoute(backend, gone); err != nil { + return fmt.Errorf("remove for a table with no VRF: %w", err) + } + if got, err := mainTableRedirect(backend); err != nil || got != newIndex { + return fmt.Errorf("after removal for a table with no VRF: redirect via ifindex %d (err %w), want %d", + got, err, newIndex) + } + + if err := removeRedirectRoute(backend, newTable); err != nil { + return fmt.Errorf("remove live VPC's route: %w", err) + } + if got, err := mainTableRedirect(backend); err != nil || got != 0 { + return fmt.Errorf("after removing the owner's route: redirect via ifindex %d (err %w), want none", got, err) + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} diff --git a/internal/ingresssidecar/routeinventory_integration_test.go b/internal/ingresssidecar/routeinventory_integration_test.go index 7aa889a9..a1ee7bc6 100644 --- a/internal/ingresssidecar/routeinventory_integration_test.go +++ b/internal/ingresssidecar/routeinventory_integration_test.go @@ -239,7 +239,7 @@ func TestListRoutes_SurvivesLinkAndRouteChurn(t *testing.T) { } churn(func(i int) { _ = ensureRedirectRoute(mustPrefix(t, fmt.Sprintf("fd00:8::%x", i%4+1)), tableID) - _ = removeRedirectRoute(mustPrefix(t, fmt.Sprintf("fd00:8::%x", (i+2)%4+1))) + _ = removeRedirectRoute(mustPrefix(t, fmt.Sprintf("fd00:8::%x", (i+2)%4+1)), tableID) }) t.Cleanup(func() { close(stop)