-
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
Cleanup alerting / actions feature controls #52286
Cleanup alerting / actions feature controls #52286
Conversation
Pinging @elastic/kibana-stack-services (Team:Stack Services) |
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.
Had a few questions about feature controls that I'm hoping someone on @elastic/kibana-security team can help me with 🙏
💔 Build Failed |
@elastic/stack-monitoring I'm not sure if you wanted alerting privileges granted on master with this PR or if you wanted to do it yourself? Let me know, the code necessary is here and easy to remove. |
@elasticmachine merge upstream |
cc @peterschretlen so you're aware of the changes for 7.6. Let me know if this doesn't align. |
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.
Checked it out, ran a few test runs and everything still seems functional from the API standpoint, so I think we are good.
LGTM
…te/kibana into alerting/cleanup-feature-controls
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.
code LGTM
@mikecote From a user perspective, what changes with this PR for stack monitoring? I pulled it down but don't see anything different in the spaces configuration. |
From a user perspective, nothing will change when it comes to managing roles / users. Though they will see the alert / actions management screens once the PR merges (#48959) if they have |
Also, you will see from a technical perspective the alerting APIs get disabled if you don't have monitoring or SIEM access. |
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.
LGTM
Ya, that's slightly weird, but understandable. Two things:
|
I believe all features / privileges are calculated at run time so any change we do would automatically apply to the users on existing systems. This would mean once stop granting full
I think the future direction would be granting a default application to user defined alerts (ex: visualization). For that scenario, as long as users have access to visualizations, they would be able to use alerts. |
@mikecote I don't think that'll be a problem. Should be fine |
@chrisronline last question I had was did you want this PR to enable alerting for Stack Monitoring or did you have it in a feature branch somewhere and want to enable it there instead? |
What does that look like exactly? I guess I'm not sure what "enabling" means in this context? Make it a dependency on the plugin? |
Me and @chrisronline discussed offline that I will remove monitoring related changes in this PR and it will be added within their feature branch of alerting. |
Did we release any versions with the |
Thanks for the explanation, seems reasonable enough for us to just remove these features in this situation :) |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Initial work * Apply changes based on feedback * Fix a broken test * Fix failing test * Revert monitoring changes * Remove UI features for now
Resolves #51121.
This PR removes the
alerting
andaction
features from feature controls. Solution teams who want to use alerting will have to grant alerting privileges within their features as this PR does for SIEM.Note: Granting alerting privileges at this time will allow full control of alerts created by any solution team. This will be solved with #43994.
Note: The alerting UI within the management section will have to look at multiple places to know whether to show or not. This will be solved with #52300 and if needed, we can automate this beforehand.
Example: