-
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] Discuss rule field diffs and conflicts to handle in the UI #148913
Conversation
9c046d7
to
26f09d3
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
3c54cd4
to
1d8ffe0
Compare
Hi All, Working to the examples above of the rule update, this link shows how each update/conflict can be displayed, please comment on the figma doc. Note I have not shown the UI for the rule name, just want to clarify the decision on this. |
|
||
1. **`base=A, current=A, target=A => merged=A, conflict=false`** | ||
- **Situation**: Stock rule was installed. The user didn't customize the field. Elastic doesn't have any updates for this field. | ||
- **How to handle**: ❓ Don't show the field in the upgrade flyout? |
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.
With the current designs, I don't believe the user would be able to view this rule in the upgrade flyout as it wouldn't even show up in the Rule Updates
table since it has no changes, no?
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.
@spong Nope, that's two different things:
- A rule will be shown in the
Rule Updates
table if there's arule.version
of this rule which is higher than the currently installedrule.version
. - In this doc, however, we're looking at six possible situations for rule fields. Once a rule is shown in the table, some of its fields will have updates, and some of them won't. I assume most of the fields would fall into the first "typical situation".
In other words, base
, current
, target
and merged
are versions of a field, not versions of the whole 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.
Yes that was the idea, so if there essentially no update we should not bother the user with this information? if we can do that?
- **How to handle**: Show this field in the flyout. Automatically pick the updated version from Elastic as the merged one. | ||
3. **`base=A, current=B, target=A => merged=B, conflict=false`** | ||
- **Situation**: Stock rule was installed. The user customized the field. Elastic doesn't have any updates for this field. | ||
- **How to handle**: ❓ Don't show the field in the upgrade flyout? Or show the field, and automatically pick the current customized version as the merged one? |
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 think we should still show the field (as it is a delta from before), but TBD on if automatically picking the customized version as the merged one. I think we need to decide on the validation flow a little, e.g. should we even auto-select one of the three radio buttons, or leave the group un-selected, thus requiring the user to select one to pass validation? Will comment on the designs with regards to 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.
sounds good to me, would be good to get lots of opinions on.
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.
Just some thoughts, I'm not sure about that, but:
I'd definitely lean towards auto-selecting in the 3rd case (the field would pass validation), but (maybe?) not auto-selecting in the 5th case where we will have auto-merged fields (such field would require the user to click "ok" or something like that to confirm that the auto-merge was done correctly; after that, the field would pass validation). I left a related comment in Figma.
I think this is about finding a balance between the easiness of upgrading rules and safety.
- **How to handle**: ❓ Don't show the field in the upgrade flyout? Or show the field, and automatically pick the current customized version as the merged one? | ||
4. **`base=A, current=B, target=B => merged=B, conflict=false`** | ||
- **Situation**: Stock rule was installed. The user customized the field. Elastic updated this field in the latest version to exactly the same value as the user did it in their cluster. | ||
- **How to handle**: ❓ Don't show the field in the upgrade flyout? |
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.
Similar to situation 1, since there ends up being no delta would this rule even show up in the Rule Updates
table?
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 explained the difference in #148913 (comment)
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.
Yes, ideally not, can this be checked prior to loading the updates table though?
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.
@ARWNightingale If all fields of a given rule fall into this 4th category, or resolve into "no updates are required" in any other way, I think technically we definitely could hide this rule in the Rule Updates
table. I think the question is should we do that? Because:
- The chance of every single customized field having the same value as the corresponding updates from Elastic approaches zero, I'd guess.
- In reality, it's possible to get a new version of Elastic rule shipped with no real updates to the content. We track this bug in [Security Solution] Side effects of no-content version bumps in prebuilt rules #130576. If we hide such no-content rule updates, we would:
- on one hand, improve the UX
- on the other hand, hide this bug
I'd say let's try to get the bug fixed on the detection-rules package side (its release process), and only if it's not fully possible handle this in the app.
1d8ffe0
to
23ce178
Compare
7fb2b64
to
668d7fe
Compare
69a294b
to
4bcfc40
Compare
c707eb5
to
d4a9de1
Compare
df3988c
to
1da1c0d
Compare
2d81e04
to
a12ef1b
Compare
a12ef1b
to
efd8a1c
Compare
I'm closing this one since we've had an initial discussion and don't need to make any further decisions. We can revisit this PR later -- once we start working on customizing prebuilt rules. By then we'll probably have multiple dedicated tickets for handling conflicts -- one per each rule field or set of related fields. |
Epics: https://github.com/elastic/security-team/issues/1974 (internal), #174168
Summary
This "PR" provides some examples we could use as a foundation for discussing and designing the UI for showing rule field diffs and resolving conflicts.
First, it describes six typical situations we can have for any rule field.
Then, it expands on how each of these six situations can look like for the following fields:
NOTE: You can comment this PR description line-by-line in
security_solution/common/detection_engine/prebuilt_rules/rule_field_updates_and_conflicts.md
.There's no intention to merge this PR.
Typical situations
Given a rule that can be upgraded, for every top-level field of this rule, we will have 3 versions of the field:
Using the above 3 versions, the app will attempt to automatically merge them into the final Merged version. The merge can either succeed, or fail with a conflict. Users will have to resolve conflicts manually in the UI. This automatic merge is supposed to improve the UX by reducing the amount of work our users will have to do during rule upgrade.
Here are six typical situations that we will have to handle in the app:
base=A, current=A, target=A => merged=A, conflict=false
base=A, current=A, target=B => merged=B, conflict=false
base=A, current=B, target=A => merged=B, conflict=false
base=A, current=B, target=B => merged=B, conflict=false
base=A, current=B, target=C => merged=D, conflict=false
base=A, current=B, target=C => merged=C, conflict=true
Let's review these situations by example, using a few different fields.
Rule name
Rule name is one of the most simple fields to reason about: it's a one-line string.
base=A, current=A, target=A => merged=A, conflict=false
base=A, current=A, target=B => merged=B, conflict=false
base=A, current=B, target=A => merged=B, conflict=false
base=A, current=B, target=B => merged=B, conflict=false
base=A, current=B, target=C => merged=D, conflict=false
base=A, current=B, target=C => merged=C, conflict=true
Rule tags
The tags field is a slightly more complex field than the rule name, because it's an array of simple single-line strings. This gives us an opportunity to auto-merge changes in it.
base=A, current=A, target=A => merged=A, conflict=false
base=A, current=A, target=B => merged=B, conflict=false
base=A, current=B, target=A => merged=B, conflict=false
base=A, current=B, target=B => merged=B, conflict=false
base=A, current=B, target=C => merged=D, conflict=false
base=A, current=B, target=C => merged=C, conflict=true
Data source: index patterns or data view
Data source is an object that can represent two different types of sources:
Examples of situations:
base=A, current=A, target=A => merged=A, conflict=false
base=A, current=A, target=B => merged=B, conflict=false
base=A, current=B, target=A => merged=B, conflict=false
base=A, current=B, target=B => merged=B, conflict=false
base=A, current=B, target=C => merged=D, conflict=false
base=A, current=B, target=C => merged=C, conflict=true
Query
Rule query is a complex object that can represent two different types of queries:
Examples of situations:
base=A, current=A, target=A => merged=A, conflict=false
base=A, current=A, target=B => merged=B, conflict=false
base=A, current=B, target=A => merged=B, conflict=false
base=A, current=B, target=B => merged=B, conflict=false
base=A, current=B, target=C => merged=D, conflict=false
base=A, current=B, target=C => merged=C, conflict=true