From 6aa3833fa0d2422298a2495d61ad69903b79b5a1 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 13:14:49 -0400 Subject: [PATCH 1/2] fix: Stop the gateway and NAT shard running without forwarding The gateway and egress shard need IPv6 forwarding on every interface their XDP datapath runs on, or bpf_fib_lookup refuses every lookup and the datapath drops every packet. Both set the sysctls at startup, but an unprivileged pod gets /proc/sys read-only, so the writes failed. ConfigureFIBLookupUplinkSysctls logged each failure and returned nil, so the error branches in both binaries never ran and the pod reported healthy. The FIB-lookup sysctl helpers now read each value back and return an error unless it reads 1. A node where something else already enabled forwarding still passes with a read-only /proc/sys. The gateway and the shard now fail startup instead, and an interface found later stays without the datapath until its sysctls hold. Both base DaemonSets mount the host's /proc/sys/net read-write at /host/proc/sys/net, and the new GALACTIC_GATEWAY_PROC_SYS_PATH and GALACTIC_NAT_PROC_SYS_PATH settings point the helpers there. hostNetwork puts the writes in the node's own network namespace, and the pods stay unprivileged. The containerlab lab never showed this because gvpc.clab.yaml enables forwarding on every node. Fixes #586 Co-Authored-By: Claude Opus 5.5 --- cmd/galactic-gateway/gateway.go | 5 +- cmd/galactic-gateway/root.go | 7 ++ cmd/galactic-nat/root.go | 7 ++ config/galactic-gateway/base/daemonset.yaml | 17 +++++ config/galactic-nat/base/daemonset.yaml | 17 +++++ docs/agents/ARCHITECTURE-GATEWAY.md | 8 +- docs/gateway/configuration.md | 31 +++++--- docs/nat/configuration.md | 30 +++++--- internal/config/config.go | 4 + internal/config/gateway.go | 13 ++++ internal/config/gateway_test.go | 10 +++ internal/config/nat.go | 13 ++++ internal/config/nat_test.go | 37 ++++++++++ internal/config/router_test.go | 2 + internal/plumbing/sysctl/sysctl.go | 64 ++++++++++++---- internal/plumbing/sysctl/sysctl_test.go | 81 +++++++++++++++++++++ 16 files changed, 311 insertions(+), 35 deletions(-) diff --git a/cmd/galactic-gateway/gateway.go b/cmd/galactic-gateway/gateway.go index 42601ef2..fe8cfd77 100644 --- a/cmd/galactic-gateway/gateway.go +++ b/cmd/galactic-gateway/gateway.go @@ -114,8 +114,9 @@ func setupGatewayDatapath( // ingress interface, meaning whichever resolved target the packet // arrived on, so this must be applied to every one of them: once the // named interface is a bond, its slaves rather than the master are - // what the kernel reports as ingress. Best-effort and non-fatal, - // matching how sysctls are configured elsewhere here. + // what the kernel reports as ingress. A target left without forwarding + // fails startup: the datapath would drop every packet arriving there + // while the process reported healthy. for _, target := range targets { if err := sysctl.ConfigureFIBLookupUplinkSysctls(target); err != nil { return nil, fmt.Errorf("configure IPv6 forwarding on public interface %q: %w", target, err) diff --git a/cmd/galactic-gateway/root.go b/cmd/galactic-gateway/root.go index 6369fec5..28ee18a2 100644 --- a/cmd/galactic-gateway/root.go +++ b/cmd/galactic-gateway/root.go @@ -28,6 +28,7 @@ import ( "go.datum.net/galactic/internal/gateway" "go.datum.net/galactic/internal/metadata" "go.datum.net/galactic/internal/plumbing/ebpf/xdpdispatch" + "go.datum.net/galactic/internal/plumbing/sysctl" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) @@ -135,6 +136,9 @@ func runCmd(cfg *config.GatewayConfig) error { }) var gwDatapath gateway.Datapath = gateway.NoopDatapath{} if cfg.DatapathEnabled { + if err := sysctl.SetProcSysPath(cfg.ProcSysPath); err != nil { + return err + } gwDatapath, err = setupGatewayDatapath(ctx, cfg.PublicInterface, cfg.InternalInterfaces, cfg.SRv6Address, cfg.XDPAttach, ctrlmetrics.Registry, coverage) if err != nil { @@ -260,6 +264,9 @@ func newRootCommand() *cobra.Command { cmd.Flags().Bool("gateway-datapath-enabled", true, "Run the edge gateway datapath; false keeps the process up but attaches nothing and withdraws "+ "this node's VIP advertisements") + cmd.Flags().String("gateway-proc-sys-path", config.DefaultProcSysPath, + "Procfs sysctl root to write the datapath's forwarding sysctls under, such as a writable mount "+ + "of the host's /proc/sys/net at /net") cmd.Flags().Bool("build-info", false, "Print build information and exit") cmd.Flags().BoolP("version", "V", false, "Print version and exit") return cmd diff --git a/cmd/galactic-nat/root.go b/cmd/galactic-nat/root.go index 8abaa2af..9a32368e 100644 --- a/cmd/galactic-nat/root.go +++ b/cmd/galactic-nat/root.go @@ -29,6 +29,7 @@ import ( "go.datum.net/galactic/internal/controller" "go.datum.net/galactic/internal/metadata" "go.datum.net/galactic/internal/plumbing/ebpf/xdpdispatch" + "go.datum.net/galactic/internal/plumbing/sysctl" bgpv1alpha1 "go.datum.net/network/api/v1alpha1" ) @@ -126,6 +127,9 @@ func runCmd(cfg *config.NATConfig) error { coverageChanged := make(chan struct{}, 1) var datapath controller.EgressDatapath if cfg.DatapathEnabled { + if err := sysctl.SetProcSysPath(cfg.ProcSysPath); err != nil { + return err + } datapath, err = setupNatDatapath(ctx, cfg, ctrlmetrics.Registry, func() { select { case coverageChanged <- struct{}{}: @@ -252,6 +256,9 @@ func newRootCommand() *cobra.Command { cmd.Flags().Bool("nat-echo-responder", false, "Answer ICMP and ICMPv6 Echo Requests addressed to this shard's own masquerade addresses, "+ "rate-limited, instead of dropping them") + cmd.Flags().String("nat-proc-sys-path", config.DefaultProcSysPath, + "Procfs sysctl root to write the datapath's forwarding sysctls under, such as a writable mount "+ + "of the host's /proc/sys/net at /net") cmd.Flags().Bool("build-info", false, "Print build information and exit") cmd.Flags().BoolP("version", "V", false, "Print version and exit") return cmd diff --git a/config/galactic-gateway/base/daemonset.yaml b/config/galactic-gateway/base/daemonset.yaml index 638ee483..92c51dba 100644 --- a/config/galactic-gateway/base/daemonset.yaml +++ b/config/galactic-gateway/base/daemonset.yaml @@ -86,6 +86,14 @@ spec: fieldPath: spec.nodeName - name: GALACTIC_GATEWAY_GRPC_HEALTH_PORT value: "5181" + # An unprivileged container gets /proc/sys read-only, so the + # forwarding sysctls the datapath's FIB lookup needs are written + # through the host's /proc/sys/net, mounted below. hostNetwork + # makes those writes land in the node's own network namespace. + # A sysctl that still does not read 1 fails startup rather than + # leaving a datapath that drops every packet (#586). + - name: GALACTIC_GATEWAY_PROC_SYS_PATH + value: /host/proc/sys # GALACTIC_GATEWAY_INTERNAL_INTERFACES is also left unset, # for the same deployment-specific reason. It names the # compute-facing interfaces, comma-separated, whose @@ -187,8 +195,17 @@ spec: # container on first pin. - name: bpf-fs mountPath: /sys/fs/bpf + # The host's net sysctls, writable, for the forwarding settings + # galactic-gateway sets. Only net.*: the rest of /proc/sys stays + # read-only. + - name: host-proc-sys-net + mountPath: /host/proc/sys/net volumes: - name: bpf-fs hostPath: path: /sys/fs/bpf type: Directory + - name: host-proc-sys-net + hostPath: + path: /proc/sys/net + type: Directory diff --git a/config/galactic-nat/base/daemonset.yaml b/config/galactic-nat/base/daemonset.yaml index 889ab25f..bd19ed83 100644 --- a/config/galactic-nat/base/daemonset.yaml +++ b/config/galactic-nat/base/daemonset.yaml @@ -102,6 +102,14 @@ spec: # internal/config/nat.go's EnvNATEchoResponder. - name: GALACTIC_NAT_ECHO_RESPONDER value: "false" + # An unprivileged container gets /proc/sys read-only, so the + # forwarding sysctls the datapath's FIB lookup needs are written + # through the host's /proc/sys/net, mounted below. hostNetwork + # makes those writes land in the node's own network namespace. + # A sysctl that still does not read 1 fails startup rather than + # leaving a datapath that drops every packet (#586). + - name: GALACTIC_NAT_PROC_SYS_PATH + value: /host/proc/sys # GALACTIC_NAT_XDP_ATTACH is left at its default, "direct": the # shard attaches its own program to the uplinks. "dispatch" runs # it from the node's shared, pinned XDP dispatcher instead, so a @@ -213,8 +221,17 @@ spec: # galactic-xdp/, the shared XDP dispatcher's, in dispatch mode. - name: bpf-fs mountPath: /sys/fs/bpf + # The host's net sysctls, writable, for the forwarding settings + # galactic-nat sets. Only net.*: the rest of /proc/sys stays + # read-only. + - name: host-proc-sys-net + mountPath: /host/proc/sys/net volumes: - name: bpf-fs hostPath: path: /sys/fs/bpf type: Directory + - name: host-proc-sys-net + hostPath: + path: /proc/sys/net + type: Directory diff --git a/docs/agents/ARCHITECTURE-GATEWAY.md b/docs/agents/ARCHITECTURE-GATEWAY.md index 170bf7bf..8b0308a8 100644 --- a/docs/agents/ARCHITECTURE-GATEWAY.md +++ b/docs/agents/ARCHITECTURE-GATEWAY.md @@ -462,7 +462,13 @@ pre-existing containerlab veth/XDP_TX blocker — the sysctl gap, not [Known Constraints](#known-constraints) below for the related, lab-only `XDP_TX` observability quirk this investigation also turned up, and for why the sysctl targets the resolved bond slave rather than -`PublicInterface` itself when the two differ). It then loads +`PublicInterface` itself when the two differ). The helper reads each sysctl +back and returns an error unless it reads `1`, which fails startup: before +#586 it logged the write failure and returned `nil`, so a pod with a +read-only `/proc/sys` ran a datapath that dropped every packet. The DaemonSet +mounts the host's `/proc/sys/net` at `/host/proc/sys/net` and points +`GALACTIC_GATEWAY_PROC_SYS_PATH` at `/host/proc/sys` so the write can succeed +without `privileged`. It then loads `edgeprog.EdgedsrObjects` (`edgeattach.Load`), attaches it to every resolved target (`edgeattach.Attach` — native XDP driver mode only, no generic/SKB-mode fallback, one `link.Link` per target), and constructs a diff --git a/docs/gateway/configuration.md b/docs/gateway/configuration.md index 057d5132..83c116b3 100644 --- a/docs/gateway/configuration.md +++ b/docs/gateway/configuration.md @@ -156,16 +156,17 @@ flags, or a combination of both (CLI flags take precedence), with the `GALACTIC_GATEWAY` env prefix — the same three-tier precedence pattern `galactic-router` uses (see [docs/router/configuration.md](../router/configuration.md)). -| Option | Environment Variable | CLI Flag | Default | Required | -| ------------------- | -------------------------------------- | ------------------------------- | ---------- | ----------------- | -| Node name | `GALACTIC_GATEWAY_NODE_NAME` | `--node-name`, `-n` | — | Yes | -| Public interface | `GALACTIC_GATEWAY_PUBLIC_INTERFACE` | `--gateway-public-interface` | — | While enabled | -| SRv6 address | `GALACTIC_GATEWAY_SRV6_ADDRESS` | `--gateway-srv6-address` | — | While enabled | -| Internal interfaces | `GALACTIC_GATEWAY_INTERNAL_INTERFACES` | `--gateway-internal-interfaces` | — | No | -| XDP attach mode | `GALACTIC_GATEWAY_XDP_ATTACH` | `--gateway-xdp-attach` | `dispatch` | No | -| Datapath enabled | `GALACTIC_GATEWAY_DATAPATH_ENABLED` | `--gateway-datapath-enabled` | `true` | No | -| Metrics port | `GALACTIC_GATEWAY_METRICS_PORT` | `--metrics-port` | `8081` | No | -| gRPC health port | `GALACTIC_GATEWAY_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5181` | No | +| Option | Environment Variable | CLI Flag | Default | Required | +| ------------------- | -------------------------------------- | ------------------------------- | ----------- | ------------- | +| Node name | `GALACTIC_GATEWAY_NODE_NAME` | `--node-name`, `-n` | — | Yes | +| Public interface | `GALACTIC_GATEWAY_PUBLIC_INTERFACE` | `--gateway-public-interface` | — | While enabled | +| SRv6 address | `GALACTIC_GATEWAY_SRV6_ADDRESS` | `--gateway-srv6-address` | — | While enabled | +| Internal interfaces | `GALACTIC_GATEWAY_INTERNAL_INTERFACES` | `--gateway-internal-interfaces` | — | No | +| XDP attach mode | `GALACTIC_GATEWAY_XDP_ATTACH` | `--gateway-xdp-attach` | `dispatch` | No | +| Datapath enabled | `GALACTIC_GATEWAY_DATAPATH_ENABLED` | `--gateway-datapath-enabled` | `true` | No | +| Procfs sysctl root | `GALACTIC_GATEWAY_PROC_SYS_PATH` | `--gateway-proc-sys-path` | `/proc/sys` | No | +| Metrics port | `GALACTIC_GATEWAY_METRICS_PORT` | `--metrics-port` | `8081` | No | +| gRPC health port | `GALACTIC_GATEWAY_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5181` | No | `GALACTIC_GATEWAY_XDP_ATTACH` is `dispatch` or `direct`. `dispatch` runs `edge_lb` and `edge_return` from the gateway slots of the node's shared, @@ -182,6 +183,16 @@ withdraws every one of this node's VIP advertisements. The NetworkGateway's `Ready` reads `False` with reason `DatapathDisabled`. The public interface and SRv6 address are not required while it is off. +`GALACTIC_GATEWAY_PROC_SYS_PATH` is the procfs root the datapath writes its +forwarding sysctls under (`net.ipv6.conf..forwarding` and +`net.ipv6.conf.all.forwarding`). `bpf_fib_lookup` refuses every lookup on +an interface with forwarding off, so a sysctl that does not read `1` after +the write stops the gateway at startup, and leaves an interface found later +without the datapath until it does. A pod that is not privileged gets `/proc/sys` read-only, so +`config/galactic-gateway/base/daemonset.yaml` mounts the host's `/proc/sys/net` at +`/host/proc/sys/net` and sets this to `/host/proc/sys`. A node where +something else already turned forwarding on passes with the default too. + `GALACTIC_GATEWAY_INTERNAL_INTERFACES` is a comma-separated list of this node's compute-facing interfaces, and it is what puts the return path in diff --git a/docs/nat/configuration.md b/docs/nat/configuration.md index 192d0e77..af19da76 100644 --- a/docs/nat/configuration.md +++ b/docs/nat/configuration.md @@ -41,15 +41,16 @@ flags, or a combination of both (CLI flags take precedence), with the `galactic-router` and `galactic-gateway` use (see [docs/router/configuration.md](../router/configuration.md)). -| Option | Environment Variable | CLI Flag | Default | Required | -| ----------------- | -------------------------------- | ------------------------- | ------------- | -------- | -| Node name | `GALACTIC_NAT_NODE_NAME` | `--node-name` | — | Yes | -| Uplink interfaces | `GALACTIC_NAT_UPLINK_INTERFACES` | `--nat-uplink-interfaces` | auto-detected | No | -| XDP attach mode | `GALACTIC_NAT_XDP_ATTACH` | `--nat-xdp-attach` | `direct` | No | -| Datapath enabled | `GALACTIC_NAT_DATAPATH_ENABLED` | `--nat-datapath-enabled` | `true` | No | -| Echo responder | `GALACTIC_NAT_ECHO_RESPONDER` | `--nat-echo-responder` | `false` | No | -| Metrics port | `GALACTIC_NAT_METRICS_PORT` | `--metrics-port` | `9182` | No | -| gRPC health port | `GALACTIC_NAT_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5182` | No | +| Option | Environment Variable | CLI Flag | Default | Required | +| ------------------ | -------------------------------- | ------------------------- | ------------- | -------- | +| Node name | `GALACTIC_NAT_NODE_NAME` | `--node-name` | — | Yes | +| Uplink interfaces | `GALACTIC_NAT_UPLINK_INTERFACES` | `--nat-uplink-interfaces` | auto-detected | No | +| XDP attach mode | `GALACTIC_NAT_XDP_ATTACH` | `--nat-xdp-attach` | `direct` | No | +| Datapath enabled | `GALACTIC_NAT_DATAPATH_ENABLED` | `--nat-datapath-enabled` | `true` | No | +| Echo responder | `GALACTIC_NAT_ECHO_RESPONDER` | `--nat-echo-responder` | `false` | No | +| Procfs sysctl root | `GALACTIC_NAT_PROC_SYS_PATH` | `--nat-proc-sys-path` | `/proc/sys` | No | +| Metrics port | `GALACTIC_NAT_METRICS_PORT` | `--metrics-port` | `9182` | No | +| gRPC health port | `GALACTIC_NAT_GRPC_HEALTH_PORT` | `--grpc-health-port` | `5182` | No | `config/galactic-nat/base/daemonset.yaml` leaves `GALACTIC_NAT_XDP_ATTACH` at the binary's default, `direct`. `dispatch` is the mode to move to: it is @@ -201,6 +202,17 @@ reply counts as `icmp_rate_limited`. The setting is written into the datapath whenever the shard programs its identity, so changing it takes a pod restart and nothing else. +**`--nat-proc-sys-path` / `GALACTIC_NAT_PROC_SYS_PATH`** +`GALACTIC_NAT_PROC_SYS_PATH` is the procfs root the datapath writes its +forwarding sysctls under (`net.ipv6.conf..forwarding` and +`net.ipv6.conf.all.forwarding`, plus the IPv4 ones once the shard serves NAT64). `bpf_fib_lookup` refuses every lookup on +an interface with forwarding off, so a sysctl that does not read `1` after +the write stops the shard at startup, and leaves an uplink found later +uncovered until it does. A pod that is not privileged gets `/proc/sys` read-only, so +`config/galactic-nat/base/daemonset.yaml` mounts the host's `/proc/sys/net` at +`/host/proc/sys/net` and sets this to `/host/proc/sys`. A node where +something else already turned forwarding on passes with the default too. + ### Capabilities and host requirements `galactic-nat` runs `hostNetwork: true` and needs: diff --git a/internal/config/config.go b/internal/config/config.go index 797c2467..015f9128 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,10 @@ const ( DefaultLogFile = "/var/log/galactic/galactic-cni.log" DefaultLogLevel = "info" + // DefaultProcSysPath is the procfs sysctl root galactic-gateway and + // galactic-nat write forwarding sysctls under unless configured otherwise. + DefaultProcSysPath = "/proc/sys" + LogLevelDebug = "debug" LogLevelWarn = "warn" LogLevelWarning = "warning" diff --git a/internal/config/gateway.go b/internal/config/gateway.go index 657b8d9b..e8f123e0 100644 --- a/internal/config/gateway.go +++ b/internal/config/gateway.go @@ -75,6 +75,13 @@ const ( // load-balance it. Every other datapath on the node keeps running. The // public interface and SRv6 address are not required while it is off. EnvGatewayDatapathEnabled = "GALACTIC_GATEWAY_DATAPATH_ENABLED" + + // EnvGatewayProcSysPath is the procfs sysctl root the datapath writes its + // interfaces' forwarding sysctls under. Optional, defaulting to + // DefaultProcSysPath. An unprivileged pod gets /proc/sys read-only, so the + // DaemonSet mounts the host's /proc/sys/net at /host/proc/sys/net and sets + // this to /host/proc/sys. + EnvGatewayProcSysPath = "GALACTIC_GATEWAY_PROC_SYS_PATH" ) // EnvGatewayXDPAttach's values. @@ -113,6 +120,9 @@ type GatewayConfig struct { // DatapathEnabled is EnvGatewayDatapathEnabled. DatapathEnabled bool + + // ProcSysPath is EnvGatewayProcSysPath. + ProcSysPath string } // NewGatewayConfig creates a config resolver reading the GALACTIC_GATEWAY @@ -130,6 +140,7 @@ func NewGatewayConfig() *GatewayConfig { v.SetDefault("srv6_address", "") v.SetDefault("xdp_attach", GatewayXDPAttachDispatch) v.SetDefault("datapath_enabled", true) + v.SetDefault("proc_sys_path", DefaultProcSysPath) cfg := &GatewayConfig{ v: v, @@ -154,6 +165,7 @@ func (c *GatewayConfig) BindFlags(flags *pflag.FlagSet) { {"gateway-srv6-address", "srv6_address"}, {"gateway-xdp-attach", "xdp_attach"}, {"gateway-datapath-enabled", "datapath_enabled"}, + {"gateway-proc-sys-path", "proc_sys_path"}, } for _, b := range bindings { if flags.Changed(b.flag) { @@ -176,6 +188,7 @@ func (c *GatewayConfig) readFields() { c.SRv6Address = c.v.GetString("srv6_address") c.XDPAttach = c.v.GetString("xdp_attach") c.DatapathEnabled = c.v.GetBool("datapath_enabled") + c.ProcSysPath = c.v.GetString("proc_sys_path") } // splitCommaList parses a comma-separated list, such as interface names or BMP diff --git a/internal/config/gateway_test.go b/internal/config/gateway_test.go index 7b1270c0..6912f7c8 100644 --- a/internal/config/gateway_test.go +++ b/internal/config/gateway_test.go @@ -207,3 +207,13 @@ func TestGatewayConfigDisabledNeedsNoInterface(t *testing.T) { t.Error("Validate() = nil for an enabled datapath with no public interface") } } + +func TestGatewayConfigProcSysPath(t *testing.T) { + if got := NewGatewayConfig().ProcSysPath; got != DefaultProcSysPath { + t.Errorf("default ProcSysPath = %q, want %q", got, DefaultProcSysPath) + } + t.Setenv(EnvGatewayProcSysPath, testHostProcSys) + if got := NewGatewayConfig().ProcSysPath; got != testHostProcSys { + t.Errorf("ProcSysPath = %q with %s set, want %s", got, EnvGatewayProcSysPath, testHostProcSys) + } +} diff --git a/internal/config/nat.go b/internal/config/nat.go index 8f3eaed4..7e18e0b4 100644 --- a/internal/config/nat.go +++ b/internal/config/nat.go @@ -90,6 +90,13 @@ const ( // are rate-limited in the datapath by a token bucket per CPU, shared with // any other ICMP the shard emits on its own behalf. EnvNATEchoResponder = "GALACTIC_NAT_ECHO_RESPONDER" + + // EnvNATProcSysPath is the procfs sysctl root the datapath writes its + // uplinks' forwarding sysctls under. Optional, defaulting to + // DefaultProcSysPath. An unprivileged pod gets /proc/sys read-only, so the + // DaemonSet mounts the host's /proc/sys/net at /host/proc/sys/net and sets + // this to /host/proc/sys. + EnvNATProcSysPath = "GALACTIC_NAT_PROC_SYS_PATH" ) // EnvNATXDPAttach's values. NATXDPAttachChain is deprecated and read as @@ -132,6 +139,9 @@ type NATConfig struct { // EchoResponder is EnvNATEchoResponder. EchoResponder bool + + // ProcSysPath is EnvNATProcSysPath. + ProcSysPath string } // NewNATConfig creates a config resolver reading the GALACTIC_NAT @@ -149,6 +159,7 @@ func NewNATConfig() *NATConfig { v.SetDefault("xdp_attach", NATXDPAttachDirect) v.SetDefault("datapath_enabled", true) v.SetDefault("echo_responder", false) + v.SetDefault("proc_sys_path", DefaultProcSysPath) cfg := &NATConfig{ v: v, @@ -172,6 +183,7 @@ func (c *NATConfig) BindFlags(flags *pflag.FlagSet) { {"nat-xdp-attach", "xdp_attach"}, {"nat-datapath-enabled", "datapath_enabled"}, {"nat-echo-responder", "echo_responder"}, + {"nat-proc-sys-path", "proc_sys_path"}, } for _, b := range bindings { if flags.Changed(b.flag) { @@ -197,6 +209,7 @@ func (c *NATConfig) readFields() { } c.DatapathEnabled = c.v.GetBool("datapath_enabled") c.EchoResponder = c.v.GetBool("echo_responder") + c.ProcSysPath = c.v.GetString("proc_sys_path") } // Validate checks that the required configuration fields are set. diff --git a/internal/config/nat_test.go b/internal/config/nat_test.go index 5403a76a..4a05ee31 100644 --- a/internal/config/nat_test.go +++ b/internal/config/nat_test.go @@ -315,3 +315,40 @@ func TestNATConfigDatapathEnabled(t *testing.T) { }) } } + +// TestNATConfigProcSysPath covers the procfs root's three tiers. /proc/sys is +// the default that matters: a shard run outside the DaemonSet, with no host +// mount, still writes where the kernel expects. +func TestNATConfigProcSysPath(t *testing.T) { + tests := []struct { + name string + env string + flag string + want string + }{ + {name: "unset", want: DefaultProcSysPath}, + {name: "env", env: testHostProcSys, want: testHostProcSys}, + {name: "flag over env", env: testHostProcSys, flag: "/elsewhere", want: "/elsewhere"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.env != "" { + t.Setenv(EnvNATProcSysPath, tt.env) + } + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String(FlagNodeName, "", "") + flags.String("nat-proc-sys-path", DefaultProcSysPath, "") + if tt.flag != "" { + if err := flags.Set("nat-proc-sys-path", tt.flag); err != nil { + t.Fatalf("set flag: %v", err) + } + } + + cfg := NewNATConfig() + cfg.BindFlags(flags) + if cfg.ProcSysPath != tt.want { + t.Errorf("ProcSysPath = %q, want %q", cfg.ProcSysPath, tt.want) + } + }) + } +} diff --git a/internal/config/router_test.go b/internal/config/router_test.go index def41a14..cfec6def 100644 --- a/internal/config/router_test.go +++ b/internal/config/router_test.go @@ -18,6 +18,8 @@ const ( // from RouterConfig by the galactic-gateway split. testGatewayIface = "eth0" testGatewaySRv6 = "2001:db8:3::1" + // testHostProcSys is the procfs root the gateway and NAT DaemonSets set. + testHostProcSys = "/host/proc/sys" ) func TestRouterConfigDefaults(t *testing.T) { diff --git a/internal/plumbing/sysctl/sysctl.go b/internal/plumbing/sysctl/sysctl.go index ffe55d50..ebef8301 100644 --- a/internal/plumbing/sysctl/sysctl.go +++ b/internal/plumbing/sysctl/sysctl.go @@ -42,6 +42,27 @@ func ConfigureInterfaceSysctls(iface string) error { return nil } +// procSysPath is the procfs root the FIB-lookup helpers read and write their +// sysctls under. See SetProcSysPath. +var procSysPath = gosysctl.DefaultPath + +// SetProcSysPath points ConfigureFIBLookupUplinkSysctls and +// ConfigureFIBLookupUplinkSysctlsIPv4 at a procfs root other than /proc/sys. +// +// An unprivileged container gets /proc/sys read-only, so a host-network pod +// that must change the host's forwarding sysctls mounts the host's +// /proc/sys/net somewhere writable, such as /host/proc/sys/net, and passes the +// parent here. A net sysctl acts on the network namespace of the process that +// opens it, not the mount, so a host-network pod reaches the host's settings. +// The other helpers here are unaffected. +func SetProcSysPath(path string) error { + if _, err := gosysctl.NewClient(path); err != nil { + return fmt.Errorf("use %s as the procfs sysctl root: %w", path, err) + } + procSysPath = path + return nil +} + // ConfigureFIBLookupUplinkSysctls enables IPv6 forwarding on iface, a gateway // or egress shard node's fabric-facing uplink, and alongside it the // all-interfaces forwarding sysctl. @@ -58,18 +79,15 @@ func ConfigureInterfaceSysctls(iface string) error { // all-interfaces one would affect every interface on the node rather than the // uplink. // -// A sysctl that does not exist is skipped silently, as elsewhere here. +// It returns an error when either sysctl does not read 1 afterwards, since a +// datapath attached there would drop every packet. A write that fails is not +// an error on its own: a node whose forwarding something else already enabled +// passes even where /proc/sys is read-only. func ConfigureFIBLookupUplinkSysctls(iface string) error { - settings := []struct{ key, value string }{ + return setForwarding([]struct{ key, value string }{ {fmt.Sprintf("net.ipv6.conf.%s.forwarding", iface), "1"}, {"net.ipv6.conf.all.forwarding", "1"}, - } - for _, s := range settings { - if err := gosysctl.Set(s.key, s.value); err != nil { - logger.Warn("failed to set sysctl (non-fatal)", "sysctl", s.key, "err", err) - } - } - return nil + }) } // ConfigureFIBLookupUplinkSysctlsIPv4 enables IPv4 forwarding on iface, the @@ -81,16 +99,36 @@ func ConfigureFIBLookupUplinkSysctls(iface string) error { // packet dies at its last instruction, counted under a fib_* reason, so the // shard reads as healthy while no IPv4 traffic ever leaves it. // -// A sysctl that does not exist is skipped silently, as elsewhere here. +// It returns an error on the same terms as ConfigureFIBLookupUplinkSysctls. func ConfigureFIBLookupUplinkSysctlsIPv4(iface string) error { - settings := []struct{ key, value string }{ + return setForwarding([]struct{ key, value string }{ {fmt.Sprintf("net.ipv4.conf.%s.forwarding", iface), "1"}, {"net.ipv4.conf.all.forwarding", "1"}, {"net.ipv4.ip_forward", "1"}, + }) +} + +// setForwarding writes each setting under procSysPath and reads it back, +// returning an error for the first one that does not hold its value. +func setForwarding(settings []struct{ key, value string }) error { + client, err := gosysctl.NewClient(procSysPath) + if err != nil { + return fmt.Errorf("open procfs sysctl root %s: %w", procSysPath, err) } for _, s := range settings { - if err := gosysctl.Set(s.key, s.value); err != nil { - logger.Warn("failed to set sysctl (non-fatal)", "sysctl", s.key, "err", err) + setErr := client.Set(s.key, s.value) + got, getErr := client.Get(s.key) + switch { + case getErr == nil && got == s.value: + if setErr != nil { + logger.Debug("sysctl already set; write failed", "sysctl", s.key, "err", setErr) + } + case setErr != nil: + return fmt.Errorf("set sysctl %s to %s: %w", s.key, s.value, setErr) + case getErr != nil: + return fmt.Errorf("read back sysctl %s: %w", s.key, getErr) + default: + return fmt.Errorf("sysctl %s reads %s after setting it to %s", s.key, got, s.value) } } return nil diff --git a/internal/plumbing/sysctl/sysctl_test.go b/internal/plumbing/sysctl/sysctl_test.go index 26dbf9e0..0a57d97e 100644 --- a/internal/plumbing/sysctl/sysctl_test.go +++ b/internal/plumbing/sysctl/sysctl_test.go @@ -7,6 +7,8 @@ package sysctl import ( "bytes" "log/slog" + "os" + "path/filepath" "strings" "testing" ) @@ -72,3 +74,82 @@ func TestInterfaceSettings_hasAllEntries(t *testing.T) { } } } + +// fakeProcSys builds a procfs sysctl root under a temporary directory holding +// the IPv6 forwarding sysctls for iface and all, each set to value, and points +// the FIB-lookup helpers at it for the rest of the test. +func fakeProcSys(t *testing.T, iface, value string, mode os.FileMode) { + t.Helper() + root := t.TempDir() + for _, dev := range []string{iface, "all"} { + dir := filepath.Join(root, "net", "ipv6", "conf", dev) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, "forwarding") + if err := os.WriteFile(path, []byte(value+"\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, mode); err != nil { + t.Fatal(err) + } + } + orig := procSysPath + if err := SetProcSysPath(root); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { procSysPath = orig }) +} + +func TestConfigureFIBLookupUplinkSysctls(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the read-only file modes these cases depend on") + } + const iface = "uplink0" + tests := []struct { + name string + value string + mode os.FileMode + iface string + wantErr bool + }{ + {name: "writable, off", value: "0", mode: 0o644, iface: iface}, + {name: "read-only, already on", value: "1", mode: 0o444, iface: iface}, + // The defect in #586: the write fails, forwarding stays off, and the + // helper used to report success anyway. + {name: "read-only, off", value: "0", mode: 0o444, iface: iface, wantErr: true}, + {name: "interface missing", value: "1", mode: 0o644, iface: "missing0", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeProcSys(t, iface, tt.value, tt.mode) + err := ConfigureFIBLookupUplinkSysctls(tt.iface) + if (err != nil) != tt.wantErr { + t.Fatalf("ConfigureFIBLookupUplinkSysctls(%q) error = %v, wantErr %v", tt.iface, err, tt.wantErr) + } + if err != nil { + return + } + for _, dev := range []string{tt.iface, "all"} { + got, readErr := os.ReadFile(filepath.Join(procSysPath, "net", "ipv6", "conf", dev, "forwarding")) + if readErr != nil { + t.Fatal(readErr) + } + if strings.TrimSpace(string(got)) != "1" { + t.Errorf("net.ipv6.conf.%s.forwarding = %q, want 1", dev, got) + } + } + }) + } +} + +func TestSetProcSysPath_missingDirectory(t *testing.T) { + orig := procSysPath + t.Cleanup(func() { procSysPath = orig }) + if err := SetProcSysPath(filepath.Join(t.TempDir(), "absent")); err == nil { + t.Fatal("SetProcSysPath accepted a directory that does not exist") + } + if procSysPath != orig { + t.Errorf("procSysPath changed to %q after a rejected path", procSysPath) + } +} From eedac7ef1896f556706a01f352eeba27e1d221d4 Mon Sep 17 00:00:00 2001 From: Peter Sprygada Date: Tue, 6 Oct 2026 14:19:33 -0400 Subject: [PATCH 2/2] fix: Set forwarding on interfaces whose names contain a dot go-sysctl turns every dot in a key into a path separator, so net.ipv6.conf.bond0.100.forwarding resolved to conf/bond0/100/forwarding, which does not exist. Before, that failure was only logged. Now that the FIB-lookup helpers return it, a gateway or NAT shard on a VLAN uplink would fail startup or leave the interface uncovered. The helpers now take each sysctl as path segments and write and read the file directly, and name it in errors the way sysctl(8) does, with the interface's dot written as a slash. The tests cover both helpers, add a dotted interface name, and skip only the read-only cases as root. The docs note that SELinux policy can still refuse the write. Related to #586 Co-Authored-By: Claude Opus 5.5 --- docs/agents/ARCHITECTURE-GATEWAY.md | 16 ++-- docs/gateway/configuration.md | 17 ++-- docs/nat/configuration.md | 20 ++--- internal/plumbing/sysctl/sysctl.go | 89 +++++++++++++------- internal/plumbing/sysctl/sysctl_test.go | 103 +++++++++++++++--------- 5 files changed, 153 insertions(+), 92 deletions(-) diff --git a/docs/agents/ARCHITECTURE-GATEWAY.md b/docs/agents/ARCHITECTURE-GATEWAY.md index 8b0308a8..2a7405fb 100644 --- a/docs/agents/ARCHITECTURE-GATEWAY.md +++ b/docs/agents/ARCHITECTURE-GATEWAY.md @@ -462,14 +462,14 @@ pre-existing containerlab veth/XDP_TX blocker — the sysctl gap, not [Known Constraints](#known-constraints) below for the related, lab-only `XDP_TX` observability quirk this investigation also turned up, and for why the sysctl targets the resolved bond slave rather than -`PublicInterface` itself when the two differ). The helper reads each sysctl -back and returns an error unless it reads `1`, which fails startup: before -#586 it logged the write failure and returned `nil`, so a pod with a -read-only `/proc/sys` ran a datapath that dropped every packet. The DaemonSet -mounts the host's `/proc/sys/net` at `/host/proc/sys/net` and points -`GALACTIC_GATEWAY_PROC_SYS_PATH` at `/host/proc/sys` so the write can succeed -without `privileged`. It then loads -`edgeprog.EdgedsrObjects` (`edgeattach.Load`), attaches it to every +`PublicInterface` itself when the two differ). The helper reads each +sysctl back and returns an error unless it reads `1`, which fails startup: +before #586 it logged the write failure and returned `nil`, so a pod with +a read-only `/proc/sys` ran a datapath that dropped every packet. The +DaemonSet mounts the host's `/proc/sys/net` at `/host/proc/sys/net` and +points `GALACTIC_GATEWAY_PROC_SYS_PATH` at `/host/proc/sys` so the write +can succeed without `privileged`. It then loads `edgeprog.EdgedsrObjects` +(`edgeattach.Load`), attaches it to every resolved target (`edgeattach.Attach` — native XDP driver mode only, no generic/SKB-mode fallback, one `link.Link` per target), and constructs a `gateway.KernelDatapath` over it, writing this node's SRv6 encap-source diff --git a/docs/gateway/configuration.md b/docs/gateway/configuration.md index 83c116b3..c3aa1c9a 100644 --- a/docs/gateway/configuration.md +++ b/docs/gateway/configuration.md @@ -185,14 +185,15 @@ and SRv6 address are not required while it is off. `GALACTIC_GATEWAY_PROC_SYS_PATH` is the procfs root the datapath writes its forwarding sysctls under (`net.ipv6.conf..forwarding` and -`net.ipv6.conf.all.forwarding`). `bpf_fib_lookup` refuses every lookup on -an interface with forwarding off, so a sysctl that does not read `1` after -the write stops the gateway at startup, and leaves an interface found later -without the datapath until it does. A pod that is not privileged gets `/proc/sys` read-only, so -`config/galactic-gateway/base/daemonset.yaml` mounts the host's `/proc/sys/net` at -`/host/proc/sys/net` and sets this to `/host/proc/sys`. A node where -something else already turned forwarding on passes with the default too. - +`net.ipv6.conf.all.forwarding`). `bpf_fib_lookup` refuses every lookup on an +interface with forwarding off, so a sysctl that does not read `1` after the +write stops the gateway at startup, and leaves an interface found later +without the datapath until it does. A pod that is not privileged gets +`/proc/sys` read-only, so `config/galactic-gateway/base/daemonset.yaml` +mounts the host's `/proc/sys/net` at `/host/proc/sys/net` and sets this to +`/host/proc/sys`. A node where something else already turned forwarding on +passes with the default too. SELinux policy on an enforcing node can still +refuse the write, and the gateway then says so at startup. `GALACTIC_GATEWAY_INTERNAL_INTERFACES` is a comma-separated list of this node's compute-facing interfaces, and it is what puts the return path in diff --git a/docs/nat/configuration.md b/docs/nat/configuration.md index af19da76..7c7678e3 100644 --- a/docs/nat/configuration.md +++ b/docs/nat/configuration.md @@ -203,15 +203,17 @@ whenever the shard programs its identity, so changing it takes a pod restart and nothing else. **`--nat-proc-sys-path` / `GALACTIC_NAT_PROC_SYS_PATH`** -`GALACTIC_NAT_PROC_SYS_PATH` is the procfs root the datapath writes its -forwarding sysctls under (`net.ipv6.conf..forwarding` and -`net.ipv6.conf.all.forwarding`, plus the IPv4 ones once the shard serves NAT64). `bpf_fib_lookup` refuses every lookup on -an interface with forwarding off, so a sysctl that does not read `1` after -the write stops the shard at startup, and leaves an uplink found later -uncovered until it does. A pod that is not privileged gets `/proc/sys` read-only, so -`config/galactic-nat/base/daemonset.yaml` mounts the host's `/proc/sys/net` at -`/host/proc/sys/net` and sets this to `/host/proc/sys`. A node where -something else already turned forwarding on passes with the default too. +The procfs root the datapath writes its forwarding sysctls under: +`net.ipv6.conf..forwarding` and `net.ipv6.conf.all.forwarding`, plus +the IPv4 ones once the shard serves NAT64. `bpf_fib_lookup` refuses every +lookup on an interface with forwarding off, so a sysctl that does not read +`1` after the write stops the shard at startup, and leaves an uplink found +later uncovered until it does. A pod that is not privileged gets `/proc/sys` +read-only, so `config/galactic-nat/base/daemonset.yaml` mounts the host's +`/proc/sys/net` at `/host/proc/sys/net` and sets this to `/host/proc/sys`. A +node where something else already turned forwarding on passes with the +default too. SELinux policy on an enforcing node can still refuse the write, +and the shard then says so at startup. ### Capabilities and host requirements diff --git a/internal/plumbing/sysctl/sysctl.go b/internal/plumbing/sysctl/sysctl.go index ebef8301..0f658a33 100644 --- a/internal/plumbing/sysctl/sysctl.go +++ b/internal/plumbing/sysctl/sysctl.go @@ -9,6 +9,9 @@ package sysctl import ( "fmt" "log/slog" + "os" + "path/filepath" + "strings" gosysctl "github.com/lorenzosaino/go-sysctl" ) @@ -43,8 +46,9 @@ func ConfigureInterfaceSysctls(iface string) error { } // procSysPath is the procfs root the FIB-lookup helpers read and write their -// sysctls under. See SetProcSysPath. -var procSysPath = gosysctl.DefaultPath +// sysctls under. See SetProcSysPath. Set once at startup, before any helper +// runs; it is not guarded for concurrent use. +var procSysPath = "/proc/sys" // SetProcSysPath points ConfigureFIBLookupUplinkSysctls and // ConfigureFIBLookupUplinkSysctlsIPv4 at a procfs root other than /proc/sys. @@ -56,9 +60,13 @@ var procSysPath = gosysctl.DefaultPath // opens it, not the mount, so a host-network pod reaches the host's settings. // The other helpers here are unaffected. func SetProcSysPath(path string) error { - if _, err := gosysctl.NewClient(path); err != nil { + info, err := os.Stat(path) + if err != nil { return fmt.Errorf("use %s as the procfs sysctl root: %w", path, err) } + if !info.IsDir() { + return fmt.Errorf("use %s as the procfs sysctl root: not a directory", path) + } procSysPath = path return nil } @@ -84,10 +92,7 @@ func SetProcSysPath(path string) error { // an error on its own: a node whose forwarding something else already enabled // passes even where /proc/sys is read-only. func ConfigureFIBLookupUplinkSysctls(iface string) error { - return setForwarding([]struct{ key, value string }{ - {fmt.Sprintf("net.ipv6.conf.%s.forwarding", iface), "1"}, - {"net.ipv6.conf.all.forwarding", "1"}, - }) + return setForwarding(ipv6ForwardingSysctls(iface)) } // ConfigureFIBLookupUplinkSysctlsIPv4 enables IPv4 forwarding on iface, the @@ -101,39 +106,63 @@ func ConfigureFIBLookupUplinkSysctls(iface string) error { // // It returns an error on the same terms as ConfigureFIBLookupUplinkSysctls. func ConfigureFIBLookupUplinkSysctlsIPv4(iface string) error { - return setForwarding([]struct{ key, value string }{ - {fmt.Sprintf("net.ipv4.conf.%s.forwarding", iface), "1"}, - {"net.ipv4.conf.all.forwarding", "1"}, - {"net.ipv4.ip_forward", "1"}, - }) + return setForwarding(ipv4ForwardingSysctls(iface)) } -// setForwarding writes each setting under procSysPath and reads it back, -// returning an error for the first one that does not hold its value. -func setForwarding(settings []struct{ key, value string }) error { - client, err := gosysctl.NewClient(procSysPath) - if err != nil { - return fmt.Errorf("open procfs sysctl root %s: %w", procSysPath, err) - } - for _, s := range settings { - setErr := client.Set(s.key, s.value) - got, getErr := client.Get(s.key) +// ipv6ForwardingSysctls and ipv4ForwardingSysctls are the sysctls, as path +// segments, the two FIB-lookup helpers set for iface. +func ipv6ForwardingSysctls(iface string) [][]string { + return [][]string{forwardingSysctl("ipv6", iface), forwardingSysctl("ipv6", "all")} +} + +func ipv4ForwardingSysctls(iface string) [][]string { + return [][]string{forwardingSysctl("ipv4", iface), forwardingSysctl("ipv4", "all"), {"net", "ipv4", "ip_forward"}} +} + +// forwardingSysctl is the path of family's forwarding sysctl for dev. +func forwardingSysctl(family, dev string) []string { + return []string{"net", family, "conf", dev, "forwarding"} +} + +// setForwarding writes 1 to each sysctl under procSysPath and reads it back, +// returning an error for the first one that does not read 1. +// +// Each sysctl is given as its path segments rather than as a dotted key: an +// interface name may itself contain a dot, as a VLAN such as bond0.100 does, +// and a dotted key cannot tell that dot from a separator. +func setForwarding(sysctls [][]string) error { + for _, segments := range sysctls { + path := filepath.Join(append([]string{procSysPath}, segments...)...) + name := sysctlName(segments) + writeErr := os.WriteFile(path, []byte("1"), 0o644) + raw, readErr := os.ReadFile(path) + got := strings.TrimSpace(string(raw)) switch { - case getErr == nil && got == s.value: - if setErr != nil { - logger.Debug("sysctl already set; write failed", "sysctl", s.key, "err", setErr) + case readErr == nil && got == "1": + if writeErr != nil { + logger.Debug("sysctl already set; write failed", "sysctl", name, "err", writeErr) } - case setErr != nil: - return fmt.Errorf("set sysctl %s to %s: %w", s.key, s.value, setErr) - case getErr != nil: - return fmt.Errorf("read back sysctl %s: %w", s.key, getErr) + case writeErr != nil: + return fmt.Errorf("set sysctl %s to 1: %w", name, writeErr) + case readErr != nil: + return fmt.Errorf("read back sysctl %s: %w", name, readErr) default: - return fmt.Errorf("sysctl %s reads %s after setting it to %s", s.key, got, s.value) + return fmt.Errorf("sysctl %s reads %s after setting it to 1", name, got) } } return nil } +// sysctlName renders path segments as sysctl(8) names them, with a dot inside +// a segment written as a slash, so net.ipv6.conf.bond0/100.forwarding. +func sysctlName(segments []string) string { + parts := make([]string, len(segments)) + for i, seg := range segments { + parts[i] = strings.ReplaceAll(seg, ".", "/") + } + return strings.Join(parts, ".") +} + // ConfigureTapSysctls applies the sysctls appropriate for a tap connected to a // VM. Unlike ConfigureInterfaceSysctls it skips proxy ARP and NDP, the VM // handling its own address resolution. A sysctl that does not exist is skipped diff --git a/internal/plumbing/sysctl/sysctl_test.go b/internal/plumbing/sysctl/sysctl_test.go index 0a57d97e..782cae26 100644 --- a/internal/plumbing/sysctl/sysctl_test.go +++ b/internal/plumbing/sysctl/sysctl_test.go @@ -76,17 +76,16 @@ func TestInterfaceSettings_hasAllEntries(t *testing.T) { } // fakeProcSys builds a procfs sysctl root under a temporary directory holding -// the IPv6 forwarding sysctls for iface and all, each set to value, and points -// the FIB-lookup helpers at it for the rest of the test. -func fakeProcSys(t *testing.T, iface, value string, mode os.FileMode) { +// each of the given sysctls, by path segment, set to value with file mode +// mode, and points the FIB-lookup helpers at it for the rest of the test. +func fakeProcSys(t *testing.T, sysctls [][]string, value string, mode os.FileMode) string { t.Helper() root := t.TempDir() - for _, dev := range []string{iface, "all"} { - dir := filepath.Join(root, "net", "ipv6", "conf", dev) - if err := os.MkdirAll(dir, 0o755); err != nil { + for _, segments := range sysctls { + path := filepath.Join(append([]string{root}, segments...)...) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { t.Fatal(err) } - path := filepath.Join(dir, "forwarding") if err := os.WriteFile(path, []byte(value+"\n"), 0o644); err != nil { t.Fatal(err) } @@ -99,47 +98,77 @@ func fakeProcSys(t *testing.T, iface, value string, mode os.FileMode) { t.Fatal(err) } t.Cleanup(func() { procSysPath = orig }) + return root } +const ( + testUplink = "uplink0" + // testVLAN is a VLAN's name, whose dot a dotted sysctl key would read as a + // separator. + testVLAN = "bond0.100" +) + func TestConfigureFIBLookupUplinkSysctls(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root ignores the read-only file modes these cases depend on") + helpers := []struct { + name string + fn func(string) error + sysctls func(string) [][]string + }{ + {"IPv6", ConfigureFIBLookupUplinkSysctls, ipv6ForwardingSysctls}, + {"IPv4", ConfigureFIBLookupUplinkSysctlsIPv4, ipv4ForwardingSysctls}, } - const iface = "uplink0" tests := []struct { - name string - value string - mode os.FileMode - iface string - wantErr bool + name string + present string // interface the fake root holds sysctls for + iface string // interface the helper is asked to configure + value string + readOnly bool + wantErr bool }{ - {name: "writable, off", value: "0", mode: 0o644, iface: iface}, - {name: "read-only, already on", value: "1", mode: 0o444, iface: iface}, + {name: "writable, off", present: testUplink, iface: testUplink, value: "0"}, + {name: "dotted interface name", present: testVLAN, iface: testVLAN, value: "0"}, + {name: "read-only, already on", present: testUplink, iface: testUplink, value: "1", readOnly: true}, // The defect in #586: the write fails, forwarding stays off, and the // helper used to report success anyway. - {name: "read-only, off", value: "0", mode: 0o444, iface: iface, wantErr: true}, - {name: "interface missing", value: "1", mode: 0o644, iface: "missing0", wantErr: true}, + {name: "read-only, off", present: testUplink, iface: testUplink, value: "0", readOnly: true, wantErr: true}, + {name: "interface missing", present: testUplink, iface: "missing0", value: "1", wantErr: true}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - fakeProcSys(t, iface, tt.value, tt.mode) - err := ConfigureFIBLookupUplinkSysctls(tt.iface) - if (err != nil) != tt.wantErr { - t.Fatalf("ConfigureFIBLookupUplinkSysctls(%q) error = %v, wantErr %v", tt.iface, err, tt.wantErr) - } - if err != nil { - return - } - for _, dev := range []string{tt.iface, "all"} { - got, readErr := os.ReadFile(filepath.Join(procSysPath, "net", "ipv6", "conf", dev, "forwarding")) - if readErr != nil { - t.Fatal(readErr) + for _, h := range helpers { + for _, tt := range tests { + t.Run(h.name+"/"+tt.name, func(t *testing.T) { + mode := os.FileMode(0o644) + if tt.readOnly { + if os.Geteuid() == 0 { + t.Skip("root ignores the read-only file mode this case depends on") + } + mode = 0o444 } - if strings.TrimSpace(string(got)) != "1" { - t.Errorf("net.ipv6.conf.%s.forwarding = %q, want 1", dev, got) + root := fakeProcSys(t, h.sysctls(tt.present), tt.value, mode) + err := h.fn(tt.iface) + if (err != nil) != tt.wantErr { + t.Fatalf("configure %q: error = %v, wantErr %v", tt.iface, err, tt.wantErr) } - } - }) + if err != nil { + return + } + for _, segments := range h.sysctls(tt.iface) { + got, readErr := os.ReadFile(filepath.Join(append([]string{root}, segments...)...)) + if readErr != nil { + t.Fatal(readErr) + } + if strings.TrimSpace(string(got)) != "1" { + t.Errorf("%s = %q, want 1", sysctlName(segments), got) + } + } + }) + } + } +} + +func TestSysctlName(t *testing.T) { + got := sysctlName(forwardingSysctl("ipv6", testVLAN)) + if want := "net.ipv6.conf.bond0/100.forwarding"; got != want { + t.Errorf("sysctlName = %q, want %q", got, want) } }