[Security Solution][Endpoint] Add event filters summary card to the fleet endpoint tab #100668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dasansol92
added
auto-backport
dasansol92
changed the title
dasansol92
changed the title
xcrzx
reviewed
May 26, 2021
...bn-securitysolution-io-ts-list-types/src/request/summary_exception_list_schema/index.test.ts
Outdated
Show resolved
Hide resolved
xcrzx
requested changes
May 26, 2021
x-pack/plugins/lists/server/services/exception_lists/get_exception_list_summary.ts
Outdated
Show resolved
Hide resolved
...manager_integration/endpoint_package_custom_extension/components/fleet_trusted_apps_card.tsx
Outdated
Show resolved
Hide resolved
...anager_integration/endpoint_package_custom_extension/components/fleet_event_filters_card.tsx
Outdated
Show resolved
Hide resolved
|
One thing with routes is we typically add structural/basic e2e tests for any new routes here: You can see a lot of examples such as this one: I would add one file to exercise the summary route |
| */ | ||
|
|
||
| import * as t from 'io-ts'; | ||
| import { _versionOrUndefined } from '../../common/underscore_version'; |
There was a problem hiding this comment.
This looks unused here.
| windows: t.number, | ||
| linux: t.number, | ||
| macos: t.number, | ||
| total: t.number, |
There was a problem hiding this comment.
If these are positive numbers, we have a type of PositiveInteger:
https://github.com/elastic/kibana/blob/master/packages/kbn-securitysolution-io-ts-types/src/positive_integer/index.ts
If you want to ensure these are always positive numbers.
| options: { | ||
| tags: ['access:lists-summary'], | ||
| }, | ||
| path: `${EXCEPTION_LIST_URL}/_summary`, |
There was a problem hiding this comment.
The reason we do the underscore such as in some cases _import is to mimic what elastic and Kibana does when it is action oriented. If you're just getting a summary of something and feel it is not an action that is being performed you can use just a normal summary without an underscore.
This looks fine with an underscore as is, though. It's kind of a grey area of when to use it vs. not use it in some cases. Just pointing out when/why I try to use it. I don't think there are hard/fast rules on it.
There was a problem hiding this comment.
That make sense to me, I'll change it, thanks! 👍
| statusCode: 404, | ||
| }); | ||
| } else { | ||
| return response.ok({ body: exceptionListSummary ?? {} }); |
There was a problem hiding this comment.
You can optionally validate responses too if that makes you feel like you will have a better chance of keeping your code working long term. We have some examples like this line here:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lists/server/routes/delete_list_item_route.ts#L47
Basically, if you define both a schema for request and one for response, you can validate both the request and the response. You can do a "strict" check or a looser non-strict check for the response. Usually I try to validate both request and response using strict checks like that above example and then I write an e2e test that exercises my route.
It makes it to where if someone changes the contract by accident by say adding an extra key/value on the response, it would fail in the validation logic during the e2e test and prevent them from checking in until they fixed the response validation schema.
There was a problem hiding this comment.
Got it! I will add those validations and also add a new e2e test! Thanks
| }: GetExceptionListSummaryOptions): Promise<ExceptionListSummarySchema | null> => { | ||
| const { savedObjectsClient } = this; | ||
| return getExceptionListSummary({ id, listId, namespaceType, savedObjectsClient }); | ||
| }; |
There was a problem hiding this comment.
++ 👍 , thanks for adding this plumbing here. Appreciate it.
…core, adding aggs type, validating response, and more
xcrzx
approved these changes
May 27, 2021
...anager_integration/endpoint_package_custom_extension/components/fleet_event_filters_card.tsx
Outdated
Show resolved
Hide resolved
|
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
| <ExceptionItemsSummary stats={stats} /> | ||
| </EuiFlexItem> | ||
| <EuiFlexItem grow={false}> | ||
| <span> |
There was a problem hiding this comment.
You should be able to use <> and </> here instead of <span> to reduce weight unless I'm missing something. Looks like you're just adding the <span> here to get two children? If so, React fragments short version are better.
There was a problem hiding this comment.
Yeah! You are totally right. This code was copy pasted from an old one. I'll change it since this span tag is not necessary.
| const response = await trustedAppsApi.getTrustedAppsSummary(); | ||
| setStats(response); | ||
| } catch (error) { | ||
| toasts.addDanger( |
There was a problem hiding this comment.
fwiw, there is another toaster a lot of people use where we don't push down the class into the dependencies and if you want to use an error toaster instead of a danger toaster you can with it and it will add any network errors to the "See more" button so that if the user clicks it they can copy and paste the network error into the forums.
Not required to use it, but it's there if you want to:
x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts
There's a lot of use cases of it across the application. It does a more in-depth job with error objects when it encounters them and tries to figure them out and adds the "show more" button. It's been very helpful for us on forum questions and issues.
The way this is written, when you encounter values: { error } and the translation sees that error object in the message of "{error}" I assume it just does a .toString() which would give less info compared to using the one mentioned above. Plus we do not have another translation key to add to the stack.
Not required, but if not, I would consider maybe using addError instead of addDanger here? Seems like most of the places we use addError which would make more sense? Unless I'm outdated and we're moving everything to addDanger ;-) which maybe we are?
There was a problem hiding this comment.
That sounds good to me! I'm ok to do this change, maybe @paul-tavares has some thoughts?
There was a problem hiding this comment.
Ohhhh - this is good to know @FrankHassanabad. I just recently mentioned to our team that I really needed to take some time and look around in our "common" folders to see whats available _(this after I found out about our class KibanaServices hook in public/common/lib/kibana/services). This useAppToasts() sounds like something our team should def. use (@elastic/security-onboarding-and-lifecycle-mgt FYI ^^)
That being said, what you suggest here might not work out (I think I tried at one point to use our useKibana() hook). This particular component here is actually rendered within Fleet's code (we register a UI extension that load this async and then renders it). I also fear that if we do use allot of the "common" stuff from security solution, we might pull "allot" and increase the size of the component's bundle that fleet code would have to download. For the most part, we try to keep these Fleet UI extensions to use most of the built in kibana services.
| @@ -63,8 +91,8 @@ export const FleetTrustedAppsCard = memo<PackageCustomExtensionComponentProps>(( | |||
| </h4> | |||
| </EuiText> | |||
| </EuiFlexItem> | |||
| <EuiFlexItem grow={false}> | |||
| <TrustedAppItemsSummary /> | |||
| <EuiFlexItem style={{ alignItems: 'center', justifyContent: 'center' }}> | |||
There was a problem hiding this comment.
I would strongly recommend to reaching into the style overrides as much as possible. This bites maintainers and developers in the long run almost always. I have seen a lot of bad things with it.
For example, this making an assumption that EuiFlexItem's implementation will be a flex group or compatible and makes it tricker if we get regressions or issues if the implementation changes later.
I would instead try to use another EuiFlexGroup inner group here and style it using its regular attributes that Eui has given us through their docs:
https://elastic.github.io/eui/#/layout/flex
They also have a grid component if a grid makes more sense here fwiw.
| await deleteListsIndex(supertest); | ||
| await deleteAllExceptions(es); | ||
| }); | ||
| beforeEach(async () => { |
There was a problem hiding this comment.
nit: Almost always beforeEach is before afterEach for readability
| }); | ||
|
|
||
| it('should return init summary when there are no items created', async () => { | ||
| const { body } = await supertest |
There was a problem hiding this comment.
Although body is an any here, if you have a response schema, you can add it as a cast here and then later your typescript system will blow up if someone breaks your types.
Just a FYI, if you don't have a response schema/response types you cannot do this.
paul-tavares
approved these changes
May 27, 2021
| .send() | ||
| .expect(200); | ||
|
|
||
| expect(body).to.eql({ |
There was a problem hiding this comment.
nit: A pattern for types we sometimes use is that we will do something like this where FooType is your response type:
const expected: FooType = {
linux: 0,
macos: 0,
total: 0,
windows: 0
};
expect(body).to.eql(expected);The reason is that now you have your expected response typed and if your response type changes your typescript will fail with errors before the test is even run and you know where to fix it. It's not required but super helpful and we usually refactor code into this pattern when we can and see it in other tests. It's just really helpful.
FrankHassanabad
approved these changes
May 27, 2021
…d small improvements on test -> ts
|
@elasticmachine merge upstream |
…_to_the_fleet_endpoint_tab-1157
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/spaces/spaces_selection·ts.Spaces app Spaces Spaces Data displays separate data for each space in the default spaceStandard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/spaces/spaces_selection·ts.Spaces app Spaces Spaces Data "after all" hook in "Spaces Data"Standard OutStack TraceMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
May 28, 2021
…leet endpoint tab (elastic#100668) * Shows event filters card on fleet page * Uses aggs instead of while loop to retrieve summary data * Add request and response types in the lists package * Fixes old import * Removes old i18n keys * Removes more old i18n keys * Use consts for exception lists url and endpoint event filter list id * Uses event filters service to retrieve summary data * Fixes addressed pr comments such as changing the route without underscore, adding aggs type, validating response, and more * Uses useMemo instead of useState to memoize object * Add new e2e test for summart endpoint * Handle api errors on event filters and trusted apps summary api calls * Add api error message to the toast * Fix wrong i18n key * Change span tag by react fragment * Uses styled components instead of modify compontent style directly and small improvements on test -> ts * Adds curls script for summary route Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
May 28, 2021
…leet endpoint tab (#100668) (#100915) * Shows event filters card on fleet page * Uses aggs instead of while loop to retrieve summary data * Add request and response types in the lists package * Fixes old import * Removes old i18n keys * Removes more old i18n keys * Use consts for exception lists url and endpoint event filter list id * Uses event filters service to retrieve summary data * Fixes addressed pr comments such as changing the route without underscore, adding aggs type, validating response, and more * Uses useMemo instead of useState to memoize object * Add new e2e test for summart endpoint * Handle api errors on event filters and trusted apps summary api calls * Add api error message to the toast * Fix wrong i18n key * Change span tag by react fragment * Uses styled components instead of modify compontent style directly and small improvements on test -> ts * Adds curls script for summary route Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: David Sánchez <[email protected]>
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
May 28, 2021
* master: (77 commits) [RAC][Security Solution] Register Security Detection Rules with Rule Registry (elastic#96015) [Enterprise Search] Log warning for Kibana/EntSearch version mismatches (elastic#100809) updating the saved objects test to include more saved object types (elastic#100828) [ML] Fix categorization job view examples link when datafeed uses multiple indices (elastic#100789) Fixing ES archive mapping failure (elastic#100835) Fix bug with Observability > APM header navigation (elastic#100845) [Security Solution][Endpoint] Add event filters summary card to the fleet endpoint tab (elastic#100668) [Actions] Taking space id into account when creating email footer link (elastic#100734) Ensure comments on parameters in arrow functions are captured in the docs and ci metrics. (elastic#100823) [Security Solution] Improve find rule and find rule status route performance (elastic#99678) [DOCS] Adds video to introduction (elastic#100906) [Fleet] Improve combo box for fleet settings (elastic#100603) [Security Solution][Endpoint] Endpoint generator and data loader support for Host Isolation (elastic#100813) [DOCS] Adds Lens video (elastic#100898) [TSVB] [Table tab] Fix "Math" aggregation (elastic#100765) chore(NA): moving @kbn/io-ts-utils into bazel (elastic#100810) [Alerting] Adding feature flag for enabling/disabling rule import and export (elastic#100718) [TSVB] Fix Upgrading from 7.12.1 to 7.13.0 breaks TSVB (elastic#100864) [Lens] Adds dynamic table cell coloring (elastic#95217) [Security Solution][Endpoint] Do not display searchbar in security-trusted apps if there are no items (elastic#100853) ...
Merged
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pr shows a new card on the fleet page with the event filters stats like the trusted apps one.
Items summary component has been changed to be used by event filter and trusted apps card.
Now, api calls are done in the card itself and the responses are passed as props to the summary component.
A new route on the lists plugin has been added to retrieve the summary of an exception list.
Also new types for this route have been added to the
kbn-securitysolution-io-ts-list-typespackage.Screenshots
For maintainers