[Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities #189633

logeekal · 2024-07-31T13:43:42Z

Note

This Change is only applicable to Serverless Security Solution as of now. In follow-up PRs, support will be added to ESS as well based data-sources such as index or intergrations.

Summary

Resolves #189151

This PR is foundation for the work described in #186783. This just enables expandable flyout for entity details, which is currently only used in security solution, in discover as well.

As a part of One Discover work, we need to make sure that cell rendering in Discover should behave exactly like it does in security solution.

To enable this, a new shared-browser package @kbn/security-solution-common in x-pack/packages/security-solution has been created which can used to share components between security solution and discover. Below is the usage pattern

flowchart TD
    disc-utils[@kbn/discover-utils] --> sscommon
    sscommon[@kbn/security-solution-common] --> ssplugin[security_solution]
    sscommon[@kbn/security-solution-common] --> discover[discover]
    disc-utils[@kbn/discover-utils] --> discover

Desk Testing Guide.

Enable Security profile in serverless by adding below to kibana.yml

discover.experimental.enabledProfiles: ['security-root-profile']

Load Some data
Navigate to discover and add host.name as one of the column.
Should open an expandable flyout as shown below.

discover_flyout.mp4

Code Review Guide

Most of the changes in the PR are code-organization. There are NO changes in security solution but only the changes to import statements.

You can focus regarding the changes in below packages:

x-pack/packages/security-solution/common
packages/kbn-discover-utils
packages/kbn-expandable-flyout

logeekal · 2024-08-01T12:33:59Z

/ci

elasticmachine · 2024-08-01T12:37:09Z

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

tomsonpl

Defend Workflows code review LGTM 👍
Thanks!

stephmilovic

GenAI team code changes LGTM

davismcphee

Thanks for getting things started on the Discover Security profile! The Data Discovery changes look great, just one comment to address about making sure the profile isn't enabled by Default in Discover yet, then it'll be good to approve on my end.

One request is that when we make the switch to actually enable the Security profile by default, we should have sufficient unit and functional tests in place for the profile, but I don't see that as necessary for this PR since it's still experimental for now.

packages/kbn-discover-utils/src/utils/get_field_value.ts

davismcphee · 2024-08-13T16:06:48Z

src/plugins/discover/public/context_awareness/profile_providers/register_profile_providers.ts

@@ -88,7 +89,7 @@ const extractProfileIds = (providers: Array<BaseProfileProvider<{}>>) =>
  providers.map(({ profileId }) => profileId);

 const createRootProfileProviders = (_providerServices: ProfileProviderServices) =>
-  [] as RootProfileProvider[];
+  [createSecurityRootProfileProvider(_providerServices)] as RootProfileProvider[];


Adding createSecurityRootProfileProvider here means it will be enabled by default in Discover instead of using discover.experimental.enabledProfiles: ['security-root-profile']. I would recommend creating a separate createExperimentalRootProfileProviders function or similar to register this for now, but not adding the IDs to enabledProfileIds by default.

@davismcphee , do you think it make sense, if I add isEnabled flag to profiles.

This way, experimental profiles will always have isEnabled as No and it can be overidden by discover.experimental.enabledProfiles.

Additionally, with that we do not have to maintain seperate enabledProfiles

I have add the proposed solution here : 91a699c. See if this makes sense as it takes the responisbility of enabling profile providers outside of register_profile_services and profile providers can decide if profile should be enabled or not.

If not, i will be happy to revert.

Nice, I like this approach better! My only suggestion is rather than using an isEnabled flag that every profile needs to add, what if we go with something like an optional isExperimental flag to explicitly mark only the experimental profiles?

Works for me. Done changes here: 0739ef2.

davismcphee · 2024-08-13T17:39:05Z

.../discover/public/context_awareness/profile_providers/security/security_root_profile/index.ts

Creating a security subfolder for security-specific profiles seems like a good pattern to me, and we can do similar for other solutions 👍 Do you think we should update CODEOWNERS to specify both Data Discovery and Security as owners of src/plugins/discover/public/context_awareness/profile_providers/security so both teams get pinged on PRs?

Added here: e7ed9df.

I have also added the code ownership of packages/kbn-unified-data-table to @elastic/security-threat-hunting-investigations so that we are aware of the changes done to the table.

Great, thanks. And we open a lot of PRs that touch Unified Data Table, but I think it should be fine to add so you can be aware of changes and have a chance to raise concerns if they affect your usage. Unfortunately it looks like CI reverted the CODEOWNERS changes: bff6cec. I think we may need to update packages/kbn-unified-data-table/kibana.jsonc for Unified Data Table and add the override for the security subfolder below the autogenerated section of CODEOWNERS with the other Data Discovery ones.

And we open a lot of PRs that touch Unified Data Table, but I think it should be fine to add so you can be aware of changes

Exactly, we are trying to add as many tests so that we are aware of the changes but it is easy to miss. And as you mentioned the speed of dev is very high, for some time we would like to be apprised for changes going in.

I think we may need to update packages/kbn-unified-data-table/kibana.jsonc for Unified Data Table and add the override for the security subfolder below the autogenerated section of CODEOWNERS with the other Data Discovery ones.

Yes, TIL about this. Done here 7ed0376

logeekal · 2024-08-23T11:17:29Z

One request is that when we make the switch to actually enable the Security profile by default, we should have sufficient unit and functional tests in place for the profile, but I don't see that as necessary for this PR since it's still experimental for now.

@davismcphee. I 100% agree with this statement. So far I have tried to cover the current partial code with unit tests.. Functional test will come when Flyout is fully implemented.

If you think unit test are missing, feel free to point out.

davismcphee

The latest changes look good! Just a bit of minor feedback and looks like there's a few test failures to look into, but otherwise looks good on my end.

davismcphee · 2024-08-23T23:00:44Z

.../discover/public/context_awareness/profile_providers/security/security_root_profile/index.ts

Great, thanks. And we open a lot of PRs that touch Unified Data Table, but I think it should be fine to add so you can be aware of changes and have a chance to raise concerns if they affect your usage. Unfortunately it looks like CI reverted the CODEOWNERS changes: bff6cec. I think we may need to update packages/kbn-unified-data-table/kibana.jsonc for Unified Data Table and add the override for the security subfolder below the autogenerated section of CODEOWNERS with the other Data Discovery ones.

davismcphee · 2024-08-23T23:03:52Z

src/plugins/discover/public/context_awareness/profile_providers/register_profile_providers.ts

+  const enabledRootProfileProviders = rootProfileProviders.filter(
+    ({ isEnabled = true, profileId }) => isEnabled || experimentalProfileIds.includes(profileId)
+  );
+
+  const enabledDataSourceProfileProviders = dataSourceProfileProviders.filter(
+    ({ isEnabled = true, profileId }) => isEnabled || experimentalProfileIds.includes(profileId)
+  );
+
+  const enabledDocumentProfileProviders = documentProfileProviders.filter(
+    ({ isEnabled = true, profileId }) => isEnabled || experimentalProfileIds.includes(profileId)
+  );


Maybe we could add this filtering logic directly to registerProfileProvidersInternal instead and pass down experimentalProfileIds.

done here: 0739ef2

davismcphee · 2024-08-23T23:04:33Z

...ugins/discover/public/context_awareness/profile_providers/register_profile_providers.test.ts

    });
    const context = await rootProfileServiceMock.resolve({ solutionNavId: null });
    expect(rootProfileServiceMock.getProfile(context)).toBe(rootProfileProviderMock.profile);
  });
-
-  it('should not register disabled profile providers', async () => {


If we move the filtering logic to registerProfileProvidersInternal, we should be able to restore an updated version of this test.

Done in the same commit as mentioned above.

davismcphee · 2024-08-23T23:06:52Z

src/plugins/discover/public/context_awareness/profile_providers/register_profile_providers.ts

+  [
+    exampleRootProfileProvider,
+    createSecurityRootProfileProvider(_providerServices),
+  ] as RootProfileProvider[];


Suggested change

] as RootProfileProvider[];

];

Nit: I think we can drop the cast now that we have entries in the array.

👍 . Great catch. Done in the same commit as above.

davismcphee · 2024-08-23T23:12:56Z

src/plugins/discover/public/context_awareness/profile_providers/register_profile_providers.ts

@@ -88,7 +89,7 @@ const extractProfileIds = (providers: Array<BaseProfileProvider<{}>>) =>
  providers.map(({ profileId }) => profileId);

 const createRootProfileProviders = (_providerServices: ProfileProviderServices) =>
-  [] as RootProfileProvider[];
+  [createSecurityRootProfileProvider(_providerServices)] as RootProfileProvider[];


Nice, I like this approach better! My only suggestion is rather than using an isEnabled flag that every profile needs to add, what if we go with something like an optional isExperimental flag to explicitly mark only the experimental profiles?

kibana-ci · 2024-08-26T12:45:12Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`canvas`	1287	1288	+1
`cloudSecurityPosture`	703	704	+1
`discover`	985	1029	+44
`esqlDataGrid`	417	418	+1
`eventAnnotationListing`	641	642	+1
`lens`	1504	1505	+1
`logsExplorer`	617	618	+1
`observability`	1116	1117	+1
`securitySolution`	5677	5691	+14
`slo`	898	899	+1
`unifiedDocViewer`	281	282	+1
total			+67

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/discover-utils`	106	109	+3
`@kbn/expandable-flyout`	14	17	+3
`@kbn/security-solution-common`	-	38	+38
total			+44

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	843.6KB	855.8KB	+12.2KB
`securitySolution`	18.0MB	18.0MB	-3.1KB
total			+9.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/expandable-flyout`	1	2	+1
`@kbn/security-solution-common`	-	5	+5
total			+6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	47.7KB	47.9KB	+171.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/discover-utils`	133	136	+3
`@kbn/expandable-flyout`	39	42	+3
`@kbn/security-solution-common`	-	59	+59
total			+65

History

💔 Build #229720 failed ffa845d
💔 Build #229423 failed bff6cec
💚 Build #227288 succeeded c3e4de9
💔 Build #227252 failed c35e82c
💔 Build #227018 failed 8825804
💔 Build #226335 failed 8983242

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

cc @logeekal

davismcphee

Data Discovery changes LGTM 👍 Thanks for helping improve profile registration!

logeekal added 8 commits July 19, 2024 13:30

new package

6a6dcd3

Merge branch 'main' into one-discover/cell-renderer

ff93dd0

incrmental change

7fc3ada

incremental save : exp. flyout working

462f128

left over incrmental changes

d8b8ddb

fix: types

8f62331

fix: code organization

1d719d4

fix: discover renderers

3d0ee32

logeekal mentioned this pull request Aug 1, 2024

[Cloud Security] POC sharing Cloud Security APIs #179143

Closed

3 tasks

logeekal added 3 commits August 1, 2024 13:29

fix: better organization

c538a60

tests

a471d23

fix: housekeeping + tests

8dfe265

logeekal changed the title ~~One discover/cell renderer~~ [Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities Aug 1, 2024

logeekal self-assigned this Aug 1, 2024

logeekal marked this pull request as ready for review August 1, 2024 12:37

logeekal requested review from a team as code owners August 1, 2024 12:37

logeekal requested review from jaredburgettelastic, pzl and tomsonpl August 1, 2024 12:37

logeekal added 3 commits August 7, 2024 13:44

incremental change: tests

dcf257d

better mocks

2ea3d95

expandable flyout mocks

55ff166

logeekal requested a review from a team as a code owner August 7, 2024 13:15

logeekal added 2 commits August 7, 2024 17:27

types: fix

75eb37d

Merge branch 'main' into one-discover/cell-renderer

8983242

tomsonpl approved these changes Aug 8, 2024

View reviewed changes

hop-dev approved these changes Aug 8, 2024

View reviewed changes

ashokaditya approved these changes Aug 8, 2024

View reviewed changes

christineweng approved these changes Aug 8, 2024

View reviewed changes

add missing test

8825804

stephmilovic approved these changes Aug 12, 2024

View reviewed changes

logeekal added 2 commits August 13, 2024 10:11

Merge branch 'main' into one-discover/cell-renderer

c35e82c

fix: merge issues

c3e4de9

davismcphee reviewed Aug 13, 2024

View reviewed changes

logeekal added 2 commits August 23, 2024 13:11

codeowners

e7ed9df

fix: add isEnabled Flag

91a699c

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

bff6cec

davismcphee reviewed Aug 23, 2024

View reviewed changes

logeekal and others added 5 commits August 26, 2024 11:53

review: isEnabled --> isExperimental

0739ef2

fix: codeowners

7ed0376

Merge branch 'main' into one-discover/cell-renderer

c5193a7

[CI] Auto-commit changed files from 'node scripts/yarn_deduplicate'

ffa845d

fix: types

f670ac9

logeekal removed the request for review from jaredburgettelastic August 26, 2024 15:32

davismcphee approved these changes Aug 27, 2024

View reviewed changes

logeekal merged commit 9293bc1 into elastic:main Aug 27, 2024
42 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities #189633

[Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities #189633

logeekal commented Jul 31, 2024 •

edited

Loading

logeekal commented Aug 1, 2024

elasticmachine commented Aug 1, 2024

tomsonpl left a comment

stephmilovic left a comment

davismcphee left a comment

davismcphee Aug 13, 2024

logeekal Aug 23, 2024 •

edited

Loading

logeekal Aug 23, 2024

davismcphee Aug 23, 2024

logeekal Aug 26, 2024

davismcphee Aug 13, 2024

logeekal Aug 23, 2024

davismcphee Aug 23, 2024

logeekal Aug 26, 2024 •

edited

Loading

logeekal commented Aug 23, 2024

davismcphee left a comment

davismcphee Aug 23, 2024

davismcphee Aug 23, 2024

logeekal Aug 26, 2024

davismcphee Aug 23, 2024

logeekal Aug 26, 2024

davismcphee Aug 23, 2024

logeekal Aug 26, 2024

davismcphee Aug 23, 2024

kibana-ci commented Aug 26, 2024

API count

davismcphee left a comment

[Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities #189633

[Security Solution] One Discover - Enable Security Solution Expandable Flyout in One Discover entities #189633

Conversation

logeekal commented Jul 31, 2024 • edited Loading

Summary

Desk Testing Guide.

Code Review Guide

logeekal commented Aug 1, 2024

elasticmachine commented Aug 1, 2024

tomsonpl left a comment

Choose a reason for hiding this comment

stephmilovic left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logeekal Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

logeekal commented Aug 23, 2024

davismcphee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Aug 26, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

History

davismcphee left a comment

Choose a reason for hiding this comment

logeekal commented Jul 31, 2024 •

edited

Loading

logeekal Aug 23, 2024 •

edited

Loading

logeekal Aug 26, 2024 •

edited

Loading