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..2a7405fb 100644 --- a/docs/agents/ARCHITECTURE-GATEWAY.md +++ b/docs/agents/ARCHITECTURE-GATEWAY.md @@ -462,8 +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). 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 057d5132..c3aa1c9a 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,17 @@ 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. 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 192d0e77..7c7678e3 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,19 @@ 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`** +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 `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..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" ) @@ -42,6 +45,32 @@ func ConfigureInterfaceSysctls(iface string) error { return nil } +// procSysPath is the procfs root the FIB-lookup helpers read and write their +// 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. +// +// 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 { + 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 +} + // 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 +87,12 @@ 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 }{ - {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 + return setForwarding(ipv6ForwardingSysctls(iface)) } // ConfigureFIBLookupUplinkSysctlsIPv4 enables IPv4 forwarding on iface, the @@ -81,21 +104,65 @@ 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 }{ - {fmt.Sprintf("net.ipv4.conf.%s.forwarding", iface), "1"}, - {"net.ipv4.conf.all.forwarding", "1"}, - {"net.ipv4.ip_forward", "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 setForwarding(ipv4ForwardingSysctls(iface)) +} + +// 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 readErr == nil && got == "1": + if writeErr != nil { + logger.Debug("sysctl already set; write failed", "sysctl", name, "err", writeErr) + } + 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 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 26dbf9e0..782cae26 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,111 @@ func TestInterfaceSettings_hasAllEntries(t *testing.T) { } } } + +// fakeProcSys builds a procfs sysctl root under a temporary directory holding +// 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 _, segments := range sysctls { + path := filepath.Join(append([]string{root}, segments...)...) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + 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 }) + 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) { + helpers := []struct { + name string + fn func(string) error + sysctls func(string) [][]string + }{ + {"IPv6", ConfigureFIBLookupUplinkSysctls, ipv6ForwardingSysctls}, + {"IPv4", ConfigureFIBLookupUplinkSysctlsIPv4, ipv4ForwardingSysctls}, + } + tests := []struct { + 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", 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", present: testUplink, iface: testUplink, value: "0", readOnly: true, wantErr: true}, + {name: "interface missing", present: testUplink, iface: "missing0", value: "1", wantErr: true}, + } + 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 + } + 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) + } +} + +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) + } +}