Fix Fleet Enrollment Handling for Containerized Agent #6568
Conversation
This file contains 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
pkoutsovasilis
added
bug
pkoutsovasilis
force-pushed
the
k8s/enrollment_token_update
branch
from
f6d06ec to
ab63682
Compare
pkoutsovasilis
changed the title
blakerouse
reviewed
Jan 22, 2025
| // this elastic-agent has not valid api token thus it should enroll | ||
| return true, nil | ||
| case err != nil: | ||
| return false, fmt.Errorf("failed to validate api token: %w", err) |
There was a problem hiding this comment.
A simple network error could cause this, should there be some retries in here?
There was a problem hiding this comment.
I added a really simple retry logic, let me know if that's aligned with what you had on mind 🙂
There was a problem hiding this comment.
@blakerouse any objections merging this PR?
There was a problem hiding this comment.
I am happy with the added retry loop. Would have been a little cleaner to place it in its own function, but that is okay. Feel free to merge.
pkoutsovasilis
force-pushed
the
k8s/enrollment_token_update
branch
2 times, most recently
from
bd3acad to
db8078b
Compare
pkoutsovasilis
force-pushed
the
k8s/enrollment_token_update
branch
from
db8078b to
f7fd4cb
Compare
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
pchila
approved these changes
Jan 27, 2025
...ontainerised-agent-when-there-is-an-enrollement-token-change-or-the-agent-is-unenrolled.yaml
Outdated
Show resolved
Hide resolved
blakerouse
approved these changes
Jan 28, 2025
| // this elastic-agent has not valid api token thus it should enroll | ||
| return true, nil | ||
| case err != nil: | ||
| return false, fmt.Errorf("failed to validate api token: %w", err) |
There was a problem hiding this comment.
I am happy with the added retry loop. Would have been a little cleaner to place it in its own function, but that is okay. Feel free to merge.
|
mergify bot
pushed a commit
that referenced
this pull request
Jan 28, 2025
* feat: add k8s integration test to check fleet enrollment * fix: container correct fleet enrollment when token changes or the agent is unenrolled * fix: update TestDiagnosticLocalConfig to include enrollment_token_hash * feat: add a simple retry logic while validate the stored agent api token * feat: add unit-test for shouldFleetEnroll * fix: improve unit-test explicitness and check for expected number of calls * fix: kind in changelog fragment * fix: split up ack-ing fleet in a separate function (cherry picked from commit 17814cc)
8 tasks
pkoutsovasilis
added a commit
that referenced
this pull request
Jan 28, 2025
* feat: add k8s integration test to check fleet enrollment * fix: container correct fleet enrollment when token changes or the agent is unenrolled * fix: update TestDiagnosticLocalConfig to include enrollment_token_hash * feat: add a simple retry logic while validate the stored agent api token * feat: add unit-test for shouldFleetEnroll * fix: improve unit-test explicitness and check for expected number of calls * fix: kind in changelog fragment * fix: split up ack-ing fleet in a separate function (cherry picked from commit 17814cc) Co-authored-by: Panos Koutsovasilis <panos.koutsovasilis@elastic.co>
cmacknz
reviewed
Jan 29, 2025
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 does this PR do?
This PR introduces a fix for containerized Fleet-managed Elastic Agents to handle scenarios where:
It achieves the above by enhancing the agent's logic inside the
containercmd to:The PR also adds a Kubernetes integration tests to verify the proper behaviour of enrollment and re-enrollment under various scenarios, including:
Key changes include:
shouldFleetEnrollto centralize logic for fleet enrollment decisions.agent/{id}/ackspath of Fleet server with empty events to check if the agent API token is still valid. More than happy to introduce a separate path just for this cause on Fleet server, although it will be the same the ACKs one with empty events (as of now at least)You can easily see here in the CI run of the first commit in this PR that the Elastic Agent, before this PR, doesn't handle enrollment correctly and always resorts to using the Fleet token and URL stored in its state. This PR addresses that issue and ensures enrollment uses the correct configuration and token.
PS: the actual changes of this PR are this commit b6596d0 which is
+305 -25thus I consider this PR aligned with the team policies 🙂Why is it important?
This fix ensures robust and predictable behavior for containerized Fleet-managed Elastic Agents and enhance user experience of managing elastic-agent in Kubernetes.
Checklist
./changelog/fragmentsusing the changelog toolDisruptive User Impact
The changes introduced in this PR are non-disruptive. They improve fleet enrollment handling and maintain backward compatibility.
How to test this PR locally
Related issues