Add `/api/fleet/agents/:id/audit/unenroll` endpoint #3818

michel-laterman · 2024-08-13T23:17:37Z

What is the problem this PR solves?

Uninstalled agents appear as offline in the fleet UI.

How does this PR solve the problem?

Add /api/fleet/agents/:id/audit/unenroll API that an elastic-agent or Endpoint process may use to annotate the agent document so the agent may appear with a different status.

Changes from RFC

RFC here

Changes are:

409 responses will include a JSON body to be consistent with other errors
Successfull requests will alter the following agent document attributes:
- audit_unenrolled_time - time from request
- audit_unenrolled_reason - reason from request
- unenrolled_at - server current time UTC
- updated_at - server current time UTC (new)
- ~~active - set to false (new)~~ removed change, was causing the api key auth to fail breaking integration and e2e tests

How to test this PR locally

make test-e2e will run e2e tests as we need to change the elastic-agent or other test tools in order to use the endpoint

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

Relates [Feature Request] Have Elastic Agent send a final message to its fleet server when making changes elastic-agent#484

michel-laterman

I think I'll also need to make a change to the ES templates in order to add audit_unenrolled_time and audit_unenrolled_reason to the agent documents. (edit: pr here)

I'll also make a followup PR to change the behaviour of the checkin API to clear the attributes that are set by the audit/unenroll API to allow Endpoint to mark itself as "orphaned" temporarily.

michel-laterman · 2024-08-13T23:18:59Z

internal/pkg/config/defaults/lte10000_limits.yml

@@ -53,3 +53,7 @@ server_limits:
    interval: 1ms
    burst: 2000
    max: 4000
+  audit_unenroll_limit:


The default/env limits for the audit/unenroll endpoint are set to the same values as the ack endpoint (excluding body which is set to 1 kb)

blakerouse

Well done!

Just a few comments, no blockers.

blakerouse · 2024-08-15T20:06:58Z

internal/pkg/api/handleAudit.go

+	}
+
+	if err := audit.bulk.Update(ctx, dl.FleetAgents, agent.Id, body, bulk.WithRefresh(), bulk.WithRetryOnConflict(3)); err != nil {
+		return fmt.Errorf("auditUnenroll update: %w", err)


Seems this will return a 500 error back to the caller, is that the correct error in the case its a conflict? Should it instead be the same conflict error in the case the field is set? I don't know if we want the caller to error.

It's detected much earlier on line 54 of handleAudit.go
https://github.com/elastic/fleet-server/pull/3818/files/#diff-bca31366226def7a73d6e6b142a9e17bbb4c2375a8f0fa16c5c81d4e6a374f13R54-R56

blakerouse · 2024-08-15T20:08:31Z

internal/pkg/api/server_test.go

@@ -349,7 +349,7 @@ key: %s`,
 		cfg.TLS = tlsCFG

 		st := NewStatusT(cfg, nil, nil)
-		srv := NewServer(addr, cfg, nil, nil, nil, nil, st, sm, fbuild.Info{}, nil, nil, nil, nil, nil)
+		srv := NewServer(addr, cfg, nil, nil, nil, nil, st, sm, fbuild.Info{}, nil, nil, nil, nil, nil, nil)


This has become a ton of parameters will most being nil. Switching to a struct as a parameter would be better and cleaner. (don't do it in this PR, but an overall improvement)

Created #3823 to track

blakerouse · 2024-08-15T20:08:51Z

internal/pkg/config/defaults/lte10000_limits.yml

@@ -53,3 +53,7 @@ server_limits:
    interval: 1ms
    burst: 2000
    max: 4000
+  audit_unenroll_limit:


blakerouse · 2024-08-15T20:09:30Z

internal/pkg/server/fleet.go


 	for _, endpoint := range (&cfg.Inputs[0].Server).BindEndpoints() {
-		apiServer := api.NewServer(endpoint, &cfg.Inputs[0].Server, ct, et, at, ack, st, sm, f.bi, ut, ft, pt, bulker, tracer)
+		apiServer := api.NewServer(endpoint, &cfg.Inputs[0].Server, ct, et, at, ack, st, sm, f.bi, ut, ft, pt, auditT, bulker, tracer)


struct would be much better, this is a lot...

internal/pkg/api/error.go

pchila

Just a typo and a question about an error message (non-blocking as it can also be fixed in a follow-up PR), looks good otherwise...

+1 on refactoring long lists of parameters into structs though... in those places the code is not very readable 😅

Co-authored-by: Paolo Chilà <[email protected]>

elastic-sonarqube · 2024-08-19T16:44:37Z

Quality Gate passed

Issues
4 New issues
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
94.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

michel-laterman added 2 commits August 13, 2024 11:32

Add schema definitions and auto generated code

a9ab996

Add boilerplate for new endpoint

360d12d

michel-laterman added enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Aug 13, 2024

michel-laterman requested a review from a team as a code owner August 13, 2024 23:17

michel-laterman requested review from andrzej-stencel and pchila August 13, 2024 23:17

michel-laterman commented Aug 13, 2024

View reviewed changes

Audit unenroll endpoint logic

19ad629

michel-laterman force-pushed the auditUnenroll branch from 51c2e3a to 19ad629 Compare August 13, 2024 23:24

michel-laterman mentioned this pull request Aug 13, 2024

[Feature Request] Have Elastic Agent send a final message to its fleet server when making changes elastic/elastic-agent#484

Open

michel-laterman added 2 commits August 13, 2024 18:50

Don't set inactive state

4855f5b

fix e2e tests

ecdee7f

michel-laterman mentioned this pull request Aug 14, 2024

Call fleet-server audit/unenroll endpoint on uninstall elastic/elastic-agent#5302

Merged

5 tasks

ycombinator requested a review from intxgo August 15, 2024 13:35

blakerouse approved these changes Aug 15, 2024

View reviewed changes

pchila reviewed Aug 19, 2024

View reviewed changes

internal/pkg/api/error.go Outdated Show resolved Hide resolved

pchila approved these changes Aug 19, 2024

View reviewed changes

Fix typo in ErrAuditUnenrollReason message

b933d9d

Co-authored-by: Paolo Chilà <[email protected]>

michel-laterman merged commit b8f864b into elastic:main Aug 20, 2024
8 checks passed

michel-laterman deleted the auditUnenroll branch August 20, 2024 15:55

michel-laterman mentioned this pull request Aug 20, 2024

Alter checkin API to remove attributes set by audit/unenroll API #3827

Merged

6 tasks

mergify bot mentioned this pull request Oct 3, 2024

[8.x](backport #5302) Call fleet-server audit/unenroll endpoint on uninstall elastic/elastic-agent#5688

Merged

5 tasks

ycombinator mentioned this pull request Oct 3, 2024

[Fleet] GET /api/fleet/agents/:id should return audit_unenrolled_reason in response body elastic/kibana#194884

Open

juliaElastic mentioned this pull request Oct 23, 2024

[Fleet]: Unable to force unenroll multiple offline agents from bulk actions. elastic/kibana#197180

Open

michel-laterman mentioned this pull request Oct 23, 2024

fix audit unenroll #4036

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `/api/fleet/agents/:id/audit/unenroll` endpoint #3818

Add `/api/fleet/agents/:id/audit/unenroll` endpoint #3818

michel-laterman commented Aug 13, 2024 •

edited

Loading

michel-laterman left a comment •

edited

Loading

michel-laterman Aug 13, 2024

blakerouse Aug 15, 2024

blakerouse left a comment

blakerouse Aug 15, 2024

michel-laterman Aug 19, 2024

blakerouse Aug 15, 2024

michel-laterman Aug 19, 2024

blakerouse Aug 15, 2024

blakerouse Aug 15, 2024

pchila left a comment

elastic-sonarqube bot commented Aug 19, 2024

Add /api/fleet/agents/:id/audit/unenroll endpoint #3818

Add /api/fleet/agents/:id/audit/unenroll endpoint #3818

Conversation

michel-laterman commented Aug 13, 2024 • edited Loading

What is the problem this PR solves?

How does this PR solve the problem?

Changes from RFC

How to test this PR locally

Design Checklist

Checklist

Related issues

michel-laterman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakerouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pchila left a comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Aug 19, 2024

Quality Gate passed

Add `/api/fleet/agents/:id/audit/unenroll` endpoint #3818

Add `/api/fleet/agents/:id/audit/unenroll` endpoint #3818

michel-laterman commented Aug 13, 2024 •

edited

Loading

michel-laterman left a comment •

edited

Loading