From 2a0eafc20fd56a1386258d23033a064950bf70ed Mon Sep 17 00:00:00 2001 From: Michael Ben-Ami Date: Thu, 11 Jun 2026 12:03:20 -0400 Subject: [PATCH] feature/conn25: drop returned error from NewFlow signature The returned error in the signature is left over from previous implementations and was only returning nil. If we know NewFlow will succeed we can fire a create hook (implemented in a future commit) before NewFlow, which will prevent a remove hook for a flow from firing before the create hook for the same flow. Updates tailscale/corp#38630 Signed-off-by: Michael Ben-Ami --- feature/conn25/datapath.go | 16 ++++++---------- feature/conn25/flowtable.go | 4 +--- feature/conn25/flowtable_test.go | 33 +++++++++++++------------------- 3 files changed, 20 insertions(+), 33 deletions(-) diff --git a/feature/conn25/datapath.go b/feature/conn25/datapath.go index 43daa89db..fac18667a 100644 --- a/feature/conn25/datapath.go +++ b/feature/conn25/datapath.go @@ -175,13 +175,11 @@ func (dh *datapathHandler) HandlePacketFromWireGuard(p *packet.Parsed, tun *tstu Tuple: flowtrack.MakeTuple(p.IPProto, netip.AddrPortFrom(realIP, p.Dst.Port()), p.Src), Action: dh.snatAction(transitIP), } - if err := dh.connectorFlowTable.NewFlow(FlowData{ + dh.connectorFlowTable.NewFlow(FlowData{ FromTun: incoming, FromWG: outgoing, - }); err != nil { - dh.debugLogf("error installing flow, passing packet unmodified: %v", err) - return filter.Accept - } + }) + outgoing.Action(p) return filter.Accept } @@ -244,13 +242,11 @@ func (dh *datapathHandler) HandlePacketFromTunDevice(p *packet.Parsed) filter.Re Tuple: flowtrack.MakeTuple(p.IPProto, netip.AddrPortFrom(transitIP, p.Dst.Port()), p.Src), Action: dh.snatAction(magicIP), } - if err := dh.clientFlowTable.NewFlow(FlowData{ + dh.clientFlowTable.NewFlow(FlowData{ FromTun: outgoing, FromWG: incoming, - }); err != nil { - dh.debugLogf("error installing flow from tun device, passing packet unmodified: %v", err) - return filter.Accept - } + }) + outgoing.Action(p) return filter.Accept } diff --git a/feature/conn25/flowtable.go b/feature/conn25/flowtable.go index 22d5654bb..3e9415543 100644 --- a/feature/conn25/flowtable.go +++ b/feature/conn25/flowtable.go @@ -196,7 +196,7 @@ func (t *FlowTable) lookup(k flowtrack.Tuple, dir Origin) (PacketAction, bool) { // would cause the table to exceed its maximum size, the least recently used // (looked-up or created) flow is evicted. data is not validated, the caller must // supply non-nil packet actions. -func (t *FlowTable) NewFlow(data FlowData) error { +func (t *FlowTable) NewFlow(data FlowData) { t.mu.Lock() defer t.mu.Unlock() @@ -217,8 +217,6 @@ func (t *FlowTable) NewFlow(data FlowData) error { t.fromTunCache[data.FromTun.Tuple] = ele t.fromWGCache[data.FromWG.Tuple] = ele - - return nil } // StartExpiredSweeper starts a sweeper that removes idle flows that have diff --git a/feature/conn25/flowtable_test.go b/feature/conn25/flowtable_test.go index f8a8f0f0c..6ce289df0 100644 --- a/feature/conn25/flowtable_test.go +++ b/feature/conn25/flowtable_test.go @@ -59,13 +59,6 @@ func mkFlows(n int) []FlowData { return flows } -func mustInstallFlow(t *testing.T, ft *FlowTable, flow FlowData) { - t.Helper() - if err := ft.NewFlow(flow); err != nil { - t.Fatalf("error installing flow: %v", err) - } -} - func assertFlowHit(t *testing.T, ft *FlowTable, dir Origin, tuple flowtrack.Tuple) PacketAction { t.Helper() return assertFlowLookup(t, ft, dir, tuple, true) @@ -113,7 +106,7 @@ func TestFlowTable_NewFlow_Lookup(t *testing.T) { fromWGTuple := mkTuple("4.3.2.2:80", "1.2.3.4:1000") flow1, tunCount1, wgCount1 := mkFlowWithActions(fromTunTuple, fromWGTuple) - mustInstallFlow(t, ft, flow1) + ft.NewFlow(flow1) // Test basic lookups, and perform actions on packet. assertFlowHit(t, ft, FromTun, fromTunTuple)(nilPacket) @@ -136,13 +129,13 @@ func TestFlowTable_NewFlow_Lookup(t *testing.T) { // Overwriting from-tun tuple removes the from-wg tuple as well. newFromWGTuple := mkTuple("9.9.9.9:99", "8.8.8.8:88") flow2 := mkFlow(fromTunTuple, newFromWGTuple) - mustInstallFlow(t, ft, flow2) + ft.NewFlow(flow2) assertFlowMiss(t, ft, FromWireGuard, fromWGTuple) // Overwriting the from-wg tuple removes the from-tun tuple as well. newFromTunTuple := mkTuple("8.8.8.8:88", "9.9.9.9:99") flow3 := mkFlow(newFromTunTuple, newFromWGTuple) - mustInstallFlow(t, ft, flow3) + ft.NewFlow(flow3) assertFlowMiss(t, ft, FromTun, fromTunTuple) } @@ -162,8 +155,8 @@ func TestFlowTable_OneReplacesTwo(t *testing.T) { flow2, tunCount2, wgCount2 := mkFlowWithActions(tunTuple2, wgTuple2) // Install the first two flows. - mustInstallFlow(t, ft, flow1) - mustInstallFlow(t, ft, flow2) + ft.NewFlow(flow1) + ft.NewFlow(flow2) // Confirm they are properly installed through lookups. assertFlowHit(t, ft, FromTun, tunTuple1) @@ -173,7 +166,7 @@ func TestFlowTable_OneReplacesTwo(t *testing.T) { // flow3 tuples overlap with flow1 and flow2. flow3, tunCount3, wgCount3 := mkFlowWithActions(tunTuple1, wgTuple2) - mustInstallFlow(t, ft, flow3) + ft.NewFlow(flow3) // flow3 lookups hit on both of their tuples. tunAction3 := assertFlowHit(t, ft, FromTun, tunTuple1) @@ -213,14 +206,14 @@ func TestFlowTable_Eviction(t *testing.T) { d := mkFlow(dTun, dWG) // Install a and b. - mustInstallFlow(t, ft, a) - mustInstallFlow(t, ft, b) + ft.NewFlow(a) + ft.NewFlow(b) // Move a to the front from tun side, b is ready for eviction. assertFlowHit(t, ft, FromTun, aTun) // Install c. - mustInstallFlow(t, ft, c) + ft.NewFlow(c) // Check b is out. assertFlowMiss(t, ft, FromTun, bTun) @@ -234,7 +227,7 @@ func TestFlowTable_Eviction(t *testing.T) { assertFlowHit(t, ft, FromWireGuard, aWG) // Install d. - mustInstallFlow(t, ft, d) + ft.NewFlow(d) // Check c is out. assertFlowMiss(t, ft, FromTun, cTun) @@ -334,7 +327,7 @@ func TestFlowTable_removeIdle(t *testing.T) { flows := mkFlows(len(tt.flowSpecs)) for i, spec := range tt.flowSpecs { time.Sleep(time.Until(start.Add(spec.installAt))) - mustInstallFlow(t, ft, flows[i]) + ft.NewFlow(flows[i]) } var wantRemovedCount int @@ -365,9 +358,9 @@ func TestFlowTable_removeIdle(t *testing.T) { ft := NewFlowTable(0, WithFlowIdleTimeout(time.Minute)) flows := mkFlows(2) - mustInstallFlow(t, ft, flows[0]) // t=0 (flow 0 install) + ft.NewFlow(flows[0]) // t=0 (flow 0 install) time.Sleep(30 * time.Second) // - mustInstallFlow(t, ft, flows[1]) // t=30s (flow 1 install) + ft.NewFlow(flows[1]) // t=30s (flow 1 install) time.Sleep(60 * time.Second) // assertFlowHit(t, ft, FromTun, flows[0].FromTun.Tuple) // t=90s (flow 0 looked up, lastSeen bumped) time.Sleep(15 * time.Second) //