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

merged 7 commits into from
Jul 14, 2020

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Jul 7, 2020

Summary

Resolve #70442

Enable ingest manager plugin by default.

Todo

  • Update ingest manager guide.

@nchaulet nchaulet added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team Ingest Management:beta1 labels Jul 7, 2020
@nchaulet nchaulet requested a review from a team July 7, 2020 13:21
@nchaulet nchaulet self-assigned this Jul 7, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@nchaulet nchaulet force-pushed the feature-enable-ingest-manager-by-default branch from 74757fc to 97c5c44 Compare July 7, 2020 13:51
@paul-tavares
Copy link
Contributor

paul-tavares commented Jul 7, 2020 •
edited
Loading

Never mind 🤦‍♂️
I think the moment you made it enabled by default, it automatically got picked up by that test Suite I reference (and perhaps thus the build failures).

Sorry for the delayed in getting my feedback to this PR.
I have some concerns around turning this Plugin in by default without also enabling it in the UI Functional tests for the configuration that runs most of the Kibana plugins (those from here: https://github.com/elastic/kibana/blob/94a18fda5d3a70dd7ee670dfd93105319935746f/x-pack/test/functional/config.js)

Currently I think that the only suite of functional tests that runs with Ingest Manager enabled is the Endpoint suite, but that only runs the tests cases for Enpoint (not even the full SIEM tests).

Should that be address with this PR?

@ph
Copy link
Contributor

ph commented Jul 8, 2020

@nchaulet can we wait on monday to merge this?

@nchaulet
Copy link
Member Author

nchaulet commented Jul 8, 2020

Yes tests are still not passing

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@jen-huang
Copy link
Contributor

Hi, sorry for hijacking this PR. I was wondering if we can consider doing #68658 as part of this PR too as it's also related to our config?

@nchaulet
Copy link
Member Author

@jen-huang Yes I will remove it 👍

@jen-huang
Copy link
Contributor

@nchaulet I thought about the epm.enabled flag some more and realized that we also have epm.registryUrl, which has the same confusing usage of "epm". I'll follow up in #68658 with my thoughts, but we should probably leave those changes out of this PR for now (no epm flag changes). Sorry for the confusion!

@nchaulet nchaulet requested review from a team as code owners July 13, 2020 18:57
jfsiii
jfsiii reviewed Jul 13, 2020
View reviewed changes
x-pack/plugins/ingest_manager/public/plugin.ts
);

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.

paul-tavares
paul-tavares approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for making the changes in endpoint.
I'm still wondering if we (endpoint) should explicitly call isInitialized() from both the UI and from our Functional tests - just to be safe.

From a UI standpoint, that would raise some UX questions as to how errors should be handled (personally, think a toast notification is not enough), but we can think more about this perhaps for 7.10

Also,
I wish this (initialization) could have been handled via the Plugin's API layer - example: by having some middleware (not sure platform supports it) that would detect that initialization was needed, and it would then be done prior to serving any API request.

@nchaulet
Copy link
Member Author

I'm still wondering if we (endpoint) should explicitly call isInitialized() from both the UI and from our Functional tests - just to be safe.

I think it will be a good improvements and it will make the contract between the two plugins more explicit

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@nchaulet nchaulet merged commit 561b5be into elastic:master Jul 14, 2020
@nchaulet nchaulet deleted the feature-enable-ingest-manager-by-default branch July 14, 2020 15:05
@nchaulet nchaulet mentioned this pull request Jul 14, 2020
Merged
nchaulet added a commit to nchaulet/kibana that referenced this pull request Jul 14, 2020
@nchaulet
[Ingest Manager] Enable ingest manager plugin by default. (elastic#70955
72ffd99 
)
nchaulet added a commit that referenced this pull request Jul 14, 2020
@nchaulet
[Ingest Manager] Enable ingest manager plugin by default. (#70955) (#…
77ba571 
…71669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfsiii jfsiii jfsiii left review comments

@jen-huang jen-huang jen-huang approved these changes

@paul-tavares paul-tavares paul-tavares approved these changes

Assignees

@nchaulet nchaulet

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Ingest Manager] Enable ingest manager by default.
7 participants
@nchaulet @elasticmachine @paul-tavares @ph @jen-huang @kibanamachine @jfsiii