Add option to enable graceful unenroll to invalid API key agents. - #7593
Add option to enable graceful unenroll to invalid API key agents.#7593blakerouse wants to merge 23 commits into
Conversation
|
This pull request does not have a backport label. Could you fix it @blakerouse? 🙏
|
There was a problem hiding this comment.
Pull request overview
Adds a feature-flagged behavior in Fleet Server check-in authentication to return a 200 OK with a single UNENROLL action (instead of 401) when an agent checks in with an invalid/disabled API key, enabling agents to gracefully stop retrying after force-unenroll.
Changes:
- Introduces
unenroll_on_invalid_api_keyunderinputs[].server.feature_flagsand documents it in the reference config + changelog. - Updates check-in handling to emit an
UNENROLLaction response when the flag is enabled and auth fails due to invalid/disabled API keys (or inactive agent). - Adds integration + e2e coverage to validate the new behavior end-to-end (including real elastic-agent behavior).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/e2e/testdata/stand-alone-https-unenroll.tpl | New standalone HTTPS config template enabling the feature flag for e2e. |
| testing/e2e/stand_alone_test.go | New e2e test validating elastic-agent self-unenroll behavior after API key invalidation. |
| internal/pkg/server/fleet_integration_test.go | New integration test validating 200+UNENROLL vs 401 behavior behind the flag. |
| internal/pkg/config/input.go | Adds the UnenrollOnInvalidAPIKey feature flag to config. |
| internal/pkg/api/handleCheckin.go | Implements UNENROLL response path for invalid/disabled API key auth errors. |
| internal/pkg/api/handleCheckin_test.go | Adds unit tests for invalid-key detection + UNENROLL response generation. |
| fleet-server.reference.yml | Documents the new unenroll_on_invalid_api_key setting and default. |
| changelog/fragments/1786137338-unenroll-on-invalid-api-key.yaml | Changelog entry describing the enhancement and configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/pkg/api/handleCheckin_test.go:1145
- TestWriteUnenrollResponse calls writeUnenrollResponse with an extra *http.Request argument, but the method signature is writeUnenrollResponse(logger, w, agentID). This won’t compile as written.
wr := httptest.NewRecorder()
logger := testlog.SetLogger(t)
err = ct.writeUnenrollResponse(logger, wr, agentID)
require.NoError(t, err)
internal/pkg/api/handleCheckin.go:217
- This log message says “invalid API key”, but the UNENROLL response path is also used for inactive agent records (ErrAgentInactive). The message should reflect both cases to avoid misleading operational logs.
zlog.Info().
Str(ecs.AgentID, agentID).
Str(ecs.ActionID, action.Id).
Msg("Returning UNENROLL action for agent with invalid API key")
testing/e2e/stand_alone_test.go:746
- The doc comment says the agent “stops running”, but later in the test it notes that an unenrolled agent keeps running and stops checking in. This is inconsistent and can confuse future maintainers reading the test.
// The test observes only the elastic-agent's own log output — not fleet-server's API response —
// to confirm the agent processes the UNENROLL action and stops running.
internal/pkg/api/handleCheckin.go:200
- writeUnenrollResponse can be triggered for ErrAgentInactive (inactive agent record) as well as invalid/disabled API keys, but the comment currently states it is only used when the API key is invalid.
This issue also appears on line 214 of the same file.
// writeUnenrollResponse writes a 200 check-in response containing a single UNENROLL action.
// It is used when UnenrollOnInvalidAPIKey is enabled and the agent's API key is invalid.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/pkg/api/handleCheckin.go:201
- This comment states the UNENROLL response is used only for “invalid API key”, but this function is also used when the agent is inactive (ErrAgentInactive). Updating the comment avoids misleading documentation.
// writeUnenrollResponse writes a 200 check-in response containing a single UNENROLL action.
// It is used when UnenrollOnInvalidAPIKey is enabled and the agent's API key is invalid.
func (ct *CheckinT) writeUnenrollResponse(zlog zerolog.Logger, w http.ResponseWriter, agentID string) error {
internal/pkg/api/handleCheckin.go:192
- The comment says this helper detects “invalid or disabled API key” errors, but the implementation also treats ErrAgentInactive as a match. Please update the comment to reflect the actual behavior so future readers don’t miss that inactive-agent check-ins are also converted to UNENROLL when the flag is enabled.
This issue also appears on line 199 of the same file.
// isInvalidAPIKeyErr reports whether err represents an invalid or disabled API key
// that would normally produce a 401 response on check-in.
func isInvalidAPIKeyErr(err error) bool {
internal/pkg/api/handleCheckin.go:217
- The log message claims the API key is invalid, but this path can also be hit for inactive agents (ErrAgentInactive). Consider making the message more general to avoid incorrect operational signals.
Msg("Returning UNENROLL action for agent with invalid API key")
testing/e2e/stand_alone_test.go:746
- This test comment says the agent “stops running”, but the assertions below verify it “stops checking in” while the process may continue running. Updating the wording will keep the test description consistent with the behavior being asserted.
// to confirm the agent processes the UNENROLL action and stops running.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (5)
internal/pkg/api/handleCheckin.go:347
- Same as writeEmptyPolicyChangeResponse: this path bypasses the normal response writer and does not increment cntCheckin.bodyOut, so successful UNENROLL responses won’t be reflected in check-in response byte metrics.
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:314
- This response path writes directly to the ResponseWriter, bypassing the normal check-in response writer. As a result, check-in response byte metrics (cntCheckin.bodyOut) won’t include these successful responses, which can skew /stats and monitoring for this feature.
This issue also appears on line 346 of the same file.
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:1519
- If max_bytes is configured smaller than a single entry (250 bytes), Store() will still insert one entry and allow used > maxBytes, so the configured cap is not actually enforced for small values.
func newInvalidKeyLRU(maxBytes int64) *invalidKeyLRU {
if maxBytes <= 0 {
maxBytes = config.DefaultGracefulForceUnenrollMaxBytes
}
return &invalidKeyLRU{
maxBytes: maxBytes,
l: list.New(),
items: make(map[string]*list.Element),
}
changelog/fragments/1786137338-unenroll-on-invalid-api-key.yaml:14
- Grammar: “a UNENROLL action” is more correct than “an UNENROLL action” (UN- is typically pronounced “you-en”).
2. Second occurrence: HTTP 200 with an UNENROLL action, causing the agent to
disenroll itself and exit.
internal/pkg/api/handleCheckin.go:1588
- CleanExpired() scans the entire map while holding the LRU mutex. With a large max_bytes (e.g., default ~200k entries), this can block invalid-key check-ins on this instance during the sweep and create latency spikes under load.
// CleanExpired removes all entries whose firstSeen is not after cutoff.
func (c *invalidKeyLRU) CleanExpired(cutoff time.Time) {
c.mu.Lock()
defer c.mu.Unlock()
var expired []string
for agentID, el := range c.items {
if !mustEntry(el).state.firstSeen.After(cutoff) {
expired = append(expired, agentID)
}
}
for _, id := range expired {
c.delete(id)
}
…SNAPSHOT Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
internal/pkg/server/fleet.go:550
- The invalid-key state cleaner goroutine is started unconditionally. When graceful_force_unenroll is disabled (default), this ticker + LRU scan does unnecessary work. Consider starting the cleaner only when the feature flag is enabled.
f.checkinT = ct
g.Go(loggedRunFunc(ctx, "Invalid API key state cleaner", ct.RunInvalidKeyStateCleaner))
et, err := api.NewEnrollerT(f.verCon, &cfg.Inputs[0].Server, bulker, f.cache)
testing/e2e/stand_alone_test.go:947
- Elasticsearch API key invalidation in this e2e test uses DELETE /_security/api_key, but Fleet Server’s own API key invalidation code uses the official Invalidate API Key endpoint (POST /_security/api_key/_invalidate). Using the wrong method/path risks test failures against real ES versions.
// invalidateESAPIKey calls DELETE /_security/api_key to invalidate the given key ID.
func (suite *StandAloneSuite) invalidateESAPIKey(ctx context.Context, keyID string) {
suite.T().Helper()
body, err := json.Marshal(map[string]any{"ids": []string{keyID}})
suite.Require().NoError(err)
req, err := http.NewRequestWithContext(ctx, http.MethodDelete,
"http://"+suite.ESHosts+"/_security/api_key",
bytes.NewReader(body))
| s.count++ | ||
| if s.count == 1 { | ||
| s.firstSeen = now | ||
| } | ||
| ct.invalidKeyStates.Store(agentID, s) |
…on is running In agent 9.6.0-SNAPSHOT, elastic-agent enroll always attempts a daemon reload via Unix socket after writing credentials. --delay-enroll defers the actual enrollment to the first start of elastic-agent run, which is the correct pattern when enrolling before starting the daemon. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
--delay-enroll defers enrollment to first start which is wrong; we want enroll to complete immediately and just skip the daemon reload step since no daemon is running yet. --skip-daemon-reload does exactly that. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/pkg/config/input.go:127
- The MaxBytes field is documented as a strict “memory cap in bytes”, but the implementation in invalidKeyLRU uses a fixed per-entry estimate (invalidKeyLRUEntryBytes=250) rather than tracking real memory usage. This can mislead operators configuring max_bytes; please clarify that this is an approximate budget/entry-count estimate.
// MaxBytes is the memory cap in bytes for the in-memory LRU that tracks per-agent
// escalation state. Each entry costs ~250 bytes; the default 50000000 (50 MB) holds
// ~200,000 entries. When full, the least-recently-used entry is evicted, resetting
// that agent's escalation back to step 1 on its next check-in. A value of 0 uses
// the default.
fleet-server.reference.yml:302
- The reference config describes max_bytes as a strict “memory cap in bytes”, but the implementation uses a fixed per-entry estimate (~250 bytes) to approximate entry count. Updating this wording would prevent operators from assuming the cap is exact.
# // Memory cap in bytes for the in-memory LRU that tracks per-agent escalation
# // state. Each entry costs ~250 bytes; the default 50000000 (50 MB) holds
# // ~200,000 entries. When full, the least-recently-used entry is evicted,
# // resetting that agent's escalation back to step 1 on its next check-in.
# // A value of 0 uses the default.
internal/pkg/api/handleCheckin.go:110
- This comment says invalidKeyStates is “memory-bounded” with a cap in bytes, but invalidKeyLRU enforces the cap using a fixed per-entry estimate (invalidKeyLRUEntryBytes) rather than actual memory accounting. Consider clarifying that the MaxBytes cap is approximate/entry-count-derived.
// invalidKeyStates is a memory-bounded LRU that tracks per-agent invalid-API-key escalation
// state for the GracefulForceUnenroll feature. Its cap is set by
// cfg.Features.GracefulForceUnenroll.MaxBytes (default 50 MB, ~200,000 entries).
changelog/fragments/1786137338-unenroll-on-invalid-api-key.yaml:21
- The changelog fragment describes max_bytes as a strict “memory-bounded” LRU sized in bytes, but the implementation uses a fixed per-entry estimate (~250 bytes). Please clarify that the 50 MB/~200k figure is approximate to avoid overstating how precise the limit is.
Per-agent escalation state is stored in a memory-bounded LRU (default 50 MB,
~200,000 entries). The cap is configurable via `max_bytes` (integer bytes; the
default 50000000 holds ~200,000 entries). When the LRU is full, the least-recently-used
entry is evicted, resetting that agent's escalation back to step 1.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (5)
internal/pkg/api/handleCheckin.go:347
- This response path bypasses the normal check-in response writer and doesn’t update cntCheckin.bodyOut, so /stats will undercount response bytes for the UNENROLL step.
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:314
- The invalid-key fast-path writes the JSON payload directly but doesn’t update cntCheckin.bodyOut (used by /stats/autoscaling signals). This makes check-in egress metrics undercount when graceful_force_unenroll is enabled.
This issue also appears on line 346 of the same file.
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:1519
- newInvalidKeyLRU allows max_bytes values smaller than a single entry (~250 bytes). In that case Store() will still admit at least one entry and c.used can exceed c.maxBytes, violating the documented memory cap behavior.
func newInvalidKeyLRU(maxBytes int64) *invalidKeyLRU {
if maxBytes <= 0 {
maxBytes = config.DefaultGracefulForceUnenrollMaxBytes
}
return &invalidKeyLRU{
maxBytes: maxBytes,
l: list.New(),
items: make(map[string]*list.Element),
}
internal/pkg/api/handleCheckin.go:1589
- CleanExpired builds a slice of all expired agent IDs before deleting them. With a large LRU (default ~200k entries) this creates avoidable allocations/GC pressure during periodic cleanup; deleting while iterating is safe in Go map iteration and avoids the extra slice.
// CleanExpired removes all entries whose firstSeen is not after cutoff.
func (c *invalidKeyLRU) CleanExpired(cutoff time.Time) {
c.mu.Lock()
defer c.mu.Unlock()
var expired []string
for agentID, el := range c.items {
if !mustEntry(el).state.firstSeen.After(cutoff) {
expired = append(expired, agentID)
}
internal/pkg/server/fleet.go:549
- The invalid-key state cleaner goroutine is started unconditionally, even when graceful_force_unenroll is disabled. This adds a permanent ticker wakeup (every 5m) in all deployments for a feature-gated behavior.
f.checkinT = ct
g.Go(loggedRunFunc(ctx, "Invalid API key state cleaner", ct.RunInvalidKeyStateCleaner))
This comment has been minimized.
This comment has been minimized.
If the agent process exits before becoming online, print the agent log immediately and fail fast. Log each non-fleet-server agent's status on every poll so we can see whether the agent appears with a wrong status. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
internal/pkg/api/handleCheckin.go:347
- writeUnenrollResponse writes directly to the ResponseWriter without updating the check-in route body_out metric (cntCheckin.bodyOut), which will underreport egress for this new 200-response path when the feature flag is enabled.
payload, err := json.Marshal(&resp)
if err != nil {
return fmt.Errorf("writeUnenrollResponse marshal: %w", err)
}
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:314
- writeEmptyPolicyChangeResponse writes directly to the ResponseWriter without updating the check-in route body_out metric (cntCheckin.bodyOut), which will underreport egress for this new 200-response path when the feature flag is enabled.
This issue also appears on line 341 of the same file.
payload, err := json.Marshal(&resp)
if err != nil {
return fmt.Errorf("writeEmptyPolicyChangeResponse marshal: %w", err)
}
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:1513
- newInvalidKeyLRU can be configured with max_bytes smaller than a single entry (invalidKeyLRUEntryBytes), but Store() will still insert one element (c.l.Len()==0 prevents eviction), allowing used to exceed maxBytes. Clamping the configured cap to at least one entry size avoids violating the stated memory cap semantics for small values.
func newInvalidKeyLRU(maxBytes int64) *invalidKeyLRU {
if maxBytes <= 0 {
maxBytes = config.DefaultGracefulForceUnenrollMaxBytes
}
internal/pkg/server/fleet.go:549
- The invalid API key state cleaner goroutine is started unconditionally. When graceful_force_unenroll is disabled this ticker loop does work (and holds an extra goroutine) without any possibility of state being present. Starting it only when the feature is enabled avoids unnecessary background load.
ct, err := api.NewCheckinT(f.verCon, &cfg.Inputs[0].Server, f.cache, bc, pm, am, ad, bulker,
api.WithOutputSecretCandidateCollector(outputSecretReconciler))
if err != nil {
return err
}
f.checkinT = ct
g.Go(loggedRunFunc(ctx, "Invalid API key state cleaner", ct.RunInvalidKeyStateCleaner))
et, err := api.NewEnrollerT(f.verCon, &cfg.Inputs[0].Server, bulker, f.cache)
This comment has been minimized.
This comment has been minimized.
TL;DRBuildkite 16287 failed only in Remediation
Investigation detailsRoot CauseClassification: Test failure (inconclusive at assertion level from provided artifact). What is confirmed from this build:
At this commit, the test already uses Because the current artifact does not include the subtest assertion body, I cannot reliably identify which assertion failed among the three Evidence
Verification
Follow-up
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/pkg/api/handleCheckin.go:347
- writeUnenrollResponse writes the payload without recording the number of bytes written in cntCheckin.bodyOut (unlike writeResponse) and returns the raw write error without context. This makes check-in bodyOut metrics undercount when graceful_force_unenroll returns UNENROLL as a 200 response, and makes write failures harder to diagnose.
_, err = w.Write(payload)
return err
internal/pkg/api/handleCheckin.go:314
- writeEmptyPolicyChangeResponse writes the payload without recording the number of bytes written in cntCheckin.bodyOut (unlike writeResponse) and returns the raw write error without context. This makes check-in bodyOut metrics undercount when graceful_force_unenroll returns a 200 response, and makes write failures harder to diagnose.
This issue also appears on line 346 of the same file.
_, err = w.Write(payload)
return err
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
internal/pkg/api/handleCheckin.go:275
- invalidKeyStates is updated via a Load → modify → Store sequence across multiple lock acquisitions. Concurrent invalid check-ins for the same agent can lose increments (e.g., two goroutines both observe count=0 and both store count=1), causing the escalation steps to be skipped/delayed unpredictably.
var s invalidKeyState
if loaded, ok := ct.invalidKeyStates.Load(agentID); ok {
s = loaded
if now.Sub(s.firstSeen) >= invalidKeyStateReset {
ct.invalidKeyStates.Delete(agentID)
testing/e2e/stand_alone_test.go:745
- The test comments are internally inconsistent about UNENROLL behavior: here it says "disenrolls and exits", but later (lines 913-916) it says the process stays running and merely stops sending check-ins. This makes the test intent unclear and could mislead future debugging.
// 1. 1st invalid checkin → fleet-server returns POLICY_CHANGE with empty policy.
// The agent stops all running components.
// 2. 2nd invalid checkin → fleet-server returns UNENROLL.
// The agent disenrolls and exits.
//
internal/pkg/server/fleet.go:549
- RunInvalidKeyStateCleaner is started unconditionally. When graceful_force_unenroll is disabled (the default), this still spawns an extra goroutine + ticker even though the invalid-key LRU will never be populated.
f.checkinT = ct
g.Go(loggedRunFunc(ctx, "Invalid API key state cleaner", ct.RunInvalidKeyStateCleaner))
What is the problem this PR solves?
Once an Elastic Agent is force unenrolled the Elastic Agents will continue to communicate to Fleet Server. There are cases where it would be best to just have the Elastic Agent stop all of its components, unenroll if it can (those with tamper protection on will not be able to), and those that cannot will continue to receive 401's for a full hour, until the cycle starts again.
How does this PR solve the problem?
This changes the behavior of invalid API keys from being a 401 error that just gets retried non-stop to a 200 with a policy change action that is an empty policy, then a unenroll action, and then back to the 401 error.
How to test this PR locally
Design Checklist
[ ] I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected.[ ] I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.Checklist
./changelog/fragmentsusing the changelog tool