From 0f965e134d231dbd1feeb20524d336e495fde203 Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Fri, 4 Sep 2026 16:52:25 -0700 Subject: [PATCH] ateomnet: move the actor nftables table to the inet family Move the actor's NAT and filter rules from a table of family ip to one of family inet, so one table can hold both address families when the actor veth becomes dual-stack. Teardown sweeps tables of both ip and inet to handle the ip table an earlier ateom left behind. Tested: the kernel tests in net_linux_test.go, run as root on Linux, prove the kernel accepts the redirect in an inet nat chain, repeated setup/cleanup leaves no table behind, and teardown clears ip, inet, or both. --- internal/ateomnet/net.go | 47 ++++++-- internal/ateomnet/net_linux_test.go | 163 ++++++++++++++++++++++++-- internal/ateomnet/rules_linux_test.go | 83 +++++++++++++ 3 files changed, 271 insertions(+), 22 deletions(-) create mode 100644 internal/ateomnet/rules_linux_test.go diff --git a/internal/ateomnet/net.go b/internal/ateomnet/net.go index 91203a8e04..036ac88243 100644 --- a/internal/ateomnet/net.go +++ b/internal/ateomnet/net.go @@ -229,8 +229,8 @@ 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. + // TODO(#246): Add the IPv6 veth addressing and forwarding. The actor + // network itself is still IPv4-only. // // The rules do three things: // @@ -247,8 +247,10 @@ func InstallActorNftablesRules(egressPort uint16) error { } c := &nftables.Conn{} + // The inet family lets one table carry both address families once the + // actor veth is dual-stack. table := &nftables.Table{ - Family: nftables.TableFamilyIPv4, + Family: nftables.TableFamilyINet, Name: ActorNftTableName, } c.AddTable(table) @@ -274,7 +276,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 @@ -295,7 +297,7 @@ func InstallActorNftablesRules(egressPort uint16) error { }) if err := c.Flush(); err != nil { - return fmt.Errorf("while installing actor nftables rules: %w", err) + return fmt.Errorf("while installing actor nftables rules (inet nat needs Linux 5.2 or later): %w", err) } return nil } @@ -305,10 +307,23 @@ 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. + // + // A table name is unique per family, so both families are swept: an ateom + // from before the inet move can have left an ip table in this netns. + // TODO(ypgao): Drop the ip sweep once no live pod can predate this change. + return errors.Join( + removeActorNftablesTable("inet", nftables.TableFamilyINet), + removeActorNftablesTable("ip", nftables.TableFamilyIPv4), + ) +} + +// removeActorNftablesTable deletes the actor table in one family. A missing +// table is treated as already clean. +func removeActorNftablesTable(name string, family nftables.TableFamily) error { c := &nftables.Conn{} - tables, err := c.ListTablesOfFamily(nftables.TableFamilyIPv4) + tables, err := c.ListTablesOfFamily(family) if err != nil { - return fmt.Errorf("while listing nftables tables: %w", err) + return fmt.Errorf("while listing %s nftables tables: %w", name, err) } for _, table := range tables { if table.Name != ActorNftTableName { @@ -316,19 +331,27 @@ func RemoveActorNftablesRules() error { } c.DelTable(table) if err := c.Flush(); err != nil { - return fmt.Errorf("while deleting actor nftables table: %w", err) + return fmt.Errorf("while deleting the %s actor nftables table: %w", 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, guarded by an +// nfproto comparison so it is safe in the inet table. +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 +384,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, diff --git a/internal/ateomnet/net_linux_test.go b/internal/ateomnet/net_linux_test.go index b9c8ac45f4..e1f2297313 100644 --- a/internal/ateomnet/net_linux_test.go +++ b/internal/ateomnet/net_linux_test.go @@ -17,6 +17,7 @@ package ateomnet import ( + "bytes" "context" "errors" "runtime" @@ -24,6 +25,8 @@ import ( "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,15 +78,46 @@ 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. -func requireNftables(t *testing.T) { +// skipWithoutInetNAT skips when this kernel cannot register an inet nat +// chain, which needs Linux 5.2 and is a property of the machine rather than +// of the code under test. Call it inside withTestNetNS: the probe creates +// and deletes a real table. +func skipWithoutInetNAT(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 family. +// The family is load-bearing: the kernel filters the dump by it, so a query +// for the wrong family comes back empty rather than erroring. +func actorNftTableExists(t *testing.T, family nftables.TableFamily) bool { + t.Helper() + c := &nftables.Conn{} + tables, err := c.ListTablesOfFamily(family) + if err != nil { + t.Fatalf("listing nftables tables of family %v: %v", family, err) + } + for _, table := range tables { + if table.Name == ActorNftTableName { + return true + } } + return false } // linkByName returns the link, or nil when it does not exist. @@ -126,7 +160,7 @@ func TestSetupActorNetworkFinalState(t *testing.T) { ctx := context.Background() withTestNetNS(t, func(interior netns.NsHandle) { - requireNftables(t) + skipWithoutInetNAT(t) if err := SetupActorNetwork(ctx, NetworkConfig{InteriorNetNS: interior}); err != nil { t.Fatalf("SetupActorNetwork: %v", err) @@ -206,7 +240,7 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) { ctx := context.Background() withTestNetNS(t, func(interior netns.NsHandle) { - requireNftables(t) + skipWithoutInetNAT(t) for i := range 3 { if err := SetupActorNetwork(ctx, NetworkConfig{InteriorNetNS: interior}); err != nil { @@ -215,9 +249,17 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) { if linkByName(t, HostVethName) == nil { t.Fatalf("host veth %q missing after activation %d", HostVethName, i) } + if !actorNftTableExists(t, nftables.TableFamilyINet) { + 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; a mismatch makes + // teardown's dump come back empty and report success. + if actorNftTableExists(t, nftables.TableFamilyINet) { + 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 +270,9 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) { if stray := linkByName(t, HostVethName); stray != nil { t.Errorf("host veth %q survived cleanup", HostVethName) } + if actorNftTableExists(t, nftables.TableFamilyINet) { + 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 +284,104 @@ func TestSetupActorNetworkIsRepeatable(t *testing.T) { }) } +// TestRemoveActorNftablesRules covers cleanup of every family mix, including +// the upgrade case: a worker whose previous ateom created the actor table in +// the ip family. +func TestRemoveActorNftablesRules(t *testing.T) { + roottest.Require(t, "creating network namespaces and nftables rules") + + tests := []struct { + name string + families []nftables.TableFamily + }{{ + name: "ip only", + families: []nftables.TableFamily{nftables.TableFamilyIPv4}, + }, { + name: "ip and inet", + families: []nftables.TableFamily{nftables.TableFamilyIPv4, nftables.TableFamilyINet}, + }, { + name: "inet only", + families: []nftables.TableFamily{nftables.TableFamilyINet}, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + withTestNetNS(t, func(netns.NsHandle) { + skipWithoutInetNAT(t) + + c := &nftables.Conn{} + for _, family := range test.families { + c.AddTable(&nftables.Table{Family: family, Name: ActorNftTableName}) + } + if err := c.Flush(); err != nil { + t.Fatalf("creating the stand-in actor tables: %v", err) + } + + if err := RemoveActorNftablesRules(); err != nil { + t.Fatalf("RemoveActorNftablesRules: %v", err) + } + + for _, family := range []nftables.TableFamily{nftables.TableFamilyIPv4, nftables.TableFamilyINet} { + if actorNftTableExists(t, family) { + t.Errorf("actorNftTableExists(family %v) = true after cleanup, want false", family) + } + } + }) + }) + } +} + +// TestSetupActorNetworkEgressRedirect checks that an inet nat chain accepts +// the redirect: it is separate kernel support from the nat chain type, and it +// is the rule the whole actor egress path rides on. +func TestSetupActorNetworkEgressRedirect(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) { + skipWithoutInetNAT(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, not what the builder emitted: what is + // in doubt 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. @@ -247,7 +390,7 @@ func TestSetupActorNetworkHostVethHWAddr(t *testing.T) { ctx := context.Background() withTestNetNS(t, func(interior netns.NsHandle) { - requireNftables(t) + skipWithoutInetNAT(t) want := MustParseMAC("02:a8:1e:00:00:01") if err := SetupActorNetwork(ctx, NetworkConfig{ @@ -277,7 +420,7 @@ func TestSetupActorNetworkSweepsInteriorLinks(t *testing.T) { ctx := context.Background() withTestNetNS(t, func(interior netns.NsHandle) { - requireNftables(t) + skipWithoutInetNAT(t) const leftover = "stale-tap0" if err := NetNSDo(ctx, interior, func(context.Context) error { diff --git a/internal/ateomnet/rules_linux_test.go b/internal/ateomnet/rules_linux_test.go new file mode 100644 index 0000000000..d60d99ba69 --- /dev/null +++ b/internal/ateomnet/rules_linux_test.go @@ -0,0 +1,83 @@ +//go:build linux + +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ateomnet + +import ( + "fmt" + "reflect" + "testing" + + "github.com/google/nftables/expr" +) + +// TestActorNftablesRuleExprs pins the wire encoding of the expressions +// installed into the inet actor table, notably the nfproto guard in front of +// every IPv4 match. +func TestActorNftablesRuleExprs(t *testing.T) { + // meta nfproto ipv4; ip saddr 169.254.17.2 + actorSourceIsIPv4 := []expr.Any{ + &expr.Meta{Key: expr.MetaKeyNFPROTO, Register: 1}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{2}}, + &expr.Payload{DestRegister: 1, Base: expr.PayloadBaseNetworkHeader, Offset: 12, Len: 4}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{169, 254, 17, 2}}, + } + + tests := []struct { + name string + got []expr.Any + want []expr.Any + }{{ + name: "source match guards the payload load with nfproto", + got: ipv4SourceEqual(ActorVethIP), + want: actorSourceIsIPv4, + }, { + name: "egress redirect matches actor IPv4 TCP and redirects to the port", + got: ActorEgressRedirectRule(nil, nil, 15001).Exprs, + want: append(append([]expr.Any{}, actorSourceIsIPv4...), + // meta l4proto tcp; redirect to :15001 + &expr.Meta{Key: expr.MetaKeyL4PROTO, Register: 1}, + &expr.Cmp{Op: expr.CmpOpEq, Register: 1, Data: []byte{6}}, + &expr.Immediate{Register: 1, Data: []byte{0x3a, 0x99}}, + &expr.Redir{RegisterProtoMin: 1}, + ), + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if !reflect.DeepEqual(test.got, test.want) { + t.Errorf("rule exprs mismatch:\ngot:\n%s\nwant:\n%s", formatExprs(test.got), formatExprs(test.want)) + } + }) + } +} + +// TestActorEgressRedirectRuleDisabled covers the zero port: no rule at all, so +// actor egress stays on the masquerade path instead of being redirected to a +// listener that is not there. +func TestActorEgressRedirectRuleDisabled(t *testing.T) { + if rule := ActorEgressRedirectRule(nil, nil, 0); rule != nil { + t.Errorf("ActorEgressRedirectRule(0) = %v, want nil", rule.Exprs) + } +} + +func formatExprs(exprs []expr.Any) string { + var s string + for _, e := range exprs { + s += fmt.Sprintf(" %T%+v\n", e, e) + } + return s +}