From 152df6044de8df61270d74c8ab5a3eaddd3b74f3 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 10:55:59 +0100 Subject: [PATCH 1/5] fixed nil pointer during unenroll --- x-pack/elastic-agent/pkg/agent/application/fleet_acker.go | 8 +++++++- x-pack/elastic-agent/pkg/agent/application/lazy_acker.go | 6 +++++- .../pkg/agent/application/lazy_acker_test.go | 2 +- .../elastic-agent/pkg/agent/application/managed_mode.go | 2 +- x-pack/elastic-agent/pkg/agent/application/state_store.go | 2 +- .../pkg/agent/application/state_store_test.go | 1 - 6 files changed, 15 insertions(+), 6 deletions(-) diff --git a/x-pack/elastic-agent/pkg/agent/application/fleet_acker.go b/x-pack/elastic-agent/pkg/agent/application/fleet_acker.go index 4544fa8a772f..dac05d0c3a06 100644 --- a/x-pack/elastic-agent/pkg/agent/application/fleet_acker.go +++ b/x-pack/elastic-agent/pkg/agent/application/fleet_acker.go @@ -7,6 +7,7 @@ package application import ( "context" "fmt" + "strings" "time" "github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/agent/errors" @@ -58,6 +59,8 @@ func (f *actionAcker) Ack(ctx context.Context, action fleetapi.Action) error { return errors.New(err, fmt.Sprintf("acknowledge action '%s' for elastic-agent '%s' failed", action.ID(), agentID), errors.TypeNetwork) } + f.log.Debugf("action with id '%s' was just acknowledged", action.ID()) + return nil } @@ -65,8 +68,10 @@ func (f *actionAcker) AckBatch(ctx context.Context, actions []fleetapi.Action) e // checkin agentID := f.agentInfo.AgentID() events := make([]fleetapi.AckEvent, 0, len(actions)) + ids := make([]string, 0, len(actions)) for _, action := range actions { events = append(events, constructEvent(action, agentID)) + ids = append(ids, action.ID()) } cmd := fleetapi.NewAckCmd(f.agentInfo, f.client) @@ -74,11 +79,12 @@ func (f *actionAcker) AckBatch(ctx context.Context, actions []fleetapi.Action) e Events: events, } + f.log.Debugf("%d actions with ids '%s' acknowledging", len(ids), strings.Join(ids, ",")) + _, err := cmd.Execute(ctx, req) if err != nil { return errors.New(err, fmt.Sprintf("acknowledge %d actions '%v' for elastic-agent '%s' failed", len(actions), actions, agentID), errors.TypeNetwork) } - return nil } diff --git a/x-pack/elastic-agent/pkg/agent/application/lazy_acker.go b/x-pack/elastic-agent/pkg/agent/application/lazy_acker.go index 58b212ab8e41..4a4004e028fe 100644 --- a/x-pack/elastic-agent/pkg/agent/application/lazy_acker.go +++ b/x-pack/elastic-agent/pkg/agent/application/lazy_acker.go @@ -7,6 +7,7 @@ package application import ( "context" + "github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/core/logger" "github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/fleetapi" ) @@ -19,19 +20,22 @@ type ackForcer interface { } type lazyAcker struct { + log *logger.Logger acker batchAcker queue []fleetapi.Action } -func newLazyAcker(baseAcker batchAcker) *lazyAcker { +func newLazyAcker(baseAcker batchAcker, log *logger.Logger) *lazyAcker { return &lazyAcker{ acker: baseAcker, queue: make([]fleetapi.Action, 0), + log: log, } } func (f *lazyAcker) Ack(ctx context.Context, action fleetapi.Action) error { f.queue = append(f.queue, action) + f.log.Debugf("appending action with id '%s' to the queue", action.ID()) if _, isAckForced := action.(ackForcer); isAckForced { return f.Commit(ctx) diff --git a/x-pack/elastic-agent/pkg/agent/application/lazy_acker_test.go b/x-pack/elastic-agent/pkg/agent/application/lazy_acker_test.go index 24c708c0d91a..b3d872d49460 100644 --- a/x-pack/elastic-agent/pkg/agent/application/lazy_acker_test.go +++ b/x-pack/elastic-agent/pkg/agent/application/lazy_acker_test.go @@ -32,7 +32,7 @@ func TestLazyAcker(t *testing.T) { t.Fatal(err) } - lacker := newLazyAcker(acker) + lacker := newLazyAcker(acker, log) if acker == nil { t.Fatal("acker not initialized") diff --git a/x-pack/elastic-agent/pkg/agent/application/managed_mode.go b/x-pack/elastic-agent/pkg/agent/application/managed_mode.go index 63e8611354db..9ad1f24a3d0d 100644 --- a/x-pack/elastic-agent/pkg/agent/application/managed_mode.go +++ b/x-pack/elastic-agent/pkg/agent/application/managed_mode.go @@ -183,7 +183,7 @@ func newManaged( return nil, err } - batchedAcker := newLazyAcker(acker) + batchedAcker := newLazyAcker(acker, log) // Create the state store that will persist the last good policy change on disk. stateStore, err := newStateStoreWithMigration(log, info.AgentActionStoreFile(), info.AgentStateStoreFile()) diff --git a/x-pack/elastic-agent/pkg/agent/application/state_store.go b/x-pack/elastic-agent/pkg/agent/application/state_store.go index 81d3f901469c..283ab8e480dc 100644 --- a/x-pack/elastic-agent/pkg/agent/application/state_store.go +++ b/x-pack/elastic-agent/pkg/agent/application/state_store.go @@ -225,7 +225,7 @@ func (s *stateStore) Save() error { if apc, ok := s.state.action.(*fleetapi.ActionPolicyChange); ok { serialize.Action = &actionSerializer{apc.ActionID, apc.ActionType, apc.Policy, nil} } else if aun, ok := s.state.action.(*fleetapi.ActionUnenroll); ok { - serialize.Action = &actionSerializer{apc.ActionID, apc.ActionType, nil, &aun.IsDetected} + serialize.Action = &actionSerializer{aun.ActionID, aun.ActionType, nil, &aun.IsDetected} } else { return fmt.Errorf("incompatible type, expected ActionPolicyChange and received %T", s.state.action) } diff --git a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go index 26ea1eaca683..531690607f8c 100644 --- a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go +++ b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go @@ -111,7 +111,6 @@ func runTestStateStore(t *testing.T, ackToken string) { store, err := newStateStore(log, s) require.NoError(t, err) store.SetAckToken(ackToken) - acker := newStateStoreActionAcker(&testAcker{}, store) require.Equal(t, 0, len(store.Actions())) From ae499696f970a7108cc8fd687724ab3505d899d6 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 11:44:07 +0100 Subject: [PATCH 2/5] Update state_store_test.go --- x-pack/elastic-agent/pkg/agent/application/state_store_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go index 531690607f8c..f834c8bf31a7 100644 --- a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go +++ b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go @@ -111,6 +111,7 @@ func runTestStateStore(t *testing.T, ackToken string) { store, err := newStateStore(log, s) require.NoError(t, err) store.SetAckToken(ackToken) + acker := newStateStoreActionAcker(&testAcker{}, store) require.Equal(t, 0, len(store.Actions())) From 51603761aaee9589d763f9908f36e802e3b42d80 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 11:44:45 +0100 Subject: [PATCH 3/5] whitespacess --- x-pack/elastic-agent/pkg/agent/application/state_store_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go index f834c8bf31a7..26ea1eaca683 100644 --- a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go +++ b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go @@ -111,7 +111,7 @@ func runTestStateStore(t *testing.T, ackToken string) { store, err := newStateStore(log, s) require.NoError(t, err) store.SetAckToken(ackToken) - + acker := newStateStoreActionAcker(&testAcker{}, store) require.Equal(t, 0, len(store.Actions())) From b2b85efc3594f6ccd0c80559d77cedf226e5fd1b Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 11:54:57 +0100 Subject: [PATCH 4/5] added unit test --- .../pkg/agent/application/state_store_test.go | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go index 26ea1eaca683..1c6a7bfd7319 100644 --- a/x-pack/elastic-agent/pkg/agent/application/state_store_test.go +++ b/x-pack/elastic-agent/pkg/agent/application/state_store_test.go @@ -101,6 +101,36 @@ func runTestStateStore(t *testing.T, ackToken string) { require.Equal(t, ackToken, store.AckToken()) })) + t.Run("can save to disk unenroll action type", + withFile(func(t *testing.T, file string) { + action := &fleetapi.ActionUnenroll{ + ActionID: "abc123", + ActionType: "UNENROLL", + } + + s := storage.NewDiskStore(file) + store, err := newStateStore(log, s) + require.NoError(t, err) + + require.Equal(t, 0, len(store.Actions())) + store.Add(action) + store.SetAckToken(ackToken) + err = store.Save() + require.NoError(t, err) + require.Equal(t, 1, len(store.Actions())) + require.Equal(t, ackToken, store.AckToken()) + + s = storage.NewDiskStore(file) + store1, err := newStateStore(log, s) + require.NoError(t, err) + + actions := store1.Actions() + require.Equal(t, 1, len(actions)) + + require.Equal(t, action, actions[0]) + require.Equal(t, ackToken, store.AckToken()) + })) + t.Run("when we ACK we save to disk", withFile(func(t *testing.T, file string) { ActionPolicyChange := &fleetapi.ActionPolicyChange{ From 22ffea72ac8a1f54f6d7ea2de48c07e5350a9fa6 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 13:31:39 +0100 Subject: [PATCH 5/5] changelog --- x-pack/elastic-agent/CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/elastic-agent/CHANGELOG.asciidoc b/x-pack/elastic-agent/CHANGELOG.asciidoc index aa433f81d70f..d3fd1fa65d5d 100644 --- a/x-pack/elastic-agent/CHANGELOG.asciidoc +++ b/x-pack/elastic-agent/CHANGELOG.asciidoc @@ -28,6 +28,7 @@ - Fix Windows service installation script {pull}20203[20203] - Fix timeout issue stopping service applications {pull}20256[20256] - Fix incorrect hash when upgrading agent {pull}22322[22322] +- Fixed nil pointer during unenroll {pull}23609[23609] ==== New features