diff --git a/internal/ateomnet/net.go b/internal/ateomnet/net.go index 91203a8e04..67333078cd 100644 --- a/internal/ateomnet/net.go +++ b/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, diff --git a/internal/ateomnet/net_linux_test.go b/internal/ateomnet/net_linux_test.go index b9c8ac45f4..0216585f4c 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,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. diff --git a/internal/ateomnet/rules_linux_test.go b/internal/ateomnet/rules_linux_test.go new file mode 100644 index 0000000000..b6b8b53f3e --- /dev/null +++ b/internal/ateomnet/rules_linux_test.go @@ -0,0 +1,88 @@ +//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 expressions installed into the inet actor +// table. The nfproto guard in front of every IPv4 match is what makes those +// matches safe there -- without it the payload load reads an IPv4 offset out of +// an IPv6 header -- and no other test in the package can see it: an IPv4-only +// datapath still behaves correctly with the guard removed. +// +// The wants are spelled out as literal bytes rather than built from the same +// helpers as the code, so they pin the wire encoding and not just its spelling. +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 +}