Repository navigation
Conversation
rdner
added
the
Team:Elastic-Agent-Control-Plane
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Credential retention and state-transition issues remain, alongside action-delivery defects and an invalid integration-test fixture.
6 open issues
1 file not reviewed
internal/pkg/model/schema.go: Generated file
What changed in this PR
Adds a pending-uninstall state so Fleet Server can retain agent credentials between UNENROLL and a follow-up UNINSTALL.
Changes:
- Defers credential invalidation until UNINSTALL acknowledgement.
- Restricts pending agents to UNINSTALL/CANCEL and records premature UNINSTALL failures.
- Adds schema support, tests, and a changelog entry.
| File | Description |
|---|---|
| model/schema.json | Defines pending-uninstall state. |
| internal/pkg/server/uninstall_action_integration_test.go | Tests the two-step uninstall flow. |
| internal/pkg/model/schema.go | Adds the generated agent field. |
| internal/pkg/dl/constants.go | Adds the persistence field constant. |
| internal/pkg/api/handleCheckin.go | Filters actions and records uninstall rejections. |
| internal/pkg/api/handleCheckin_test.go | Tests filtering and rejection results. |
| internal/pkg/api/handleAck.go | Separates pending and terminal acknowledgements. |
| internal/pkg/api/handleAck_test.go | Tests credential retention after UNENROLL. |
| changelog/fragments/1792579200-keep-credentials-after-unenroll-for-uninstall.yaml | Documents the lifecycle changes. |
Review effort: Balanced
Comment on lines
+51
to
+53
| Agents: []string{resp.Item.Id}, | ||
| ActionID: unenrollActionID, | ||
| Type: string(api.UNENROLL), |
|
|
||
| now := time.Now().UTC().Format(time.RFC3339) | ||
| doc := bulk.UpdateFields{ | ||
| dl.FieldUninstallPending: true, |
Comment on lines
+592
to
594
| pendingActions, rejectedUninstalls := filterActions(zlog, agent.Id, pendingActions, agent.UninstallPending) | ||
| ct.rejectUninstallActions(r.Context(), zlog, agent.Id, rejectedUninstalls) | ||
| actions, ackToken = convertActions(zlog, agent.Id, pendingActions) |
Comment on lines
+617
to
+618
| acdocs, rejectedUninstalls := filterActions(zlog, agent.Id, acdocs, agent.UninstallPending) | ||
| ct.rejectUninstallActions(ctx, zlog, agent.Id, rejectedUninstalls) |
Comment on lines
+1023
to
+1025
| if action.Type != string(UNINSTALL) && action.Type != string(CANCEL) { | ||
| zlog.Info().Str(ecs.AgentID, agentID).Str(ecs.ActionID, action.ActionID).Str(ecs.ActionType, action.Type).Msg("Withholding non-uninstall action from pending-uninstall agent") | ||
| continue |
Contributor
|
This pull request does not have a backport label. Could you fix it @rdner? 🙏
|
rdner
force-pushed
the
keep-credentials-after-unenroll
branch
from
October 21, 2026 16:36
b456166 to
ba0701b
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The integration fixture lacks the required uninstall capability, so it takes the legacy invalidation path rather than exercising the intended flow.
6 open issues
1 resolved since last review
1 file not reviewed
internal/pkg/model/schema.go: Generated file
Review effort: Balanced
Comment on lines
+61
to
+62
| AgentAck(t, ctx, srv, gotUnenrollID, resp.Item.Id, resp.Item.AccessApiKey) | ||
| AssertAgentPendingUninstall(t, ctx, srv, resp.Item.Id) |
Member
|
I haven't read this yet since it's a draft, but if we are changing the behaviour of the existing UNENROLL action, make sure it and the code that interacts with it in fleet-server tolerates older agent versions that will be missing any of the agent side changes you've made. |
- UNENROLL keeps the agent active and its access key valid, sets uninstall_pending instead of invalidating; only UNINSTALL/CANCEL are delivered afterwards. - UNENROLL invalidates the agent's output keys (e.g. Elasticsearch) so the unenrolled agent immediately stops shipping data. - UNINSTALL is rejected with a failed action result unless the agent is unenrolled; terminal invalidation happens on the UNINSTALL ack. Co-authored-by: Cursor <cursoragent@cursor.com>
rdner
force-pushed
the
keep-credentials-after-unenroll
branch
from
October 27, 2026 10:27
ba0701b to
abd448c
Compare
Member
Author
|
@cmacknz it's already taken care of. The plan is that new agents will report The recover from UNENROLL will come as a separate follow up PR, otherwise this one would be hard to review. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A failed rejection-result write can be permanently skipped when check-in advances the action cursor.
7 open issues
1 file not reviewed
internal/pkg/model/schema.go: Generated file
Review effort: Balanced
Comment on lines
+1060
to
+1061
| if err := dl.CreateActionResult(ctx, ct.bulker, acr); err != nil { | ||
| zlog.Error().Err(err).Str(ecs.AgentID, agentID).Str(ecs.ActionID, action.ActionID).Msg("failed to write rejected UNINSTALL action result") |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the problem this PR solves?
The UNENROLL action is now the first step of the remote-uninstall flow. This PR changes how Fleet Server handles it:
uninstall_pendinginstead of invalidating, so the agent can still receive the follow-up UNINSTALL/CANCEL. While pending, only UNINSTALL/CANCEL are delivered (policy changes and all other actions are withheld)..fleet-actions-results. Terminal invalidation (access key invalidated, agent marked inactive) happens on the UNINSTALL ack.uninstallableflag in theirlocal_metadataenter the pending-uninstall flow. Older agents without the flag keep the previous behavior and have their credentials invalidated immediately on UNENROLL.How to test this PR locally
Tests have been added.
Design Checklist
- [ ] I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.Checklist
- [ ] I have made corresponding changes to the documentation- [ ] I have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog toolRelated issues
This is a follow up to #7895
Prerequisite to elastic/elastic-agent#16540