Report how a config reload went: failures, restarts required, and the keys that need one (#1935)

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

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
 	})
 

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
+		}
+	}
+}

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")
+}