-
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
[Response Ops][RAM] Add RBAC Support for Multi-Consumer Rule Creation in O11Y and Stack Management #162605
[Response Ops][RAM] Add RBAC Support for Multi-Consumer Rule Creation in O11Y and Stack Management #162605
Conversation
Nice one! Some thoughts on the modal copy: Select rule association |
throw Boom.forbidden( | ||
getUnauthorizedMessage(ruleTypeId, ScopeType.Consumer, consumer, operation, entity) | ||
); | ||
throw Boom.forbidden(getUnauthorizedMessage(ruleTypeId, consumer, operation, entity)); |
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.
By looking at the tests failures, if you feel the error messages are now misleading, you could change all the getUnauthorizedMessage
calls to something like this (use legacyConsumer):
throw Boom.forbidden(getUnauthorizedMessage(ruleTypeId, consumer, operation, entity)); | |
throw Boom.forbidden(getUnauthorizedMessage(ruleTypeId, legacyConsumer, operation, entity)); |
Pinging @elastic/response-ops (Team:ResponseOps) |
@@ -30,8 +30,8 @@ export default function ({ getService }: FtrProviderContext) { | |||
return fieldStat.name === 'geo_point'; | |||
} | |||
); | |||
expect(geoPointFieldStats.count).to.be(31); | |||
expect(geoPointFieldStats.index_count).to.be(9); | |||
expect(geoPointFieldStats.count).to.be(39); |
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.
This value had to be incremented when we enabled the new generic threshold rules: https://github.com/elastic/kibana/pull/162605/files#diff-53c98e8d9e2d61d5c9f37b003fc6de5c3f84570f7886cf4bdd89f596b32a15c9R49
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.
kibana-gis changes LGTM
code review only
Pinging @elastic/apm-ui (Team:APM) |
Pinging @elastic/uptime (Team:uptime) |
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.
Changes LGTM! I left a few nitpicks but overall the changes are looking great!
x-pack/test/alerting_api_integration/security_and_spaces/group3/tests/alerting/bulk_enable.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/security_and_spaces/group3/tests/alerting/bulk_disable.ts
Outdated
Show resolved
Hide resolved
enabled: schema.boolean({ defaultValue: false }), | ||
enabled: schema.boolean({ defaultValue: true }), |
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.
Unless O11y is ready, we may want to reverse this change in the PR and let O11y change the default when they are ready.(unless we can add it to a todo list on the feature branch PR).
If we revert this, the following changes will have to be undone as well:
- x-pack/test/plugin_api_integration/test_suites/task_manager/check_registered_task_types.ts
- x-pack/test/api_integration/apis/maps/maps_telemetry.ts
I had this code in my POC to bypass having to set the kibana.yml settings for testing.
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.
Let's add it to a to-do list, since we have tests other than maps_telemetry that depend on this change. Also, this is merging to a feature branch so won't merge to main presumably until ES types are enabled in o11y (correct me if im wrong)
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.
@maryam-saeidi or @fkanout are best positioned to say whether this can be set to true already or not.
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.
We decided to not enable it for v8.10 and aim to either enable this in v8.11 or remove it and release the feature in beta.
| typeof AlertConsumers.UPTIME | ||
| typeof AlertConsumers.SLO | ||
| 'stackAlerts' | ||
| 'discover'; |
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.
From an offline discussion last week, we'll be moving away from discover
as a consumer, in favour of stackAlerts
.
We'll need to do one of two things:
- Remove this usage if no underlying issues exist
- Add it to a list of tasks to complete before the feature branch is merged (if we have a draft PR on the feature branch, we can create a checklist list in the description)
@@ -15,6 +15,9 @@ export { BASE_ACTION_API_PATH, INTERNAL_BASE_ACTION_API_PATH } from '@kbn/action | |||
|
|||
export type Section = 'connectors' | 'rules' | 'alerts' | 'logs'; | |||
|
|||
export const OBSERVABILITY_THRESHOLD_RULE_TYPE_ID = 'observability.rules.threshold'; | |||
export const ES_QUERY_RULE_TYPE_ID = '.es-query'; |
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.
nit: can we re-use ES_QUERY_ID
from x-pack/plugins/stack_alerts/server/rule_types/es_query/constants.ts, we'll have to move it into a common folder.
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 tried to do this, unfortunately, stack alerts has triggers actions UI as a dependency. If I import from stack alerts in triggers actions UI we get a circular dependency.
If we want to share this constantly then we probably move it to a package.
@@ -123,7 +126,7 @@ function InternalAlertsPage() { | |||
try { | |||
const response = await loadRuleAggregations({ | |||
http, | |||
typesFilter: observabilityRuleTypeRegistry.list(), | |||
typesFilter: filteredRuleTypes, |
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.
Thank you, way nicer.
💔 Build FailedFailed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
A small request on this (I know this is merged and we are close to FF—sorry). Can we set the defaults of the dropdown to the following?: Rationale: And, our expectation is that Logs will be the most common use case for this, so it makes sense to default to that. @katrin-freihofner Can you confirm this request please? Curious if there are any differing arguments to this as well. |
Summary
Resolves:
consumer
#162484alerts
#160677This PR adds the ability for
logs
and/orinfrastructure
only users to create and modify ES Query and new Generic Threshold rules. TheensureAuthorized
function is modified and simplified to support this use case, by skipping producer authorization and only authorizing for consumers. When the consumer isalerts
, we will consider this legacy and replace it with the rule’s producer (consumer = ruleType.producer
)There is now a dropdown in the rule form to prompt the user when creating ES Query/Generic threshold rules to select the consumer based on their authorized consumers (we can no longer use
alerts
for these). If there is only 1 option, then the dropdown will not be shown and the option will be chosen automatically.Generic threshold rules will have the following possible consumers:
ES query rules will have the following possible consumers:
To Test:
Single Consumer:
logs
feature enabled (ensuringstackAlerts
is not enabled).logs
set in theconsumer
fieldinfrastructure
feature.Multiple Consumers:
logs
,infrastructure
andapm
features enabled (ensuringstackAlerts
is not enabled).Checklist