-
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] Rule Diff Phase 2 components #174564
Conversation
...curity_solution/public/detection_engine/rule_management/components/rule_details/constants.ts
Outdated
Show resolved
Hide resolved
...security_solution/public/detection_engine/rule_management/components/rule_details/helpers.ts
Outdated
Show resolved
Hide resolved
...curity_solution/public/detection_engine/rule_management/components/rule_details/constants.ts
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_management/components/rule_details/diff_components/index_patterns.tsx
Outdated
Show resolved
Hide resolved
...ion/public/detection_engine/rule_management/components/rule_details/diff_components/index.ts
Show resolved
Hide resolved
.../public/detection_engine/rule_management/components/rule_details/per_field_rule_diff_tab.tsx
Outdated
Show resolved
Hide resolved
if (Object.hasOwn(ruleDiff.fields, field)) { | ||
const typedField = field as keyof RuleFieldsDiff; | ||
const formattedDiffs = getFormattedFieldDiff(typedField, ruleDiff.fields); | ||
fields.push({ formattedDiffs, fieldName: typedField }); |
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.
We're going to need a dictionary to translate field names as they exist in the rule schema to their UI-version. For example, note
is actually Investigation guide
. I guess we can create a dictionary only for the one that are completely different, and for the others, not found in the dict, just take the field name and camel case it as you are doing now.
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.
Started in x-pack/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/diff_components/translations.ts
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
buildkite test this |
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.
Tested all types, looking good! LGTM ✅
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.
@dplumlee Reviewed changes in the code and left a few suggestions, most of them for further improvements in follow-up PRs.
But overall it looks great: components are clean and separated into logical pieces, the implementation is easy to comprehend. LGTM 👍
x-pack/plugins/security_solution/common/experimental_features.ts
Outdated
Show resolved
Hide resolved
...curity_solution/public/detection_engine/rule_management/components/rule_details/constants.ts
Outdated
Show resolved
Hide resolved
...curity_solution/public/detection_engine/rule_management/components/rule_details/constants.ts
Outdated
Show resolved
Hide resolved
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.
Not sure I get the current folder structure and where what components are supposed to be kept:
/rule_management/components/rule_details
/diff_components/*
/json_diff/*
/per_field_diff/*
/per_field_rule_diff_tab.tsx
/rule_diff_tab.tsx
I'd like to propose a structure that would represent the actual components' hierarchy. Something like that:
/rule_management/components/rule_details
/sections
/rule_about_section.tsx
/rule_definition_section.tsx
/etc
/flyout
/rule_details_flyout.tsx
/use_rule_details_flyout.tsx
/diffs
/common diff components and utils go here
/tabs
/json_diff_tab
/per_field_diff_tab
/overview_tab
If that makes sense, I'd suggest to do it in a separate PR.
export const getFieldDiffsForEqlQuery = (eqlQuery: AllFieldsDiff['eql_query']): FieldDiff[] => { | ||
const currentQuery = sortAndStringifyJson(eqlQuery.current_version?.query); | ||
const targetQuery = sortAndStringifyJson(eqlQuery.target_version?.query); | ||
|
||
const currentFilters = sortAndStringifyJson(eqlQuery.current_version?.filters); | ||
const targetFilters = sortAndStringifyJson(eqlQuery.target_version?.filters); | ||
return [ | ||
...(currentQuery !== targetQuery | ||
? [ | ||
{ | ||
fieldName: 'query', | ||
currentVersion: currentQuery, | ||
targetVersion: targetQuery, | ||
}, | ||
] | ||
: []), | ||
...(currentFilters !== targetFilters | ||
? [ | ||
{ | ||
fieldName: 'filters', | ||
currentVersion: currentFilters, | ||
targetVersion: targetFilters, | ||
}, | ||
] | ||
: []), | ||
]; | ||
}; |
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 file contains a lot of code (461 lines) and most of it looks very repetitive. If you take a closer look, most of the functions take some object in parameters, and for each of its properties yield either a FieldDiff
or nothing. I feel like we could replace most of the code in this file with some kind of generic implementation for calculating a FieldDiff[]
for a given ThreeWayDiff
input.
Again, I'd suggest trying to tackle this in a separate PR with refactoring and probably after covering the current implementation with tests.
...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx
Outdated
Show resolved
Hide resolved
...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx
Outdated
Show resolved
Hide resolved
fe1112f
to
773501e
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @dplumlee |
## Summary Addresses elastic#166489 Docs issue: elastic/security-docs#4783 Adds per-field diffs for the rule upgrade flyout ### Acceptance Criteria - [x] The tab with per-field diffs is hidden behind a new feature flag. When the flag is off, the tab does not appear in the flyout. The tab should work regardless of the value of `jsonPrebuiltRulesDiffingEnabled`. - [x] Per-field diffs are read-only components. We don't need to let the user "merge" differences using these components. - [x] Diffs for complex fields are rendered as JSON diffs using the same component used for rendering the JSON diff for the whole rule. This means this component should be abstracted away and should accept `unknown` values in props instead of `RuleResponse`. - [x] Diffs for related fields are grouped or rendered close to each other. For example: - [x] Index patterns + Data view id - [x] Custom query + Filters + Language + Saved query id - [x] The tab uses the response from the `upgrade/_review` API endpoint and doesn't need any other API calls to render itself. - [x] The tab renders itself under 150ms. ### Screenshots <img width="1587" alt="Screenshot 2024-02-07 at 1 36 34 AM" src="https://github.com/elastic/kibana/assets/56367316/85dce529-064e-4025-b82c-2e89f6ec800b"> <img width="994" alt="Screenshot 2024-02-07 at 1 36 52 AM" src="https://github.com/elastic/kibana/assets/56367316/c226973f-ad46-4565-90c0-437316b138b4"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: jpdjere <[email protected]>
## Summary Addresses elastic#166489 Docs issue: elastic/security-docs#4783 Adds per-field diffs for the rule upgrade flyout ### Acceptance Criteria - [x] The tab with per-field diffs is hidden behind a new feature flag. When the flag is off, the tab does not appear in the flyout. The tab should work regardless of the value of `jsonPrebuiltRulesDiffingEnabled`. - [x] Per-field diffs are read-only components. We don't need to let the user "merge" differences using these components. - [x] Diffs for complex fields are rendered as JSON diffs using the same component used for rendering the JSON diff for the whole rule. This means this component should be abstracted away and should accept `unknown` values in props instead of `RuleResponse`. - [x] Diffs for related fields are grouped or rendered close to each other. For example: - [x] Index patterns + Data view id - [x] Custom query + Filters + Language + Saved query id - [x] The tab uses the response from the `upgrade/_review` API endpoint and doesn't need any other API calls to render itself. - [x] The tab renders itself under 150ms. ### Screenshots <img width="1587" alt="Screenshot 2024-02-07 at 1 36 34 AM" src="https://github.com/elastic/kibana/assets/56367316/85dce529-064e-4025-b82c-2e89f6ec800b"> <img width="994" alt="Screenshot 2024-02-07 at 1 36 52 AM" src="https://github.com/elastic/kibana/assets/56367316/c226973f-ad46-4565-90c0-437316b138b4"> ### Checklist Delete any items that are not applicable to this PR. - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: jpdjere <[email protected]>
Summary
Addresses #166489
Docs issue: elastic/security-docs#4783
Adds per-field diffs for the rule upgrade flyout
Acceptance Criteria
jsonPrebuiltRulesDiffingEnabled
.unknown
values in props instead ofRuleResponse
.upgrade/_review
API endpoint and doesn't need any other API calls to render itself.Manual testing
We are currently going over every field returned by the rule diff api to locate any issues (either in the UI or endpoint) that occur when certain fields are added/deleted/modified
To test certain fields:
Use either a
PATCH
orPUT
method on the/api/detection_engine/rules
endpoint route with a rule object that contains at least arule_id
(notid
) and a lower version number than the current rule's version. This will allow you to see the prebuilt package update in the Rule Updates tab on the rules overview pageExample
Manually tested fields
version
name
description
author
building_block
severity
severity_mapping
risk_score
risk_score_mapping
references
false_positives
investigation_fields
license
rule_name_override
threat
threat_indicator_path
timestamp_override
tags
data_source
type
kql_query
eql_query
event_category_override
timestamp_field
tiebreaker_field
esql_query
anomaly_threshold
machine_learning_job_id
related_integrations
required_fields
timeline_template
threshold
threat_index
threat_mapping
threat_filters
threat_query
threat_indicator_path
concurrent_searches
items_per_search
alert_suppression
new_terms_fields
history_window_start
rule_schedule
setup
note
max_signals
Screenshots
Checklist
Delete any items that are not applicable to this PR.
For maintainers