Fail the reload when a config entry is skipped with a warning (#1952)

hostmap.go

@@ -316,9 +316,9 @@ type cachedPacketMetrics struct {
 func NewHostMapFromConfig(l *slog.Logger, c *config.C) *HostMap {
 	hm := newHostMap(l)
 
-	hm.reload(c, true)
-	c.RegisterReloadCallback(func(c *config.C, _ *config.Reload) {
-		hm.reload(c, false)
+	hm.reload(c, true, nil)
+	c.RegisterReloadCallback(func(c *config.C, r *config.Reload) {
+		hm.reload(c, false, r)
 	})
 
 	l.Info("Main HostMap created", "preferredRanges", hm.GetPreferredRanges())
@@ -337,7 +337,8 @@ func newHostMap(l *slog.Logger) *HostMap {
 	}
 }
 
-func (hm *HostMap) reload(c *config.C, initial bool) {
+// r is nil on the initial load
+func (hm *HostMap) reload(c *config.C, initial bool, r *config.Reload) {
 	if initial || c.HasChanged("preferred_ranges") {
 		var preferredRanges []netip.Prefix
 		rawPreferredRanges := c.GetStringSlice("preferred_ranges", []string{})
@@ -348,8 +349,9 @@ func (hm *HostMap) reload(c *config.C, initial bool) {
 			if err != nil {
 				hm.l.Warn("Failed to parse preferred ranges, ignoring",
 					"error", err,
-					"range", rawPreferredRanges,
+					"range", rawPreferredRange,
 				)
+				r.Failed()
 				continue
 			}
 

hostmap_test.go

@@ -447,3 +447,16 @@ func TestHostInfo_markOutOnly(t *testing.T) {
 	assert.True(t, h.markOut(6), "a relayed send must not consume the epoch edge")
 	assert.False(t, h.markOut(6))
 }
+
+// A preferred range that doesn't parse is skipped and the rest are used, the reload says one was skipped
+func TestHostMap_reloadBadPreferredRangeFails(t *testing.T) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+	require.NoError(t, c.LoadString("preferred_ranges: [10.0.0.0/8]\n"))
+	hm := NewHostMapFromConfig(l, c)
+
+	res := c.ReloadConfigString("preferred_ranges: [10.0.0.0/8, nope]\n")
+	require.NoError(t, res.Err)
+	assert.True(t, res.Failed)
+	assert.Equal(t, []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, hm.GetPreferredRanges())
+}

lighthouse.go

@@ -138,14 +138,14 @@ func NewLightHouseFromConfig(ctx context.Context, l *slog.Logger, c *config.C, c
 		h.metrics = newLighthouseMetrics()
 	}
 
-	err := h.reload(c, true)
+	err := h.reload(c, true, nil)
 	if err != nil {
 		return nil, err
 	}
 
 	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)
+		err := h.reload(c, false, r)
 		switch v := err.(type) {
 		case *util.ContextualError:
 			v.Log(l)
@@ -193,7 +193,8 @@ func (lh *LightHouse) GetUpdateInterval() int64 {
 	return lh.interval.Load()
 }
 
-func (lh *LightHouse) reload(c *config.C, initial bool) error {
+// r is nil on the initial load
+func (lh *LightHouse) reload(c *config.C, initial bool, r *config.Reload) error {
 	if initial || c.HasChanged("lighthouse.advertise_addrs") {
 		rawAdvAddrs := c.GetStringSlice("lighthouse.advertise_addrs", []string{})
 		advAddrs := make([]netip.AddrPort, 0)
@@ -228,6 +229,7 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 					"addr", rawAddr,
 					"entry", i+1,
 				)
+				r.Failed()
 				continue
 			}
 
@@ -386,6 +388,7 @@ func (lh *LightHouse) reload(c *config.C, initial bool) error {
 						"relay", v,
 						"error", err,
 					)
+					r.Failed()
 				} else {
 					lh.l.Info("Read relay from config", "relay", v)
 					relaysForMe = append(relaysForMe, configRIP)

lighthouse_test.go

@@ -296,7 +296,7 @@ func TestLighthouse_reload(t *testing.T) {
 	require.NoError(t, err)
 	c.ReloadConfigString(string(rc))
 
-	err = lh.reload(c, false)
+	err = lh.reload(c, false, nil)
 	require.NoError(t, err)
 }
 
@@ -1014,3 +1014,31 @@ func TestLighthouse_reloadRestartRequired(t *testing.T) {
 	require.NoError(t, res.Err)
 	assert.True(t, res.RestartRequired)
 }
+
+// A relay that doesn't parse is skipped and the rest are used, the reload says one was skipped
+func TestLighthouse_reloadBadRelayFails(t *testing.T) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+	require.NoError(t, c.LoadString("listen:\n  port: 4242\n"))
+	lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(netip.MustParsePrefix("10.128.0.1/24")), nil, nil)
+	require.NoError(t, err)
+
+	res := c.ReloadConfigString("listen:\n  port: 4242\nrelay:\n  relays: [10.128.0.2, nope]\n")
+	require.NoError(t, res.Err)
+	assert.True(t, res.Failed)
+	assert.Equal(t, []netip.Addr{netip.MustParseAddr("10.128.0.2")}, lh.GetRelaysForMe())
+}
+
+// An advertise_addrs entry inside our own network is skipped and the rest are used, the reload says one was skipped
+func TestLighthouse_reloadAdvertiseAddrInNetworkFails(t *testing.T) {
+	l := test.NewLogger()
+	c := config.NewC(l)
+	require.NoError(t, c.LoadString("listen:\n  port: 4242\n"))
+	lh, err := NewLightHouseFromConfig(t.Context(), l, c, testCertState(netip.MustParsePrefix("10.128.0.1/24")), nil, nil)
+	require.NoError(t, err)
+
+	res := c.ReloadConfigString("listen:\n  port: 4242\nlighthouse:\n  advertise_addrs: [\"1.2.3.4:4242\", \"10.128.0.5:4242\"]\n")
+	require.NoError(t, res.Err)
+	assert.True(t, res.Failed)
+	assert.Equal(t, []netip.AddrPort{netip.MustParseAddrPort("1.2.3.4:4242")}, lh.GetAdvertiseAddrs())
+}

main.go

@@ -78,7 +78,7 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev
 	wireSSHReload(l, ssh, c)
 	var sshStart func()
 	if c.GetBool("sshd.enabled", false) {
-		sshStart, err = configSSH(l, ssh, c)
+		sshStart, err = configSSH(l, ssh, c, nil)
 		if err != nil {
 			l.Warn("Failed to configure sshd, ssh debugging will not be available", "error", err)
 			sshStart = nil

ssh.go

@@ -78,7 +78,7 @@ func wireSSHReload(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) {
 				}
 				return
 			}
-			sshRun, err := configSSH(l, ssh, c)
+			sshRun, err := configSSH(l, ssh, c, r)
 			if err != nil {
 				l.Error("Failed to reconfigure the sshd", "error", err)
 				ssh.Stop()
@@ -117,7 +117,8 @@ func loadSSHHostKey(c *config.C) ([]byte, error) {
 // updates the passed-in SSHServer. On success, it returns a function
 // that callers may invoke to run the configured ssh server. On
 // failure, it returns nil, error.
-func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error) {
+// r is nil at startup, entries that are skipped with a warning fail it on a reload
+func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C, r *config.Reload) (func(), error) {
 	listen := c.GetString("sshd.listen", "")
 	if listen == "" {
 		return nil, fmt.Errorf("sshd.listen must be provided")
@@ -150,6 +151,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 		err := ssh.AddTrustedCA(caAuthorizedKey)
 		if err != nil {
 			l.Warn("SSH CA had an error, ignoring", "error", err, "sshCA", caAuthorizedKey)
+			r.Failed()
 			continue
 		}
 	}
@@ -161,12 +163,14 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 			kDef, ok := rk.(map[string]any)
 			if !ok {
 				l.Warn("Authorized user had an error, ignoring", "sshKeyConfig", rk)
+				r.Failed()
 				continue
 			}
 
 			user, ok := kDef["user"].(string)
 			if !ok {
 				l.Warn("Authorized user is missing the user field", "sshKeyConfig", rk)
+				r.Failed()
 				continue
 			}
 
@@ -180,6 +184,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 						"sshKeyConfig", rk,
 						"sshKey", v,
 					)
+					r.Failed()
 					continue
 				}
 
@@ -191,6 +196,7 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 							"sshKeyConfig", rk,
 							"sshKey", subK,
 						)
+						r.Failed()
 						continue
 					}
 
@@ -200,12 +206,14 @@ func configSSH(l *slog.Logger, ssh *sshd.SSHServer, c *config.C) (func(), error)
 							"error", err,
 							"sshKeyConfig", sk,
 						)
+						r.Failed()
 						continue
 					}
 				}
 
 			default:
 				l.Warn("Authorized user is missing the keys field or was not understood", "sshKeyConfig", rk)
+				r.Failed()
 			}
 		}
 	} else {

sshd_reload_test.go

@@ -47,7 +47,7 @@ func newSSHDReloadHarness(t *testing.T) *sshdReloadHarness {
 	h.server, err = sshd.NewSSHServer(t.Context(), l)
 	require.NoError(t, err)
 	wireSSHReload(l, h.server, h.c)
-	run, err := configSSH(l, h.server, h.c)
+	run, err := configSSH(l, h.server, h.c, nil)
 	require.NoError(t, err)
 	go run()
 	t.Cleanup(h.server.Stop)
@@ -226,3 +226,27 @@ func TestSSHDReload_BadHostKeyFileStops(t *testing.T) {
 	assert.True(t, h.c.ReloadConfigString(h.config()).Failed)
 	require.Eventually(t, func() bool { return !h.server.Running() }, 5*time.Second, 10*time.Millisecond)
 }
+
+// A trusted CA or authorized user that is skipped with a warning fails the reload, the valid ones are still used
+func TestSSHDReload_SkippedEntriesFail(t *testing.T) {
+	cases := map[string]string{
+		"bad trusted ca":             "  trusted_cas: [nope]\n",
+		"user entry not a map":       "    - nope\n",
+		"user missing":               "    - keys: [nope]\n",
+		"bad key":                    "    - user: other\n      keys: nope\n",
+		"key in a list not a string": "    - user: other\n      keys: [5]\n",
+		"bad key in a list":          "    - user: other\n      keys: [nope]\n",
+		"keys missing":               "    - user: other\n",
+	}
+	for name, extra := range cases {
+		t.Run(name, func(t *testing.T) {
+			h := newSSHDReloadHarness(t)
+			h.extra = extra
+			res := h.c.ReloadConfigString(h.config())
+			require.NoError(t, res.Err)
+			assert.True(t, res.Failed)
+			require.Eventually(t, h.server.Running, 5*time.Second, 10*time.Millisecond)
+			h.connect(t)
+		})
+	}
+}