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

[Fleet] Move Fleet Setup to start lifecycle #117552

Merged
merged 31 commits into from
Nov 15, 2021

Conversation

kpollich
Copy link
Member

@kpollich kpollich commented Nov 4, 2021

Summary

Ref #111858

Move Fleet setup from an API request on every Fleet page-load to the Fleet plugin's server-side start lifecycle method.

For reference, the current Fleet Setup process performs the following steps (see https:/elastic/kibana/blob/main/x-pack/plugins/fleet/server/services/setup.ts):

  • setupFleet()
  • If Fleet setup is already pending, return a pending status, else createSetupSideEffects()
  • Grab packages, policies, and outputs from kibana.yml
  • ensurePreconfiguredOutputs() and run settingsSetup
    • ensurePreconfiguredOuputs writes or updates output settings to the ingest-outputs saved object type
    • settingsSetup ensures that any of Fleet's default settings are properly set
  • ensureDefaultOutput()
    • Ensures that a default output it set either by config in kibana.yml or by creating one
  • If agentIdVerificationEnabled is set in config, then ensureFleetGlobalEsAssets
    • Installs Fleet's default component template and ingest pipeline if they do not already exist
  • ensurePreconfiguredPackagesAndPolicies - Install preconfigured packages + policies based on
    • Packages listed in kibana.yml
    • DEFAULT_PACKAGES hard coded in Fleet
    • AUTO_UPDATE_PACKAGES hard coded in Fleet
    • Handle upgrades for managed package policies (upgradeManagedPackagePolicies) for any managed package w/ keepPoliciesUpToDate: true set
  • cleanPreconfiguredOutputs
    • Deletes any outputs that were previously preconfigured and no longer are
  • ensureDefaultEnrollmentAPIKeysExists
  • ensureFleetServerAgentPoliciesExists

Checklist

  • Call setupFleet from Fleet plugin's server-side start lifecycle
  • Install any preconfigured packages

@kpollich kpollich added WIP Work in progress v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 labels Nov 4, 2021
@kpollich kpollich self-assigned this Nov 4, 2021
@kpollich
Copy link
Member Author

kpollich commented Nov 8, 2021

@elasticmachine merge upstream

@kpollich kpollich marked this pull request as ready for review November 8, 2021 16:54
@kpollich kpollich requested a review from a team as a code owner November 8, 2021 16:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@kpollich kpollich changed the title [Fleet] [WIP] Move Fleet Setup to start lifecycle [Fleet] Move Fleet Setup to start lifecycle Nov 8, 2021
@kpollich
Copy link
Member Author

kpollich commented Nov 8, 2021

@elasticmachine merge upstream

@joshdover joshdover self-requested a review November 8, 2021 16:55
Comment on lines 343 to 354
new Promise<void>(async (resolve, reject) => {
try {
await startFleetServerSetup();
await setupFleet(
new SavedObjectsClient(core.savedObjects.createInternalRepository()),
core.elasticsearch.client.asInternalUser
);

resolve();
} catch (error) {
reject(error);
}
Copy link
Contributor

@joshdover joshdover Nov 8, 2021

Choose a reason for hiding this comment

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

I think we should move this promise constructor outside the fleetSetupCompleted function so that it isn't called each time fleetSetupCompleted is called and instead just return the already-constructed Promise. Also what is calling fleetSetupCompleted? I don't think we should rely on this being called for setup to run.

Also more of a nit, but I don't think you need the Promise constructor and you can just use an async function directly:

const fleetSetup = (async () {
  await startFleetServerSetup();
  await setupFleet(...);
})();

Copy link
Member Author

Choose a reason for hiding this comment

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

Fleet doesn't call fleetSetupCompleted on its own, but the security_solution plugin does rely on it to handle some of their dependent setup logic here:

// Migrate artifacts to fleet and then start the minifest task after that is done
plugins.fleet.fleetSetupCompleted().then(() => {
migrateArtifactsToFleet(savedObjectsClient, artifactClient, logger).finally(() => {
logger.info('Dependent plugin setup complete - Starting ManifestTask');
if (this.manifestTask) {
this.manifestTask.start({
taskManager,
});
} else {
logger.error(new Error('User artifacts task not available.'));
}
});
});

Agreed on restructuring this - it doesn't need a Promise constructor anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fleet doesn't call fleetSetupCompleted on its own, but the security_solution plugin does rely on it to handle some of their dependent setup logic here

I still don't think we should rely on security solution to call this. In fact, we or they may remove this in 8.0 as part of removing our BWC code for pre-GA fleet. We can still support this API for now by initiating the Fleet setup directly in the start function before the return statement, but just don't await the promise and instead return it from fleetSetupCompleted().

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this logic out of fleetSetupCompleted so we don't need to rely on security solution calling it.

.then(() => {
logger.info('Fleet setup completed');
})
.catch((error) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

catching errors here seems to squash the UnhandledPromiseRejection errors that were breaking various tests in CI. 🤞

@kpollich
Copy link
Member Author

kpollich commented Nov 9, 2021

@elasticmachine merge upstream

@kpollich
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

🚀

@kpollich
Copy link
Member Author

Sample debug log output on Kibana boot:

Screen Shot 2021-11-11 at 3 24 36 PM

Comment on lines 338 to 359
const fleetSetupPromise = (async () => {
try {
logger.info('Beginning fleet setup');

const { nonFatalErrors } = await setupFleet(
new SavedObjectsClient(core.savedObjects.createInternalRepository()),
core.elasticsearch.client.asInternalUser
);

if (nonFatalErrors.length > 0) {
logger.info('Encountered non fatal errors during Fleet setup');
formatNonFatalErrors(nonFatalErrors).forEach((error) =>
logger.info(JSON.stringify(error))
);
}

logger.info('Fleet setup completed');
} catch (error) {
logger.warn('Fleet setup failed');
logger.warn(error);
}
})();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this logging logic into setupFleet itself so we get the same logging when running manually via the API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I think that's a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all the logs except for what's in our catch into setupFleet in 76138ec

@elastic elastic deleted a comment from kibanamachine Nov 15, 2021
@kpollich
Copy link
Member Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default Firefox Tests / InfraOps App Metrics UI Home page with metrics present change color palette

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kpollich

@kpollich kpollich merged commit ec504d6 into elastic:main Nov 15, 2021
@kpollich kpollich deleted the 111858-fleet-setup-on-boot branch November 15, 2021 15:33
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 15, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 15, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kyle Pollich <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Nov 17, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>
mbondyra pushed a commit to mbondyra/kibana that referenced this pull request Nov 19, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>
roeehub pushed a commit to build-security/kibana that referenced this pull request Dec 16, 2021
* Call setup on fleet start, remove API calls

* Fix unused import

* Revert removal of setup API call

* Restructor fleetSetupCompleted promise

* Add logging + handle setup failures

* Restructure logging to mix of debug/info

* Maybe fix failing tests

* Try fixing tests again

* Fix another dashboard test

* Re-add output logs after merge

* Log non-fatal errors during Fleet setup on boot

* Don't rely on fleetSetupCompleted to be called

* Fix failing test

* Track fleet setup status to avoid double calls

* Use IIFE in place of Promise ctor

* Remove unnecessary fleetSetupStatus value

* Move non-error logs into setupFleet method

* Remove unused formatNonFatalErrors import

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants