Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix audit unenroll #4036

Closed
wants to merge 3 commits into from
Closed

Conversation

juliaElastic
Copy link
Contributor

What is the problem this PR solves?

// Please do not just reference an issue. Explain WHAT the problem this PR solves here.

audit unenroll should set the agent active flag to false

How does this PR solve the problem?

// Explain HOW you solved the problem in your code. It is possible that during PR reviews this changes and then this section should be updated.

How to test this PR locally

Design Checklist

  • I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer.
  • 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

  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool

Related issues

Closes elastic/kibana#197180

@juliaElastic juliaElastic requested a review from a team as a code owner October 23, 2024 12:13
Copy link
Contributor

mergify bot commented Oct 23, 2024

This pull request does not have a backport label. Could you fix it @juliaElastic? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

Copy link
Contributor

mergify bot commented Oct 23, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 23, 2024
@juliaElastic juliaElastic added the backport-8.16 Automated backport with mergify label Oct 23, 2024
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Oct 23, 2024
@juliaElastic
Copy link
Contributor Author

juliaElastic commented Oct 23, 2024

I need confirmation on how this feature is supposed to work, read through the comments in elastic/elastic-agent#484 and the integration/e2e tests and it seems the feature is supposed to allow calling the audit unenroll API multiple times, and a checkin after.

However, the existing unenroll action sets the active flag to false (and that is needed for the UI to correctly show unenrolled state), and by adding it to the audit unenroll API, the agent will no longer accept API calls (the API key will be invalidated).

If we want this audit unenroll to be a "soft unenroll" and the agent to be able to come back online, we have to change the status calculation on the UI to show "unenrolled" if unenrolled_at is set, not when active:false is set.
If we do this, it raises the question what happens if the agent never checks in again, will it be left active indefinitely with an active API key?
So far the unenroll action was not reversible.

@@ -1485,17 +1485,6 @@ func Test_SmokeTest_AuditUnenroll(t *testing.T) {
require.Equal(t, http.StatusOK, res.StatusCode)
res.Body.Close()

t.Log("Orphaned can replace uninstall")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test/support this scenario.
It's important for Endpoint to be able to signal that it is orphaned even if the installed agent is removed

@michel-laterman
Copy link
Contributor

In the original implementation I tried setting agents to inactive (#3818) however that would cause the API key authentication we do on checkins to fail. It would require a change in our overall auth logic if we wanted to make this change

@ycombinator ycombinator removed the request for review from kaanyalti October 28, 2024 16:22
@juliaElastic
Copy link
Contributor Author

Closing this as it seems we can't move agents to inactive on audit unenroll as they wouldn't be able to check in again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify backport-8.16 Automated backport with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet]: Unable to force unenroll multiple offline agents from bulk actions.
3 participants