Don't send POLICY_CHANGE actions retrieved from index to agent.

[michel-laterman]

What is the problem this PR solves?

The fleet-server should not send any policy change actions that are written to the actions index to an agent on checkin.
The current behaviour is that policy change actions are dynamically generated by the fleet-server when it detects that the agent is not running an up to date version of the policy.

How does this PR solve the problem?

The server will remove these actions in the convert method and emit a warning message.
The ack token that is used is not altered in this case.

Checklist

• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

• Closes filter out POLICY_CHANGE actions from .fleet-actions #1773

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-10-17T18:34:33.685+0000

• Duration: 12 min 1 sec

Test stats 🧪

Test	Results
Failed	0
Passed	358
Skipped	1
Total	359

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[juliaElastic]

Code LGTM.
One question on the behavior: how does the agent get notified about he policy change if Fleet Server doesn't deliver the POLICY_CHANGE action?

[michalpristas]

the question Julia asked seems valid. agent dispatches policy change on checkin, if it won't receive it how will it keep ability to respond to policy changes

have you tested it with local agent at least for regressions?

[michalpristas]

testing this today

[michalpristas]

i see it's coming from a different channel

case policy := <-sub.Output():
		actionResp, err := processPolicy(ctx, zlog, ct.bulker, agent.Id, policy)
		if err != nil {
			return errors.Wrap(err, "processPolicy")
		}
		actions = append(actions, *actionResp)
		break LOOP

tested OK

[jsoriano]

Some nitpicking and a question.

[jsoriano]

Question: When are these policy change actions an issue on check in?

Looking at the code, it seems that TypePolicyChange actions are only created on checkin code, in the processPolicy method, so it doesn't seem to be always an issue on checkin.

Here a filter is being added to convertAction, that is used when:

• There are pending actions.
• There are not pending actions, but something is received from the actions dispatcher.

Are these policy change actions an issue on both cases?
Are they generated in some other place too?

If this checkin handler is the only source of these actions, could there be some way to avoid generating them instead of having to filter them later?

[michel-laterman]

The fleet-server creates policy change actions dynamically when it detects a difference in the config an agent is running and what is defined. This occurs in the processRequest method:

fleet-server/internal/pkg/api/handleCheckin.go

Lines 241 to 247 in a516f03

	case policy := <-sub.Output():
	actionResp, err := processPolicy(ctx, zlog, ct.bulker, agent.Id, policy)
	if err != nil {
	return errors.Wrap(err, "processPolicy")
	}
	actions = append(actions, *actionResp)
	break LOOP

We want to consider this as the source of truth.

However the processRequest method also passes actions retrieved from the actions index (pendingActions also retrieves from this index):

fleet-server/internal/pkg/api/handleCheckin.go

Lines 236 to 240 in a516f03

	case acdocs := <-actCh:
	var acs []ActionResp
	acs, ackToken = convertActions(agent.Id, acdocs)
	actions = append(actions, acs...)
	break LOOP

It's possible for a user to insert a policy change action this way. These actions can make it difficult to determine what policy an agent should be running, it is also very easy to insert a malformed action by mistake and cause the agent to get stuck.
Filtering policy change actions received from the index out prevents these issues.

[jsoriano]

Thanks for the explanations!

the actions index

It's possible for a user to insert a policy change action this way.

How can actions be added to the actions index? Or how can users insert policy changes?