[SIEM] Implement NP Plugin Setup #54030
Conversation
* Defines the setup() method for our UI plugin * Renders the app in the NP way within our setup() method * Defines a legacy file that invokes the plugin manually Things seem to be mostly working; the app mounts with no immediate errors, at least.
Our plugin function and class are both direct children of siem/public. The app folder contains both our React app and the function to render it.
Unfortunately, this can't live in the plugin for now because it doesn't get invoked when we need it. For now, it's going to live in the same spot, and once we're a real NP plugin we can move it.
This seems to be redundant with dateFormat:tz except that it always returns a real timezone, not just a preference. By wrapping that logic in our own hook, useTimeZone, we can remove this weird usage and stick to the standard dateFormat and dateFormat:tz.
Mocks our simpler wrapping hooks rather than the entire UI Settings module.
These remaining tests can mock settings directly, or otherwise were misusing the settings mocks to retrieve assertion values.
They were not adding any information to the tests.
We were previously passing this version all over the place for the sake of our framework-specific request header. The sole advantage of supplying such a header is that the client will receive an informative error modal in the case of a version mismatch between the client and server. We can successfully perform these requests with the `kbn-xsrf` header instead. Long-term, we can use core.http.fetch to perform the requests and auto-populate the version header, but it would be nicer to abstract those requests to the framework level rather than threading the HTTP client throughout the application.
These happened on master in the meantime.
rylnd
added
Team:SIEM
release_note:skip
|
Pinging @elastic/siem (Team:SIEM) |
|
Update: Since SIEM is broken on master we're going to merge this (with versionless headers) as is. I'm going to come back through this afternoon and replace these calls with equivalents from |
| @@ -43,7 +43,7 @@ export const siem = (kibana: any) => { | |||
| description: i18n.translate('xpack.siem.securityDescription', { | |||
| defaultMessage: 'Explore your SIEM App', | |||
| }), | |||
| main: 'plugins/siem/app', | |||
| main: 'plugins/siem/legacy', | |||
There was a problem hiding this comment.
FWIW, this was mimicked from:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/infra/index.ts#L36
Looking at others though they have legacy in their name now such as:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/maps/index.js
So this looks 👍 to me.
| return ( | ||
| <>{bytesFormat ? numeral(value).format(bytesFormat) : numeral(value).format('0,0.[0]b')}</> | ||
| ); | ||
| return <>{numeral(value).format(bytesFormat || '0,0.[0]b')}</>; |
There was a problem hiding this comment.
I would remove the double pipe or check || as it looks like it is not needed since you are passing in DEFAULT_BYTES_FORMAT?
There was a problem hiding this comment.
Looks like we don't ever check for null/undefined in other places:
https://github.com/elastic/kibana/blob/master/x-pack/legacy/plugins/siem/public/pages/detection_engine/components/signals/signals_utility_bar/index.tsx#L48
So I am assuming it is safe to remove.
There was a problem hiding this comment.
There is an edge case where the user has explicitly unset that setting: they would get raw values with no (manual) formatting. In practice this doesn't look great, but it's not terrible either:
I can totally get behind the 'do what I specify in the settings' behavior and remove that guard, but I wanted to mention this just in case.
x-pack/legacy/plugins/siem/public/components/formatted_date/index.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/public/components/formatted_date/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/siem/public/components/ml/api/anomalies_table_data.ts
Show resolved
Hide resolved
|
In the file: x-pack/legacy/plugins/siem/server/lib/framework/kibana_framework_adapter.tsDo you want to remove the kbn-version? passHeader: `'kbn-version': '${this.version}'`,or leave it? Pointing it out in case you do not want that anywhere. |
|
In this file: x-pack/legacy/plugins/siem/public/lib/compose/helpers.tsThere is this one headers: {
'kbn-xsrf': chrome.getXsrfToken(),
},Don't know if that matters but pointing it out in case you want to consistently set them to all be |
| return ( | ||
| <>{bytesFormat ? numeral(value).format(bytesFormat) : numeral(value).format('0,0.[0]b')}</> | ||
| ); | ||
| return <>{numeral(value).format(bytesFormat || '0,0.[0]b')}</>; |
There was a problem hiding this comment.
nit, can we create a const for '0,0.[0]b' since we are also using it there https://github.com/elastic/kibana/pull/54030/files#diff-5839ee2c63a46315927b26809c783ec7R22
There was a problem hiding this comment.
@XavierM I think we're actually going to remove this particular usage. I just noticed we're using it here as well, which I'm going to inline to pull from UI settings as well. (cc @angorayc)
That just leaves references in the tests. I'm happy to declare a const DEFAULT_BYTES_FORMAT_VALUE = '0,0.[0]b' at the top of the test file for clarity. Does that work?
There was a problem hiding this comment.
|
|
||
| describe('formatted_date', () => { | ||
| let isoDate: Date; | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
do we really need to do that in the beforeEach since it seems just to be static
There was a problem hiding this comment.
Probably not, although it does prevent mutations to isoDate from affecting other tests. I've been bit by that a few times, so I typically do test setup like this. Happy to change it if you feel strongly!
x-pack/legacy/plugins/siem/public/components/ml/api/get_ml_capabilities.ts
Show resolved
Hide resolved
Allows us to change the implementation of the empty string without breaking the test.
We're always going to get back usable values from these hooks; while the user can unset the dateFormat in their settings, we'll still get an empty string which is effectively the same as no formatting (as evidenced in the tests).
If the user has deleted this default, they presumably meant to do so and we shouldn't supersede it.
There was a problem hiding this comment.
Checked out and tested locally using both an admin and read-only user and everything looks good from my end. No more errors in the console, and each page/feature is functioning as expected. Code changes look good as well and no additional changes necessary outside what @FrankHassanabad and @XavierM have already commented on.
Thanks for these changes/fixes @rylnd! One step closer to the NP, and we have a plan for supplying kbn-version as well. This is good stuff! 🙂 LGTM! 🎉 🚀
We need a formatting function to use with our charts, so this splits out a hook from the original react component, allowing our charts to be formatted as specified in the user's UI settings.
This forces accidental changes to the return value to be explicit.
This is an unnecessary use: kibana works the same no matter what contents the `kbn-xsrf` header contains (as long as it's there).
|
Responded to all the PR comments, thanks everyone for your help! I think this is good to merge once CI's happy, but for reviewers, one thing changed that I could use feedback on: referencing the byte format setting in our DNS Histogram chart: e071875 |
When using our TestProvider components, we were previously relying on platform's UISettings mocks instead of our own, more comprehensive ones. This worked for the most part, and when we needed real settings we would mock the UI Settings client manually. When we removed some app code that defaulted UI Settings values when the client did not return a value, tests that used TestProviders but also relied on those defaults broke. This adds that behavior back, and obviates the need for manual calls to jest.mock except when we're a) not using TestProviders but b) overriding the platform mocks. Also removes some of those unneeded uses.
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (55 commits) [ui/public/utils] Copy rarely used items to where they are consumed (elastic#53819) set AppArch team as an owner of the search endpoints (elastic#54131) Don't expose Elasticsearch client as Observable (elastic#53824) [SIEM] Cleanup unnecessary use of enzyme-to-json (elastic#53980) fix ui exports doc (elastic#54138) change markdown element title (elastic#54194) [Logs UI] Refactor log position to hooks (elastic#53540) [SIEM] Implement NP Plugin Setup (elastic#54030) [DOCS] Updates ML links (elastic#53613) sort renovate packages in config Spaces - fix flakey api tests (elastic#54154) Remove dependency that was causing effect to re-execute infinitely. (elastic#54160) [dev/run] expose unexpected flags as more than just names (elastic#54080) [DOCS] Moves index pattern doc to Discover (elastic#53347) [SIEM] Cleanup React imports (elastic#53981) Update eslint related packages (elastic#54107) [Uptime] Added date range filter into expanded list query (elastic#52609) [SIEM] Add react/display-name eslint rule (elastic#53107) [SIEM] Enable eslint prefer-template rule (elastic#53983) Elasticsearch snapshots automation (elastic#53706) ...
* Set up our react app in the NP way * Defines the setup() method for our UI plugin * Renders the app in the NP way within our setup() method * Defines a legacy file that invokes the plugin manually Things seem to be mostly working; the app mounts with no immediate errors, at least. * Move files into NP structure Our plugin function and class are both direct children of siem/public. The app folder contains both our React app and the function to render it. * Register SIEM in the feature catalogue via NP format Unfortunately, this can't live in the plugin for now because it doesn't get invoked when we need it. For now, it's going to live in the same spot, and once we're a real NP plugin we can move it. * Eliminate usage of timezoneBrowser UI setting This seems to be redundant with dateFormat:tz except that it always returns a real timezone, not just a preference. By wrapping that logic in our own hook, useTimeZone, we can remove this weird usage and stick to the standard dateFormat and dateFormat:tz. * Clean up tests for FormattedDate components Mocks our simpler wrapping hooks rather than the entire UI Settings module. * Remove remaining uses of UI Settings mocks These remaining tests can mock settings directly, or otherwise were misusing the settings mocks to retrieve assertion values. * Remove unnecessary intermediate `describe` blocks They were not adding any information to the tests. * Remove use of kibana version in client requests We were previously passing this version all over the place for the sake of our framework-specific request header. The sole advantage of supplying such a header is that the client will receive an informative error modal in the case of a version mismatch between the client and server. We can successfully perform these requests with the `kbn-xsrf` header instead. Long-term, we can use core.http.fetch to perform the requests and auto-populate the version header, but it would be nicer to abstract those requests to the framework level rather than threading the HTTP client throughout the application. * Remove newly added uses of kbnVersion These happened on master in the meantime. * Use helper to generate test assertion Allows us to change the implementation of the empty string without breaking the test. * Remove guard from date formatting component We're always going to get back usable values from these hooks; while the user can unset the dateFormat in their settings, we'll still get an empty string which is effectively the same as no formatting (as evidenced in the tests). * Remove default from byte formatting component If the user has deleted this default, they presumably meant to do so and we shouldn't supersede it. * Refactor bytes formatting to allow use in our charts We need a formatting function to use with our charts, so this splits out a hook from the original react component, allowing our charts to be formatted as specified in the user's UI settings. * Refer to our constant for APP_ID * Explicit return values for some UI Settings hooks This forces accidental changes to the return value to be explicit. * Remove use of ui/chrome in request header This is an unnecessary use: kibana works the same no matter what contents the `kbn-xsrf` header contains (as long as it's there). * Mock UI Settings values in our TestProvider When using our TestProvider components, we were previously relying on platform's UISettings mocks instead of our own, more comprehensive ones. This worked for the most part, and when we needed real settings we would mock the UI Settings client manually. When we removed some app code that defaulted UI Settings values when the client did not return a value, tests that used TestProviders but also relied on those defaults broke. This adds that behavior back, and obviates the need for manual calls to jest.mock except when we're a) not using TestProviders but b) overriding the platform mocks. Also removes some of those unneeded uses. * Remove unused import Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
This PR starts massaging our frontend code into the format that NP is expecting (both files and structure, e.g.
renderApp). This adds a newsetupmethod in which the application is registered; instartit will be invoked.This also migrates a service or two away from legacy stuff, and also does a little cleanup on our use of UI settings. There's more migration work to be done here, but I want to get this merged ASAP because of the proceeding issue.
Most importantly, though, this removes our use of the
kbn-versionheader in our http requests. As of my last PR it was hardcoded, which was never a permanent solution but is already causing errors. The impact is that the client would no longer receive a helpful error in the case of a client/server mismatch. If this is unacceptable, we should discuss the best way to leverage thecore.httpclient instead, which adds both thekbn-versionheader automatically, and also obviates our need forchrome.getBasePath()which is on my short list of things to get rid of.For maintainers