Skip to content

Conversation

@aleksmaus
Copy link
Contributor

What is the problem this PR solves?

The cleanup code for enrollment errors is scattered in couple of places:

Router.handleEnroll->
	->EnrollerT.handleEnroll
		->EnrollerT.processRequest
			->EnrollerT._enroll
				->generateAccessApiKey + createFleetAgent(If Fail)->invalidateApiKey

	->writeResponse(If Fail) 
		-> wipeGhosts
			-> delete Agent document
			-> invalidateApiKey

This change consolidates the cleanup code through the rollback/cleanup wrapper.

How does this PR solve the problem?

Implemented a simple "rollback" wrapper that can have the "rollback/cleanup" cleanup functions added as needed. The Rollback is called on error which in turn calls the registered cleanup functions.

For example, the log when Fleet Server fails to create the agent record:

{"log.level":"error","service.name":"fleet-server","http.request.id":"01FMQW2DC9GXDDX3Z8S6YCTZ0M","mod":"enroll","fleet.enroll.apikey.id":"NxK5Jn0ByQ6Ip6IwWvny","error.message":"elastic fail 403:cluster_block_exception:index [.fleet-agents-7] blocked by: [FORBIDDEN/8/index write (api)];","@timestamp":"2021-11-17T21:15:31.625Z","message":"perform rollback on enrollment failure"}
{"log.level":"debug","service.name":"fleet-server","http.request.id":"01FMQW2DC9GXDDX3Z8S6YCTZ0M","mod":"enroll","name":"invalidate API key","@timestamp":"2021-11-17T21:15:31.625Z","message":"rollback function called"}
{"log.level":"info","service.name":"fleet-server","http.request.id":"01FMQW2DC9GXDDX3Z8S6YCTZ0M","mod":"enroll","fleet.enroll.apikey.id":"NxK5Jn0ByQ6Ip6IwWvny","fleet.apikey.id":"81fBL30BMo45IINiNv-r","dur":773.635427,"@timestamp":"2021-11-17T21:15:32.399Z","message":"invalidated apiKey"}
{"log.level":"debug","service.name":"fleet-server","http.request.id":"01FMQW2DC9GXDDX3Z8S6YCTZ0M","mod":"enroll","name":"invalidate API key","@timestamp":"2021-11-17T21:15:32.399Z","message":"rollback function succeeded"} 

So, now:

  1. when the api key is created the api key invalidation function is registered with the Rollback
  2. when the agent is created the delete agent function is registered with the Rollback
  3. the deferred function in handleEnroll calls Rollback if there was an error during enrollment. the Rollback calls all the registered cleanup function in order to remove/invalidate the data that was created.

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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@aleksmaus aleksmaus added enhancement New feature or request cleanup labels Nov 17, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 17, 2021

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

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

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 17, 2021
@aleksmaus aleksmaus added backport-v8.0.0 Automated backport with mergify v8.0.0 labels Nov 17, 2021
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Nov 17, 2021
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-17T21:45:38.241+0000

  • Duration: 9 min 3 sec

  • Commit: 50a76bb

Test stats 🧪

Test Results
Failed 0
Passed 212
Skipped 0
Total 212

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

Copy link

@scunningham scunningham left a comment

Choose a reason for hiding this comment

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

Seems ok. Not thrilled having to add the context to the router, but cleaner than previous implementation.

@aleksmaus aleksmaus merged commit d643e6b into elastic:master Nov 18, 2021
mergify bot pushed a commit that referenced this pull request Nov 18, 2021
mergify bot added a commit that referenced this pull request Nov 18, 2021
(cherry picked from commit d643e6b)

Co-authored-by: Aleksandr Maus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v8.0.0 Automated backport with mergify cleanup enhancement New feature or request v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants