From 170d476bdc31430b16a426c8875a35b4d8f49980 Mon Sep 17 00:00:00 2001 From: Tom Ratcliffe Date: Wed, 5 Jun 2024 20:09:26 +0100 Subject: [PATCH] Alerting: Add RBAC logic for silences creation (#87322) * Remove role requirement for editing silence (instead handled by silence editor displaying error) * Send query params for metadata/access control silence logic * Add new access control types to enum * Add folder-specific checks for silences * Remove filtering in alert manager picker * fix flakey rule viewer test and update permissions helper * Use `returnTo` in rule viewer page * Fix incorrect display of duration * Clean up some mock server behaviour in rule details tests * Tweak styles for silences alerts table * Remove alertmanager picker from silences drawer * Add error if user cannot edit a silence * Show alert rule name in silences table and consume RBAC logic * Update mocks to include RBAC access control logic * Update silences tests * Update silences type to include access control info * Update comment for missing alertmanager * Update mock handlers and query param logic * Tweak query params again * Update access control mock * Revert AM picker fix as user may not have access to Grafana AM * Swap ternary order * Change text for no alert rule targeted * Don't show error alert from RTKQ query and remove alert instance preview in case of error * Add missing translations * Fix test adding missing mock for getting alert rule * Add missing translations in SilencesTable * Add translations autogenerated files * fix allowing edit silence in external alert manager --------- Co-authored-by: Sonia Aguilar Co-authored-by: Yuri Tseretyan --- .betterer.results | 15 +- public/app/features/alerting/routes.tsx | 4 - .../alerting/unified/Silences.test.tsx | 88 ++++++----- .../alerting/unified/api/alertSilencesApi.ts | 18 ++- .../alerting/unified/api/alertmanagerApi.ts | 11 +- .../unified/components/AlertManagerPicker.tsx | 37 ++--- .../rule-viewer/RuleViewer.test.tsx | 69 ++++----- .../components/rule-viewer/RuleViewer.tsx | 4 +- .../components/rules/RuleDetails.test.tsx | 58 +------ .../components/silences/SilenceDetails.tsx | 13 +- .../silences/SilenceGrafanaRuleDrawer.tsx | 16 +- .../silences/SilencedAlertsTable.tsx | 9 +- .../silences/SilencedAlertsTableRow.tsx | 2 +- .../silences/SilencedInstancesPreview.tsx | 14 +- .../components/silences/SilencesEditor.tsx | 14 +- .../components/silences/SilencesTable.tsx | 143 +++++++++++++----- .../unified/hooks/useAbilities.test.tsx | 63 +++++--- .../alerting/unified/hooks/useAbilities.ts | 24 ++- .../app/features/alerting/unified/mockApi.ts | 4 + public/app/features/alerting/unified/mocks.ts | 33 +++- .../unified/mocks/server/handlers/silences.ts | 35 ++++- .../plugins/datasource/alertmanager/types.ts | 11 +- public/app/types/accessControl.ts | 5 + public/locales/en-US/grafana.json | 15 ++ public/locales/pseudo-LOCALE/grafana.json | 15 ++ 25 files changed, 446 insertions(+), 274 deletions(-) diff --git a/.betterer.results b/.betterer.results index c51790263b9..3a181be57f2 100644 --- a/.betterer.results +++ b/.betterer.results @@ -2463,13 +2463,8 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], [0, 0, 0, "No untranslated strings. Wrap text with ", "4"] ], - "public/app/features/alerting/unified/components/silences/SilencedAlertsTable.tsx:5381": [ - [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] - ], "public/app/features/alerting/unified/components/silences/SilencedAlertsTableRow.tsx:5381": [ - [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] + [0, 0, 0, "No untranslated strings. Wrap text with ", "0"] ], "public/app/features/alerting/unified/components/silences/SilencedInstancesPreview.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], @@ -2490,14 +2485,6 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], [0, 0, 0, "No untranslated strings. Wrap text with ", "4"] ], - "public/app/features/alerting/unified/components/silences/SilencesTable.tsx:5381": [ - [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "1"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "2"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "4"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "5"] - ], "public/app/features/alerting/unified/home/GettingStarted.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], [0, 0, 0, "No untranslated strings. Wrap text with ", "1"], diff --git a/public/app/features/alerting/routes.tsx b/public/app/features/alerting/routes.tsx index c34f680bc8e..f00ebfa8c20 100644 --- a/public/app/features/alerting/routes.tsx +++ b/public/app/features/alerting/routes.tsx @@ -79,10 +79,6 @@ export function getAlertingRoutes(cfg = config): RouteDescriptor[] { }, { path: '/alerting/silence/:id/edit', - roles: evaluateAccess([ - AccessControlAction.AlertingInstanceUpdate, - AccessControlAction.AlertingInstancesExternalWrite, - ]), component: importAlertingComponent( () => import(/* webpackChunkName: "AlertSilences" */ 'app/features/alerting/unified/Silences') ), diff --git a/public/app/features/alerting/unified/Silences.test.tsx b/public/app/features/alerting/unified/Silences.test.tsx index 739d4bb47fe..7f338d58962 100644 --- a/public/app/features/alerting/unified/Silences.test.tsx +++ b/public/app/features/alerting/unified/Silences.test.tsx @@ -1,47 +1,49 @@ import React from 'react'; -import { render, waitFor, userEvent, screen } from 'test/test-utils'; +import { render, screen, userEvent, waitFor, within } from 'test/test-utils'; import { byLabelText, byPlaceholderText, byRole, byTestId, byText } from 'testing-library-selector'; import { dateTime } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; -import { config, locationService, setDataSourceSrv } from '@grafana/runtime'; -import { setupMswServer } from 'app/features/alerting/unified/mockApi'; +import { config, locationService } from '@grafana/runtime'; +import { mockAlertRuleApi, setupMswServer } from 'app/features/alerting/unified/mockApi'; import { waitForServerRequest } from 'app/features/alerting/unified/mocks/server/events'; +import { MOCK_GRAFANA_ALERT_RULE_TITLE } from 'app/features/alerting/unified/mocks/server/handlers/alertRules'; import { - MOCK_DATASOURCE_UID_BROKEN_ALERTMANAGER, MOCK_DATASOURCE_NAME_BROKEN_ALERTMANAGER, + MOCK_DATASOURCE_UID_BROKEN_ALERTMANAGER, } from 'app/features/alerting/unified/mocks/server/handlers/datasources'; import { silenceCreateHandler } from 'app/features/alerting/unified/mocks/server/handlers/silences'; -import { MatcherOperator } from 'app/plugins/datasource/alertmanager/types'; +import { MatcherOperator, SilenceState } from 'app/plugins/datasource/alertmanager/types'; import { AccessControlAction } from 'app/types'; import Silences from './Silences'; -import { grantUserPermissions, MOCK_SILENCE_ID_EXISTING, mockDataSource, MockDataSourceSrv } from './mocks'; -import { AlertmanagerProvider } from './state/AlertmanagerContext'; +import { + MOCK_SILENCE_ID_EXISTING, + MOCK_SILENCE_ID_EXISTING_ALERT_RULE_UID, + MOCK_SILENCE_ID_LACKING_PERMISSIONS, + grantUserPermissions, + mockDataSource, + mockSilences, +} from './mocks'; +import { grafanaRulerRule } from './mocks/alertRuleApi'; import { setupDataSources } from './testSetup/datasources'; -import { DataSourceType } from './utils/datasource'; +import { DataSourceType, GRAFANA_RULES_SOURCE_NAME } from './utils/datasource'; jest.mock('app/core/services/context_srv'); const TEST_TIMEOUT = 60000; const renderSilences = (location = '/alerting/silences/') => { - locationService.push(location); - return render( - - - , - { - historyOptions: { - initialEntries: [location], - }, - } - ); + return render(, { + historyOptions: { + initialEntries: [location], + }, + }); }; const dataSources = { am: mockDataSource({ - name: 'Alertmanager', + name: GRAFANA_RULES_SOURCE_NAME, type: DataSourceType.Alertmanager, }), [MOCK_DATASOURCE_NAME_BROKEN_ALERTMANAGER]: mockDataSource({ @@ -61,6 +63,7 @@ const ui = { addSilenceButton: byRole('link', { name: /add silence/i }), queryBar: byPlaceholderText('Search'), existingSilenceNotFound: byRole('alert', { name: /existing silence .* not found/i }), + noPermissionToEdit: byRole('alert', { name: /do not have permission/i }), editor: { timeRange: byTestId(selectors.components.TimePicker.openButton), durationField: byLabelText('Duration'), @@ -74,6 +77,7 @@ const ui = { addMatcherButton: byRole('button', { name: 'Add matcher' }), submit: byText(/save silence/i), createdBy: byText(/created by \*/i), + loadingIndicator: byTestId('Spinner'), }, }; @@ -107,7 +111,7 @@ const addAdditionalMatcher = async () => { await user.click(ui.editor.addMatcherButton.get()); }; -setupMswServer(); +const server = setupMswServer(); beforeEach(() => { setupDataSources(dataSources.am, dataSources[MOCK_DATASOURCE_NAME_BROKEN_ALERTMANAGER]); @@ -117,10 +121,6 @@ describe('Silences', () => { beforeAll(resetMocks); afterEach(resetMocks); - beforeEach(() => { - setDataSourceSrv(new MockDataSourceSrv(dataSources)); - }); - it( 'loads and shows silences', async () => { @@ -133,10 +133,10 @@ describe('Silences', () => { expect(ui.expiredTable.get()).toBeInTheDocument(); const allSilences = ui.silenceRow.queryAll(); - expect(allSilences).toHaveLength(3); - expect(allSilences[0]).toHaveTextContent('foo=bar'); - expect(allSilences[1]).toHaveTextContent('foo!=bar'); - expect(allSilences[2]).toHaveTextContent('foo=bar'); + expect(allSilences).toHaveLength(mockSilences.length); + expect(within(allSilences[0]).getByLabelText('Tags')).toHaveTextContent('foo=bar'); + expect(within(allSilences[1]).getByLabelText('Tags')).toHaveTextContent('foo!=bar'); + expect(allSilences[2]).toHaveTextContent(MOCK_GRAFANA_ALERT_RULE_TITLE); await user.click(ui.expiredCaret.get()); @@ -144,7 +144,10 @@ describe('Silences', () => { expect(ui.expiredTable.query()).not.toBeInTheDocument(); const activeSilences = ui.silenceRow.queryAll(); - expect(activeSilences).toHaveLength(2); + const expectedActiveSilences = mockSilences.filter( + (silence) => silence.status.state !== SilenceState.Expired + ).length; + expect(activeSilences).toHaveLength(expectedActiveSilences); expect(activeSilences[0]).toHaveTextContent('foo=bar'); expect(activeSilences[1]).toHaveTextContent('foo!=bar'); }, @@ -161,6 +164,7 @@ describe('Silences', () => { expect(notExpiredTable).toBeInTheDocument(); const silencedAlertRows = await ui.silencedAlertCell.findAll(notExpiredTable); + expect(silencedAlertRows[0]).toHaveTextContent('2'); expect(silencedAlertRows[1]).toHaveTextContent('0'); }, @@ -175,8 +179,8 @@ describe('Silences', () => { const queryBar = await ui.queryBar.find(); await user.type(queryBar, 'foo=bar'); - - await waitFor(() => expect(ui.silenceRow.getAll()).toHaveLength(2)); + await screen.findByRole('button', { name: /clear filters/i }); + expect(ui.silenceRow.getAll()).toHaveLength(1); }, TEST_TIMEOUT ); @@ -211,6 +215,7 @@ describe('Silence create/edit', () => { afterEach(resetMocks); beforeEach(() => { + mockAlertRuleApi(server).getAlertRule(MOCK_SILENCE_ID_EXISTING_ALERT_RULE_UID, grafanaRulerRule); setUserLogged(true); }); @@ -258,7 +263,7 @@ describe('Silence create/edit', () => { 'creates a new silence', async () => { const user = userEvent.setup(); - renderSilences(`${baseUrlPath}?alertmanager=Alertmanager`); + renderSilences(`${baseUrlPath}?alertmanager=${GRAFANA_RULES_SOURCE_NAME}`); expect(await ui.editor.durationField.find()).toBeInTheDocument(); const postRequest = waitForServerRequest(silenceCreateHandler()); @@ -314,6 +319,11 @@ describe('Silence create/edit', () => { expect(await ui.existingSilenceNotFound.find()).toBeInTheDocument(); }); + it('shows an error when user cannot edit/recreate silence', async () => { + renderSilences(`/alerting/silence/${MOCK_SILENCE_ID_LACKING_PERMISSIONS}/edit`); + expect(await ui.noPermissionToEdit.find()).toBeInTheDocument(); + }); + it('populates form with existing silence information', async () => { renderSilences(`/alerting/silence/${MOCK_SILENCE_ID_EXISTING}/edit`); @@ -321,7 +331,13 @@ describe('Silence create/edit', () => { // existing fields have been filled out as well await waitFor(() => expect(ui.editor.matcherName.get()).toHaveValue('foo')); expect(ui.editor.matcherValue.get()).toHaveValue('bar'); - expect(ui.editor.comment.get()).toHaveValue('Silence noisy alerts'); + expect(ui.editor.comment.get()).toHaveValue('Happy path silence'); + }); + + it('populates form with existing silence information that has __alert_rule_uid__', async () => { + mockAlertRuleApi(server).getAlertRule(MOCK_SILENCE_ID_EXISTING_ALERT_RULE_UID, grafanaRulerRule); + renderSilences(`/alerting/silence/${MOCK_SILENCE_ID_EXISTING_ALERT_RULE_UID}/edit`); + expect(await screen.findByLabelText(/alert rule/i)).toHaveValue(grafanaRulerRule.grafana_alert.title); }); it( @@ -331,7 +347,7 @@ describe('Silence create/edit', () => { const postRequest = waitForServerRequest(silenceCreateHandler()); - renderSilences(`${baseUrlPath}?alertmanager=Alertmanager`); + renderSilences(`${baseUrlPath}?alertmanager=${GRAFANA_RULES_SOURCE_NAME}`); await waitFor(() => expect(ui.editor.durationField.query()).not.toBeNull()); await enterSilenceLabel(0, 'foo', MatcherOperator.equal, 'bar'); @@ -340,7 +356,7 @@ describe('Silence create/edit', () => { expect(await ui.notExpiredTable.find()).toBeInTheDocument(); - expect(locationService.getSearch().get('alertmanager')).toBe('Alertmanager'); + expect(locationService.getSearch().get('alertmanager')).toBe(GRAFANA_RULES_SOURCE_NAME); const createSilenceRequest = await postRequest; const requestBody = await createSilenceRequest.clone().json(); diff --git a/public/app/features/alerting/unified/api/alertSilencesApi.ts b/public/app/features/alerting/unified/api/alertSilencesApi.ts index f3645ee6a45..3c361f270a5 100644 --- a/public/app/features/alerting/unified/api/alertSilencesApi.ts +++ b/public/app/features/alerting/unified/api/alertSilencesApi.ts @@ -14,10 +14,17 @@ export const alertSilencesApi = alertingApi.injectEndpoints({ Silence[], { datasourceUid: string; + ruleMetadata?: boolean; + accessControl?: boolean; } >({ - query: ({ datasourceUid }) => ({ + query: ({ datasourceUid, ruleMetadata, accessControl }) => ({ url: `/api/alertmanager/${datasourceUid}/api/v2/silences`, + params: { + ruleMetadata, + // query param is lowercased on backend for consistency with folder endpoint + accesscontrol: accessControl, + }, }), providesTags: (result) => result ? result.map(({ id }) => ({ type: 'AlertmanagerSilences', id })) : ['AlertmanagerSilences'], @@ -28,11 +35,18 @@ export const alertSilencesApi = alertingApi.injectEndpoints({ { datasourceUid: string; id: string; + ruleMetadata?: boolean; + accessControl?: boolean; } >({ - query: ({ datasourceUid, id }) => ({ + query: ({ datasourceUid, id, ruleMetadata, accessControl }) => ({ url: `/api/alertmanager/${datasourceUid}/api/v2/silence/${id}`, showErrorAlert: false, + params: { + ruleMetadata, + // query param is lowercased on backend for consistency with folder endpoint + accesscontrol: accessControl, + }, }), providesTags: (result, error, { id }) => result ? [{ type: 'AlertmanagerSilences', id }] : ['AlertmanagerSilences'], diff --git a/public/app/features/alerting/unified/api/alertmanagerApi.ts b/public/app/features/alerting/unified/api/alertmanagerApi.ts index 8530b3fc13f..f519acd1259 100644 --- a/public/app/features/alerting/unified/api/alertmanagerApi.ts +++ b/public/app/features/alerting/unified/api/alertmanagerApi.ts @@ -4,13 +4,13 @@ import { dispatch } from 'app/store/store'; import { ReceiversStateDTO } from 'app/types/alerting'; import { + AlertManagerCortexConfig, AlertmanagerAlert, AlertmanagerChoice, - AlertManagerCortexConfig, AlertmanagerGroup, - GrafanaAlertingConfiguration, ExternalAlertmanagersConnectionStatus, ExternalAlertmanagersStatusResponse, + GrafanaAlertingConfiguration, GrafanaManagedContactPoint, Matcher, MuteTimeInterval, @@ -19,8 +19,8 @@ import { NotifierDTO } from '../../../../types'; import { withPerformanceLogging } from '../Analytics'; import { matcherToOperator } from '../utils/alertmanager'; import { - getDatasourceAPIUid, GRAFANA_RULES_SOURCE_NAME, + getDatasourceAPIUid, isVanillaPrometheusAlertManagerDataSource, } from '../utils/datasource'; import { retryWhile, wrapWithQuotes } from '../utils/misc'; @@ -52,9 +52,9 @@ export const alertmanagerApi = alertingApi.injectEndpoints({ endpoints: (build) => ({ getAlertmanagerAlerts: build.query< AlertmanagerAlert[], - { amSourceName: string; filter?: AlertmanagerAlertsFilter } + { amSourceName: string; filter?: AlertmanagerAlertsFilter; showErrorAlert?: boolean } >({ - query: ({ amSourceName, filter }) => { + query: ({ amSourceName, filter, showErrorAlert = true }) => { // TODO Add support for active, silenced, inhibited, unprocessed filters const filterMatchers = filter?.matchers ?.filter((matcher) => matcher.name && matcher.value) @@ -77,6 +77,7 @@ export const alertmanagerApi = alertingApi.injectEndpoints({ return { url: `/api/alertmanager/${getDatasourceAPIUid(amSourceName)}/api/v2/alerts`, params, + showErrorAlert, }; }, providesTags: ['AlertmanagerAlerts'], diff --git a/public/app/features/alerting/unified/components/AlertManagerPicker.tsx b/public/app/features/alerting/unified/components/AlertManagerPicker.tsx index a7fbb7507bb..a18fb39981e 100644 --- a/public/app/features/alerting/unified/components/AlertManagerPicker.tsx +++ b/public/app/features/alerting/unified/components/AlertManagerPicker.tsx @@ -9,43 +9,32 @@ import { AlertManagerDataSource, GRAFANA_RULES_SOURCE_NAME } from '../utils/data interface Props { disabled?: boolean; - /** - * If true, only show alertmanagers that are receiving alerts from Grafana - */ - showOnlyReceivingGrafanaAlerts?: boolean; } function getAlertManagerLabel(alertManager: AlertManagerDataSource) { return alertManager.name === GRAFANA_RULES_SOURCE_NAME ? 'Grafana' : alertManager.name.slice(0, 37); } -export const AlertManagerPicker = ({ disabled = false, showOnlyReceivingGrafanaAlerts }: Props) => { +export const AlertManagerPicker = ({ disabled = false }: Props) => { const styles = useStyles2(getStyles); const { selectedAlertmanager, availableAlertManagers, setSelectedAlertmanager } = useAlertmanager(); const options: Array> = useMemo(() => { - return availableAlertManagers - .filter(({ name, handleGrafanaManagedAlerts }) => { - const isReceivingGrafanaAlerts = name === GRAFANA_RULES_SOURCE_NAME || handleGrafanaManagedAlerts; - return showOnlyReceivingGrafanaAlerts ? isReceivingGrafanaAlerts : true; - }) - .map((ds) => ({ - label: getAlertManagerLabel(ds), - value: ds.name, - imgUrl: ds.imgUrl, - meta: ds.meta, - })); - }, [availableAlertManagers, showOnlyReceivingGrafanaAlerts]); + return availableAlertManagers.map((ds) => ({ + label: getAlertManagerLabel(ds), + value: ds.name, + imgUrl: ds.imgUrl, + meta: ds.meta, + })); + }, [availableAlertManagers]); + + const isDisabled = disabled || options.length === 1; + const label = isDisabled ? 'Alertmanager' : 'Choose Alertmanager'; return ( - +