-
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
[RAC][Rule Registry] Fix bug where namespaces with dashes could cause conflicts #107991
[RAC][Rule Registry] Fix bug where namespaces with dashes could cause conflicts #107991
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
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 nesting depth of the try
s and if
s is reaching a concerning level, but it looks correct to me. This is one of the instances when using a flat promise chain might pay off.
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 would be great to see some unit / integration tests codifying the proposed fix here.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
Looks great 👍
We could probably refactor createWriteTargetIfNeeded
a bit, but this can be done later (I'll make sure to do some cleanup work after all the important issues are addressed).
The comments are incredibly helpful 🙏
I agree with @dhurley14 that we need to start covering indexing logic with tests, I'd try to start working on tests in 7.15 as a separate task if we have enough time and free hands.
What I did: review the changes.
What I didn't do: checkout and test locally.
try { | ||
// When a new namespace is created we expect getAlias to return a 404 error, | ||
// we'll catch it below and continue on. A non-404 error is a real problem so we throw. | ||
|
||
// It's critical that we specify *both* the index pattern and alias in this request. The alias prevents the | ||
// request from finding other namespaces that could match the -* part of the index pattern | ||
// (see https://github.com/elastic/kibana/issues/107704). The index pattern prevents the request from | ||
// finding legacy .siem-signals indices that we add the alias to for backwards compatibility reasons. Together, | ||
// the index pattern and alias should ensure that we retrieve only the "new" backing indices for this | ||
// particular alias. | ||
const { body: aliases } = await clusterClient.indices.getAlias({ |
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.
These are awesome comments in the code 👍 👍
… conflicts (elastic#107991) * Fix bug where namespaces with dashes could cause conflicts * Missing word in comment * Apply logic changes to consolidated RuleData implementation
Summary
Related to #107704
If one namespace is a prefix of another namespace followed by a dash, then the
-*
at the end of an index pattern would cause the pattern to match both namespace's indices. For example, spaces namedmanagers
andmanagers-usa
would conflict because in the index creation logic ifmanagers-usa
creates an index first then this would causecreateWriteTargetIfNeeded
to find indices that match.alerts-...-managers-*
and not create any index for themanagers
namespace. This PR fixes that issue by fetching indices for a specific alias and also filtering by the index pattern, only creating a new write index if no existing indices are found.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers