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.

Sign up for GitHub

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

Jump to bottom

Alerting: Add RBAC logic for silences creation #87322

Open
wants to merge 23 commits into
base: tmp/silence-with-metadata-TR
Choose a base branch
from
Open

Alerting: Add RBAC logic for silences creation #87322

wants to merge 23 commits into from

Conversation

tomratcliffe
Copy link
Member

@tomratcliffe tomratcliffe commented May 3, 2024 •
edited

What is this feature?

Adds RBAC logic for Silences creation UI.

Note

Relies on the changes from #88000. This PR is targeting a version of that branch + latest from main (I will change to main once that PR is merged)
Also relies on that PR changing the response structure so that it returns accessControl and not metadata.permissions

When scoped in a role, will only allow silence creation for rules with the specific folder.
If the user has the global permissions (i.e. alert instances create), then they can create global silences (those that do not target a specific alert rule UID).

Custom role (replace bdeymnpj3jhtse with some folder UID from your environment):

---
# config file version
apiVersion: 2

roles:
  - name: 'Silences for folder'
    uid: customsilence
    description: 'Create/write silences for specific folder'
    version: 1
    orgId: 1
    permissions:
      - action: 'alert.silences:create'
        scope: 'folders:uid:bdeymnpj3jhtse'
      - action: 'alert.silences:write'
        scope: 'folders:uid:bdeymnpj3jhtse'
      - action: 'alert.silences:read'
        scope: 'folders:uid:bdeymnpj3jhtse'

@tomratcliffe tomratcliffe force-pushed the alerting/silences-rbac-ui branch 4 times, most recently from 1559db5 to 0c06b39 Compare May 4, 2024 07:17
@tomratcliffe tomratcliffe marked this pull request as ready for review May 7, 2024 13:10
@tomratcliffe tomratcliffe requested review from a team as code owners May 7, 2024 13:10
@tomratcliffe tomratcliffe requested review from joshhunt, Clarity-89, gillesdemey, konrad147 and soniaAguilarPeiron and removed request for a team May 7, 2024 13:10
@tomratcliffe
Copy link
Member Author

/deploy-to-hg

@ephemeral-instances-bot
Copy link

  • Preparing your instance. A comment containing your instance's url will be added to this PR when the instance is ready.
  • Your instance will be ready in ~10 minutes. Follow the workflow progress
  • Slack channel: #proj-ephemeral-hg-instances
  • Building instance with alerting/silences-rbac-ui oss branch and main enterprise branch. How to choose a branch

@ephemeral-instances-bot
Copy link

@tomratcliffe tomratcliffe added this to the 11.1.x milestone May 10, 2024
@tomratcliffe
Copy link
Member Author

/deploy-to-hg

@ephemeral-instances-bot
Copy link

  • Preparing your instance. A comment containing your instance's url will be added to this PR when the instance is ready.
  • Your instance will be ready in ~10 minutes. Follow the workflow progress
  • Slack channel: #proj-ephemeral-hg-instances
  • Building instance with alerting/silences-rbac-ui oss branch and main enterprise branch. How to choose a branch

@ephemeral-instances-bot
Copy link

gillesdemey
gillesdemey reviewed May 28, 2024
View reviewed changes
public/app/features/alerting/unified/components/AlertManagerPicker.tsx Outdated
Comment on lines 34 to 38
// User may have selected an alertmanager elsewhere in the application that has then ended up being filtered out
// In this case, we default back to Grafana AM
const selectedValue = options.some((am) => am.value === selectedAlertmanager)
? selectedAlertmanager
: GRAFANA_RULES_SOURCE_NAME;
Copy link
Member

Choose a reason for hiding this comment

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

IIRC the user wouldn't see anything only if they have no access to either the internal AM or any external AMs.

With this change I believe we would be showing the Grafana AM even if they might not have access to it?

public/app/features/alerting/unified/components/rule-viewer/RuleViewer.tsx Outdated Show resolved Hide resolved
public/app/features/alerting/unified/components/silences/SilenceDetails.tsx Show resolved Hide resolved
public/app/features/alerting/unified/components/silences/SilencedAlertsTableRow.tsx
@@ -35,7 +35,7 @@ export const SilencedAlertsTableRow = ({ alert, className }: Props) => {
<td>
<AmAlertStateTag state={alert.status.state} />
</td>
<td>for {duration} seconds</td>
<td>for {duration}</td>
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! :D

public/app/features/alerting/unified/components/silences/SilencesTable.tsx
return (
<>
<Divider />
<SilenceDetails silence={data} />
Copy link
Member

Choose a reason for hiding this comment

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

Much nicer!

gillesdemey
gillesdemey approved these changes May 29, 2024
View reviewed changes
Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gillesdemey gillesdemey gillesdemey approved these changes

@JacobsonMT JacobsonMT Awaiting requested review from JacobsonMT JacobsonMT was automatically assigned from grafana/alerting-backend-product

@konrad147 konrad147 Awaiting requested review from konrad147 konrad147 is a code owner automatically assigned from grafana/alerting-frontend

@soniaAguilarPeiron soniaAguilarPeiron Awaiting requested review from soniaAguilarPeiron soniaAguilarPeiron is a code owner automatically assigned from grafana/alerting-frontend

Assignees
No one assigned
Labels
add to changelog add to what's new area/alerting Grafana Alerting area/frontend
Projects
Alerting
Status: In review
Milestone
11.1.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tomratcliffe @soniaAguilarPeiron @gillesdemey