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)