diff --git a/client/mdm/ticker.go b/client/mdm/ticker.go index ad5b83fe4..88356ad74 100644 --- a/client/mdm/ticker.go +++ b/client/mdm/ticker.go @@ -27,7 +27,7 @@ var policyLoader = LoadPolicy // goroutine; cancel the supplied context to stop. type Ticker struct { interval time.Duration - onChange func(prev, curr *Policy) + onChange func(prev, curr *Policy) error prev *Policy } @@ -39,7 +39,7 @@ type Ticker struct { // onChange when the policy actually changed since boot — without // this baseline the first tick would report every currently-managed // key as "added" and trigger a spurious engine restart. -func NewTicker(reloadInterval time.Duration, onChange func(prev, curr *Policy)) *Ticker { +func NewTicker(reloadInterval time.Duration, onChange func(prev, curr *Policy) error) *Ticker { return &Ticker{ interval: reloadInterval, onChange: onChange, @@ -68,9 +68,13 @@ func (t *Ticker) Run(ctx context.Context) { log.Infof("MDM policy changed: added=%v removed=%v changed=%v", added, removed, changed) prev := t.prev - t.prev = curr + if t.onChange != nil { - t.onChange(prev, curr) + if err := t.onChange(prev, curr); err != nil { + log.Errorf("MDM policy change handler failed (retrying in 1 minute): %v", err) + continue + } + t.prev = curr } } } diff --git a/client/mdm/ticker_test.go b/client/mdm/ticker_test.go index a55acdb49..098688e44 100644 --- a/client/mdm/ticker_test.go +++ b/client/mdm/ticker_test.go @@ -34,11 +34,12 @@ func TestTicker_FiresOnChangeWithDelta(t *testing.T) { type change struct{ prev, curr *Policy } changes := make(chan change, 1) - tk := NewTicker(testReloadInterval, func(prev, curr *Policy) { + tk := NewTicker(testReloadInterval, func(prev, curr *Policy) error { select { case changes <- change{prev, curr}: default: } + return nil }) require.Equal(t, testReloadInterval, tk.interval) @@ -70,11 +71,12 @@ func TestTicker_NoCallbackWhenPolicyUnchanged(t *testing.T) { }) fired := make(chan struct{}, 1) - tk := NewTicker(testReloadInterval, func(_, _ *Policy) { + tk := NewTicker(testReloadInterval, func(_, _ *Policy) error { select { case fired <- struct{}{}: default: } + return nil }) ctx, cancel := context.WithCancel(context.Background()) diff --git a/client/server/mdm.go b/client/server/mdm.go index 98cdba01d..d2d686a39 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -33,7 +33,7 @@ var loadMDMPolicy = mdm.LoadPolicy // // The callback runs in the ticker's own goroutine. Ticker has already // logged the per-key diff before invoking this hook. -func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { +func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) error { log.Warn("MDM policy changed; restarting engine to apply new configuration") // Hold s.mutex for the entire restart sequence (cancel + quiescence @@ -46,6 +46,10 @@ func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { s.mutex.Lock() defer s.mutex.Unlock() + if !s.clientRunning { + // The client is not running, so there's no engine to restart. + return nil + } if s.actCancel != nil { s.actCancel() } @@ -60,14 +64,14 @@ func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { if s.clientGiveUpChan != nil { select { case <-s.clientGiveUpChan: - case <-time.After(5 * time.Second): - log.Warn("MDM restart: timeout waiting for previous engine goroutine; proceeding anyway") + case <-time.After(10 * time.Second): + return fmt.Errorf("failed to restart the engine due to timeout") } } if err := s.restartEngineForMDMLocked(); err != nil { log.Errorf("MDM restart failed: %v", err) - return + return err } // publishConfigChangedEvent has already fired inside @@ -82,6 +86,7 @@ func (s *Server) onMDMPolicyChange(_, curr *mdm.Policy) { "NetBird configuration was updated by your IT policy.", map[string]string{"source": "mdm", "type": "policy_applied"}, ) + return nil } // publishConfigChangedEvent broadcasts a SystemEvent informing any active