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

Jump to bottom

[Ingest Manager] Enable ingest manager plugin by default. #70955

Merged
merged 7 commits into from
Jul 14, 2020
Merged

[Ingest Manager] Enable ingest manager plugin by default. #70955

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97c5c44
[Ingest Manager] enable the plugin by default
nchaulet Jul 7, 2020
a4bcf99
Fix ingest manager setup for non superuser
nchaulet Jul 7, 2020
af87915
Merge branch 'master' into feature-enable-ingest-manager-by-default
elasticmachine Jul 13, 2020
31c3462
Lazy ingestManager setup
nchaulet Jul 13, 2020
7a8e48c
Rename after code review
nchaulet Jul 14, 2020
dcb769f
Merge branch 'master' of github.com:elastic/kibana into feature-enabl…
nchaulet Jul 14, 2020
ec5b11a
Rename after code review
nchaulet Jul 14, 2020
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
45 changes: 25 additions & 20 deletions x-pack/plugins/ingest_manager/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export interface IngestManagerSetup {}
*/
export interface IngestManagerStart {
registerPackageConfigComponent: typeof registerPackageConfigComponent;
success: Promise<true>;
waitForInitialization: () => Promise<true>;
nchaulet marked this conversation as resolved.
Show resolved Hide resolved
}

export interface IngestManagerSetupDeps {
Expand Down Expand Up @@ -100,27 +100,32 @@ export class IngestManagerPlugin
}

public async start(core: CoreStart): Promise<IngestManagerStart> {
let successPromise: IngestManagerStart['success'];
try {
const permissionsResponse = await core.http.get<CheckPermissionsResponse>(
appRoutesService.getCheckPermissionsPath()
);

if (permissionsResponse?.success) {
successPromise = core.http
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving from eager/guaranteed to lazy/optional is an important change and we should be on the watch for effects elsewhere.

Until now all the various services could be sure that the plugin setup had at least been started, if not completed, before the other services are used. This meant that some may rely on implied side-effects (default packages, templates, etc) or even race conditions instead of defining those dependencies.

When/if we find them we can add code which uses the global promise or more fine-grained checks, but I don't think we know where those changes will happen right now.

Copy link
Member Author

@nchaulet nchaulet Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Right now I think the only two consumer of this are the ingest manager plugin and endpoint plugin, it should work ( @elastic/endpoint-management could you confirm it's working for you as expected? ).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not just about changing the shape returned from start. We are significantly changing the behavior. We cannot know for sure what services depend on setup having been called already, but we do know for sure that it was previously guaranteed to be and now it is not.

It not a deal breaker, but it also not insignificant not easily known (e.g statically analyzable)

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 agree the change is significant. But also consumer of ingest manager should probably use isInitialized to know if the plugin is correctly initialized, this will allow to statically know every consumers.

.post<PostIngestSetupResponse>(setupRouteService.getSetupPath())
.then(({ isInitialized }) =>
isInitialized ? Promise.resolve(true) : Promise.reject(new Error('Unknown setup error'))
);
} else {
throw new Error(permissionsResponse?.error || 'Unknown permissions error');
}
} catch (error) {
successPromise = Promise.reject(error);
}
let successPromise: ReturnType<IngestManagerStart['waitForInitialization']>;

return {
success: successPromise,
waitForInitialization: () => {
if (!successPromise) {
successPromise = Promise.resolve().then(async () => {
const permissionsResponse = await core.http.get<CheckPermissionsResponse>(
appRoutesService.getCheckPermissionsPath()
);

if (permissionsResponse?.success) {
return core.http
.post<PostIngestSetupResponse>(setupRouteService.getSetupPath())
.then(({ isInitialized }) =>
isInitialized
? Promise.resolve(true)
: Promise.reject(new Error('Unknown setup error'))
);
} else {
throw new Error(permissionsResponse?.error || 'Unknown permissions error');
}
});
}

return successPromise;
},
registerPackageConfigComponent,
};
}
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugins/security_solution/public/app/home/setup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ export const Setup: React.FunctionComponent<{
});
};

ingestManager.success.catch((error: Error) => displayToastWithModal(error.message));
ingestManager
.waitForInitialization()
.catch((error: Error) => displayToastWithModal(error.message));
}, [ingestManager, notifications.toasts]);

return null;
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/security_solution/public/common/mock/endpoint/dependencies_start_mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export const depsStartMock: () => DepsStartMock = () => {

return {
data: dataMock,
ingestManager: { success: Promise.resolve(true), registerPackageConfigComponent },
ingestManager: {
waitForInitialization: () => Promise.resolve(true),
registerPackageConfigComponent,
},
};
};