-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Exceptions] - Moves remaining exceptions builder logic into lists plugin #95266
Changes from all commits
fc1f774
11e2a2e
db512ab
d9606b3
279e511
f43dfdc
213ef13
45597a9
b12ad8c
34ee433
05adb77
ba5e490
a578547
57d86d2
f55f1b8
e91d08b
d87f432
c951b53
47ea537
cb9a0ca
e4e1785
255419d
4a621cb
dfb5ce6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,7 @@ pageLoadAssetSize: | |
lens: 96624 | ||
licenseManagement: 41817 | ||
licensing: 29004 | ||
lists: 202261 | ||
lists: 228500 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this added 26,239 be removed from the securitySolution plugin defined here? I understand it's outside the scope of this PR, but I do want to advocate and ask that your team works to reduce these limits in the near future. At the very highest end, we believe these should be under 100k. The files reported here are served whenever Kibana is loaded, affecting the performance of Kibana as a whole. There are details about these metrics here and details on how to reduce the sizes here. Feel free to reach out if you have any questions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at ways to decrease size. So the example given in the link deals with lazy loading components on register. If there isn't a dedicated exceptions parent page that loads these components, where is the best place to ask that they be lazily loaded? The lists plugin currently does not register with core. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean by the lists plugin doesn't currently register with core. It's doing that here, which is why this bundle is being generated and included on every page refresh. How is this "lists" plugin currently being used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we can take this part of the discussion to a dedicated issue. Bringing up the overall bundle sizes is to bring awareness to this increasing problem, which we across Kibana have been trying to reduce for a while now. As far as this PR, if we have in-fact move logic from the |
||
logstash: 53548 | ||
management: 46112 | ||
maps: 80000 | ||
|
@@ -68,7 +68,7 @@ pageLoadAssetSize: | |
searchprofiler: 67080 | ||
security: 189428 | ||
securityOss: 30806 | ||
securitySolution: 283440 | ||
securitySolution: 235402 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tylersmalley Hey! Thanks for chatting with me earlier. I've updated the |
||
share: 99061 | ||
snapshotRestore: 79032 | ||
spaces: 387915 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,13 +35,13 @@ describe(`enumeratePatterns`, () => { | |
'src/plugins/charts/public/static/color_maps/color_maps.ts kibana-app' | ||
); | ||
}); | ||
it(`should resolve x-pack/plugins/security_solution/public/common/components/exceptions/builder/translations.ts to kibana-security`, () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too familiar with these tests but could/should this be replaced with an updated path to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @dplumlee This test was "unlucky" to randomly select path that you changed. I think you just need to update it with one you have now and we are good. But please don't remove it completely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was actually me. Thanks for taking a look @dmlemeshko . I'll update path and restore test. |
||
it(`should resolve x-pack/plugins/security_solution/public/common/components/exceptions/edit_exception_modal/translations.ts to kibana-security`, () => { | ||
const short = 'x-pack/plugins/security_solution'; | ||
const actual = enumeratePatterns(REPO_ROOT)(log)(new Map([[short, ['kibana-security']]])); | ||
|
||
expect( | ||
actual[0].includes( | ||
`${short}/public/common/components/exceptions/builder/translations.ts kibana-security` | ||
`${short}/public/common/components/exceptions/edit_exception_modal/translations.ts kibana-security` | ||
) | ||
).toBe(true); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,358 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0; you may not use this file except in compliance with the Elastic License | ||
* 2.0. | ||
*/ | ||
|
||
import { Story, addDecorator } from '@storybook/react'; | ||
import React from 'react'; | ||
import { ThemeProvider } from 'styled-components'; | ||
import euiLightVars from '@elastic/eui/dist/eui_theme_light.json'; | ||
import { HttpStart } from 'kibana/public'; | ||
|
||
import { AutocompleteStart } from '../../../../../../../src/plugins/data/public'; | ||
import { | ||
fields, | ||
getField, | ||
} from '../../../../../../../src/plugins/data/common/index_patterns/fields/fields.mocks'; | ||
import { getMockTheme } from '../../../common/test_utils/kibana_react.mock'; | ||
import { getEntryMatchAnyMock } from '../../../../common/schemas/types/entry_match_any.mock'; | ||
import { getEntryMatchMock } from '../../../../common/schemas/types/entry_match.mock'; | ||
import { getEntryExistsMock } from '../../../../common/schemas/types/entry_exists.mock'; | ||
import { getEntryNestedMock } from '../../../../common/schemas/types/entry_nested.mock'; | ||
import { getExceptionListItemSchemaMock } from '../../../../common/schemas/response/exception_list_item_schema.mock'; | ||
|
||
import { | ||
ExceptionBuilderComponent, | ||
ExceptionBuilderProps, | ||
OnChangeProps, | ||
} from './exception_items_renderer'; | ||
|
||
const mockTheme = getMockTheme({ | ||
darkMode: false, | ||
eui: euiLightVars, | ||
}); | ||
const mockHttpService: HttpStart = ({ | ||
addLoadingCountSource: (): void => {}, | ||
anonymousPaths: { | ||
isAnonymous: (): void => {}, | ||
register: (): void => {}, | ||
}, | ||
basePath: {}, | ||
delete: (): void => {}, | ||
externalUrl: { | ||
validateUrl: (): void => {}, | ||
}, | ||
fetch: (): void => {}, | ||
get: (): void => {}, | ||
getLoadingCount$: (): void => {}, | ||
head: (): void => {}, | ||
intercept: (): void => {}, | ||
options: (): void => {}, | ||
patch: (): void => {}, | ||
post: (): void => {}, | ||
put: (): void => {}, | ||
} as unknown) as HttpStart; | ||
const mockAutocompleteService = ({ | ||
getValueSuggestions: () => | ||
new Promise((resolve) => { | ||
setTimeout(() => { | ||
resolve([ | ||
'siem-kibana', | ||
'win2019-endpoint-mr-pedro', | ||
'rock01', | ||
'windows-endpoint', | ||
'siem-windows', | ||
'mothra', | ||
]); | ||
}, 300); | ||
}), | ||
} as unknown) as AutocompleteStart; | ||
|
||
addDecorator((storyFn) => <ThemeProvider theme={mockTheme}>{storyFn()}</ThemeProvider>); | ||
|
||
export default { | ||
argTypes: { | ||
allowLargeValueLists: { | ||
control: { | ||
type: 'boolean', | ||
}, | ||
description: '`boolean` - set to true to allow large value lists.', | ||
table: { | ||
defaultValue: { | ||
summary: true, | ||
}, | ||
}, | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
autocompleteService: { | ||
control: { | ||
type: 'object', | ||
}, | ||
description: | ||
'`AutocompleteStart` - Kibana data plugin autocomplete service used for field value autocomplete.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
exceptionListItems: { | ||
control: { | ||
type: 'array', | ||
}, | ||
description: | ||
'`ExceptionsBuilderExceptionItem[]` - Any existing exception items - would be populated when editing an exception item.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
httpService: { | ||
control: { | ||
type: 'object', | ||
}, | ||
description: '`HttpStart` - Kibana service.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
indexPatterns: { | ||
description: | ||
'`IIndexPattern` - index patterns used to populate field options and value autocomplete.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
isAndDisabled: { | ||
control: { | ||
type: 'boolean', | ||
}, | ||
description: | ||
'`boolean` - set to true to disallow users from using the `AND` button to add entries.', | ||
table: { | ||
defaultValue: { | ||
summary: false, | ||
}, | ||
}, | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
isNestedDisabled: { | ||
control: { | ||
type: 'boolean', | ||
}, | ||
description: | ||
'`boolean` - set to true to disallow users from using the `Add nested` button to add nested entries.', | ||
table: { | ||
defaultValue: { | ||
summary: false, | ||
}, | ||
}, | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
isOrDisabled: { | ||
control: { | ||
type: 'boolean', | ||
}, | ||
description: | ||
'`boolean` - set to true to disallow users from using the `OR` button to add multiple exception items within the builder.', | ||
table: { | ||
defaultValue: { | ||
summary: false, | ||
}, | ||
}, | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
listId: { | ||
control: { | ||
type: 'string', | ||
}, | ||
description: '`string` - the exception list id.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
listNamespaceType: { | ||
control: { | ||
options: ['agnostic', 'single'], | ||
type: 'select', | ||
}, | ||
description: '`NamespaceType` - Determines whether the list is global or space specific.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
listType: { | ||
control: { | ||
options: ['detection', 'endpoint'], | ||
type: 'select', | ||
}, | ||
description: | ||
'`ExceptionListType` - Depending on the list type, certain validations may apply.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
listTypeSpecificIndexPatternFilter: { | ||
description: | ||
'`(pattern: IIndexPattern, type: ExceptionListType) => IIndexPattern` - callback invoked when index patterns filtered. Optional to be used if you would only like certain fields displayed.', | ||
type: { | ||
required: false, | ||
}, | ||
}, | ||
onChange: { | ||
description: | ||
'`(arg: OnChangeProps) => void` - callback invoked any time builder update to propagate changes up to parent.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
ruleName: { | ||
description: '`string` - name of the rule list is associated with.', | ||
type: { | ||
required: true, | ||
}, | ||
}, | ||
}, | ||
component: ExceptionBuilderComponent, | ||
title: 'ExceptionBuilderComponent', | ||
}; | ||
|
||
const BuilderTemplate: Story<ExceptionBuilderProps> = (args) => ( | ||
<ExceptionBuilderComponent {...args} /> | ||
); | ||
|
||
export const Default = BuilderTemplate.bind({}); | ||
Default.args = { | ||
allowLargeValueLists: true, | ||
autocompleteService: mockAutocompleteService, | ||
exceptionListItems: [], | ||
httpService: mockHttpService, | ||
indexPatterns: { fields, id: '1234', title: 'logstash-*' }, | ||
isAndDisabled: false, | ||
isNestedDisabled: false, | ||
isOrDisabled: false, | ||
listId: '1234', | ||
listNamespaceType: 'single', | ||
listType: 'detection', | ||
onChange: (): OnChangeProps => ({ | ||
errorExists: false, | ||
exceptionItems: [], | ||
exceptionsToDelete: [], | ||
}), | ||
ruleName: 'My awesome rule', | ||
}; | ||
|
||
const sampleExceptionItem = { | ||
...getExceptionListItemSchemaMock(), | ||
entries: [ | ||
{ ...getEntryMatchAnyMock(), field: getField('ip').name, value: ['some ip'] }, | ||
{ ...getEntryMatchMock(), field: getField('ssl').name, value: 'false' }, | ||
{ ...getEntryExistsMock(), field: getField('@timestamp').name }, | ||
], | ||
}; | ||
|
||
const sampleNestedExceptionItem = { | ||
...getExceptionListItemSchemaMock(), | ||
entries: [ | ||
{ ...getEntryMatchAnyMock(), field: getField('ip').name, value: ['some ip'] }, | ||
{ | ||
...getEntryNestedMock(), | ||
entries: [ | ||
{ | ||
...getEntryMatchMock(), | ||
field: 'child', | ||
value: 'some nested value', | ||
}, | ||
], | ||
field: 'nestedField', | ||
}, | ||
], | ||
}; | ||
|
||
const BuilderSingleExceptionItem: Story<ExceptionBuilderProps> = (args) => ( | ||
<ExceptionBuilderComponent {...args} /> | ||
); | ||
|
||
export const SingleExceptionItem = BuilderSingleExceptionItem.bind({}); | ||
SingleExceptionItem.args = { | ||
allowLargeValueLists: true, | ||
autocompleteService: mockAutocompleteService, | ||
exceptionListItems: [sampleExceptionItem], | ||
httpService: mockHttpService, | ||
indexPatterns: { fields, id: '1234', title: 'logstash-*' }, | ||
isAndDisabled: false, | ||
isNestedDisabled: false, | ||
isOrDisabled: false, | ||
listId: '1234', | ||
listNamespaceType: 'single', | ||
listType: 'detection', | ||
onChange: (): OnChangeProps => ({ | ||
errorExists: false, | ||
exceptionItems: [sampleExceptionItem], | ||
exceptionsToDelete: [], | ||
}), | ||
ruleName: 'My awesome rule', | ||
}; | ||
|
||
const BuilderMultiExceptionItems: Story<ExceptionBuilderProps> = (args) => ( | ||
<ExceptionBuilderComponent {...args} /> | ||
); | ||
|
||
export const MultiExceptionItems = BuilderMultiExceptionItems.bind({}); | ||
MultiExceptionItems.args = { | ||
allowLargeValueLists: true, | ||
autocompleteService: mockAutocompleteService, | ||
exceptionListItems: [sampleExceptionItem, sampleExceptionItem], | ||
httpService: mockHttpService, | ||
indexPatterns: { fields, id: '1234', title: 'logstash-*' }, | ||
isAndDisabled: false, | ||
isNestedDisabled: false, | ||
isOrDisabled: false, | ||
listId: '1234', | ||
listNamespaceType: 'single', | ||
listType: 'detection', | ||
onChange: (): OnChangeProps => ({ | ||
errorExists: false, | ||
exceptionItems: [sampleExceptionItem, sampleExceptionItem], | ||
exceptionsToDelete: [], | ||
}), | ||
ruleName: 'My awesome rule', | ||
}; | ||
|
||
const BuilderWithNested: Story<ExceptionBuilderProps> = (args) => ( | ||
<ExceptionBuilderComponent {...args} /> | ||
); | ||
|
||
export const WithNestedExceptionItem = BuilderWithNested.bind({}); | ||
WithNestedExceptionItem.args = { | ||
allowLargeValueLists: true, | ||
autocompleteService: mockAutocompleteService, | ||
exceptionListItems: [sampleNestedExceptionItem, sampleExceptionItem], | ||
httpService: mockHttpService, | ||
indexPatterns: { fields, id: '1234', title: 'logstash-*' }, | ||
isAndDisabled: false, | ||
isNestedDisabled: false, | ||
isOrDisabled: false, | ||
listId: '1234', | ||
listNamespaceType: 'single', | ||
listType: 'detection', | ||
onChange: (): OnChangeProps => ({ | ||
errorExists: false, | ||
exceptionItems: [sampleNestedExceptionItem, sampleExceptionItem], | ||
exceptionsToDelete: [], | ||
}), | ||
ruleName: 'My awesome rule', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected. Looking at the metrics, the security solution was minus about the same amount as was added to the lists plugin. Since the lists plugin previously did not include UI components, it was able to stay pretty small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 was curious about that here. Would you mind reducing the securitySolutions limit since that was moved out?