From 1a0ac25db053e5073970bbf25949ee5f6058fc0c Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Thu, 15 Oct 2026 16:36:39 -0400 Subject: [PATCH] Report how a config reload went: failures, restarts required, and the keys that need one (#1935) --- cmd/nebula-service/main.go | 3 +- cmd/nebula-service/service.go | 3 +- cmd/nebula/main.go | 3 +- config/config.go | 53 +++++++++------ config/config_test.go | 4 +- config/reload.go | 98 +++++++++++++++++++++++++++ config/reload_test.go | 108 ++++++++++++++++++++++++++++++ connection_manager.go | 3 +- control_lifecycle_test.go | 6 +- dns_server.go | 3 +- e2e/handshakes_test.go | 2 +- e2e/tunnels_test.go | 4 +- examples/config.yml | 26 +++++-- hostmap.go | 2 +- interface.go | 12 ++-- interface_test.go | 30 +++++++-- lighthouse.go | 5 +- lighthouse_test.go | 43 ++++++++---- main.go | 12 +++- main_restart_keys_linux.go | 6 ++ main_restart_keys_other.go | 6 ++ overlay/tun.go | 7 ++ overlay/tun_android.go | 9 +-- overlay/tun_bypass_windows.go | 3 + overlay/tun_bypass_windows_386.go | 3 + overlay/tun_darwin.go | 36 ++++++---- overlay/tun_darwin_route_test.go | 8 ++- overlay/tun_freebsd.go | 29 +++++--- overlay/tun_ios.go | 9 +-- overlay/tun_linux.go | 42 ++++++++---- overlay/tun_netbsd.go | 29 +++++--- overlay/tun_openbsd.go | 29 +++++--- overlay/tun_reload_linux_test.go | 59 ++++++++++++++-- overlay/tun_test.go | 24 +++++++ overlay/tun_windows.go | 30 ++++++--- overlay/tun_windows_route_test.go | 30 +++++++-- pki.go | 39 ++++++++--- pki_test.go | 86 ++++++++++++++++++++++++ punchy.go | 3 +- punchy_test.go | 22 ++++-- relay_manager.go | 3 +- ssh.go | 21 ++++-- ssh_reload_test.go | 83 +++++++++++++++++++++++ sshd_reload_test.go | 12 ++-- stats.go | 3 +- udp/conn.go | 5 +- udp/netchange.go | 6 ++ udp/netchange_darwin.go | 3 + udp/netchange_generic.go | 3 + udp/netchange_test.go | 16 +++++ udp/udp_bypass_windows.go | 6 +- udp/udp_darwin.go | 2 +- udp/udp_generic.go | 2 +- udp/udp_linux.go | 5 +- udp/udp_linux_reload_test.go | 31 +++++++++ udp/udp_rio_windows.go | 2 +- udp/udp_tester.go | 2 +- 57 files changed, 941 insertions(+), 193 deletions(-) create mode 100644 config/reload.go create mode 100644 config/reload_test.go create mode 100644 main_restart_keys_linux.go create mode 100644 main_restart_keys_other.go create mode 100644 overlay/tun_test.go create mode 100644 ssh_reload_test.go create mode 100644 udp/udp_linux_reload_test.go diff --git a/cmd/nebula-service/main.go b/cmd/nebula-service/main.go index e0b335f5..d385bb8c 100644 --- a/cmd/nebula-service/main.go +++ b/cmd/nebula-service/main.go @@ -85,9 +85,10 @@ func main() { fmt.Printf("failed to apply logging config: %s", err) os.Exit(1) } - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { if err := logging.ApplyConfig(l, c); err != nil { l.Error("Failed to reconfigure logger on reload", "error", err) + r.Failed() } }) diff --git a/cmd/nebula-service/service.go b/cmd/nebula-service/service.go index abe9abe0..9ef8cd5a 100644 --- a/cmd/nebula-service/service.go +++ b/cmd/nebula-service/service.go @@ -34,9 +34,10 @@ func (p *program) Start(s service.Service) error { if err := logging.ApplyConfig(l, c); err != nil { return fmt.Errorf("failed to apply logging config: %s", err) } - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { if err := logging.ApplyConfig(l, c); err != nil { l.Error("Failed to reconfigure logger on reload", "error", err) + r.Failed() } }) diff --git a/cmd/nebula/main.go b/cmd/nebula/main.go index 3c786b84..31da63e4 100644 --- a/cmd/nebula/main.go +++ b/cmd/nebula/main.go @@ -71,9 +71,10 @@ func main() { fmt.Printf("failed to apply logging config: %s", err) os.Exit(1) } - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { if err := logging.ApplyConfig(l, c); err != nil { l.Error("Failed to reconfigure logger on reload", "error", err) + r.Failed() } }) diff --git a/config/config.go b/config/config.go index 79cdce0f..ffa039be 100644 --- a/config/config.go +++ b/config/config.go @@ -26,9 +26,12 @@ type C struct { files []string Settings map[string]any oldSettings map[string]any - callbacks []func(*C) + callbacks []func(*C, *Reload) l *slog.Logger reloadLock sync.Mutex + + // Set by the first reload that needs a restart and never cleared. Guarded by reloadLock + needsRestart bool } func NewC(l *slog.Logger) *C { @@ -72,8 +75,9 @@ func (c *C) LoadString(raw string) error { // RegisterReloadCallback stores a function to be called when a config reload is triggered. The functions registered // here should decide if they need to make a change to the current process before making the change. HasChanged can be // used to help decide if a change is necessary. -// These functions should return quickly or spawn their own go routine if they will take a while -func (c *C) RegisterReloadCallback(f func(*C)) { +// These functions should return quickly or spawn their own go routine if they will take a while. +// Use r to report anything that failed or needs a restart to apply. +func (c *C) RegisterReloadCallback(f func(c *C, r *Reload)) { c.callbacks = append(c.callbacks, f) } @@ -144,50 +148,55 @@ func (c *C) CatchHUP(ctx context.Context) { return case <-ch: c.l.Info("Caught HUP, reloading config") - c.ReloadConfig() + c.ReloadConfig().Log(c.l) } } }() } -func (c *C) ReloadConfig() { +// ReloadConfig reloads the config from the path provided to Load and runs every registered callback. A config that +// fails to load is logged here, like any other failure, and the result carries the error for the summary. +func (c *C) ReloadConfig() ReloadResult { c.reloadLock.Lock() defer c.reloadLock.Unlock() c.oldSettings = make(map[string]any) maps.Copy(c.oldSettings, c.Settings) - err := c.Load(c.path) - if err != nil { - c.l.Error("Error occurred while reloading config", - "config_path", c.path, - "error", err, - ) - return + if err := c.Load(c.path); err != nil { + c.l.Error("Error occurred while reloading config", "config_path", c.path, "error", err) + return ReloadResult{Err: err, RestartRequired: c.needsRestart} } - for _, v := range c.callbacks { - v(c) - } + return c.runCallbacks() } -func (c *C) ReloadConfigString(raw string) error { +// ReloadConfigString is ReloadConfig for a config held in memory rather than on disk. +func (c *C) ReloadConfigString(raw string) ReloadResult { c.reloadLock.Lock() defer c.reloadLock.Unlock() c.oldSettings = make(map[string]any) maps.Copy(c.oldSettings, c.Settings) - err := c.LoadString(raw) - if err != nil { - return err + if err := c.LoadString(raw); err != nil { + c.l.Error("Error occurred while reloading config", "error", err) + return ReloadResult{Err: err, RestartRequired: c.needsRestart} } + return c.runCallbacks() +} + +func (c *C) runCallbacks() ReloadResult { + r := &Reload{c: c} for _, v := range c.callbacks { - v(c) + v(c, r) } - return nil + if r.restart { + c.needsRestart = true + } + return ReloadResult{Failed: r.failed, RestartRequired: c.needsRestart, RestartKeys: r.keys} } // GetString will get the string for k or return the default d if not found or invalid @@ -391,7 +400,7 @@ func (c *C) parse() error { var nm map[string]any err = yaml.Unmarshal(b, &nm) if err != nil { - return err + return fmt.Errorf("%s: %w", path, err) } // We need to use WithAppendSlice so that firewall rules in separate diff --git a/config/config_test.go b/config/config_test.go index aba7b2a8..e5d21e5b 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -19,7 +19,7 @@ func TestConfig_Load(t *testing.T) { // invalid yaml c := NewC(l) os.WriteFile(filepath.Join(dir, "01.yaml"), []byte(" invalid yaml"), 0644) - require.EqualError(t, c.Load(dir), "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `invalid...` into map[string]interface {}") + require.EqualError(t, c.Load(dir), filepath.Join(dir, "01.yaml")+": yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `invalid...` into map[string]interface {}") // simple multi config merge c = NewC(l) @@ -129,7 +129,7 @@ func TestConfig_ReloadConfig(t *testing.T) { os.WriteFile(filepath.Join(dir, "01.yaml"), []byte("outer:\n inner: ho"), 0644) - c.RegisterReloadCallback(func(c *C) { + c.RegisterReloadCallback(func(c *C, _ *Reload) { done <- true }) diff --git a/config/reload.go b/config/reload.go new file mode 100644 index 00000000..f0fbd01e --- /dev/null +++ b/config/reload.go @@ -0,0 +1,98 @@ +package config + +import ( + "context" + "fmt" + "log/slog" + "slices" +) + +// ReloadResult describes how a reload went. Problems are logged where they happen, Log writes a one line summary. +type ReloadResult struct { + // Err is set when the new config could not be read or parsed, no callbacks ran + Err error + // Failed is true when a callback could not apply part of the new config + Failed bool + // RestartRequired is true once any reload has needed a restart, and stays true until the process restarts + RestartRequired bool + // RestartKeys are the keys this reload changed that only apply on a restart + RestartKeys []string +} + +// Summary returns a one line summary of the reload and the level to log it at. +func (r ReloadResult) Summary() (slog.Level, string) { + switch { + case r.Err != nil && r.RestartRequired: + return slog.LevelError, "Config reload failed, the new config did not load and a restart is still required" + case r.Err != nil: + return slog.LevelError, "Config reload failed, the new config did not load" + case r.Failed && r.RestartRequired: + return slog.LevelError, "Config reload completed with errors and requires a restart to fully apply, review the logs" + case r.Failed: + return slog.LevelError, "Config reload completed with errors, review the logs" + case r.RestartRequired: + return slog.LevelWarn, "Config reload was completed but requires a restart to fully apply" + default: + return slog.LevelInfo, "Config reload complete" + } +} + +// String is the summary with Err and RestartKeys included, for output other than the log. +func (r ReloadResult) String() string { + _, msg := r.Summary() + if r.Err != nil { + msg = fmt.Sprintf("%s: %v", msg, r.Err) + } + if len(r.RestartKeys) > 0 { + msg = fmt.Sprintf("%s, keys %v", msg, r.RestartKeys) + } + return msg +} + +// Log writes Summary to l. +func (r ReloadResult) Log(l *slog.Logger) { + level, msg := r.Summary() + if len(r.RestartKeys) > 0 { + l.Log(context.Background(), level, msg, "keys", r.RestartKeys) + return + } + l.Log(context.Background(), level, msg) +} + +// Reload is passed to each reload callback to report how applying the new config went. +type Reload struct { + c *C + failed bool + restart bool + // Changed keys that only apply on a restart, from every callback + keys []string +} + +// Failed marks the reload as failed, the caller logs why. Safe to call on a nil Reload, which is what the initial load +// passes. +func (r *Reload) Failed() { + if r != nil { + r.failed = true + } +} + +// RestartRequired marks the reload as needing a restart, the caller logs why. Safe to call on a nil Reload. +func (r *Reload) RestartRequired() { + if r != nil { + r.restart = true + } +} + +// RestartRequiredIfChanged marks the reload as needing a restart if any of keys changed, and adds them to RestartKeys. +// Use it for keys that are only read at startup. Safe to call on a nil Reload. +func (r *Reload) RestartRequiredIfChanged(keys ...string) { + if r == nil { + return + } + for _, k := range keys { + if r.c.HasChanged(k) && !slices.Contains(r.keys, k) { + r.keys = append(r.keys, k) + r.restart = true + } + } +} diff --git a/config/reload_test.go b/config/reload_test.go new file mode 100644 index 00000000..86d16025 --- /dev/null +++ b/config/reload_test.go @@ -0,0 +1,108 @@ +package config + +import ( + "errors" + "log/slog" + "testing" + + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReloadConfigString(t *testing.T) { + c := NewC(test.NewLogger()) + require.NoError(t, c.LoadString("a: 1")) + fail := false + ran := 0 + c.RegisterReloadCallback(func(c *C, r *Reload) { + ran++ + if fail { + r.Failed() + } + }) + + assert.Equal(t, ReloadResult{}, c.ReloadConfigString("a: 2")) + fail = true + assert.Equal(t, ReloadResult{Failed: true}, c.ReloadConfigString("a: 3")) + assert.Equal(t, 2, ran) + + // A config that fails to load runs no callbacks and returns the error + res := c.ReloadConfigString("a: [") + require.Error(t, res.Err) + assert.False(t, res.Failed) + assert.Equal(t, 2, ran) + assert.Equal(t, 3, c.GetInt("a", 0)) +} + +func TestReloadRestartRequiredLatches(t *testing.T) { + c := NewC(test.NewLogger()) + require.NoError(t, c.LoadString("a: 1\nb: 1")) + c.RegisterReloadCallback(func(c *C, r *Reload) { r.RestartRequiredIfChanged("a", "b") }) + + // Nothing that needs a restart changed + assert.Equal(t, ReloadResult{}, c.ReloadConfigString("a: 1\nb: 1\nc: 1")) + assert.Equal(t, ReloadResult{RestartRequired: true, RestartKeys: []string{"a"}}, c.ReloadConfigString("a: 2\nb: 1\nc: 1")) + + // The restart sticks through a reload with no changes, reverting the key and a config that fails to load. Only the + // reload that changed the key lists it + assert.Equal(t, ReloadResult{RestartRequired: true}, c.ReloadConfigString("a: 2\nb: 1\nc: 1")) + assert.Equal(t, ReloadResult{RestartRequired: true, RestartKeys: []string{"a"}}, c.ReloadConfigString("a: 1\nb: 1\nc: 1")) + + res := c.ReloadConfigString("a: [") + require.Error(t, res.Err) + assert.True(t, res.RestartRequired) +} + +// A key reported by more than one callback is listed once +func TestReloadRestartKeysDeduped(t *testing.T) { + c := NewC(test.NewLogger()) + require.NoError(t, c.LoadString("a: 1\nb: 1")) + c.RegisterReloadCallback(func(c *C, r *Reload) { r.RestartRequiredIfChanged("a") }) + c.RegisterReloadCallback(func(c *C, r *Reload) { r.RestartRequiredIfChanged("a", "b") }) + + assert.Equal(t, []string{"a", "b"}, c.ReloadConfigString("a: 2\nb: 2").RestartKeys) +} + +// A callback can require a restart without a key changing, a file the config points at may have +func TestReloadRestartRequiredWithoutAChange(t *testing.T) { + c := NewC(test.NewLogger()) + require.NoError(t, c.LoadString("a: 1")) + c.RegisterReloadCallback(func(c *C, r *Reload) { r.RestartRequired() }) + + assert.Equal(t, ReloadResult{RestartRequired: true}, c.ReloadConfigString("a: 1")) +} + +func TestReloadResultSummary(t *testing.T) { + cases := []struct { + res ReloadResult + level slog.Level + msg string + }{ + {ReloadResult{}, slog.LevelInfo, "Config reload complete"}, + {ReloadResult{RestartRequired: true}, slog.LevelWarn, "Config reload was completed but requires a restart to fully apply"}, + {ReloadResult{Failed: true}, slog.LevelError, "Config reload completed with errors, review the logs"}, + {ReloadResult{Failed: true, RestartRequired: true}, slog.LevelError, "Config reload completed with errors and requires a restart to fully apply, review the logs"}, + {ReloadResult{Err: errors.New("bad yaml")}, slog.LevelError, "Config reload failed, the new config did not load"}, + {ReloadResult{Err: errors.New("bad yaml"), RestartRequired: true}, slog.LevelError, "Config reload failed, the new config did not load and a restart is still required"}, + } + for _, tc := range cases { + level, msg := tc.res.Summary() + assert.Equal(t, tc.level, level, tc.msg) + assert.Equal(t, tc.msg, msg) + } + + res := ReloadResult{RestartRequired: true, RestartKeys: []string{"a", "b"}} + assert.Equal(t, "Config reload was completed but requires a restart to fully apply, keys [a b]", res.String()) + + // String includes the error + assert.Equal(t, "Config reload failed, the new config did not load: bad yaml", ReloadResult{Err: errors.New("bad yaml")}.String()) +} + +// The initial load passes a nil Reload +func TestReloadNilIsSafe(t *testing.T) { + var r *Reload + r.Failed() + r.RestartRequired() + r.RestartRequiredIfChanged("a") +} diff --git a/connection_manager.go b/connection_manager.go index 4ec9f614..af6d7d06 100644 --- a/connection_manager.go +++ b/connection_manager.go @@ -57,7 +57,8 @@ func newConnectionManagerFromConfig(l *slog.Logger, c *config.C, hm *HostMap, p } cm.reload(c, true) - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("timers.connection_alive_interval", "timers.pending_deletion_interval") cm.reload(c, false) }) diff --git a/control_lifecycle_test.go b/control_lifecycle_test.go index 4bc75273..6c0387b6 100644 --- a/control_lifecycle_test.go +++ b/control_lifecycle_test.go @@ -151,9 +151,9 @@ func (c *fakeConn) WriteTo(_ []byte, _ netip.AddrPort) error { return nil } func (c *fakeConn) WriteBatch(bufs [][]byte, _ []netip.AddrPort) (int, error) { return len(bufs), nil } -func (c *fakeConn) ReloadConfig(_ *config.C) {} -func (c *fakeConn) SupportsMultipleReaders() bool { return true } -func (c *fakeConn) Close() error { c.closed = true; return nil } +func (c *fakeConn) ReloadConfig(_ *config.C, _ *config.Reload) {} +func (c *fakeConn) SupportsMultipleReaders() bool { return true } +func (c *fakeConn) Close() error { c.closed = true; return nil } type multiqueueDevice struct { *fakeDevice diff --git a/dns_server.go b/dns_server.go index 9339b068..6a3b9e4d 100644 --- a/dns_server.go +++ b/dns_server.go @@ -69,9 +69,10 @@ func newDnsServerFromConfig(ctx context.Context, l *slog.Logger, pki *PKI, hostM ds.mux = dns.NewServeMux() ds.mux.HandleFunc(".", ds.handleDnsRequest) - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { if err := ds.reload(c, false); err != nil { ds.l.Error("Failed to reload DNS responder from config", "error", err) + r.Failed() } }) diff --git a/e2e/handshakes_test.go b/e2e/handshakes_test.go index aef2e8d1..770ab0f4 100644 --- a/e2e/handshakes_test.go +++ b/e2e/handshakes_test.go @@ -1513,7 +1513,7 @@ func TestLighthouseUpdateOnReload(t *testing.T) { // Reload the config. The lighthouse.hosts change triggers TriggerUpdate, // which wakes the update worker. It calls SendUpdate, initiating a // handshake to the new lighthouse and caching the HostUpdateNotification. - require.NoError(t, myConfig.ReloadConfigString(string(newCfg))) + require.NoError(t, myConfig.ReloadConfigString(string(newCfg)).Err) // Route until the lighthouse receives the HostUpdateNotification. // This covers: handshake stage 1, stage 2, then the cached update. diff --git a/e2e/tunnels_test.go b/e2e/tunnels_test.go index 7831b7dd..e2272ef8 100644 --- a/e2e/tunnels_test.go +++ b/e2e/tunnels_test.go @@ -129,7 +129,7 @@ func TestCertUpgrade(t *testing.T) { } r.Logf("reload new v2-only config") - err = myC.ReloadConfigString(string(cb)) + err = myC.ReloadConfigString(string(cb)).Err assert.NoError(t, err) r.Log("yay, spin until their sees it") waitStart := time.Now() @@ -222,7 +222,7 @@ func TestCertDowngrade(t *testing.T) { } r.Logf("reload new v1-only config") - err = myC.ReloadConfigString(string(cb)) + err = myC.ReloadConfigString(string(cb)).Err assert.NoError(t, err) r.Log("yay, spin until their sees it") waitStart := time.Now() diff --git a/examples/config.yml b/examples/config.yml index 0d1a9180..ef29812e 100644 --- a/examples/config.yml +++ b/examples/config.yml @@ -43,7 +43,7 @@ static_host_map: lighthouse: # am_lighthouse is used to enable lighthouse functionality for a node. This should ONLY be true on nodes - # you have configured to be lighthouses in your network + # you have configured to be lighthouses in your network. Not reloadable. am_lighthouse: false # serve_dns optionally starts a dns listener that responds to various queries and can even be # delegated to for resolution @@ -126,8 +126,9 @@ lighthouse: # Port Nebula will be listening on. The default here is 4242. For a lighthouse node, the port should be defined, # however using port 0 will dynamically assign a port and is recommended for roaming nodes. listen: - # To listen on only ipv4, use "0.0.0.0" + # To listen on only ipv4, use "0.0.0.0". Not reloadable. host: "::" + # Not reloadable. port: 4242 # Sets the max number of packets to pull from the kernel for each syscall (under systems that support recvmmsg) # default is 64, does not support reload @@ -135,7 +136,8 @@ listen: # superpacket, so the receive scratch is batch * 64KiB per listening socket (~4MiB per routine at the # default of 64). Lower this to trade peak per-syscall throughput for memory on constrained hosts. #batch: 64 - # Linux only. Configure socket buffers for the udp side (outside), leave unset to use the system defaults. Values will be doubled by the kernel + # Linux only. Reloadable, but removing a value leaves the socket at the old size until Nebula restarts. + # Configure socket buffers for the udp side (outside), leave unset to use the system defaults. Values will be doubled by the kernel # Default is net.core.rmem_default and net.core.wmem_default (/proc/sys/net/core/rmem_default and /proc/sys/net/core/rmem_default) # Maximum is limited by memory in the system, SO_RCVBUFFORCE and SO_SNDBUFFORCE is used to avoid having to raise the system wide # max, net.core.rmem_max and net.core.wmem_max @@ -179,7 +181,7 @@ listen: # Currently, this defaults to 1 which means we have 1 tun queue reader and 1 # UDP queue reader. Setting this above one will set IFF_MULTI_QUEUE on the tun # device and SO_REUSEPORT on the UDP socket to allow multiple queues. -# This option is only supported on Linux. +# This option is only supported on Linux. Not reloadable. #routines: 1 punchy: @@ -203,6 +205,7 @@ punchy: # Cipher allows you to choose between the available ciphers for your network. Options are chachapoly or aes # IMPORTANT: this value must be identical on ALL NODES/LIGHTHOUSES. We do not/will not support use of different ciphers simultaneously! +# Not reloadable. #cipher: aes # Preferred ranges is used to define a hint about the local network ranges, which speeds up discovering the fastest @@ -234,6 +237,7 @@ punchy: # and absolute paths outside of it will be rejected. Default is $TMP/nebula-debug. # The directory is NOT automatically created. # Overriding this to "" is the same as "/" and will allow overwriting any path on the host. + # Not reloadable. #sandbox_dir: /var/tmp/nebula-debug # EXPERIMENTAL: relay support for networks that can't establish direct connections. @@ -253,19 +257,25 @@ relay: # Configure the private interface. Note: addr is baked into the nebula certificate tun: # When tun is disabled, a lighthouse can be started without a local tun interface (and therefore without root) + # Not reloadable. disabled: false # Name of the device. If not set, a default will be chosen by the OS. # For macOS: if set, must be in the form `utun[0-9]+`. # For NetBSD: Required to be set, must be in the form `tun[0-9]+` + # Not reloadable. dev: nebula1 # Toggles forwarding of local broadcast packets, the address of which depends on the ip/mask encoded in pki.cert + # Not reloadable. drop_local_broadcast: false - # Toggles forwarding of multicast packets + # Toggles forwarding of multicast packets. Not reloadable. drop_multicast: false # Sets the transmit queue length, if you notice lots of transmit drops on the tun it may help to raise this number. Default is 5000 + # Not reloadable. tx_queue: 5000 # Default MTU for every packet, safe setting is (and the default) 1300 for internet based traffic # Anything above 8937, here or on a route below, is capped to 8937 with a warning + # Reloadable on Linux only. Where the tun is handed to nebula by a host app (iOS, Android, macOS network extensions) + # the host app owns the MTU. mtu: 1300 # the use_offloads setting controls if Nebula will attempt to enable GSO and GRO for the tun device. Linux only, not reloadable. @@ -358,7 +368,7 @@ tun: #use_system_route_table: false # Buffer size for reading routes updates. 0 means default system buffer size. (/proc/sys/net/core/rmem_default). # If using massive routes updates, for example BGP, you may need to increase this value to avoid packet loss. - # SO_RCVBUFFORCE is used to avoid having to raise the system wide max + # SO_RCVBUFFORCE is used to avoid having to raise the system wide max. Not reloadable. #use_system_route_table_buffer_size: 0 # Configure logging level @@ -408,14 +418,16 @@ logging: # Handshakes are sent to all known addresses at each interval with a linear backoff, # Wait try_interval after the 1st attempt, 2 * try_interval after the 2nd, etc, until the handshake is older than timeout # A 100ms interval with the default 10 retries will give a handshake 5.5 seconds to resolve before timing out + # Not reloadable. #try_interval: 100ms #retries: 20 # query_buffer is the size of the buffer channel for querying lighthouses, drops are recorded in the lighthouse.query.dropped metric. + # Not reloadable. #query_buffer: 1024 # trigger_buffer is the size of the buffer channel for quickly sending handshakes - # after receiving the response for lighthouse queries + # after receiving the response for lighthouse queries. Not reloadable. #trigger_buffer: 64 # Tunnel manager settings diff --git a/hostmap.go b/hostmap.go index 71814a2f..a3eb0c29 100644 --- a/hostmap.go +++ b/hostmap.go @@ -317,7 +317,7 @@ func NewHostMapFromConfig(l *slog.Logger, c *config.C) *HostMap { hm := newHostMap(l) hm.reload(c, true) - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, _ *config.Reload) { hm.reload(c, false) }) diff --git a/interface.go b/interface.go index 29051be4..96d44446 100644 --- a/interface.go +++ b/interface.go @@ -509,10 +509,11 @@ func (f *Interface) flushSendBatch(sb *batch.SendBatch, q int) { func (f *Interface) RegisterConfigChangeCallbacks(c *config.C) { c.RegisterReloadCallback(f.reloadFirewall) - c.RegisterReloadCallback(f.reloadSendRecvError) - c.RegisterReloadCallback(f.reloadAcceptRecvError) - c.RegisterReloadCallback(f.reloadDisconnectInvalid) - c.RegisterReloadCallback(f.reloadMisc) + for _, reload := range []func(*config.C){ + f.reloadSendRecvError, f.reloadAcceptRecvError, f.reloadDisconnectInvalid, f.reloadMisc, + } { + c.RegisterReloadCallback(func(c *config.C, _ *config.Reload) { reload(c) }) + } for _, udpConn := range f.writers { c.RegisterReloadCallback(udpConn.ReloadConfig) @@ -529,7 +530,7 @@ func (f *Interface) reloadDisconnectInvalid(c *config.C) { } } -func (f *Interface) reloadFirewall(c *config.C) { +func (f *Interface) reloadFirewall(c *config.C, r *config.Reload) { cs := f.pki.getCertState() curCert := cs.getCertificate(cert.Version2) if curCert == nil { @@ -548,6 +549,7 @@ func (f *Interface) reloadFirewall(c *config.C) { fw, err := NewFirewallFromConfig(f.l, cs, c) if err != nil { f.l.Error("Error while creating firewall during reload", "error", err) + r.Failed() return } diff --git a/interface_test.go b/interface_test.go index 1b912bbb..b66f1e81 100644 --- a/interface_test.go +++ b/interface_test.go @@ -65,10 +65,10 @@ func TestReloadFirewall_CertUnsafeNetworksChanged(t *testing.T) { pki.cs.Store(&CertState{v2Cert: c2, initiatingVersion: cert.Version2}) // Reload with the same YAML so HasChanged("firewall") reports false. - require.NoError(t, cfg.ReloadConfigString(rawYAML)) + require.NoError(t, cfg.ReloadConfigString(rawYAML).Err) require.False(t, cfg.HasChanged("firewall")) - f.reloadFirewall(cfg) + f.reloadFirewall(cfg, nil) assert.NotSame(t, fw, f.firewall, "firewall pointer should have been replaced") assert.Equal(t, newUnsafe, f.firewall.unsafeNetworks) @@ -113,8 +113,30 @@ func TestReloadFirewall_NoChange(t *testing.T) { l: l, } - require.NoError(t, cfg.ReloadConfigString(rawYAML)) - f.reloadFirewall(cfg) + require.NoError(t, cfg.ReloadConfigString(rawYAML).Err) + f.reloadFirewall(cfg, nil) assert.Same(t, fw, f.firewall, "firewall should not have been replaced") } + +// Rules that don't parse keep the old firewall, and the reload says it could not apply them +func TestReloadFirewall_BadRulesFailTheReload(t *testing.T) { + l := test.NewLogger() + pki := &PKI{} + pki.cs.Store(&CertState{v2Cert: &dummyCert{version: cert.Version2, networks: []netip.Prefix{netip.MustParsePrefix("10.0.0.1/24")}}, initiatingVersion: cert.Version2}) + + rules := func(proto string) string { + return "firewall:\n outbound:\n - port: any\n proto: any\n host: any\n inbound:\n - port: any\n proto: " + proto + "\n host: any\n" + } + cfg := config.NewC(l) + require.NoError(t, cfg.LoadString(rules("any"))) + fw, err := NewFirewallFromConfig(l, pki.getCertState(), cfg) + require.NoError(t, err) + f := &Interface{pki: pki, firewall: fw, l: l} + cfg.RegisterReloadCallback(f.reloadFirewall) + + res := cfg.ReloadConfigString(rules("nope")) + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.Same(t, fw, f.firewall) +} diff --git a/lighthouse.go b/lighthouse.go index 619c16a0..5e610111 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -143,13 +143,16 @@ func NewLightHouseFromConfig(ctx context.Context, l *slog.Logger, c *config.C, c return nil, err } - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("lighthouse.am_lighthouse", "listen.port", "handshakes.query_buffer", "stats.lighthouse_metrics") err := h.reload(c, false) switch v := err.(type) { case *util.ContextualError: v.Log(l) + r.Failed() case error: l.Error("failed to reload lighthouse", "error", err) + r.Failed() } }) diff --git a/lighthouse_test.go b/lighthouse_test.go index ace29c15..4d1c5368 100644 --- a/lighthouse_test.go +++ b/lighthouse_test.go @@ -87,15 +87,15 @@ func TestReloadLighthouseInterval(t *testing.T) { lh.ifce = &mockEncWriter{} // The first one routine is kicked off by main.go currently, lets make sure that one dies - require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 5")) + require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 5").Err) assert.Equal(t, int64(5), lh.interval.Load()) // Subsequent calls are killed off by the LightHouse.Reload function - require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 10")) + require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 10").Err) assert.Equal(t, int64(10), lh.interval.Load()) // If this completes then nothing is stealing our reload routine - require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 11")) + require.NoError(t, c.ReloadConfigString("lighthouse:\n interval: 11").Err) assert.Equal(t, int64(11), lh.interval.Load()) } @@ -334,7 +334,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { } rc, err := yaml.Marshal(nc) require.NoError(t, err) - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) rl := lh.Query(staticHost) require.NotNil(t, rl) @@ -350,7 +350,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { } rc, err = yaml.Marshal(nc) require.NoError(t, err) - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) rl = lh.Query(staticHost) require.NotNil(t, rl) @@ -358,7 +358,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { assert.Equal(t, []netip.AddrPort{netip.MustParseAddrPort("1.1.1.1:4242")}, rl.CopyAddrs([]netip.Prefix{})) // Reload with the same config. An unchanged entry must not duplicate. - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) rl = lh.Query(staticHost) require.NotNil(t, rl) @@ -373,7 +373,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { } rc, err = yaml.Marshal(nc) require.NoError(t, err) - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) // Add a second host alongside the first. Both should be present, neither duplicated. nc = map[string]any{ @@ -384,7 +384,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { } rc, err = yaml.Marshal(nc) require.NoError(t, err) - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) rl = lh.Query(staticHost) require.NotNil(t, rl) @@ -406,7 +406,7 @@ func TestLighthouse_reloadStaticHostMap(t *testing.T) { } rc, err = yaml.Marshal(nc) require.NoError(t, err) - require.NoError(t, c.ReloadConfigString(string(rc))) + require.NoError(t, c.ReloadConfigString(string(rc)).Err) _, isStatic := lh.GetStaticHostList()[staticHost] assert.False(t, isStatic) @@ -966,13 +966,13 @@ func TestLighthouse_reloadRelaysTriggersUpdate(t *testing.T) { require.NoError(t, err) assert.Empty(t, lh.updateTrigger) - require.NoError(t, c.ReloadConfigString(base+"relay:\n relays: [10.128.0.6]\n")) + require.NoError(t, c.ReloadConfigString(base+"relay:\n relays: [10.128.0.6]\n").Err) assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.6")}, lh.GetRelaysForMe()) require.Len(t, lh.updateTrigger, 1) <-lh.updateTrigger // The config changed but what we advertise did not, a bad entry is skipped - require.NoError(t, c.ReloadConfigString(base+"relay:\n relays: [10.128.0.6, nope]\n")) + require.NoError(t, c.ReloadConfigString(base+"relay:\n relays: [10.128.0.6, nope]\n").Err) assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.6")}, lh.GetRelaysForMe()) assert.Empty(t, lh.updateTrigger) } @@ -988,12 +988,29 @@ func TestLighthouse_reloadAmRelayAlone(t *testing.T) { assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.5")}, lh.GetRelaysForMe()) // Relays aren't allowed to have relays, flipping am_relay alone must drop them and tell the lighthouses - require.NoError(t, c.ReloadConfigString(base+" am_relay: true\n")) + require.NoError(t, c.ReloadConfigString(base+" am_relay: true\n").Err) assert.Empty(t, lh.GetRelaysForMe()) require.Len(t, lh.updateTrigger, 1) <-lh.updateTrigger - require.NoError(t, c.ReloadConfigString(base+" am_relay: false\n")) + require.NoError(t, c.ReloadConfigString(base+" am_relay: false\n").Err) assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.5")}, lh.GetRelaysForMe()) require.Len(t, lh.updateTrigger, 1) } + +func TestLighthouse_reloadRestartRequired(t *testing.T) { + l := test.NewLogger() + c := config.NewC(l) + base := "lighthouse:\n am_lighthouse: true\n" + require.NoError(t, c.LoadString(base+"listen:\n port: 4242\n")) + + _, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(netip.MustParsePrefix("10.128.0.1/24")), nil, nil) + require.NoError(t, err) + + res := c.ReloadConfigString(base + "listen:\n port: 4242\n") + require.NoError(t, res.Err) + assert.False(t, res.RestartRequired) + res = c.ReloadConfigString(base + "listen:\n port: 4243\n") + require.NoError(t, res.Err) + assert.True(t, res.RestartRequired) +} diff --git a/main.go b/main.go index a8d01779..e7adfd58 100644 --- a/main.go +++ b/main.go @@ -197,7 +197,7 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev if err != nil { return nil, util.NewContextualError("Failed to open udp listener", m{"queue": i}, err) } - udpServer.ReloadConfig(c) + udpServer.ReloadConfig(c, nil) udpConns[i] = udpServer // If port is dynamic, discover it before the next pass through the for loop @@ -311,6 +311,16 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev lightHouse.ifce = ifce ifce.RegisterConfigChangeCallbacks(c) + // These are only read at startup + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged( + "routines", "tun.routines", "listen.routines", "firewall.conntrack.routine_cache_timeout", + "listen.port", "listen.host", "stats.message_metrics", + "handshakes.try_interval", "handshakes.retries", "handshakes.trigger_buffer", + "tun.drop_local_broadcast", "tun.drop_multicast", + ) + r.RestartRequiredIfChanged(osStartupOnlyKeys...) + }) ifce.reloadDisconnectInvalid(c) ifce.reloadSendRecvError(c) ifce.reloadAcceptRecvError(c) diff --git a/main_restart_keys_linux.go b/main_restart_keys_linux.go new file mode 100644 index 00000000..5f7071c1 --- /dev/null +++ b/main_restart_keys_linux.go @@ -0,0 +1,6 @@ +//go:build linux && !android && !e2e_testing + +package nebula + +// osStartupOnlyKeys are startup only keys that only matter on linux: udp batch size, udp offloads and thread pinning +var osStartupOnlyKeys = []string{"listen.batch", "listen.udp_offloads", "tun.pin_threads", "tun.pin_threads_key", "tun.cpu_affinity"} diff --git a/main_restart_keys_other.go b/main_restart_keys_other.go new file mode 100644 index 00000000..faaaed80 --- /dev/null +++ b/main_restart_keys_other.go @@ -0,0 +1,6 @@ +//go:build !linux || android || e2e_testing + +package nebula + +// osStartupOnlyKeys is empty, none of the OS specific startup keys matter here +var osStartupOnlyKeys []string diff --git a/overlay/tun.go b/overlay/tun.go index 0ba130b6..dbf2e843 100644 --- a/overlay/tun.go +++ b/overlay/tun.go @@ -41,9 +41,16 @@ func (e *NameError) Error() string { type DeviceFactory func(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, routines int) (Device, error) func NewDeviceFromConfig(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, routines int) (Device, error) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.disabled") + }) + switch { case c.GetBool("tun.disabled", false): tun := newDisabledTun(vpnNetworks, c.GetInt("tun.tx_queue", 5000), c.GetBool("stats.message_metrics", false), l) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.tx_queue", "stats.message_metrics") + }) return tun, nil default: diff --git a/overlay/tun_android.go b/overlay/tun_android.go index 819aa1f7..4d21f526 100644 --- a/overlay/tun_android.go +++ b/overlay/tun_android.go @@ -38,16 +38,17 @@ func newTunFromFd(c *config.C, l *slog.Logger, deviceFd int, vpnNetworks []netip l: l, } - err := t.reload(c, true) + err := t.reload(c, true, nil) if err != nil { _ = file.Close() return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -67,7 +68,7 @@ func (t *tun) Activate() error { return nil } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err diff --git a/overlay/tun_bypass_windows.go b/overlay/tun_bypass_windows.go index 1f62373c..d5a9708b 100644 --- a/overlay/tun_bypass_windows.go +++ b/overlay/tun_bypass_windows.go @@ -10,6 +10,9 @@ import ( "github.com/slackhq/nebula/wfp" ) +// interfaceBypassKeys are only read when the interface bypass is installed +var interfaceBypassKeys = []string{"tun.windows_bypass_wdf"} + // installInterfaceBypass installs a WFP PERMIT filter scoped to the wintun interface LUID so inbound traffic on the // nebula adapter bypasses Windows Defender Firewall. func installInterfaceBypass(l *slog.Logger, luid uint64) closer { diff --git a/overlay/tun_bypass_windows_386.go b/overlay/tun_bypass_windows_386.go index d8878b60..4cf9fb04 100644 --- a/overlay/tun_bypass_windows_386.go +++ b/overlay/tun_bypass_windows_386.go @@ -4,6 +4,9 @@ package overlay import "log/slog" +// interfaceBypassKeys is empty, windows-386 has no bypass +var interfaceBypassKeys []string + // installInterfaceBypass is a no-op on windows-386 because we don't currently build for it. func installInterfaceBypass(_ *slog.Logger, _ uint64) closer { return nil diff --git a/overlay/tun_darwin.go b/overlay/tun_darwin.go index 0824d8bf..dcde7fae 100644 --- a/overlay/tun_darwin.go +++ b/overlay/tun_darwin.go @@ -132,15 +132,17 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t l: l, } - err = t.reload(c, true) + err = t.reload(c, true, nil) if err != nil { return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.mtu") + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -174,14 +176,15 @@ func newTunFromFd(c *config.C, l *slog.Logger, deviceFd int, vpnNetworks []netip l: l, } - if err := t.reload(c, true); err != nil { + if err := t.reload(c, true, nil); err != nil { _ = file.Close() return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - if err := t.reload(c, false); err != nil { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + if err := t.reload(c, false, r); err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -268,7 +271,7 @@ func (t *tun) Activate() error { } // Unsafe path routes - return t.addRoutes(false) + return t.addRoutes(false, nil) } // activate4 assigns our vpn address as a /32 and routes the vpn network to the tun explicitly. @@ -366,7 +369,7 @@ func (t *tun) activate6(network netip.Prefix) error { return nil } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -387,16 +390,18 @@ func (t *tun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes), r) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) + r.Failed() } // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // Catch any stray logs util.LogWithContextIfNeeded("Failed to add routes", err, t.l) + r.Failed() } } @@ -489,7 +494,7 @@ func getLinkAddr(name string) (*netroute.LinkAddr, error) { return nil, nil } -func (t *tun) addRoutes(logErrors bool) error { +func (t *tun) addRoutes(logErrors bool, rl *config.Reload) error { // The route tree is still ours, the system routing table is not if t.hostOwned { return nil @@ -524,6 +529,7 @@ func (t *tun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() } else { return retErr } @@ -538,7 +544,7 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } -func (t *tun) removeRoutes(routes []Route) error { +func (t *tun) removeRoutes(routes []Route, rl *config.Reload) error { linkAddr := t.linkAddr.Load() if t.hostOwned || linkAddr == nil { return nil @@ -551,8 +557,12 @@ func (t *tun) removeRoutes(routes []Route) error { } err := delRoute(r.Cidr, linkAddr) - if err != nil { + if errors.Is(err, unix.ESRCH) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_darwin_route_test.go b/overlay/tun_darwin_route_test.go index 72fe008b..a566dd38 100644 --- a/overlay/tun_darwin_route_test.go +++ b/overlay/tun_darwin_route_test.go @@ -72,8 +72,8 @@ func TestTunRoutesBeforeActivate(t *testing.T) { }} tn.Routes.Store(&routes) - assert.NoError(t, tn.addRoutes(true)) - assert.NoError(t, tn.removeRoutes(routes)) + assert.NoError(t, tn.addRoutes(true, nil)) + assert.NoError(t, tn.removeRoutes(routes, nil)) } // A tun.routes entry with no via is never installed, so removing it must not delete a route for the same cidr that @@ -96,7 +96,9 @@ func TestTunReloadLeavesRoutesItNeverInstalled(t *testing.T) { t.Cleanup(func() { _ = tn.Close() }) require.NoError(t, tn.Activate()) - require.NoError(t, c.ReloadConfigString("tun: {}\n")) + res := c.ReloadConfigString("tun: {}\n") + require.NoError(t, res.Err) + assert.False(t, res.Failed, "removing a route we never installed failed the reload") msgs, err := fetchRoutes() require.NoError(t, err) assert.Equal(t, lo0.Index, routeInterfaceIn(msgs, netip.MustParsePrefix(cidr))) diff --git a/overlay/tun_freebsd.go b/overlay/tun_freebsd.go index 8ed90daf..d68e3df6 100644 --- a/overlay/tun_freebsd.go +++ b/overlay/tun_freebsd.go @@ -385,16 +385,18 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t }, } - err = t.reload(c, true) + err = t.reload(c, true, nil) if err != nil { return nil, err } closeOnErr = false - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.mtu") + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -493,7 +495,7 @@ func (t *tun) Activate() error { } } - return t.addRoutes(false) + return t.addRoutes(false, nil) } func (t *tun) setMTU() error { @@ -509,7 +511,7 @@ func (t *tun) setMTU() error { return err } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -530,16 +532,18 @@ func (t *tun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes), r) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) + r.Failed() } // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // Catch any stray logs util.LogWithContextIfNeeded("Failed to add routes", err, t.l) + r.Failed() } } @@ -563,7 +567,7 @@ func (t *tun) Queues(int) ([]tio.Queue, error) { return []tio.Queue{tio.NewSingleQueue(t, defaultBatchBufSize)}, nil } -func (t *tun) addRoutes(logErrors bool) error { +func (t *tun) addRoutes(logErrors bool, rl *config.Reload) error { routes := *t.Routes.Load() for _, r := range routes { if len(r.Via) == 0 || !r.Install { @@ -576,6 +580,7 @@ func (t *tun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() } else { return retErr } @@ -587,7 +592,7 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } -func (t *tun) removeRoutes(routes []Route) error { +func (t *tun) removeRoutes(routes []Route, rl *config.Reload) error { for _, r := range routes { if len(r.Via) == 0 || !r.Install { // addRoutes never installs these @@ -595,8 +600,12 @@ func (t *tun) removeRoutes(routes []Route) error { } err := delRoute(r.Cidr, t.linkAddr) - if err != nil { + if errors.Is(err, unix.ESRCH) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_ios.go b/overlay/tun_ios.go index 9ee30f77..cbbad940 100644 --- a/overlay/tun_ios.go +++ b/overlay/tun_ios.go @@ -47,16 +47,17 @@ func newTunFromFd(c *config.C, l *slog.Logger, deviceFd int, vpnNetworks []netip l: l, } - err := t.reload(c, true) + err := t.reload(c, true, nil) if err != nil { _ = file.Close() return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -67,7 +68,7 @@ func (t *tun) Activate() error { return nil } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err diff --git a/overlay/tun_linux.go b/overlay/tun_linux.go index 5e27ad9b..541ef6b8 100644 --- a/overlay/tun_linux.go +++ b/overlay/tun_linux.go @@ -185,6 +185,10 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, multiqueue } t.Device = name + // Only read when we create the device, an fd handed to us already has its name and offloads + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.use_offloads") + }) return t, nil } @@ -228,22 +232,24 @@ func newTunGeneric(c *config.C, l *slog.Logger, fd int, vnetHdr bool, offloadFla l: l, } - if err = t.reload(c, true); err != nil { + if err = t.reload(c, true, nil); err != nil { _ = t.Close() return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.tx_queue", "tun.use_system_route_table", "tun.use_system_route_table_buffer_size") + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) return t, nil } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { routeChange, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -297,7 +303,7 @@ func (t *tun) reload(c *config.C, initial bool) error { if !initial { if oldMaxMTU != newMaxMTU { - t.setMTU() + t.setMTU(r) t.l.Info("Set max MTU", "mtu", t.MaxMTU, "oldMTU", oldMaxMTU) } @@ -305,7 +311,8 @@ func (t *tun) reload(c *config.C, initial bool) error { for i := range t.vpnNetworks { err := t.setDefaultRoute(t.vpnNetworks[i]) if err != nil { - t.l.Warn(err.Error()) + t.l.Error(err.Error()) + r.Failed() } else { t.l.Info("Set default MTU", "mtu", t.DefaultMTU, "oldMTU", oldDefaultMTU) } @@ -313,13 +320,14 @@ func (t *tun) reload(c *config.C, initial bool) error { } // Remove first, if the system removes a wanted route hopefully it will be re-added next - t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + t.removeRoutes(findRemovedRoutes(routes, *oldRoutes), r) // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // This should never be called since addRoutes should log its own errors in a reload condition util.LogWithContextIfNeeded("Failed to refresh routes", err, t.l) + r.Failed() } } @@ -474,7 +482,7 @@ func (t *tun) Activate() error { t.deviceIndex = link.Attrs().Index // Setup our default MTU - t.setMTU() + t.setMTU(nil) // Set the transmit queue length ifrq := ifreqQLEN{Name: devName, Value: int32(t.TXQueueLen)} @@ -506,7 +514,7 @@ func (t *tun) Activate() error { } // Set the routes - if err = t.addRoutes(false); err != nil { + if err = t.addRoutes(false, nil); err != nil { return err } @@ -519,12 +527,13 @@ func (t *tun) Activate() error { return nil } -func (t *tun) setMTU() { +func (t *tun) setMTU(r *config.Reload) { // Set the MTU on the device ifm := ifreqMTU{Name: t.deviceBytes(), MTU: int32(t.MaxMTU)} if err := ioctl(t.ioctlFd, unix.SIOCSIFMTU, uintptr(unsafe.Pointer(&ifm))); err != nil { // This is currently a non fatal condition because the route table must have the MTU set appropriately as well t.l.Error("Failed to set tun mtu", "error", err) + r.Failed() } } @@ -577,7 +586,7 @@ func (t *tun) setDefaultRoute(cidr netip.Prefix) error { return nil } -func (t *tun) addRoutes(logErrors bool) error { +func (t *tun) addRoutes(logErrors bool, rl *config.Reload) error { // Path routes routes := *t.Routes.Load() for _, r := range routes { @@ -607,6 +616,7 @@ func (t *tun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() } else { return retErr } @@ -618,7 +628,7 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } -func (t *tun) removeRoutes(routes []Route) { +func (t *tun) removeRoutes(routes []Route, rl *config.Reload) { for _, r := range routes { if !r.Install { continue @@ -642,8 +652,12 @@ func (t *tun) removeRoutes(routes []Route) { } err := netlink.RouteDel(&nr) - if err != nil { + if errors.Is(err, unix.ESRCH) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_netbsd.go b/overlay/tun_netbsd.go index d9852ba5..056e5f98 100644 --- a/overlay/tun_netbsd.go +++ b/overlay/tun_netbsd.go @@ -103,15 +103,17 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t l: l, } - err = t.reload(c, true) + err = t.reload(c, true, nil) if err != nil { return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.mtu") + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -324,7 +326,7 @@ func (t *tun) Activate() error { } } - return t.addRoutes(false) + return t.addRoutes(false, nil) } func (t *tun) doIoctlByName(ctl uintptr, value uint32) error { @@ -339,7 +341,7 @@ func (t *tun) doIoctlByName(ctl uintptr, value uint32) error { return err } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -360,16 +362,18 @@ func (t *tun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes), r) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) + r.Failed() } // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // Catch any stray logs util.LogWithContextIfNeeded("Failed to add routes", err, t.l) + r.Failed() } } @@ -393,7 +397,7 @@ func (t *tun) Queues(int) ([]tio.Queue, error) { return []tio.Queue{tio.NewSingleQueue(t, defaultBatchBufSize)}, nil } -func (t *tun) addRoutes(logErrors bool) error { +func (t *tun) addRoutes(logErrors bool, rl *config.Reload) error { routes := *t.Routes.Load() for _, r := range routes { @@ -407,6 +411,7 @@ func (t *tun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() } else { return retErr } @@ -418,7 +423,7 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } -func (t *tun) removeRoutes(routes []Route) error { +func (t *tun) removeRoutes(routes []Route, rl *config.Reload) error { for _, r := range routes { if len(r.Via) == 0 || !r.Install { // addRoutes never installs these @@ -426,8 +431,12 @@ func (t *tun) removeRoutes(routes []Route) error { } err := delRoute(r.Cidr, t.vpnNetworks) - if err != nil { + if errors.Is(err, unix.ESRCH) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_openbsd.go b/overlay/tun_openbsd.go index b310a0d1..d9aa7a30 100644 --- a/overlay/tun_openbsd.go +++ b/overlay/tun_openbsd.go @@ -94,15 +94,17 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*t l: l, } - err = t.reload(c, true) + err = t.reload(c, true, nil) if err != nil { return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.mtu") + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) @@ -303,7 +305,7 @@ func (t *tun) Activate() error { } } - return t.addRoutes(false) + return t.addRoutes(false, nil) } func (t *tun) doIoctlByName(ctl uintptr, value uint32) error { @@ -318,7 +320,7 @@ func (t *tun) doIoctlByName(ctl uintptr, value uint32) error { return err } -func (t *tun) reload(c *config.C, initial bool) error { +func (t *tun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -339,16 +341,18 @@ func (t *tun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedRoutes(routes, *oldRoutes), r) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) + r.Failed() } // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // Catch any stray logs util.LogWithContextIfNeeded("Failed to add routes", err, t.l) + r.Failed() } } @@ -372,7 +376,7 @@ func (t *tun) Queues(int) ([]tio.Queue, error) { return []tio.Queue{tio.NewSingleQueue(t, defaultBatchBufSize)}, nil } -func (t *tun) addRoutes(logErrors bool) error { +func (t *tun) addRoutes(logErrors bool, rl *config.Reload) error { routes := *t.Routes.Load() for _, r := range routes { @@ -386,6 +390,7 @@ func (t *tun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() } else { return retErr } @@ -397,7 +402,7 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } -func (t *tun) removeRoutes(routes []Route) error { +func (t *tun) removeRoutes(routes []Route, rl *config.Reload) error { for _, r := range routes { if len(r.Via) == 0 || !r.Install { // addRoutes never installs these @@ -405,8 +410,12 @@ func (t *tun) removeRoutes(routes []Route) error { } err := delRoute(r.Cidr, t.vpnNetworks) - if err != nil { + if errors.Is(err, unix.ESRCH) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_reload_linux_test.go b/overlay/tun_reload_linux_test.go index 0fcf48f4..de358ebf 100644 --- a/overlay/tun_reload_linux_test.go +++ b/overlay/tun_reload_linux_test.go @@ -4,6 +4,7 @@ package overlay import ( "net/netip" + "os" "testing" "github.com/slackhq/nebula/config" @@ -25,10 +26,10 @@ func TestReloadMTUOnlyKeepsRoutes(t *testing.T) { ioctlFd: ^uintptr(0), routesFromSystem: map[netip.Prefix]routing.Gateways{}, } - require.NoError(t, tn.reload(c, true)) + require.NoError(t, tn.reload(c, true, nil)) - require.NoError(t, c.ReloadConfigString("tun:\n mtu: 1400\n"+routes)) - _ = tn.reload(c, false) + require.NoError(t, c.ReloadConfigString("tun:\n mtu: 1400\n"+routes).Err) + _ = tn.reload(c, false, nil) got := *tn.Routes.Load() require.Len(t, got, 1) @@ -50,7 +51,57 @@ func TestReloadMaxMTUIsTheLargestRoute(t *testing.T) { ioctlFd: ^uintptr(0), routesFromSystem: map[netip.Prefix]routing.Gateways{}, } - require.NoError(t, tn.reload(c, true)) + require.NoError(t, tn.reload(c, true, nil)) assert.Equal(t, 1500, tn.MaxMTU) assert.Equal(t, 1300, tn.DefaultMTU) } + +// There is no device behind this tun, so adding a route always fails. Removing one is refused without root, as root the +// kernel answers that the route isn't there, which is what we wanted +func TestReloadRouteFailures(t *testing.T) { + route := "tun:\n mtu: 1300\n unsafe_routes:\n - route: 192.168.100.0/24\n via: 10.1.0.1\n" + cases := []struct { + name string + from, to string + wantFails bool + }{ + {"added", "tun:\n mtu: 1300\n", route, true}, + {"removed", route, "tun:\n mtu: 1300\n", os.Geteuid() != 0}, + {"unchanged", route, route + "punchy:\n punch: true\n", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString(tc.from)) + tn := &tun{ + l: test.NewLogger(), + vpnNetworks: []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, + ioctlFd: ^uintptr(0), + routesFromSystem: map[netip.Prefix]routing.Gateways{}, + } + require.NoError(t, tn.reload(c, true, nil)) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + require.NoError(t, tn.reload(c, false, r)) + }) + + res := c.ReloadConfigString(tc.to) + require.NoError(t, res.Err) + assert.Equal(t, tc.wantFails, res.Failed) + }) + } +} + +// Keys only read when the device is created need a restart to apply +func TestTunReloadRestartKeys(t *testing.T) { + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString("tun:\n dev: nebrk0\n")) + tn, err := NewDeviceFromConfig(c, test.NewLogger(), []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, 1) + if err != nil { + t.Skipf("can't create a tun device here: %v", err) + } + t.Cleanup(func() { _ = tn.Close() }) + + res := c.ReloadConfigString("tun:\n disabled: true\n dev: nebrk1\n tx_queue: 100\n") + require.NoError(t, res.Err) + assert.ElementsMatch(t, []string{"tun.disabled", "tun.dev", "tun.tx_queue"}, res.RestartKeys) +} diff --git a/overlay/tun_test.go b/overlay/tun_test.go new file mode 100644 index 00000000..16354fe5 --- /dev/null +++ b/overlay/tun_test.go @@ -0,0 +1,24 @@ +package overlay + +import ( + "net/netip" + "testing" + + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// tun.disabled and the disabled tun's settings are only read when the device is made +func TestDisabledTunRestartKeys(t *testing.T) { + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString("tun:\n disabled: true\n")) + d, err := NewDeviceFromConfig(c, test.NewLogger(), []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, 1) + require.NoError(t, err) + t.Cleanup(func() { _ = d.Close() }) + + res := c.ReloadConfigString("tun:\n disabled: false\n tx_queue: 100\n") + require.NoError(t, res.Err) + assert.ElementsMatch(t, []string{"tun.disabled", "tun.tx_queue"}, res.RestartKeys) +} diff --git a/overlay/tun_windows.go b/overlay/tun_windows.go index 18680f2b..fcb6ac93 100644 --- a/overlay/tun_windows.go +++ b/overlay/tun_windows.go @@ -84,7 +84,7 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*w l: l, } - err = t.reload(c, true) + err = t.reload(c, true, nil) if err != nil { return nil, err } @@ -108,17 +108,20 @@ func newTun(c *config.C, l *slog.Logger, vpnNetworks []netip.Prefix, _ bool) (*w // Disable registration before Activate assigns addresses and triggers it. disableDNSRegistration(l, *guid) - c.RegisterReloadCallback(func(c *config.C) { - err := t.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("tun.dev", "tun.mtu", "tun.network_category") + r.RestartRequiredIfChanged(interfaceBypassKeys...) + err := t.reload(c, false, r) if err != nil { util.LogWithContextIfNeeded("failed to reload tun device", err, t.l) + r.Failed() } }) return t, nil } -func (t *winTun) reload(c *config.C, initial bool) error { +func (t *winTun) reload(c *config.C, initial bool, r *config.Reload) error { change, routes, err := getAllRoutesFromConfig(t.l, c, t.vpnNetworks, initial) if err != nil { return err @@ -139,16 +142,18 @@ func (t *winTun) reload(c *config.C, initial bool) error { if !initial { // Remove first, if the system removes a wanted route hopefully it will be re-added next - err := t.removeRoutes(findRemovedSystemRoutes(routes, *oldRoutes)) + err := t.removeRoutes(findRemovedSystemRoutes(routes, *oldRoutes), r) if err != nil { util.LogWithContextIfNeeded("Failed to remove routes", err, t.l) + r.Failed() } // Ensure any routes we actually want are installed - err = t.addRoutes(true) + err = t.addRoutes(true, r) if err != nil { // Catch any stray logs util.LogWithContextIfNeeded("Failed to add routes", err, t.l) + r.Failed() } } @@ -163,7 +168,7 @@ func (t *winTun) Activate() error { return fmt.Errorf("failed to set address: %w", err) } - err = t.addRoutes(false) + err = t.addRoutes(false, nil) if err != nil { return err } @@ -182,7 +187,7 @@ func (t *winTun) Activate() error { return nil } -func (t *winTun) addRoutes(logErrors bool) error { +func (t *winTun) addRoutes(logErrors bool, rl *config.Reload) error { luid := winipcfg.LUID(t.tun.LUID()) routes := *t.Routes.Load() foundDefault4 := false @@ -213,6 +218,7 @@ func (t *winTun) addRoutes(logErrors bool) error { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { retErr.Log(t.l) + rl.Failed() continue } else { return retErr @@ -274,7 +280,7 @@ func (t *winTun) setMTU(luid winipcfg.LUID, foundDefault4, carriesV6 bool) error return nil } -func (t *winTun) removeRoutes(routes []Route) error { +func (t *winTun) removeRoutes(routes []Route, rl *config.Reload) error { luid := winipcfg.LUID(t.tun.LUID()) for _, r := range routes { @@ -284,8 +290,12 @@ func (t *winTun) removeRoutes(routes []Route) error { // See comment on luid.AddRoute err := luid.DeleteRoute(r.Cidr, unspecifiedNextHop(r.Cidr)) - if err != nil { + if errors.Is(err, windows.ERROR_NOT_FOUND) { + // Already gone, which is what we wanted + t.l.Info("Route was already removed", "route", r) + } else if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) + rl.Failed() } else { t.l.Info("Removed route", "route", r) } diff --git a/overlay/tun_windows_route_test.go b/overlay/tun_windows_route_test.go index 71c5c232..1546dfd6 100644 --- a/overlay/tun_windows_route_test.go +++ b/overlay/tun_windows_route_test.go @@ -53,6 +53,14 @@ func (rt *routeTest) metric(t *testing.T, cidr string) uint32 { return row.Metric } +// reload applies cfg and expects nothing in it to fail +func (rt *routeTest) reload(t *testing.T, cfg string) { + t.Helper() + res := rt.c.ReloadConfigString(cfg) + require.NoError(t, res.Err) + assert.False(t, res.Failed, "the reload reported a failure") +} + func unsafeRoutes(routes ...string) string { var b strings.Builder b.WriteString("tun:\n dev: nebroutetest\n unsafe_routes:\n") @@ -77,7 +85,7 @@ func TestWinTun_ReloadKeepsInstalledRoutes(t *testing.T) { rt := newRouteTest(t, unsafeRoutes(a)) require.NoError(t, rt.tun.Activate()) - require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a, b))) + rt.reload(t, unsafeRoutes(a, b)) assert.NotContains(t, rt.log.String(), "Failed to add route") rt.metric(t, a) rt.metric(t, b) @@ -92,7 +100,7 @@ func TestWinTun_ReloadMetricChangeApplies(t *testing.T) { require.NoError(t, rt.tun.Activate()) require.Equal(t, uint32(100), rt.metric(t, a)) - require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@200"))) + rt.reload(t, unsafeRoutes(a+"@200")) assert.NotContains(t, rt.log.String(), "Failed to add route") assert.NotContains(t, rt.log.String(), "Removed route") assert.Equal(t, uint32(200), rt.metric(t, a)) @@ -120,7 +128,7 @@ func TestWinTun_DuplicateCidrInstallsTheLastEntry(t *testing.T) { require.NoError(t, rt.tun.Activate()) assert.Equal(t, uint32(200), rt.metric(t, a)) - require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@100", a+"@200", b))) + rt.reload(t, unsafeRoutes(a+"@100", a+"@200", b)) assert.Equal(t, uint32(200), rt.metric(t, a)) assert.NotContains(t, rt.log.String(), "Updated route metric") assert.NotContains(t, rt.log.String(), "Failed to add route") @@ -134,7 +142,7 @@ func TestWinTun_DroppingAnUninstalledDuplicateKeepsTheRoute(t *testing.T) { rt := newRouteTest(t, unsafeRoutes(a+"@100", a+"@200")) require.NoError(t, rt.tun.Activate()) - require.NoError(t, rt.c.ReloadConfigString(unsafeRoutes(a+"@200"))) + rt.reload(t, unsafeRoutes(a+"@200")) assert.NotContains(t, rt.log.String(), "Removed route") assert.Equal(t, uint32(200), rt.metric(t, a)) }) @@ -155,12 +163,24 @@ func TestWinTun_ReloadMatchesAFreshStart(t *testing.T) { rt := newRouteTest(t, unsafeRoutes(a+"@100")) require.NoError(t, rt.tun.Activate()) - require.NoError(t, rt.c.ReloadConfigString(routes)) + rt.reload(t, routes) _, err = rt.luid.Route(cidr, unspecifiedNextHop(cidr)) assert.Error(t, err, "the reload left it installed") }) } +// Removing a route that is already gone is what we wanted, not a failure +func TestWinTun_ReloadRouteAlreadyGone(t *testing.T) { + forFamilies(t, func(t *testing.T, a, b string) { + rt := newRouteTest(t, unsafeRoutes(a, b)) + require.NoError(t, rt.tun.Activate()) + p := netip.MustParsePrefix(b) + require.NoError(t, rt.luid.DeleteRoute(p, unspecifiedNextHop(p))) + + rt.reload(t, unsafeRoutes(a)) + }) +} + // forFamilies runs f with two unsafe_routes CIDRs of each address family, the tun's own network stays IPv4 func forFamilies(t *testing.T, f func(t *testing.T, a, b string)) { t.Run("ipv4", func(t *testing.T) { f(t, "10.251.1.0/24", "10.251.2.0/24") }) diff --git a/pki.go b/pki.go index 17efe7c1..0e4deea3 100644 --- a/pki.go +++ b/pki.go @@ -50,15 +50,19 @@ type CertState struct { myVpnBroadcastAddrsTable *bart.Lite } +// errCertNeedsRestart marks a new cert that is refused on reload because only a restart can apply it +var errCertNeedsRestart = errors.New("a restart is required to use the new cert") + func NewPKIFromConfig(l *slog.Logger, c *config.C) (*PKI, error) { pki := &PKI{l: l} - err := pki.reload(c, true) + err := pki.reload(c, true, nil) if err != nil { return nil, err } - c.RegisterReloadCallback(func(c *config.C) { - rErr := pki.reload(c, false) + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("cipher") + rErr := pki.reload(c, false, r) if rErr != nil { util.LogWithContextIfNeeded("Failed to reload PKI from config", rErr, l) } @@ -75,13 +79,29 @@ func (p *PKI) getCertState() *CertState { return p.cs.Load() } -func (p *PKI) reload(c *config.C, initial bool) error { +// r is nil on the initial load +func (p *PKI) reload(c *config.C, initial bool, r *config.Reload) error { + before := p.cs.Load() err := p.reloadCerts(c, initial) if err != nil { if initial { return err } err.Log(p.l) + if errors.Is(err, errCertNeedsRestart) { + r.RestartRequired() + } else { + r.Failed() + } + } else if !initial { + // A cert for a version we weren't running is accepted without comparing its networks to ours. The tun and the + // lighthouse keep the networks they started with + after := p.cs.Load() + if !slices.Equal(before.myVpnNetworks, after.myVpnNetworks) { + p.l.Warn("The new cert changes our networks, a restart is required to use them", + "old_networks", before.myVpnNetworks, "new_networks", after.myVpnNetworks) + r.RestartRequired() + } } err = p.reloadCAPool(c) @@ -90,6 +110,7 @@ func (p *PKI) reload(c *config.C, initial bool) error { return err } err.Log(p.l) + r.Failed() } return nil @@ -132,7 +153,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError { return util.NewContextualError( "Networks in new cert was different from old", m{"new_networks": newState.v1Cert.Networks(), "old_networks": currentState.v1Cert.Networks(), "cert_version": cert.Version1}, - nil, + errCertNeedsRestart, ) } @@ -140,7 +161,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError { return util.NewContextualError( "Curve in new v1 cert was different from old", m{"new_curve": newState.v1Cert.Curve(), "old_curve": currentState.v1Cert.Curve(), "cert_version": cert.Version1}, - nil, + errCertNeedsRestart, ) } } @@ -155,7 +176,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError { return util.NewContextualError( "Networks in new cert was different from old", m{"new_networks": newState.v2Cert.Networks(), "old_networks": currentState.v2Cert.Networks(), "cert_version": cert.Version2}, - nil, + errCertNeedsRestart, ) } @@ -163,7 +184,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError { return util.NewContextualError( "Curve in new cert was different from old", m{"new_curve": newState.v2Cert.Curve(), "old_curve": currentState.v2Cert.Curve(), "cert_version": cert.Version2}, - nil, + errCertNeedsRestart, ) } } @@ -178,7 +199,7 @@ func (p *PKI) reloadCerts(c *config.C, initial bool) *util.ContextualError { return util.NewContextualError( "Removing a V2 cert is not permitted unless it has identical networks to the new V1 cert", m{"new_v1_networks": newState.v1Cert.Networks(), "old_v2_networks": currentState.v2Cert.Networks()}, - nil, + errCertNeedsRestart, ) } } diff --git a/pki_test.go b/pki_test.go index 7bd1a24e..572d4c21 100644 --- a/pki_test.go +++ b/pki_test.go @@ -1,10 +1,16 @@ package nebula import ( + "fmt" + "net/netip" "strings" "testing" + "time" "github.com/slackhq/nebula/cert" + "github.com/slackhq/nebula/cert_test" + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -92,3 +98,83 @@ func TestNewCipherSuiteUnsupportedCurve(t *testing.T) { assert.True(t, strings.HasPrefix(err.Error(), "unsupported curve:"), "got: %v", err) assert.Nil(t, cs) } + +func TestPKIReloadRestartRequired(t *testing.T) { + ca, _, caKey, caPEM := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, time.Time{}, time.Time{}, nil, nil, nil) + pkiConfig := func(cipher, network string) string { + _, _, keyPEM, certPEM := cert_test.NewTestCert(cert.Version2, cert.Curve_CURVE25519, ca, caKey, "host", time.Time{}, time.Time{}, []netip.Prefix{netip.MustParsePrefix(network)}, nil, nil) + return fmt.Sprintf("cipher: %s\npki:\n ca: %q\n cert: %q\n key: %q\n", cipher, caPEM, certPEM, keyPEM) + } + + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString(pkiConfig("aes", "10.1.0.2/24"))) + p, err := NewPKIFromConfig(test.NewLogger(), c) + require.NoError(t, err) + + // A cert that changes our networks is refused and needs a restart, it is not a failure + res := c.ReloadConfigString(pkiConfig("aes", "10.1.0.3/24")) + require.NoError(t, res.Err) + assert.True(t, res.RestartRequired) + assert.False(t, res.Failed) + assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.1.0.2/24")}, p.getCertState().myVpnNetworks) + + // The cipher only applies at startup + c2 := config.NewC(test.NewLogger()) + require.NoError(t, c2.LoadString(pkiConfig("aes", "10.1.0.2/24"))) + p, err = NewPKIFromConfig(test.NewLogger(), c2) + require.NoError(t, err) + res = c2.ReloadConfigString(pkiConfig("chachapoly", "10.1.0.2/24")) + require.NoError(t, res.Err) + assert.True(t, res.RestartRequired) + assert.Equal(t, "aes", p.getCertState().cipher) + + // A cert that keeps our networks applies without a restart + c3 := config.NewC(test.NewLogger()) + require.NoError(t, c3.LoadString(pkiConfig("aes", "10.1.0.2/24"))) + _, err = NewPKIFromConfig(test.NewLogger(), c3) + require.NoError(t, err) + res = c3.ReloadConfigString(pkiConfig("aes", "10.1.0.2/24")) + require.NoError(t, res.Err) + assert.False(t, res.RestartRequired) + + // A cert that can't be read is a failure, and a restart wouldn't help + res = c3.ReloadConfigString(strings.Replace(pkiConfig("aes", "10.1.0.2/24"), "cert: \"", "cert: \"garbage", 1)) + require.NoError(t, res.Err) + assert.True(t, res.Failed) + assert.False(t, res.RestartRequired) +} + +// A cert for a version we weren't running is accepted without comparing networks, so switching to one with different +// networks needs a restart. One with the same networks doesn't +func TestPKIReloadNewVersionWithOtherNetworks(t *testing.T) { + ca1, _, ca1Key, ca1PEM := cert_test.NewTestCaCert(cert.Version1, cert.Curve_CURVE25519, time.Time{}, time.Time{}, nil, nil, nil) + ca2, _, ca2Key, ca2PEM := cert_test.NewTestCaCert(cert.Version2, cert.Curve_CURVE25519, time.Time{}, time.Time{}, nil, nil, nil) + caPEM := string(ca1PEM) + string(ca2PEM) + pkiConfig := func(v cert.Version, network string) string { + ca, caKey := ca1, ca1Key + if v == cert.Version2 { + ca, caKey = ca2, ca2Key + } + _, _, keyPEM, certPEM := cert_test.NewTestCert(v, cert.Curve_CURVE25519, ca, caKey, "host", time.Time{}, time.Time{}, []netip.Prefix{netip.MustParsePrefix(network)}, nil, nil) + return fmt.Sprintf("pki:\n ca: %q\n cert: %q\n key: %q\n", caPEM, certPEM, keyPEM) + } + + for _, tc := range []struct { + network string + restart bool + }{ + {"10.9.0.7/24", true}, + {"10.1.0.2/24", false}, + } { + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString(pkiConfig(cert.Version1, "10.1.0.2/24"))) + p, err := NewPKIFromConfig(test.NewLogger(), c) + require.NoError(t, err) + + res := c.ReloadConfigString(pkiConfig(cert.Version2, tc.network)) + require.NoError(t, res.Err) + assert.False(t, res.Failed, tc.network) + assert.Equal(t, tc.restart, res.RestartRequired, tc.network) + assert.Equal(t, []netip.Prefix{netip.MustParsePrefix(tc.network)}, p.getCertState().myVpnNetworks, "accepted either way, as before") + } +} diff --git a/punchy.go b/punchy.go index 4bce4392..67876269 100644 --- a/punchy.go +++ b/punchy.go @@ -67,7 +67,8 @@ func NewPunchyFromConfig(l *slog.Logger, c *config.C, punchConn udp.Conn) *Punch } p.reload(c, true) - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("stats.lighthouse_metrics") p.reload(c, false) }) diff --git a/punchy_test.go b/punchy_test.go index e56f3eff..25f8de79 100644 --- a/punchy_test.go +++ b/punchy_test.go @@ -76,7 +76,7 @@ punchy: punch: true delay: 10m respond: true -`)) +`).Err) p.reload(c, false) assert.True(t, p.punch.Load()) assert.Equal(t, newDelay, time.Duration(p.delay.Load())) @@ -170,7 +170,7 @@ func TestPunchy_LogFormat_ReloadPunch(t *testing.T) { NewPunchyFromConfig(l, c, nil) hook.entries = nil - require.NoError(t, c.ReloadConfigString(`punchy: {punch: true}`)) + require.NoError(t, c.ReloadConfigString(`punchy: {punch: true}`).Err) entry := findEntry(t, hook.entries, "punchy.punch changed") assert.Equal(t, slog.LevelInfo, entry.Level) @@ -184,7 +184,7 @@ func TestPunchy_LogFormat_ReloadRespond(t *testing.T) { NewPunchyFromConfig(l, c, nil) hook.entries = nil - require.NoError(t, c.ReloadConfigString(`punchy: {respond: true}`)) + require.NoError(t, c.ReloadConfigString(`punchy: {respond: true}`).Err) entry := findEntry(t, hook.entries, "punchy.respond changed") assert.Equal(t, slog.LevelInfo, entry.Level) @@ -198,7 +198,7 @@ func TestPunchy_LogFormat_ReloadDelay(t *testing.T) { NewPunchyFromConfig(l, c, nil) hook.entries = nil - require.NoError(t, c.ReloadConfigString(`punchy: {delay: 10s}`)) + require.NoError(t, c.ReloadConfigString(`punchy: {delay: 10s}`).Err) entry := findEntry(t, hook.entries, "punchy.delay changed") assert.Equal(t, slog.LevelInfo, entry.Level) @@ -212,7 +212,7 @@ func TestPunchy_LogFormat_ReloadTargetAllRemotes(t *testing.T) { NewPunchyFromConfig(l, c, nil) hook.entries = nil - require.NoError(t, c.ReloadConfigString(`punchy: {target_all_remotes: true}`)) + require.NoError(t, c.ReloadConfigString(`punchy: {target_all_remotes: true}`).Err) entry := findEntry(t, hook.entries, "punchy.target_all_remotes changed") assert.Equal(t, slog.LevelInfo, entry.Level) @@ -226,9 +226,19 @@ func TestPunchy_LogFormat_ReloadRespondDelay(t *testing.T) { NewPunchyFromConfig(l, c, nil) hook.entries = nil - require.NoError(t, c.ReloadConfigString(`punchy: {respond_delay: 15s}`)) + require.NoError(t, c.ReloadConfigString(`punchy: {respond_delay: 15s}`).Err) entry := findEntry(t, hook.entries, "punchy.respond_delay changed") assert.Equal(t, slog.LevelInfo, entry.Level) assert.Equal(t, map[string]any{"respond_delay": 15 * time.Second}, entry.Attrs) } + +func TestPunchyReloadRestartRequired(t *testing.T) { + c := config.NewC(test.NewLogger()) + require.NoError(t, c.LoadString("stats:\n lighthouse_metrics: false\n")) + NewPunchyFromConfig(test.NewLogger(), c, nil) + + res := c.ReloadConfigString("stats:\n lighthouse_metrics: true\n") + require.NoError(t, res.Err) + assert.True(t, res.RestartRequired) +} diff --git a/relay_manager.go b/relay_manager.go index 46d7a2bb..0c8f67eb 100644 --- a/relay_manager.go +++ b/relay_manager.go @@ -28,10 +28,11 @@ func NewRelayManager(ctx context.Context, l *slog.Logger, hostmap *HostMap, c *c hostmap: hostmap, } rm.reload(c, true) - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { err := rm.reload(c, false) if err != nil { rm.l.Error("Failed to reload relay_manager", "error", err) + r.Failed() } }) return rm diff --git a/ssh.go b/ssh.go index 34b5c3a7..e59cc858 100644 --- a/ssh.go +++ b/ssh.go @@ -58,7 +58,9 @@ type sshDeviceInfoFlags struct { } func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) { - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + // sshd.sandbox_dir is read once, when the commands are attached + r.RestartRequiredIfChanged("sshd.sandbox_dir") if c.GetBool("sshd.enabled", false) { // Restarting the server drops every open session, the one that asked for this reload included. Only restart // when it isn't running or its config changed. The host key file can change under the same config, new @@ -71,6 +73,7 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) { if err != nil { l.Error("Failed to reconfigure the sshd", "error", err) ssh.Stop() + r.Failed() } return } @@ -78,6 +81,7 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) { if err != nil { l.Error("Failed to reconfigure the sshd", "error", err) ssh.Stop() + r.Failed() } if sshRun != nil { go sshRun() @@ -276,7 +280,7 @@ func attachCommands(l *slog.Logger, c *config.C, ssh *sshd.SSHServer, f *Interfa Name: "reload", ShortDescription: "Reloads configuration from disk, same as sending HUP to the process", Callback: func(fs any, a []string, w sshd.StringWriter) error { - return sshReload(c, w) + return sshReload(l, c, w) }, }) @@ -1075,8 +1079,13 @@ func sshDeviceInfo(ifce *Interface, fs any, w sshd.StringWriter) error { } } -func sshReload(c *config.C, w sshd.StringWriter) error { - err := w.WriteLine("Reloading config") - c.ReloadConfig() - return err +// sshReload reloads the config and writes the result to the client. It reloads even if the client has gone away +func sshReload(l *slog.Logger, c *config.C, w sshd.StringWriter) error { + writeErr := w.WriteLine("Reloading config") + res := c.ReloadConfig() + res.Log(l) + if err := w.WriteLine(res.String()); writeErr == nil { + writeErr = err + } + return writeErr } diff --git a/ssh_reload_test.go b/ssh_reload_test.go new file mode 100644 index 00000000..2727d1c7 --- /dev/null +++ b/ssh_reload_test.go @@ -0,0 +1,83 @@ +package nebula + +import ( + "errors" + "io" + "os" + "path/filepath" + "testing" + + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type lineWriter struct { + lines []string + err error +} + +func (w *lineWriter) WriteLine(s string) error { + w.lines = append(w.lines, s) + return w.err +} +func (w *lineWriter) Write(s string) error { return w.WriteLine(s) } +func (w *lineWriter) WriteBytes(b []byte) error { return w.WriteLine(string(b)) } +func (w *lineWriter) GetWriter() io.Writer { return io.Discard } + +func TestSSHReloadReportsTheResult(t *testing.T) { + dir := t.TempDir() + write := func(body string) { + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte(body), 0o600)) + } + write("a: 1\nb: 1") + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.Load(dir)) + fail := false + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("b") + if fail { + r.Failed() + } + }) + + reload := func() []string { + w := &lineWriter{} + require.NoError(t, sshReload(l, c, w)) + return w.lines + } + + write("a: 2\nb: 1") + assert.Equal(t, []string{"Reloading config", "Config reload complete"}, reload()) + + fail = true + write("a: 3\nb: 1") + assert.Equal(t, []string{"Reloading config", "Config reload completed with errors, review the logs"}, reload()) + + fail = false + write("a: 3\nb: 2") + assert.Equal(t, []string{"Reloading config", "Config reload was completed but requires a restart to fully apply, keys [b]"}, reload()) + + write("a: [") + lines := reload() + require.Len(t, lines, 2) + assert.Contains(t, lines[1], "Config reload failed, the new config did not load") + assert.Contains(t, lines[1], dir, "names the config that did not load") +} + +// A client that went away still gets the reload it asked for +func TestSSHReloadReloadsWhenTheClientIsGone(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yml"), []byte("a: 1"), 0o600)) + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.Load(dir)) + reloaded := false + c.RegisterReloadCallback(func(*config.C, *config.Reload) { reloaded = true }) + + gone := errors.New("client is gone") + require.ErrorIs(t, sshReload(l, c, &lineWriter{err: gone}), gone) + assert.True(t, reloaded) +} diff --git a/sshd_reload_test.go b/sshd_reload_test.go index 8226fd09..0e81cece 100644 --- a/sshd_reload_test.go +++ b/sshd_reload_test.go @@ -130,7 +130,7 @@ func TestSSHDReload_UnchangedKeepsSessions(t *testing.T) { h := newSSHDReloadHarness(t) client, _ := h.connect(t) - require.NoError(t, h.c.ReloadConfigString(h.config()+"punchy:\n punch: true\n")) + require.NoError(t, h.c.ReloadConfigString(h.config()+"punchy:\n punch: true\n").Err) assert.False(t, dropped(client, 500*time.Millisecond), "an unchanged sshd was restarted") } @@ -139,7 +139,7 @@ func TestSSHDReload_ChangedConfigRestarts(t *testing.T) { client, _ := h.connect(t) h.extra = " trusted_cas: []\n" - require.NoError(t, h.c.ReloadConfigString(h.config())) + require.NoError(t, h.c.ReloadConfigString(h.config()).Err) assert.True(t, dropped(client, 5*time.Second), "a changed sshd config was not applied") require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond) h.connect(t) @@ -152,7 +152,7 @@ func TestSSHDReload_ChangedHostKeyFile(t *testing.T) { client, _ := h.connect(t) want := h.writeHostKey(t, newEd25519Key(t)) - require.NoError(t, h.c.ReloadConfigString(h.config())) + require.NoError(t, h.c.ReloadConfigString(h.config()).Err) _, got := h.connect(t) assert.Equal(t, want.Marshal(), got.Marshal(), "a new host key was not applied") assert.False(t, dropped(client, 500*time.Millisecond), "a new host key restarted sshd") @@ -166,7 +166,7 @@ func TestSSHDReload_HostKeyAlgorithmChange(t *testing.T) { rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) require.NoError(t, err) want := h.writeHostKey(t, rsaKey) - require.NoError(t, h.c.ReloadConfigString(h.config())) + require.NoError(t, h.c.ReloadConfigString(h.config()).Err) _, got := h.connect(t) assert.Equal(t, want.Marshal(), got.Marshal()) @@ -180,7 +180,7 @@ func TestSSHDReload_NotRunningRestarts(t *testing.T) { h.server.Stop() require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond) - require.NoError(t, h.c.ReloadConfigString(h.config())) + require.NoError(t, h.c.ReloadConfigString(h.config()).Err) require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond) h.connect(t) } @@ -191,6 +191,6 @@ func TestSSHDReload_BadHostKeyFileStops(t *testing.T) { h.connect(t) require.NoError(t, os.WriteFile(h.keyFile, []byte("not a key"), 0o600)) - require.NoError(t, h.c.ReloadConfigString(h.config())) + assert.True(t, h.c.ReloadConfigString(h.config()).Failed) require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond) } diff --git a/stats.go b/stats.go index 7e42d44b..9b957650 100644 --- a/stats.go +++ b/stats.go @@ -99,9 +99,10 @@ func newStatsServerFromConfig(ctx context.Context, l *slog.Logger, c *config.C, emitStats: emitStats, } - c.RegisterReloadCallback(func(c *config.C) { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { if err := s.reload(c, false); err != nil { s.l.Error("Failed to reload stats from config", "error", err) + r.Failed() } }) diff --git a/udp/conn.go b/udp/conn.go index 6d20cbbf..62806569 100644 --- a/udp/conn.go +++ b/udp/conn.go @@ -43,7 +43,8 @@ type Conn interface { // its own packet, so a short count means some peers were undeliverable, not that the batch failed. // Not safe for concurrent use on the same Conn. WriteBatch(bufs [][]byte, addrs []netip.AddrPort) (int, error) - ReloadConfig(c *config.C) + // ReloadConfig applies the listen settings that can change, r is nil at startup + ReloadConfig(c *config.C, r *config.Reload) SupportsMultipleReaders() bool Close() error } @@ -68,7 +69,7 @@ func (NoopConn) WriteTo(_ []byte, _ netip.AddrPort) error { func (NoopConn) WriteBatch(bufs [][]byte, _ []netip.AddrPort) (int, error) { return len(bufs), nil } -func (NoopConn) ReloadConfig(_ *config.C) { +func (NoopConn) ReloadConfig(_ *config.C, _ *config.Reload) { return } func (NoopConn) Close() error { diff --git a/udp/netchange.go b/udp/netchange.go index e7b21e6b..86956212 100644 --- a/udp/netchange.go +++ b/udp/netchange.go @@ -24,6 +24,12 @@ type NetworkChangeMonitor struct { // NewNetworkChangeMonitor builds a monitor for local network changes. The returned monitor is always usable: Start // is safe to call unconditionally, it no-ops when disabled or on a platform that does not need it. func NewNetworkChangeMonitor(ctx context.Context, l *slog.Logger, c *config.C) *NetworkChangeMonitor { + if watchesNetworkChanges { + c.RegisterReloadCallback(func(c *config.C, r *config.Reload) { + r.RestartRequiredIfChanged("listen.rebind_on_network_change") + }) + } + return &NetworkChangeMonitor{ l: l, ctx: ctx, diff --git a/udp/netchange_darwin.go b/udp/netchange_darwin.go index ac67ea79..e4bc6144 100644 --- a/udp/netchange_darwin.go +++ b/udp/netchange_darwin.go @@ -13,6 +13,9 @@ import ( "golang.org/x/sys/unix" ) +// watchesNetworkChanges is true where Start does anything +const watchesNetworkChanges = true + const ( // netChangeSettleWindow is how long we keep swallowing routing messages after the first interesting one. A // single network change is never a single message, it is a burst: the link drops, addresses go away, new ones diff --git a/udp/netchange_generic.go b/udp/netchange_generic.go index 00cc9214..a210137f 100644 --- a/udp/netchange_generic.go +++ b/udp/netchange_generic.go @@ -7,6 +7,9 @@ import ( "log/slog" ) +// watchesNetworkChanges is true where Start does anything +const watchesNetworkChanges = false + // watchNetworkChanges is a no-op outside of darwin. // // Darwin is the platform that scopes a udp socket to the interface it came up on, so it is the platform whose socket diff --git a/udp/netchange_test.go b/udp/netchange_test.go index fe57a2e2..d91c6242 100644 --- a/udp/netchange_test.go +++ b/udp/netchange_test.go @@ -37,3 +37,19 @@ func TestNetworkChangeMonitorNilRebindIsANoOp(t *testing.T) { m := newMonitor(t, context.Background(), "listen:\n rebind_on_network_change: true\n") m.Start(nil) } + +// The monitor only reads listen.rebind_on_network_change when it is made +func TestNetworkChangeMonitorRestartKey(t *testing.T) { + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.LoadString("listen:\n rebind_on_network_change: true\n")) + NewNetworkChangeMonitor(context.Background(), l, c) + + res := c.ReloadConfigString("listen:\n rebind_on_network_change: false\n") + require.NoError(t, res.Err) + if watchesNetworkChanges { + assert.Equal(t, []string{"listen.rebind_on_network_change"}, res.RestartKeys) + } else { + assert.Empty(t, res.RestartKeys, "the setting does nothing here") + } +} diff --git a/udp/udp_bypass_windows.go b/udp/udp_bypass_windows.go index b8b06b1e..bf261223 100644 --- a/udp/udp_bypass_windows.go +++ b/udp/udp_bypass_windows.go @@ -26,7 +26,9 @@ type bypassConn struct { session *wfp.Session } -func (b *bypassConn) ReloadConfig(c *config.C) { +func (b *bypassConn) ReloadConfig(c *config.C, r *config.Reload) { + // Only read on the first call, at startup + r.RestartRequiredIfChanged("listen.windows_bypass_wdf") b.installOnce.Do(func() { if !c.GetBool("listen.windows_bypass_wdf", true) { return @@ -45,7 +47,7 @@ func (b *bypassConn) ReloadConfig(c *config.C) { "port", addr.Port()) b.session = s }) - b.Conn.ReloadConfig(c) + b.Conn.ReloadConfig(c, r) } func (b *bypassConn) Close() error { diff --git a/udp/udp_darwin.go b/udp/udp_darwin.go index 2bb0cf0d..516e5d7b 100644 --- a/udp/udp_darwin.go +++ b/udp/udp_darwin.go @@ -171,7 +171,7 @@ func (u *StdConn) LocalAddr() (netip.AddrPort, error) { } } -func (u *StdConn) ReloadConfig(c *config.C) { +func (u *StdConn) ReloadConfig(c *config.C, r *config.Reload) { // TODO } diff --git a/udp/udp_generic.go b/udp/udp_generic.go index 0ba1d412..54f00ad2 100644 --- a/udp/udp_generic.go +++ b/udp/udp_generic.go @@ -73,7 +73,7 @@ func (u *GenericConn) LocalAddr() (netip.AddrPort, error) { } } -func (u *GenericConn) ReloadConfig(c *config.C) { +func (u *GenericConn) ReloadConfig(c *config.C, r *config.Reload) { } diff --git a/udp/udp_linux.go b/udp/udp_linux.go index cc834131..f9987341 100644 --- a/udp/udp_linux.go +++ b/udp/udp_linux.go @@ -392,7 +392,7 @@ func writeSockaddr(buf []byte, addr netip.AddrPort, isV4 bool) (int, error) { return unix.SizeofSockaddrInet6, nil } -func (u *StdConn) ReloadConfig(c *config.C) { +func (u *StdConn) ReloadConfig(c *config.C, r *config.Reload) { b := c.GetInt("listen.read_buffer", 0) if b > 0 { if err := u.SetRecvBuffer(b); err == nil { @@ -403,6 +403,7 @@ func (u *StdConn) ReloadConfig(c *config.C) { } } else { u.l.Error("Failed to set listen.read_buffer", "error", err) + r.Failed() } } @@ -416,6 +417,7 @@ func (u *StdConn) ReloadConfig(c *config.C) { } } else { u.l.Error("Failed to set listen.write_buffer", "error", err) + r.Failed() } } @@ -430,6 +432,7 @@ func (u *StdConn) ReloadConfig(c *config.C) { } } else { u.l.Error("Failed to set listen.so_mark", "error", err) + r.Failed() } } } diff --git a/udp/udp_linux_reload_test.go b/udp/udp_linux_reload_test.go new file mode 100644 index 00000000..28caefa2 --- /dev/null +++ b/udp/udp_linux_reload_test.go @@ -0,0 +1,31 @@ +//go:build linux && !android && !e2e_testing + +package udp + +import ( + "net/netip" + "os" + "testing" + + "github.com/slackhq/nebula/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A setting the kernel refuses fails the reload +func TestReloadConfig_RefusedSettingFailsTheReload(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root may set listen.so_mark, run as an unprivileged user") + } + conn, err := NewListener(testLogger(), Settings{Listen: netip.MustParseAddrPort("127.0.0.1:0"), Batch: 1}) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + c := config.NewC(testLogger()) + require.NoError(t, c.LoadString("listen:\n port: 0\n")) + c.RegisterReloadCallback(conn.ReloadConfig) + + res := c.ReloadConfigString("listen:\n port: 0\n so_mark: 42\n") + require.NoError(t, res.Err) + assert.True(t, res.Failed) +} diff --git a/udp/udp_rio_windows.go b/udp/udp_rio_windows.go index 435d71a6..36ae2040 100644 --- a/udp/udp_rio_windows.go +++ b/udp/udp_rio_windows.go @@ -351,7 +351,7 @@ func (u *RIOConn) Rebind() error { return nil } -func (u *RIOConn) ReloadConfig(*config.C) {} +func (u *RIOConn) ReloadConfig(*config.C, *config.Reload) {} func (u *RIOConn) Close() error { if !u.isOpen.CompareAndSwap(true, false) { diff --git a/udp/udp_tester.go b/udp/udp_tester.go index db90b370..292111ab 100644 --- a/udp/udp_tester.go +++ b/udp/udp_tester.go @@ -197,7 +197,7 @@ func (u *TesterConn) ListenOut(r EncReader, flush func()) error { } } -func (u *TesterConn) ReloadConfig(*config.C) {} +func (u *TesterConn) ReloadConfig(*config.C, *config.Reload) {} func NewUDPStatsEmitter(_ []Conn) func() { // No UDP stats for non-linux