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

[Security Solution][Ingest Manager][Endpoint] Optional ingest manager #71198

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

madirey
Copy link
Contributor

@madirey madirey commented Jul 9, 2020

Summary

Makes ingestManager an optional dependency for securitySolution.

Ingest entirely disabled:
image

image

Ingest enabled:
image

image

Checklist

N/A

For maintainers

@madirey madirey added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Response Endpoint Response Team Team:Endpoint Management Feature:Endpoint Elastic Endpoint feature v7.9.0 labels Jul 9, 2020
@madirey madirey requested review from a team as code owners July 9, 2020 05:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-management (Team:Endpoint Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-response (Team:Endpoint Response)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@kevinlog
Copy link
Contributor

kevinlog commented Jul 9, 2020

@elasticmachine merge upstream

@paul-tavares
Copy link
Contributor

👀

'endpoint',
ConfigureEndpointPackageConfig
);
if (plugins.ingestManager) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem with tests failing without this conditional should have been caught by typescript. Is it because we should have have also set the ingestManager StartPlugins type as optional? https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/types.ts#L35

@paul-tavares @XavierM @kevinlog thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@peluja1012 yeah, that sounds like it needs to be set to optional in the Type

@paul-tavares
Copy link
Contributor

FYI - #70911
This PR will enable us to set Ingest Manager as optional without having to make additional code changes to avoid loading code that imports ingest manager code.

@madirey
Copy link
Contributor Author

madirey commented Jul 9, 2020

Also related: feb1202

@paul-tavares
Copy link
Contributor

This PR should be held until @spalger merges #70911 . I just applied these changes ontop of his PR by adding ingestManager to the new Plugin kibana.json requiredBundles and we get the expected results.

@kevinlog
Copy link
Contributor

@elasticmachine merge upstream

@@ -33,6 +33,7 @@
"requiredBundles": [
"kibanaUtils",
"esUiShared",
"kibanaReact"
"kibanaReact",
"ingestManager"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a new feature from kibana platform, see #70911

This allows us to still load the shared code that we import from ingestManager without the plugin actually being loaded. So, since "ingestManager" is optional we can either load it or not and the Administration section will handle accordingly.

There are existing tests that already checked the behavior of the Admin section with or without ingestManager either within Space or if the plugin is loaded at all.

@kevinlog
Copy link
Contributor

@madirey @paul-tavares @alexk307 @peluja1012 I believe this one is ready to go. The kibana platform PR is in #70911

I pulled it down, tested it - Admin and other Security pages are working as expected. There are already tests in place to check the Admin pages when Ingest is disabled.

FYI @XavierM

@kevinlog
Copy link
Contributor

Something else we can add here is a suite that doesn't enable ingestManager and, at the least, run through the Security tabs to ensure they still load and perhaps smoke test some basic functionality. I did this manually, but it would be good to have that automated as well, especially in the future as Ingest and Endpoint Admin develop further.

I put this up as a quick experiment with taking the ingestManager requirement out of the Security Cypress tests and it seems to work. #71370

My thought is that we could set up a similar suite and take this out of the config -https://github.com/elastic/kibana/pull/71370/files#diff-502bf387cf33d051febd25bc823df26dL49

Wondering what some thoughts are on approaches - @paul-tavares @XavierM ?

@madirey
Copy link
Contributor Author

madirey commented Jul 10, 2020

@kevinlog I know you're referring to UI functional tests, but just want to point out that we have something similar for the server-side APIs: https://github.com/elastic/kibana/tree/master/x-pack/test/endpoint_api_integration_no_ingest

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@kevinlog kevinlog merged commit e1253ed into elastic:master Jul 10, 2020
@kevinlog kevinlog deleted the optional-ingest-manager branch July 10, 2020 22:23
kevinlog added a commit that referenced this pull request Jul 11, 2020
…#71198) (#71380)

Co-authored-by: Madison Caldwell <madison.rey.caldwell@gmail.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 13, 2020
* master: (78 commits)
  Bump lodash package version (elastic#71392)
  refactor: 💡 use allow-list in AppArch codebase (elastic#71400)
  improve bugfix 7198 test stability (elastic#71250)
  [Security Solution][Ingest Manager][Endpoint] Optional ingest manager (elastic#71198)
  [Metrics UI] Round metric threshold time buckets to nearest unit (elastic#71172)
  [Security Solution][Endpoint] Policy creation callback fixes + Improved error handling in user manifest loop (elastic#71269)
  [Security Solution] Allow to configure Event Renderers settings (elastic#69693)
  Fix a11y keyboard overlay (elastic#71214)
  [APM] UI text updates (elastic#71333)
  [Logs UI] Limit `extendDatemath` to valid ranges (elastic#71113)
  [SIEM] fix tooltip of notes (elastic#71342)
  address index templates feedback (elastic#71353)
  Upgrade EUI to v26.3.1 (elastic#70243)
  [build] Creates Linux aarch64 archive (elastic#69165)
  [SIEM][Detection Engine] Fixes skipped tests (elastic#71347)
  [SIEM][Detection Engine][Lists] Adds read_privileges route for lists and list items
  [kbn/optimizer] implement "requiredBundles" property of KP plugins (elastic#70911)
  [Security Solution][Exceptions] - Exceptions modal pt 2 (elastic#70886)
  [ML] DF Analytics:  stop status polling when job stopped (elastic#71159)
  [SIEM][CASE] IBM Resilient Connector (elastic#66385)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Response Endpoint Response Team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants