-
Notifications
You must be signed in to change notification settings - Fork 8k
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] Fix incorrect detection rules sort order #130105
Conversation
fields: { | ||
keyword: { | ||
type: 'keyword', | ||
normalizer: 'lowercase', |
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.
The addition of a new normalizer to the existing fields creates a mappings conflict:
Mapper for [alert.name.keyword] conflicts with existing mapper:
Cannot update parameter [normalizer] from [null] to [lowercase]
However, I'm not sure if we should handle conflicts like that in code. I suppose it affects only dev environments but not users.
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 asked to @rudolf and we are in good shape, I tested locally and were able to move forward by deleting .kibana
const mappingKey = 'properties.' + key.split('.').join('.properties.'); | ||
const field = get(mappings, mappingKey); | ||
const field = get(alertMappings, mappingKey); | ||
if (field != null && field.type === 'nested') { | ||
localNestedKeys = ast.value; | ||
} |
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.
It looks like this condition was always false. field
was always undefined
because the mappingKey
starts with properties.
but according to the mappings.json
structure, the key should start with alert.properties.
. So I fixed that in this PR.
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.
Good catch!!!
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/response-ops (Team:ResponseOps) |
7a19913
to
0a6afe8
Compare
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.
💛 Build succeeded, but was flakyTest FailuresMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @xcrzx |
Resolves: #124335
Summary
Added
lowercase
normalizer to thealert.name.keyword
mappings. That makes rules sorting case-insensitive and produces the correct order when rules are sorted in UI.Before
After