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

[Rules migration][UI] Basic rule migrations UI (#10820) #200978

Merged
merged 10 commits into from
Nov 22, 2024

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Nov 20, 2024

Summary

Internal link to the feature details

This is a very first version of the SIEM rules migrations UI functionality. The main goal is to setup and agree on a folder structure where the feature gonna live. Tests covering feature will follow in a separate PR (see internal link for more details).

The code follows the structure of prebuilt rules feature https://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table and hidden behind siemMigrationsEnabled feature flag.

Key UI changes

  • New "SIEM Rules Migrations." rules management sub-page
  • Navigation between different "finished" migrations
  • InMemory table with all the translations within the selected migration
  • Translation details preview flyout with Translation and Overview tabs
  • User cannot modify translations via UI

Testing locally

Enable the flag

xpack.securitySolution.enableExperimental: ['siemMigrationsEnabled']

Screenshot

Screen.Recording.2024-11-20.at.17.06.50.mov

@e40pud e40pud added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 20, 2024
@e40pud e40pud requested a review from semd November 20, 2024 16:08
@e40pud e40pud self-assigned this Nov 20, 2024
@e40pud e40pud requested review from a team as code owners November 20, 2024 16:08
@e40pud e40pud requested a review from nikitaindik November 20, 2024 16:08
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@e40pud e40pud requested a review from P1llus November 20, 2024 16:13
@banderror banderror requested review from banderror and removed request for nikitaindik November 20, 2024 17:11
@semd
Copy link
Contributor

semd commented Nov 21, 2024

@e40pud The SIEM rule migrations process generates detection rules as a result, but it is not part of any security detection workflow, it would be better if we decouple the code from Detections completely.
Can we create a security_solution/public/siem_migrations directory as a sub-plugin, and a /rules directory inside for all the rule migrations specific UI code? We'll have migrations for other entities (dashboards, saved objects) in the future.

This way we have explicit ownership, RBAC, licensing... The code organization is independent of where we locate the link in the nav tree, we can still place the link inside the Rules category/landing.

@e40pud e40pud removed the Team:Detection Rule Management Security Detection Rule Management Team label Nov 21, 2024

export type TableFilterOptions = Pick<FilterOptions, 'filter'>;

export const useFilterRulesToInstall = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to do this on the ES side once we add pagination, but it's ok for the initial PR

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

LGTM!

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #82 / discover/context_awareness extension getAdditionalCellActions data view mode should not render incompatible cell action for message column

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 6227 6264 +37

Async chunks

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

id before after diff
securitySolution 13.4MB 13.4MB +18.9KB

Page load bundle

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

id before after diff
securitySolution 86.2KB 86.4KB +233.0B
securitySolutionEss 11.8KB 11.8KB +46.0B
securitySolutionServerless 25.6KB 25.7KB +46.0B
total +325.0B

History

cc @e40pud

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Not sure which file I had to review as a code owner and what was the initial code structure in this PR, but @semd's suggestion makes sense and was an important one to address.

The final code structure LGTM 👍 I left a couple of minor comments. I'd recommend to clean up the code in a follow-up PR to make sure you use descriptive names specific to the siem_migrations subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way too generic and conflict with the rule management terminology, which would make code search more difficult.

Other than that, I'm really glad we're creating a separate page and organizing the code properly 👍 Should allow us to move forward way easier.

Small reminder: when you get to researching and planning how you're going to deal with prebuilt rules, please don't forget to share your plan with me and @xcrzx before starting to work on the implementation 🙏 This would likely save all of us some time.

🚀

Comment on lines +124 to +125
export const RulesTable = React.memo(RulesTableComponent);
RulesTable.displayName = 'RulesTable';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid ambiguity with the Rules table in the Rule management subdomain? Could be something like that:

Suggested change
export const RulesTable = React.memo(RulesTableComponent);
RulesTable.displayName = 'RulesTable';
export const RuleMigrationResultsTable = React.memo(RuleMigrationResultsTableComponent);
RuleMigrationResultsTable.displayName = 'RuleMigrationResultsTable';

import { useRulePreviewFlyout } from '../hooks/use_rule_preview_flyout';
import { NoMigrations } from '../components/no_migrations';

const RulesPageComponent: React.FC = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure we don't leave any copy-pasted names from the rule management / prebuilt rules areas...

@e40pud e40pud merged commit a627e01 into elastic:main Nov 22, 2024
46 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11974804649

@e40pud
Copy link
Contributor Author

e40pud commented Nov 25, 2024

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

e40pud added a commit to e40pud/kibana that referenced this pull request Nov 25, 2024
…ic#200978)

## Summary

[Internal link](elastic/security-team#10820)
to the feature details

This is a very first version of the SIEM rules migrations UI
functionality. The main goal is to setup and agree on a folder structure
where the feature gonna live. Tests covering feature will follow in a
separate PR (see [internal
link](elastic/security-team#11232) for more
details).

The code follows the structure of prebuilt rules feature
https://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table
and hidden behind `siemMigrationsEnabled` feature flag.

### Key UI changes

* New "SIEM Rules Migrations." rules management sub-page
* Navigation between different "finished" migrations
* InMemory table with all the translations within the selected migration
* Translation details preview flyout with `Translation` and `Overview`
tabs
* User cannot modify translations via UI

### Testing locally

Enable the flag

```
xpack.securitySolution.enableExperimental: ['siemMigrationsEnabled']
```
### Screenshot

https://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac
(cherry picked from commit a627e01)

# Conflicts:
#	.github/CODEOWNERS
e40pud added a commit that referenced this pull request Nov 25, 2024
… (#201545)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Rules migration][UI] Basic rule migrations UI (#10820)
(#200978)](#200978)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-22T14:48:14Z","message":"[Rules
migration][UI] Basic rule migrations UI (#10820) (#200978)\n\n##
Summary\r\n\r\n[Internal
link](https://github.com/elastic/security-team/issues/10820)\r\nto the
feature details\r\n\r\nThis is a very first version of the SIEM rules
migrations UI\r\nfunctionality. The main goal is to setup and agree on a
folder structure\r\nwhere the feature gonna live. Tests covering feature
will follow in a\r\nseparate PR (see
[internal\r\nlink](elastic/security-team#11232)
for more\r\ndetails).\r\n\r\nThe code follows the structure of prebuilt
rules
feature\r\nhttps://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table\r\nand
hidden behind `siemMigrationsEnabled` feature flag.\r\n\r\n### Key UI
changes\r\n\r\n* New \"SIEM Rules Migrations.\" rules management
sub-page\r\n* Navigation between different \"finished\" migrations\r\n*
InMemory table with all the translations within the selected
migration\r\n* Translation details preview flyout with `Translation` and
`Overview`\r\ntabs\r\n* User cannot modify translations via
UI\r\n\r\n### Testing locally\r\n\r\nEnable the
flag\r\n\r\n```\r\nxpack.securitySolution.enableExperimental:
['siemMigrationsEnabled']\r\n```\r\n###
Screenshot\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac","sha":"a627e011a892e9eaa7ec234b7a08fc5572801bbc","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Threat
Hunting","Team:
SecuritySolution","backport:prev-minor"],"number":200978,"url":"https://github.com/elastic/kibana/pull/200978","mergeCommit":{"message":"[Rules
migration][UI] Basic rule migrations UI (#10820) (#200978)\n\n##
Summary\r\n\r\n[Internal
link](https://github.com/elastic/security-team/issues/10820)\r\nto the
feature details\r\n\r\nThis is a very first version of the SIEM rules
migrations UI\r\nfunctionality. The main goal is to setup and agree on a
folder structure\r\nwhere the feature gonna live. Tests covering feature
will follow in a\r\nseparate PR (see
[internal\r\nlink](elastic/security-team#11232)
for more\r\ndetails).\r\n\r\nThe code follows the structure of prebuilt
rules
feature\r\nhttps://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table\r\nand
hidden behind `siemMigrationsEnabled` feature flag.\r\n\r\n### Key UI
changes\r\n\r\n* New \"SIEM Rules Migrations.\" rules management
sub-page\r\n* Navigation between different \"finished\" migrations\r\n*
InMemory table with all the translations within the selected
migration\r\n* Translation details preview flyout with `Translation` and
`Overview`\r\ntabs\r\n* User cannot modify translations via
UI\r\n\r\n### Testing locally\r\n\r\nEnable the
flag\r\n\r\n```\r\nxpack.securitySolution.enableExperimental:
['siemMigrationsEnabled']\r\n```\r\n###
Screenshot\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac","sha":"a627e011a892e9eaa7ec234b7a08fc5572801bbc"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200978","number":200978,"mergeCommit":{"message":"[Rules
migration][UI] Basic rule migrations UI (#10820) (#200978)\n\n##
Summary\r\n\r\n[Internal
link](https://github.com/elastic/security-team/issues/10820)\r\nto the
feature details\r\n\r\nThis is a very first version of the SIEM rules
migrations UI\r\nfunctionality. The main goal is to setup and agree on a
folder structure\r\nwhere the feature gonna live. Tests covering feature
will follow in a\r\nseparate PR (see
[internal\r\nlink](elastic/security-team#11232)
for more\r\ndetails).\r\n\r\nThe code follows the structure of prebuilt
rules
feature\r\nhttps://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table\r\nand
hidden behind `siemMigrationsEnabled` feature flag.\r\n\r\n### Key UI
changes\r\n\r\n* New \"SIEM Rules Migrations.\" rules management
sub-page\r\n* Navigation between different \"finished\" migrations\r\n*
InMemory table with all the translations within the selected
migration\r\n* Translation details preview flyout with `Translation` and
`Overview`\r\ntabs\r\n* User cannot modify translations via
UI\r\n\r\n### Testing locally\r\n\r\nEnable the
flag\r\n\r\n```\r\nxpack.securitySolution.enableExperimental:
['siemMigrationsEnabled']\r\n```\r\n###
Screenshot\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac","sha":"a627e011a892e9eaa7ec234b7a08fc5572801bbc"}}]}]
BACKPORT-->
paulinashakirova pushed a commit to paulinashakirova/kibana that referenced this pull request Nov 26, 2024
…ic#200978)

## Summary

[Internal link](elastic/security-team#10820)
to the feature details

This is a very first version of the SIEM rules migrations UI
functionality. The main goal is to setup and agree on a folder structure
where the feature gonna live. Tests covering feature will follow in a
separate PR (see [internal
link](elastic/security-team#11232) for more
details).

The code follows the structure of prebuilt rules feature
https://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table
and hidden behind `siemMigrationsEnabled` feature flag.

### Key UI changes

* New "SIEM Rules Migrations." rules management sub-page
* Navigation between different "finished" migrations
* InMemory table with all the translations within the selected migration
* Translation details preview flyout with `Translation` and `Overview`
tabs
* User cannot modify translations via UI

### Testing locally

Enable the flag

```
xpack.securitySolution.enableExperimental: ['siemMigrationsEnabled']
```
### Screenshot


https://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac
e40pud added a commit that referenced this pull request Dec 4, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 4, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.

* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.

* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?

(cherry picked from commit 4d8f711)
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Dec 9, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
SoniaSanzV pushed a commit to SoniaSanzV/kibana that referenced this pull request Dec 9, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 9, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
Samiul-TheSoccerFan pushed a commit to Samiul-TheSoccerFan/kibana that referenced this pull request Dec 10, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
…ic#200978)

## Summary

[Internal link](elastic/security-team#10820)
to the feature details

This is a very first version of the SIEM rules migrations UI
functionality. The main goal is to setup and agree on a folder structure
where the feature gonna live. Tests covering feature will follow in a
separate PR (see [internal
link](elastic/security-team#11232) for more
details).

The code follows the structure of prebuilt rules feature
https://github.com/elastic/kibana/tree/main/x-pack/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/add_prebuilt_rules_table
and hidden behind `siemMigrationsEnabled` feature flag.

### Key UI changes

* New "SIEM Rules Migrations." rules management sub-page
* Navigation between different "finished" migrations
* InMemory table with all the translations within the selected migration
* Translation details preview flyout with `Translation` and `Overview`
tabs
* User cannot modify translations via UI

### Testing locally

Enable the flag

```
xpack.securitySolution.enableExperimental: ['siemMigrationsEnabled']
```
### Screenshot


https://github.com/user-attachments/assets/a5a7e777-c5f8-40b4-be1d-1bd07a2729ac
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
## Summary

These are the followup updated to address feedback from my previous PRs:

* Make sure to use descriptive names specific to the `siem_migrations`
subdomain
([comment](elastic#200978 (review))):

> Make sure you use descriptive names specific to the siem_migrations
subdomain. Names like RulesPage, RulesTable, useRulesColumns etc are way
too generic and conflict with the rule management terminology, which
would make code search more difficult.


* Export the memo component directly everywhere
([comment](elastic#201597 (comment))):

> Could we export the memo component directly everywhere? It's shorter
and it makes it easier to find the references in the IDE.


* Use one hook to access APIs instead of two
([comment](elastic#202494 (comment))):

> I see that for every API request we have to implement 2 separate
hooks. Why don't we add error handling to the same hook that does the
useQuery? so we have everything in one hook. Or is there a reason to
have them separate?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants