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.

Sign up for GitHub

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
Merged

[Fleet] Move Fleet Setup to start lifecycle #117552

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3f504b9
Call setup on fleet start, remove API calls
kpollich Nov 4, 2021
ff41a00
Fix unused import
kpollich Nov 4, 2021
7f5330c
Revert removal of setup API call
kpollich Nov 4, 2021
b377871
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 5, 2021
539e6a3
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 8, 2021
ee899c3
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 8, 2021
28c6fbf
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 8, 2021
252cf54
Restructor fleetSetupCompleted promise
kpollich Nov 8, 2021
b5d900b
Add logging + handle setup failures
kpollich Nov 9, 2021
6c2f05e
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 9, 2021
1fc6f01
Restructure logging to mix of debug/info
kpollich Nov 9, 2021
06afb9e
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 9, 2021
c850ae8
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 9, 2021
23c4ac1
Maybe fix failing tests
kpollich Nov 9, 2021
8fe61e2
Try fixing tests again
kpollich Nov 10, 2021
e135fdd
Fix another dashboard test
kpollich Nov 10, 2021
6311892
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 10, 2021
4c2e113
Re-add output logs after merge
kpollich Nov 10, 2021
0682d83
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 10, 2021
162dbd6
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 11, 2021
3c019ba
Log non-fatal errors during Fleet setup on boot
kpollich Nov 11, 2021
8e2e1be
Don't rely on fleetSetupCompleted to be called
kpollich Nov 11, 2021
ede4a18
Fix failing test
kpollich Nov 11, 2021
95475d4
Track fleet setup status to avoid double calls
kpollich Nov 11, 2021
f1f9fc6
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 11, 2021
c937d62
Use IIFE in place of Promise ctor
kpollich Nov 11, 2021
76e4829
Remove unnecessary fleetSetupStatus value
kpollich Nov 11, 2021
1c4a465
Merge branch 'main' into 111858-fleet-setup-on-boot
kpollich Nov 15, 2021
76138ec
Move non-error logs into setupFleet method
kpollich Nov 15, 2021
59190ba
Remove unused formatNonFatalErrors import
kpollich Nov 15, 2021
697f8be
Merge branch 'main' into 111858-fleet-setup-on-boot
kibanamachine Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions x-pack/plugins/fleet/public/applications/fleet/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
FleetStatusProvider,
KibanaVersionContext,
sendGetPermissionsCheck,
sendSetup,
useBreadcrumbs,
useStartServices,
UIExtensionsContext,
Expand Down Expand Up @@ -141,21 +140,6 @@ export const WithPermissionsAndSetup: React.FC = memo(({ children }) => {
const permissionsResponse = await sendGetPermissionsCheck();
setIsPermissionsLoading(false);
if (permissionsResponse.data?.success) {
try {
const setupResponse = await sendSetup();
if (setupResponse.error) {
setInitializationError(setupResponse.error);
}
if (setupResponse.data?.nonFatalErrors?.length) {
notifications.toasts.addError(setupResponse.data.nonFatalErrors[0], {
title: i18n.translate('xpack.fleet.setup.uiPreconfigurationErrorTitle', {
defaultMessage: 'Configuration error',
}),
});
}
} catch (err) {
setInitializationError(err);
}
kpollich marked this conversation as resolved.
Show resolved Hide resolved
setIsInitialized(true);
} else {
setPermissionsError(permissionsResponse.data?.error || 'REQUEST_ERROR');
Expand Down
20 changes: 15 additions & 5 deletions x-pack/plugins/fleet/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import type { Observable } from 'rxjs';
import { first } from 'rxjs/operators';
import type {
CoreSetup,
CoreStart,
Expand All @@ -20,7 +21,7 @@ import type { UsageCollectionSetup } from 'src/plugins/usage_collection/server';

import type { TelemetryPluginSetup, TelemetryPluginStart } from 'src/plugins/telemetry/server';

import { DEFAULT_APP_CATEGORIES } from '../../../../src/core/server';
import { DEFAULT_APP_CATEGORIES, SavedObjectsClient } from '../../../../src/core/server';
import type { PluginStart as DataPluginStart } from '../../../../src/plugins/data/server';
import type { LicensingPluginSetup, ILicense } from '../../licensing/server';
import type {
Expand Down Expand Up @@ -86,6 +87,7 @@ import { startFleetServerSetup } from './services/fleet_server';
import { FleetArtifactsClient } from './services/artifacts';
import type { FleetRouter } from './types/request_context';
import { TelemetryEventsSender } from './telemetry/sender';
import { setupFleet } from './services/setup';

export interface FleetSetupDeps {
licensing: LicensingPluginSetup;
Expand Down Expand Up @@ -335,14 +337,22 @@ export class FleetPlugin
});
licenseService.start(this.licensing$);

const fleetServerSetup = startFleetServerSetup();

this.telemetryEventsSender.start(plugins.telemetry, core);

return {
fleetSetupCompleted: () =>
new Promise<void>((resolve) => {
Promise.all([fleetServerSetup]).finally(() => resolve());
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 •
edited
Loading

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:

kibana/x-pack/plugins/security_solution/server/plugin.ts

Lines 368 to 381 in 828a579

// 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.

}),
esIndexPatternService: new ESIndexPatternSavedObjectService(),
packageService: {
Expand Down