Skip to content

ateomnet: move the actor nftables table to the inet family #1116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Yuan Gao (ygao-g) wants to merge 1 commit into
base: main
Choose a base branch
from
+271 −22
Open

ateomnet: move the actor nftables table to the inet family #1116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions internal/ateomnet/net.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
//
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -305,30 +307,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.
//
// 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 {
continue
}
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,
Expand Down Expand Up @@ -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,
Expand Down
163 changes: 153 additions & 10 deletions internal/ateomnet/net_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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.
Expand All @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading