Remove policy change action acks

Author: michel-laterman

internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go

@@ -111,7 +111,7 @@ func (h *PolicyChangeHandler) Handle(ctx context.Context, a fleetapi.Action, ack
 		return err
 	}
 
-	h.ch <- newPolicyChange(ctx, c, a, acker, false)
+	h.ch <- newPolicyChange(c, false)
 	return nil
 }
 
@@ -469,31 +469,20 @@ func fleetToReader(agentID string, headers map[string]string, cfg *configuration
 }
 
 type policyChange struct {
-	ctx        context.Context
 	cfg        *config.Config
-	action     fleetapi.Action
-	acker      acker.Acker
-	commit     bool
 	ackWatcher chan struct{}
 }
 
 func newPolicyChange(
-	ctx context.Context,
 	config *config.Config,
-	action fleetapi.Action,
-	acker acker.Acker,
-	commit bool) *policyChange {
+	createCh bool) *policyChange {
 	var ackWatcher chan struct{}
-	if commit {
+	if createCh {
 		// we don't need it otherwise
 		ackWatcher = make(chan struct{})
 	}
 	return &policyChange{
-		ctx:        ctx,
 		cfg:        config,
-		action:     action,
-		acker:      acker,
-		commit:     true,
 		ackWatcher: ackWatcher,
 	}
 }
@@ -502,30 +491,19 @@ func (l *policyChange) Config() *config.Config {
 	return l.cfg
 }
 
+// Ack will close the channel that WaitAck uses but is otherwise a nop.
 func (l *policyChange) Ack() error {
-	if l.action == nil {
-		return nil
-	}
-	err := l.acker.Ack(l.ctx, l.action)
-	if err != nil {
-		return err
-	}
-	if l.commit {
-		err := l.acker.Commit(l.ctx)
-		if l.ackWatcher != nil && err == nil {
-			close(l.ackWatcher)
-		}
-		return err
+	if l.ackWatcher != nil {
+		close(l.ackWatcher)
 	}
 	return nil
 }
 
 // WaitAck waits for policy change to be acked.
-// Policy change ack is awaitable only in case commit flag was set.
 // Caller is responsible to use any reasonable deadline otherwise
 // function call can be endlessly blocking.
 func (l *policyChange) WaitAck(ctx context.Context) {
-	if !l.commit || l.ackWatcher == nil {
+	if l.ackWatcher == nil {
 		return
 	}
 

internal/pkg/agent/application/actions/handlers/handler_action_policy_change_test.go

@@ -105,7 +105,7 @@ func TestPolicyAcked(t *testing.T) {
 	agentInfo := &info.AgentInfo{}
 	nullStore := &storage.NullStore{}
 
-	t.Run("Config change should ACK", func(t *testing.T) {
+	t.Run("Config change shouldn't ACK", func(t *testing.T) {
 		ch := make(chan coordinator.ConfigChange, 1)
 		tacker := &testAcker{}
 
@@ -129,8 +129,7 @@ func TestPolicyAcked(t *testing.T) {
 		require.NoError(t, change.Ack())
 
 		actions := tacker.Items()
-		assert.EqualValues(t, 1, len(actions))
-		assert.Equal(t, actionID, actions[0])
+		assert.Empty(t, actions)
 	})
 }
 

internal/pkg/agent/application/actions/handlers/handler_action_unenroll.go

@@ -93,7 +93,7 @@ func (h *Unenroll) handle(ctx context.Context, a fleetapi.Action, acker acker.Ac
 	}
 
 	// Generate empty policy change, this removing all the running components
-	unenrollPolicy := newPolicyChange(ctx, config.New(), a, acker, true)
+	unenrollPolicy := newPolicyChange(config.New(), true)
 	h.ch <- unenrollPolicy
 
 	// backup action for future start to avoid starting fleet gateway loop

internal/pkg/agent/application/actions/handlers/handler_action_unenroll_test.go

@@ -226,7 +226,7 @@ func TestActionUnenrollHandler(t *testing.T) {
 
 			// autoUnenroll isn't acked
 			if tc.wantErr == nil && !tc.autoUnenroll {
-				require.Len(t, acker.Acked, 1)
+				require.Empty(t, acker.Acked)
 			}
 			require.Equal(t, tc.wantPerformedActions, coord.performedActions.Load())
 		})

internal/pkg/agent/application/coordinator/coordinator.go

@@ -1463,23 +1463,7 @@ func (c *Coordinator) runLoopIteration(ctx context.Context) {
 			c.logger.Errorf("applying new policy: %s", err.Error())
 			change.Fail(err)
 		} else {
-			if err := change.Ack(); err != nil {
-				err = fmt.Errorf("failed to ack configuration change: %w", err)
-				// Workaround: setConfigManagerError is usually used by the config
-				// manager to report failed ACKs / etc when communicating with Fleet.
-				// We need to report a failed ACK here, but the policy change has
-				// already been successfully applied so we don't want to report it as
-				// a general Coordinator or policy failure.
-				// This arises uniquely here because this is the only case where an
-				// action is responsible for reporting the failure of its own ACK
-				// call. The "correct" fix is to make this Ack() call unfailable
-				// and handle ACK retries and reporting in the config manager like
-				// with other action types -- this error would then end up invoking
-				// setConfigManagerError "organically" via the config manager's
-				// reporting channel. In the meantime, we do it manually.
-				c.setConfigManagerError(err)
-				c.logger.Errorf("%s", err.Error())
-			}
+			_ = change.Ack() // will not return errors
 		}
 
 	case vars := <-c.managerChans.varsManagerUpdate: