-
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] Fixes detection page and details to work when signals are not prefixed with .siem-signals #75817
[Security solution] Fixes detection page and details to work when signals are not prefixed with .siem-signals #75817
Conversation
… the word .siem-signals
Pinging @elastic/siem (Team:SIEM) |
x-pack/plugins/security_solution/public/timelines/containers/helpers.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/containers/helpers.ts
Outdated
Show resolved
Hide resolved
detectionsTimelineIds.some((timelineId) => timelineId === id) && | ||
!defaultIndex.some((di) => di.toLowerCase().startsWith('.siem-signals')); | ||
isEqual(defaultIndex.sort(), defaultKibanaIndex.sort()); |
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 do not think we should do it like that since we are going to bring kibana index patterns and we might have more index than the defaultKibanaIndex and then we will have a bug again.
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.
and these tests won't help to realize that we have a bug back
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 am thinking more about this one, I think we should use the value of signalIndexName
from useSignalIndex
and check that with defaultIndex
. I feel like it might be more bulletproof that what we are offering/offered.
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 investigated that and since parts of the timeline are still using classes I will have to move upwards and use that hook before the class and then drill it down since hooks cannot be used directly in classes.
I was trying to avoid introducing more complexity and tech debt within timeline as this felt more like a temp workaround we're doing here already and the better fix is that timeline knows when it has its correct index being set before conducting its query generically.
I figured this is all just temporary code in other words and was trying to be careful to not add more complexity and debt by introducing more drill props into timeline within this PR as my primary mission.
With all that said, yes, if everyone feels comfortable with another drill prop and a this extra piece of data flowing downwards, I can change this with a pull request and add the additional drill props and the hook above the class.
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 will be fine with it, it is just a string.
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
After discussions today about risk and how this is not a documented feature to change the prefix we are going to put this PR on hold for the moment and rely on other 7.10.0 fixes for this to be a non-issue rather than a back-port. Issues with fixes were I am going to introduce more querying of the siem signals index and there's other activity with back-ports going on so I think less is more. |
Closing this PR for now. We shouldn't encounter bugs from this fix in the field and we do not document this feature and it's mostly a dev feature to utilize at this point. |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
Fixes Kibana 7.9.0
For on prem deployments but not cloud based ones, it is possible to name your signals index differently from the default prefix of
.siem-signals-${space id}
like so in yourkibana.yml
If you do rename it without the prefix, then it no longer loads correctly as we rely on a hard fixed name within the code. Instead you see this loading screen that never goes away:
This fixes it to where it works by using a equals compare between the kibana defaultIndex and the passed in index and refuses to load if it is the same as the defaultIndex and the id of the timeline is the detection page/detection details.
Checklist