From 7bdbf28368ea7621f31b20a54a66d379c0379ae9 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Thu, 21 Jan 2021 19:30:48 +0100 Subject: [PATCH] [Ingest Manager] Fixed nil pointer during unenroll (#23609) [Ingest Manager] Fixed nil pointer during unenroll (#23609) --- x-pack/elastic-agent/CHANGELOG.asciidoc | 1 + .../pkg/agent/application/fleet_acker.go | 8 ++++- .../pkg/agent/application/lazy_acker.go | 6 +++- .../pkg/agent/application/lazy_acker_test.go | 2 +- .../pkg/agent/application/managed_mode.go | 2 +- .../pkg/agent/application/state_store.go | 2 +- .../pkg/agent/application/state_store_test.go | 30 +++++++++++++++++++ 7 files changed, 46 insertions(+), 5 deletions(-) diff --git a/x-pack/elastic-agent/CHANGELOG.asciidoc b/x-pack/elastic-agent/CHANGELOG.asciidoc index aa433f81d70..d3fd1fa65d5 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 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 4544fa8a772..dac05d0c3a0 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 58b212ab8e4..4a4004e028f 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 24c708c0d91..b3d872d4946 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 63e8611354d..9ad1f24a3d0 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 81d3f901469..283ab8e480d 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 26ea1eaca68..1c6a7bfd731 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{