From 09fc5558a1304e6de0ec8d03ca09fb7d269b2ffd Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Thu, 6 Oct 2022 14:09:08 -0700 Subject: [PATCH 1/3] Don't send POLICY_CHANGE actions retrieved from index to agent. The fleet-server should not send any policy change actions that are written to the actions index to an agent on checkin. 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. 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. --- CHANGELOG.next.asciidoc | 3 +- internal/pkg/api/handleCheckin.go | 4 ++ internal/pkg/api/handleChecking_test.go | 65 +++++++++++++++++++------ 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 49affcaac..1a0c0cdbc 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -15,6 +15,7 @@ - LoadServerLimits will not overwrite specified limits when loading default/agent number specified values. {issue}1841[1841] {pull}1912[1912] - Use seperate rate limiters for internal and external API listeners. {issue}1859[1859] {pull}1904[1904] - Fix fleet.migration.total log key overlap {pull}1951[1951] +- Remove POLICY_CHANGE actions from list retrieved from actions index before sending actions to agent on Checkin. {issue}1773[1773] {pull}1963[1963] ==== New Features @@ -25,4 +26,4 @@ - Fleet Server now allows setting global labels on APM instrumentation. {pull}1649[1649] - Fleet Server now allows setting transaction sample rate on APM instrumentation {pull}1681[1681] - Log redacted config when config updates. {issue}1626[1626] {pull}1668[1668] -- Storing checkin message in last_checkin_message {pull}1932[1932] \ No newline at end of file +- Storing checkin message in last_checkin_message {pull}1932[1932] diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index 62bd80fd9..c60295a94 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -376,6 +376,10 @@ func convertActions(agentID string, actions []model.Action) ([]ActionResp, strin respList := make([]ActionResp, 0, sz) for _, action := range actions { + if action.Type == "POLICY_CHANGE" { + log.Warn().Str("agent_id", agentID).Str("action_id", action.ActionID).Msg("Removing POLICY_CHANGE action found in index from check in response") + continue + } respList = append(respList, ActionResp{ AgentID: agentID, CreatedAt: action.Timestamp, diff --git a/internal/pkg/api/handleChecking_test.go b/internal/pkg/api/handleChecking_test.go index 151f56055..c68aedcff 100644 --- a/internal/pkg/api/handleChecking_test.go +++ b/internal/pkg/api/handleChecking_test.go @@ -15,25 +15,60 @@ import ( "github.com/stretchr/testify/assert" ) -func TestConvertActionsEmpty(t *testing.T) { - resp, token := convertActions("1234", nil) - assert.Equal(t, resp, []ActionResp{}) - assert.Equal(t, token, "") -} - func TestConvertActions(t *testing.T) { - actions := []model.Action{ - { - ActionID: "1234", + tests := []struct { + name string + actions []model.Action + resp []ActionResp + token string + }{{ + name: "empty actions", + actions: nil, + resp: []ActionResp{}, + token: "", + }, { + name: "single action", + actions: []model.Action{{ActionID: "1234"}}, + resp: []ActionResp{{ + AgentID: "agent-id", + ID: "1234", + Data: json.RawMessage(nil), + }}, + token: "", + }, { + name: "multiple actions", + actions: []model.Action{ + {ActionID: "1234"}, + {ActionID: "5678"}, }, - } - resp, token := convertActions("agent-id", actions) - assert.Equal(t, resp, []ActionResp{ - { + resp: []ActionResp{{ AgentID: "agent-id", ID: "1234", Data: json.RawMessage(nil), + }, { + AgentID: "agent-id", + ID: "5678", + Data: json.RawMessage(nil), + }}, + token: "", + }, { + name: "remove POLICY_CHANGE action", + actions: []model.Action{ + {ActionID: "1234", Type: "POLICY_CHANGE"}, + {ActionID: "5678"}, }, - }) - assert.Equal(t, token, "") + resp: []ActionResp{{ + AgentID: "agent-id", + ID: "5678", + Data: json.RawMessage(nil), + }}, + token: "", + }} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp, token := convertActions("agent-id", tc.actions) + assert.Equal(t, tc.resp, resp) + assert.Equal(t, tc.token, token) + }) + } } From d8712bf28e82adb67a412cbda3ddcefc7fd3c4af Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Mon, 17 Oct 2022 09:39:59 -0700 Subject: [PATCH 2/3] move filtering to its own method --- internal/pkg/api/handleCheckin.go | 22 ++++++++-- internal/pkg/api/handleChecking_test.go | 53 +++++++++++++++++++------ 2 files changed, 59 insertions(+), 16 deletions(-) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index c60295a94..e8930ea26 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -225,6 +225,7 @@ func (ct *CheckinT) processRequest(zlog zerolog.Logger, w http.ResponseWriter, r if err != nil { return err } + pendingActions = filterActions(agent.Id, pendingActions) actions, ackToken = convertActions(agent.Id, pendingActions) if len(actions) == 0 { @@ -235,6 +236,7 @@ func (ct *CheckinT) processRequest(zlog zerolog.Logger, w http.ResponseWriter, r return ctx.Err() case acdocs := <-actCh: var acs []ActionResp + acdocs = filterActions(agent.Id, acdocs) acs, ackToken = convertActions(agent.Id, acdocs) actions = append(actions, acs...) break LOOP @@ -370,16 +372,28 @@ func (ct *CheckinT) fetchAgentPendingActions(ctx context.Context, seqno sqn.SeqN return actions, err } +// filterActions removes the POLICY_CHANGE action from the passed list. +// The source of this list are documents from the fleet actions index. +// The POLICY_CHANGE action that the agent recieves are generated by the fleet-server when it detects a different policy in processRequest() +func filterActions(agentID string, actions []model.Action) []model.Action { + resp := make([]model.Action, 0, len(actions)) + for _, action := range actions { + if action.Type == TypePolicyChange { + log.Info().Str("agent_id", agentID).Str("action_id", action.ActionID).Msg("Removing POLICY_CHANGE action found in index from check in response") + continue + } + resp = append(resp, action) + } + return resp + +} + func convertActions(agentID string, actions []model.Action) ([]ActionResp, string) { var ackToken string sz := len(actions) respList := make([]ActionResp, 0, sz) for _, action := range actions { - if action.Type == "POLICY_CHANGE" { - log.Warn().Str("agent_id", agentID).Str("action_id", action.ActionID).Msg("Removing POLICY_CHANGE action found in index from check in response") - continue - } respList = append(respList, ActionResp{ AgentID: agentID, CreatedAt: action.Timestamp, diff --git a/internal/pkg/api/handleChecking_test.go b/internal/pkg/api/handleChecking_test.go index c68aedcff..7fd65ec28 100644 --- a/internal/pkg/api/handleChecking_test.go +++ b/internal/pkg/api/handleChecking_test.go @@ -51,18 +51,6 @@ func TestConvertActions(t *testing.T) { Data: json.RawMessage(nil), }}, token: "", - }, { - name: "remove POLICY_CHANGE action", - actions: []model.Action{ - {ActionID: "1234", Type: "POLICY_CHANGE"}, - {ActionID: "5678"}, - }, - resp: []ActionResp{{ - AgentID: "agent-id", - ID: "5678", - Data: json.RawMessage(nil), - }}, - token: "", }} for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -72,3 +60,44 @@ func TestConvertActions(t *testing.T) { }) } } + +func TestFilterActions(t *testing.T) { + tests := []struct { + name string + actions []model.Action + results []model.Action + }{{ + name: "empty list", + actions: []model.Action{}, + resp: []model.Action{}, + }, { + name: "nothing filtered", + actions: []model.Action{{ + ActionID: "1234", + }, { + ActionID: "5678", + }}, + resp: []model.Action{{ + ActionID: "1234", + }, { + ActionID: "5678", + }}, + }, { + name: "filter POLICY_CHANGE action", + actions: []model.Action{{ + ActionID: "1234", + Type: TypePolicyChange, + }, { + ActionID: "5678", + }}, + resp: []model.Action{{ + ActionID: "5678", + }}, + }} + for _, tc := range tests { + t.Run(ts.name, func(t *testing.T) { + resp := filterActions("agent-id", tc.actions) + assert.Equal(t, tc.resp, resp) + }) + } +} From 100a4ebb8a3a62edd8c47e2fc593b06e9c0b9f73 Mon Sep 17 00:00:00 2001 From: michel-laterman Date: Mon, 17 Oct 2022 09:49:58 -0700 Subject: [PATCH 3/3] Fix linter, tests, fix file name --- internal/pkg/api/handleCheckin.go | 2 +- .../pkg/api/{handleChecking_test.go => handleCheckin_test.go} | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename internal/pkg/api/{handleChecking_test.go => handleCheckin_test.go} (97%) diff --git a/internal/pkg/api/handleCheckin.go b/internal/pkg/api/handleCheckin.go index e8930ea26..6a8901b72 100644 --- a/internal/pkg/api/handleCheckin.go +++ b/internal/pkg/api/handleCheckin.go @@ -374,7 +374,7 @@ func (ct *CheckinT) fetchAgentPendingActions(ctx context.Context, seqno sqn.SeqN // filterActions removes the POLICY_CHANGE action from the passed list. // The source of this list are documents from the fleet actions index. -// The POLICY_CHANGE action that the agent recieves are generated by the fleet-server when it detects a different policy in processRequest() +// The POLICY_CHANGE action that the agent receives are generated by the fleet-server when it detects a different policy in processRequest() func filterActions(agentID string, actions []model.Action) []model.Action { resp := make([]model.Action, 0, len(actions)) for _, action := range actions { diff --git a/internal/pkg/api/handleChecking_test.go b/internal/pkg/api/handleCheckin_test.go similarity index 97% rename from internal/pkg/api/handleChecking_test.go rename to internal/pkg/api/handleCheckin_test.go index 7fd65ec28..569963cbb 100644 --- a/internal/pkg/api/handleChecking_test.go +++ b/internal/pkg/api/handleCheckin_test.go @@ -65,7 +65,7 @@ func TestFilterActions(t *testing.T) { tests := []struct { name string actions []model.Action - results []model.Action + resp []model.Action }{{ name: "empty list", actions: []model.Action{}, @@ -95,7 +95,7 @@ func TestFilterActions(t *testing.T) { }}, }} for _, tc := range tests { - t.Run(ts.name, func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) { resp := filterActions("agent-id", tc.actions) assert.Equal(t, tc.resp, resp) })