ateomnet: move the actor nftables table to the inet family

internal/ateomnet/net.go

@@ -229,8 +229,15 @@ func InstallActorNftablesRules(egressPort uint16) error {
 	// rules in an ateom-owned table makes cleanup simple and avoids mutating
 	// Kubernetes or CNI-managed chains directly.
 	//
-	// TODO: Add IPv6 veth addressing, forwarding, and nftables rules once actor
-	// networking supports dual-stack pods. The current actor network is IPv4-only.
+	// The table is in the inet family so one table can carry both address
+	// families once the actor veth is dual-stack. NAT there needs Linux 5.2.
+	// A bare payload match in that family is ambiguous, so every IPv4 match
+	// opens with an NFPROTO comparison. An inet nat chain registers the nat
+	// hooks for both families, so IPv6 traffic in this netns is conntracked
+	// where an ip table left it untracked.
+	//
+	// TODO(#246): Add the IPv6 veth addressing and forwarding this table is
+	// waiting on. The actor network itself is still IPv4-only.
 	//
 	// The rules do three things:
 	//
@@ -248,7 +255,7 @@ func InstallActorNftablesRules(egressPort uint16) error {
 
 	c := &nftables.Conn{}
 	table := &nftables.Table{
-		Family: nftables.TableFamilyIPv4,
+		Family: nftables.TableFamilyINet,
 		Name:   ActorNftTableName,
 	}
 	c.AddTable(table)
@@ -274,7 +281,7 @@ func InstallActorNftablesRules(egressPort uint16) error {
 	c.AddRule(&nftables.Rule{
 		Table: table,
 		Chain: postrouting,
-		Exprs: append(IPSourceEqual(ActorVethIP), &expr.Masq{}),
+		Exprs: append(ipv4SourceEqual(ActorVethIP), &expr.Masq{}),
 	})
 
 	acceptPolicy := nftables.ChainPolicyAccept
@@ -286,6 +293,9 @@ func InstallActorNftablesRules(egressPort uint16) error {
 		Priority: nftables.ChainPriorityFilter,
 		Policy:   &acceptPolicy,
 	})
+	// Unqualified, so this accepts forwarded IPv6 too -- what the actor needs
+	// once it is dual-stack. accept is per-table, so it cannot override a drop
+	// from the CNI's own forward chains.
 	c.AddRule(&nftables.Rule{
 		Table: table,
 		Chain: forward,
@@ -305,30 +315,51 @@ func RemoveActorNftablesRules() error {
 	// Delete the whole ateom nftables table if it exists. The table is
 	// per-worker and currently per-active-actor because this worker path runs at
 	// most one actor at a time. Missing tables are treated as already clean.
+	//
+	// Both families are swept, not just the inet one this installs into: a table
+	// name is unique per family, so an ip table left by an earlier ateom would
+	// survive every later cleanup and keep redirecting alongside the new one.
+	// The pod netns outlives an in-place container restart, so an ateom that
+	// predates the inet table can share a netns with one that follows it
+	// wherever WorkerPool.spec.ateomImage is a mutable tag -- the dev loop.
+	// TODO(ypgao): Drop the ip sweep once no live pod can predate this change.
 	c := &nftables.Conn{}
-	tables, err := c.ListTablesOfFamily(nftables.TableFamilyIPv4)
-	if err != nil {
-		return fmt.Errorf("while listing nftables tables: %w", err)
-	}
-	for _, table := range tables {
-		if table.Name != ActorNftTableName {
-			continue
+	for _, family := range []struct {
+		name string
+		id   nftables.TableFamily
+	}{{"inet", nftables.TableFamilyINet}, {"ip", nftables.TableFamilyIPv4}} {
+		tables, err := c.ListTablesOfFamily(family.id)
+		if err != nil {
+			return fmt.Errorf("while listing %s nftables tables: %w", family.name, err)
 		}
-		c.DelTable(table)
-		if err := c.Flush(); err != nil {
-			return fmt.Errorf("while deleting actor nftables table: %w", err)
+		for _, table := range tables {
+			if table.Name != ActorNftTableName {
+				continue
+			}
+			c.DelTable(table)
+			if err := c.Flush(); err != nil {
+				return fmt.Errorf("while deleting the %s actor nftables table: %w", family.name, err)
+			}
 		}
-		return nil
 	}
 	return nil
 }
 
-func IPSourceEqual(ip string) []expr.Any {
-	return IPPayloadEqual(12, ip)
+func ipv4SourceEqual(ip string) []expr.Any {
+	return ipv4PayloadEqual(12, ip)
 }
 
-func IPPayloadEqual(offset uint32, ip string) []expr.Any {
+// ipv4PayloadEqual matches a 4-byte IPv4 network-header field. The leading
+// nfproto comparison is what makes it safe in the inet table: without it the
+// payload load would read the same offset out of an IPv6 header.
+func ipv4PayloadEqual(offset uint32, ip string) []expr.Any {
 	return []expr.Any{
+		&expr.Meta{Key: expr.MetaKeyNFPROTO, Register: 1},
+		&expr.Cmp{
+			Op:       expr.CmpOpEq,
+			Register: 1,
+			Data:     []byte{unix.NFPROTO_IPV4},
+		},
 		&expr.Payload{
 			DestRegister: 1,
 			Base:         expr.PayloadBaseNetworkHeader,
@@ -361,7 +392,7 @@ func ActorEgressRedirectRule(table *nftables.Table, chain *nftables.Chain, port
 	if port == 0 {
 		return nil
 	}
-	exprs := append(IPSourceEqual(ActorVethIP), TCPProtocol()...)
+	exprs := append(ipv4SourceEqual(ActorVethIP), TCPProtocol()...)
 	exprs = append(exprs,
 		&expr.Immediate{
 			Register: 1,

internal/ateomnet/net_linux_test.go

@@ -17,13 +17,16 @@
 package ateomnet
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"runtime"
 	"testing"
 
 	"github.com/agent-substrate/substrate/internal/roottest"
 	"github.com/google/nftables"
+	"github.com/google/nftables/binaryutil"
+	"github.com/google/nftables/expr"
 	"github.com/vishvananda/netlink"
 	"github.com/vishvananda/netns"
 )
@@ -75,17 +78,51 @@ func withTestNetNS(t *testing.T, fn func(interior netns.NsHandle)) {
 	fn(interior)
 }
 
-// requireNftables skips when the kernel in this environment cannot serve the
-// nftables netlink API at all, which SetupActorNetwork needs and which is a
-// property of the machine rather than of the code under test.
+// requireNftables skips when this kernel cannot serve what SetupActorNetwork
+// installs, which is a property of the machine rather than of the code under
+// test. Listing the inet family is not a sufficient probe: inet filter is far
+// older than inet nat, which needs Linux 5.2, so this builds and drops a nat
+// chain in the family the actor table uses.
 func requireNftables(t *testing.T) {
 	t.Helper()
 	c := &nftables.Conn{}
-	if _, err := c.ListTablesOfFamily(nftables.TableFamilyIPv4); err != nil {
-		t.Skipf("nftables unavailable in this environment: %v", err)
+	probe := c.AddTable(&nftables.Table{Family: nftables.TableFamilyINet, Name: "ateom_nft_probe"})
+	c.AddChain(&nftables.Chain{
+		Name:     "prerouting",
+		Table:    probe,
+		Type:     nftables.ChainTypeNAT,
+		Hooknum:  nftables.ChainHookPrerouting,
+		Priority: nftables.ChainPriorityNATDest,
+	})
+	if err := c.Flush(); err != nil {
+		t.Skipf("nftables inet nat unavailable in this environment: %v", err)
+	}
+	c.DelTable(probe)
+	if err := c.Flush(); err != nil {
+		t.Fatalf("deleting the nftables probe table: %v", err)
 	}
 }
 
+// actorNftTableExists reports whether the actor table is present in the family
+// InstallActorNftablesRules creates it in. The family is load-bearing:
+// ListTablesOfFamily puts it in the netlink dump header, so the kernel filters
+// the dump and a query for the wrong family comes back empty rather than
+// erroring.
+func actorNftTableExists(t *testing.T) bool {
+	t.Helper()
+	c := &nftables.Conn{}
+	tables, err := c.ListTablesOfFamily(nftables.TableFamilyINet)
+	if err != nil {
+		t.Fatalf("listing inet nftables tables: %v", err)
+	}
+	for _, table := range tables {
+		if table.Name == ActorNftTableName {
+			return true
+		}
+	}
+	return false
+}
+
 // linkByName returns the link, or nil when it does not exist.
 func linkByName(t *testing.T, name string) netlink.Link {
 	t.Helper()
@@ -215,9 +252,20 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) {
 			if linkByName(t, HostVethName) == nil {
 				t.Fatalf("host veth %q missing after activation %d", HostVethName, i)
 			}
+			if !actorNftTableExists(t) {
+				t.Fatalf("nftables table %q missing after activation %d", ActorNftTableName, i)
+			}
 			if err := CleanupActorNetwork(ctx, interior); err != nil {
 				t.Fatalf("CleanupActorNetwork (activation %d): %v", i, err)
 			}
+			// Install and teardown have to name the same family. When they do not,
+			// teardown's dump comes back empty, its "missing tables are already
+			// clean" path reports success, and the table survives -- so the next
+			// activation stacks another copy of every chain and rule onto it and
+			// the leak is invisible to every other assertion here.
+			if actorNftTableExists(t) {
+				t.Fatalf("nftables table %q survived cleanup after activation %d", ActorNftTableName, i)
+			}
 		}
 
 		// Cleanup is idempotent: the extra call after the loop's last one must
@@ -228,6 +276,9 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) {
 		if stray := linkByName(t, HostVethName); stray != nil {
 			t.Errorf("host veth %q survived cleanup", HostVethName)
 		}
+		if actorNftTableExists(t) {
+			t.Errorf("nftables table %q survived a repeated cleanup", ActorNftTableName)
+		}
 		if err := NetNSDo(ctx, interior, func(context.Context) error {
 			if stray := linkByName(t, ActorVethName); stray != nil {
 				t.Errorf("actor veth %q survived cleanup", ActorVethName)
@@ -239,6 +290,93 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) {
 	})
 }
 
+// TestRemoveActorNftablesRulesSweepsIPv4Family covers the upgrade case: a
+// worker whose previous ateom created the actor table in the ip family. Table
+// names are unique per family, so an inet-only cleanup could never see that
+// table, and it would have kept redirecting alongside the inet one installed
+// next to it.
+func TestRemoveActorNftablesRulesSweepsIPv4Family(t *testing.T) {
+	roottest.Require(t, "creating network namespaces and nftables rules")
+
+	withTestNetNS(t, func(netns.NsHandle) {
+		requireNftables(t)
+
+		c := &nftables.Conn{}
+		c.AddTable(&nftables.Table{Family: nftables.TableFamilyIPv4, Name: ActorNftTableName})
+		if err := c.Flush(); err != nil {
+			t.Fatalf("creating the stand-in ip actor table: %v", err)
+		}
+
+		if err := RemoveActorNftablesRules(); err != nil {
+			t.Fatalf("RemoveActorNftablesRules: %v", err)
+		}
+
+		tables, err := c.ListTablesOfFamily(nftables.TableFamilyIPv4)
+		if err != nil {
+			t.Fatalf("listing ip nftables tables: %v", err)
+		}
+		for _, table := range tables {
+			if table.Name == ActorNftTableName {
+				t.Fatal("the ip actor table survived cleanup")
+			}
+		}
+	})
+}
+
+// TestSetupActorNetworkInstallsEgressRedirect covers the rule no other test in
+// this package builds: they all leave EgressRedirectPort zero, so the kernel
+// never sees the redirect. Its acceptance is not implied by the masquerade rule
+// next to it -- redirect in the inet family is separate kernel support from the
+// nat chain type -- and it is the rule the whole actor egress path rides on.
+func TestSetupActorNetworkInstallsEgressRedirect(t *testing.T) {
+	roottest.Require(t, "creating network namespaces, veth pairs, and nftables rules")
+	ctx := context.Background()
+
+	const egressPort = 15001
+
+	withTestNetNS(t, func(interior netns.NsHandle) {
+		requireNftables(t)
+
+		if err := SetupActorNetwork(ctx, NetworkConfig{
+			InteriorNetNS:      interior,
+			EgressRedirectPort: egressPort,
+		}); err != nil {
+			t.Fatalf("SetupActorNetwork: %v", err)
+		}
+
+		c := &nftables.Conn{}
+		rules, err := c.GetRules(
+			&nftables.Table{Family: nftables.TableFamilyINet, Name: ActorNftTableName},
+			&nftables.Chain{Name: "prerouting"},
+		)
+		if err != nil {
+			t.Fatalf("listing prerouting rules of the actor table: %v", err)
+		}
+		if len(rules) != 1 {
+			t.Fatalf("prerouting holds %d rules, want the egress redirect alone", len(rules))
+		}
+
+		// Read back what the kernel stored rather than what the builder emitted:
+		// TestActorNftablesRuleExprs already pins the builder, and what is in
+		// doubt here is whether an inet nat chain takes these expressions at all.
+		var haveNFProto, havePort, haveRedir bool
+		for _, e := range rules[0].Exprs {
+			switch e := e.(type) {
+			case *expr.Meta:
+				haveNFProto = haveNFProto || e.Key == expr.MetaKeyNFPROTO
+			case *expr.Immediate:
+				havePort = havePort || bytes.Equal(e.Data, binaryutil.BigEndian.PutUint16(egressPort))
+			case *expr.Redir:
+				haveRedir = true
+			}
+		}
+		if !haveNFProto || !havePort || !haveRedir {
+			t.Errorf("installed redirect has nfproto=%t port=%t redir=%t, want all three, got %v",
+				haveNFProto, havePort, haveRedir, rules[0].Exprs)
+		}
+	})
+}
+
 // TestSetupActorNetworkHostVethHWAddr covers the micro-VM requirement: a CH
 // snapshot freezes the guest's ARP entry for the gateway, so the worker-side
 // veth MAC has to be exactly the one the caller asked for, on every pod.