-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Allow users to edit required_fields field for custom rules #180682
[Security Solution] Allow users to edit required_fields field for custom rules #180682
Conversation
@nikitaindik Can you give an update on the progress on this PR? Is anything blocking you from completing the test coverage and opening it for review? |
f32c820
to
d47d40b
Compare
@banderror Sorry for the late reply. My plan is to wrap up the tests and open the PR for review today. |
d47d40b
to
20c1645
Compare
/ci |
20c1645
to
8017f66
Compare
/ci |
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.
Feature looks great! I suggested some tweaks to UI text here and there, let me know if you have any questions. Thanks!
...blic/detection_engine/rule_creation/components/required_fields/required_fields_help_info.tsx
Outdated
Show resolved
Hide resolved
...ty_solution/public/detection_engine/rule_creation/components/required_fields/translations.ts
Outdated
Show resolved
Hide resolved
...ty_solution/public/detection_engine/rule_creation/components/required_fields/translations.ts
Outdated
Show resolved
Hide resolved
...ty_solution/public/detection_engine/rule_creation/components/required_fields/translations.ts
Outdated
Show resolved
Hide resolved
[indexPatternFields] | ||
); | ||
|
||
const allFieldNames = useMemo(() => fieldsWithTypes.map(({ name }) => name), [fieldsWithTypes]); |
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.
Nit: Do we still need to use useMemo
in this component? There are a lot of moving parts, and most of the variables in this component are written without using useMemo
. I'd say we either need to wrap all of them with memoization or none, but not a mix of the two approaches. If the performance is okay, given the watch
optimization, I'd say we can omit using useMemo
.
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.
I thought about removing this, but then decided to leave it in, because fieldsWithTypes
array can be really long – almost 2000 items for default index patterns.
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 the PR locally, and everything works as expected. The code changes also look good to me. Thank you for the implementation, @nikitaindik 👍
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.
Thanks for addressing my comments @nikitaindik, I like the look of the default state a lot more now!
Exploratory testingTested and working as expected:
|
<EuiText style={{ width: POPOVER_WIDTH }} size="s"> | ||
<FormattedMessage | ||
id="xpack.securitySolution.detectionEngine.ruleDescription.requiredFields.fieldRequiredFieldsHelpText" | ||
defaultMessage="Choose the fields and data types needed for this rule to function. You can select the fields available in the rule's {source} index patterns or data view." |
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.
@nikitaindik Here's some revised popover text, after our chat about custom fields. I also wanted to make it clearer that this is just informational — the user isn't actually configuring the rule's logic to require these fields, they're basically just documenting how the rule works.
defaultMessage="Choose the fields and data types needed for this rule to function. You can select the fields available in the rule's {source} index patterns or data view." | |
defaultMessage="Create an informational list of fields and data types this rule needs to function. Select fields in the rule's {source} index patterns or data view, or type in custom fields." |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Canvas Sharable Runtime
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @nikitaindik |
Resolves: #173594
Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5915
Summary
This PR adds an ability to add and edit custom rule's required fields. "Required fields" is an optional field that shows the user which Elasticsearch fields are needed for the rule to run properly. The values in "required fields" don't affect rule execution in any way. It's purely documentational, similar to "setup guide" and "investigation guide". This functionality is added to both rule creation and rule editing screens. It's available for all rule types except ML.
Details
The basic flow goes like this: first you specify your index patterns (or a data view), then you can set required fields for these index patterns. Once a user adds a required field and chooses its name, he can then choose its type from the dropdown. The first available type for the field name selected automatically. User can also add their own custom names and types.
Warnings
If a field that is not present in the selected index pattern, you will see a warning message.
This can happen in the following cases:
In any of these cases, you'll see a general warning message above the form section. And then also a more specific warning message next to the field that is causing the issue.
ESQL and ML rules
Here's how available dropdown options are determined for different rule types:
For all rule types except ESQL and ML, we take the index patterns specified by the user and fetch their mappings. Then we use these fields and types to populate the dropdowns.
For ESQL rules we parse index patterns out of the query since there's no explicit index pattern form field. We then fetch the mappings for these index patterns and use them to populate the dropdowns.
For ML rules, we don't show "required fields" at all. ML rules are a special case.
Screenshots