Skip to content
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

Add monitoring-edit role #754

Conversation

simonpasquier
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020
@simonpasquier
Copy link
Contributor Author

cc @openshift/openshift-team-monitoring

- monitoring.coreos.com
resources:
- servicemonitors
- podmonitors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I'm missing context, but I would have expected a role for monitoring configurations, that is not just about targets but both scraping and alerting configuration

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For configuration we specified a dedicated role, see https://github.com/openshift/enhancements/blob/master/enhancements/monitoring/user-workload-monitoring.md#monitoring-targets-edit, and https://github.com/openshift/enhancements/blob/master/enhancements/monitoring/user-workload-monitoring.md#monitoring-workload-config-edit:

monitoring-targets-edit: This is a cluster role which can be bound against a concrete namespace by the cluster admin. Embeds the get/create/edit/delete permissions for the following custom resources:

  • ServiceMonitor
  • PodMonitor
  • PrometheusRule

It allows to create new scraping targets for services/pods and allows to create new recording or alerting rules.

monitoring-workload-config-edit:
This is a named role against the workload-monitoring-config configmap resource in the openshift-monitoring namespace embedding get/create/edit/delete permissions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or did we miss something else here? 🤔

- monitoring.coreos.com
resources:
- servicemonitors
- podmonitors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the OEP we need to have PrometheusRule here too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on an after-thought: I believe we added PrometheusRule too much in the enhancement proposal, because as per https://issues.redhat.com/browse/MON-936?focusedCommentId=13977634&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13977634 this refers only to ServiceMonitor/PodMonitor custom resources. We should submit a PR against the enhancement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I meant. I think PrometheusRule should be part of this, although then just "targets" doesn't make sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can unify monitoring-rules-edit and monitoring-targets-edit, but that would imply that the user cannot be given distinct permissions to silence alerts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

People can still create those roles if they need but I don't see a standard use case where the distinction needs to be present. The basic use case is self service monitoring, and a user will always need both for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a comment to the Jira ticket for Christian.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonpasquier we reached consensus that we need to add prometheusrule custom resources here. The OEP doesn't need an update as it specifies that already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a small change that we should do though, which is change the naming, as this doesn't reflect just targets. How about just monitoring-config-edit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about changing the name. The OEP already provisions a monitoring-workload-config-edit role so having monitoring-config-edit might be confusing?
Maybe simply monitoring-edit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with monitoring-edit as well.

@simonpasquier simonpasquier force-pushed the add-monitoring-targets-edit-role branch from 559daff to 59ef25e Compare April 15, 2020 11:48
@simonpasquier simonpasquier force-pushed the add-monitoring-targets-edit-role branch from 59ef25e to af6a5b4 Compare April 15, 2020 12:50
@@ -37,7 +37,7 @@ spec:
)
for: 5m
labels:
severity: critical
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsonnet file lock did not change? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also no need to bump this, as I will do as part of the release checklist thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what happens when you push code changes during a meeting 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha :D

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier simonpasquier force-pushed the add-monitoring-targets-edit-role branch from af6a5b4 to ef99daf Compare April 15, 2020 13:21
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

/hold

Feel free to unhold @brancz

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilic, simonpasquier

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [lilic,simonpasquier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@simonpasquier simonpasquier changed the title Add monitoring-targets-edit role Add monitoring-edit role Apr 15, 2020
@simonpasquier
Copy link
Contributor Author

/retest

@brancz
Copy link
Contributor

brancz commented Apr 15, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 2bd3798 into openshift:master Apr 16, 2020
@simonpasquier simonpasquier deleted the add-monitoring-targets-edit-role branch April 16, 2020 08:19
@juzhao
Copy link

juzhao commented Jan 25, 2021

/label qe-approved

@openshift-ci-robot openshift-ci-robot added the qe-approved Signifies that QE has signed off on this PR label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants