From 05b6f7f3965410c80dbe13ac1a380cb7342c2398 Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Mon, 30 Oct 2023 18:24:21 +0100 Subject: [PATCH] Alerting: Apply negative matchers for route matching (#77292) --- .../alerting/unified/routeGroupsMatcher.ts | 12 +- .../notification-policies.test.ts.snap | 56 +++++++ .../alerting/unified/utils/matchers.ts | 31 ---- .../utils/notification-policies.test.ts | 62 ++++++- .../unified/utils/notification-policies.ts | 157 ++++++++++-------- 5 files changed, 207 insertions(+), 111 deletions(-) create mode 100644 public/app/features/alerting/unified/utils/__snapshots__/notification-policies.test.ts.snap diff --git a/public/app/features/alerting/unified/routeGroupsMatcher.ts b/public/app/features/alerting/unified/routeGroupsMatcher.ts index 9b5dd3c8c6f..80b951ffc14 100644 --- a/public/app/features/alerting/unified/routeGroupsMatcher.ts +++ b/public/app/features/alerting/unified/routeGroupsMatcher.ts @@ -32,17 +32,13 @@ export const routeGroupsMatcher = { instancesToMatch.forEach((instance) => { const matchingRoutes = findMatchingRoutes(normalizedRootRoute, Object.entries(instance)); - matchingRoutes.forEach(({ route, details, labelsMatch }) => { - // Only to convert Label[] to Labels[] - needs better approach - const matchDetails = new Map( - Array.from(details.entries()).map(([matcher, labels]) => [matcher, Object.fromEntries(labels)]) - ); - + matchingRoutes.forEach(({ route, labelsMatch }) => { const currentRoute = result.get(route.id); + if (currentRoute) { - currentRoute.push({ instance, matchDetails, labelsMatch }); + currentRoute.push({ instance, labelsMatch }); } else { - result.set(route.id, [{ instance, matchDetails, labelsMatch }]); + result.set(route.id, [{ instance, labelsMatch }]); } }); }); diff --git a/public/app/features/alerting/unified/utils/__snapshots__/notification-policies.test.ts.snap b/public/app/features/alerting/unified/utils/__snapshots__/notification-policies.test.ts.snap new file mode 100644 index 00000000000..891dc986133 --- /dev/null +++ b/public/app/features/alerting/unified/utils/__snapshots__/notification-policies.test.ts.snap @@ -0,0 +1,56 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`matchLabels should match with non-equal matchers 1`] = ` +Map { + [ + "team", + "operations", + ] => { + "match": true, + "matcher": [ + "team", + "=", + "operations", + ], + }, +} +`; + +exports[`matchLabels should match with non-matching matchers 1`] = ` +Map { + [ + "team", + "operations", + ] => { + "match": true, + "matcher": [ + "team", + "=", + "operations", + ], + }, +} +`; + +exports[`matchLabels should not match with a set of matchers 1`] = ` +Map { + [ + "team", + "operations", + ] => { + "match": true, + "matcher": [ + "team", + "=", + "operations", + ], + }, + [ + "foo", + "bar", + ] => { + "match": false, + "matcher": null, + }, +} +`; diff --git a/public/app/features/alerting/unified/utils/matchers.ts b/public/app/features/alerting/unified/utils/matchers.ts index acf751a1109..aa1b7187f85 100644 --- a/public/app/features/alerting/unified/utils/matchers.ts +++ b/public/app/features/alerting/unified/utils/matchers.ts @@ -109,34 +109,3 @@ export const normalizeMatchers = (route: Route): ObjectMatcher[] => { }; export type Label = [string, string]; -type OperatorPredicate = (labelValue: string, matcherValue: string) => boolean; -const OperatorFunctions: Record = { - [MatcherOperator.equal]: (lv, mv) => lv === mv, - [MatcherOperator.notEqual]: (lv, mv) => lv !== mv, - [MatcherOperator.regex]: (lv, mv) => new RegExp(mv).test(lv), - [MatcherOperator.notRegex]: (lv, mv) => !new RegExp(mv).test(lv), -}; - -function isLabelMatch(matcher: ObjectMatcher, label: Label) { - const [labelKey, labelValue] = label; - const [matcherKey, operator, matcherValue] = matcher; - - // not interested, keys don't match - if (labelKey !== matcherKey) { - return false; - } - - const matchFunction = OperatorFunctions[operator]; - if (!matchFunction) { - throw new Error(`no such operator: ${operator}`); - } - - return matchFunction(labelValue, matcherValue); -} - -// check if every matcher returns "true" for the set of labels -export function labelsMatchObjectMatchers(matchers: ObjectMatcher[], labels: Label[]) { - return matchers.every((matcher) => { - return labels.some((label) => isLabelMatch(matcher, label)); - }); -} diff --git a/public/app/features/alerting/unified/utils/notification-policies.test.ts b/public/app/features/alerting/unified/utils/notification-policies.test.ts index 2c2e3514533..4880ee33098 100644 --- a/public/app/features/alerting/unified/utils/notification-policies.test.ts +++ b/public/app/features/alerting/unified/utils/notification-policies.test.ts @@ -5,6 +5,7 @@ import { normalizeRoute, getInheritedProperties, computeInheritedTree, + matchLabels, } from './notification-policies'; import 'core-js/stable/structured-clone'; @@ -22,6 +23,7 @@ describe('findMatchingRoutes', () => { routes: [ { receiver: 'A', + object_matchers: [['team', MatcherOperator.equal, 'operations']], routes: [ { receiver: 'B1', @@ -29,10 +31,9 @@ describe('findMatchingRoutes', () => { }, { receiver: 'B2', - object_matchers: [['region', MatcherOperator.notEqual, 'europe']], + object_matchers: [['region', MatcherOperator.equal, 'nasa']], }, ], - object_matchers: [['team', MatcherOperator.equal, 'operations']], }, { receiver: 'C', @@ -55,6 +56,19 @@ describe('findMatchingRoutes', () => { expect(matches[0].route).toHaveProperty('receiver', 'A'); }); + it('should match route with negative matchers', () => { + const policiesWithNegative = { + ...policies, + routes: policies.routes?.concat({ + receiver: 'D', + object_matchers: [['name', MatcherOperator.notEqual, 'gilles']], + }), + }; + const matches = findMatchingRoutes(policiesWithNegative, [['name', 'konrad']]); + expect(matches).toHaveLength(1); + expect(matches[0].route).toHaveProperty('receiver', 'D'); + }); + it('should match child route of matching parent', () => { const matches = findMatchingRoutes(policies, [ ['team', 'operations'], @@ -378,3 +392,47 @@ describe('normalizeRoute', () => { expect(normalized).not.toHaveProperty('match_re'); }); }); + +describe('matchLabels', () => { + it('should match with non-matching matchers', () => { + const result = matchLabels( + [ + ['foo', MatcherOperator.equal, ''], + ['team', MatcherOperator.equal, 'operations'], + ], + [['team', 'operations']] + ); + + expect(result).toHaveProperty('matches', true); + expect(result.labelsMatch).toMatchSnapshot(); + }); + + it('should match with non-equal matchers', () => { + const result = matchLabels( + [ + ['foo', MatcherOperator.notEqual, 'bar'], + ['team', MatcherOperator.equal, 'operations'], + ], + [['team', 'operations']] + ); + + expect(result).toHaveProperty('matches', true); + expect(result.labelsMatch).toMatchSnapshot(); + }); + + it('should not match with a set of matchers', () => { + const result = matchLabels( + [ + ['foo', MatcherOperator.notEqual, 'bar'], + ['team', MatcherOperator.equal, 'operations'], + ], + [ + ['team', 'operations'], + ['foo', 'bar'], + ] + ); + + expect(result).toHaveProperty('matches', false); + expect(result.labelsMatch).toMatchSnapshot(); + }); +}); diff --git a/public/app/features/alerting/unified/utils/notification-policies.ts b/public/app/features/alerting/unified/utils/notification-policies.ts index 42f7c8951ba..81ce2822345 100644 --- a/public/app/features/alerting/unified/utils/notification-policies.ts +++ b/public/app/features/alerting/unified/utils/notification-policies.ts @@ -11,111 +11,84 @@ import { Labels } from 'app/types/unified-alerting-dto'; import { Label, normalizeMatchers } from './matchers'; -type OperatorPredicate = (labelValue: string, matcherValue: string) => boolean; - -const OperatorFunctions: Record = { - [MatcherOperator.equal]: (lv, mv) => lv === mv, - [MatcherOperator.notEqual]: (lv, mv) => lv !== mv, - [MatcherOperator.regex]: (lv, mv) => Boolean(lv.match(new RegExp(mv))), - [MatcherOperator.notRegex]: (lv, mv) => !Boolean(lv.match(new RegExp(mv))), -}; - -function isLabelMatch(matcher: ObjectMatcher, label: Label) { - const [labelKey, labelValue] = label; - const [matcherKey, operator, matcherValue] = matcher; - - // not interested, keys don't match - if (labelKey !== matcherKey) { - return false; - } - - const matchFunction = OperatorFunctions[operator]; - if (!matchFunction) { - throw new Error(`no such operator: ${operator}`); - } - - return matchFunction(labelValue, matcherValue); -} - +// If a policy has no matchers it still can be a match, hence matchers can be empty and match can be true +// So we cannot use null as an indicator of no match interface LabelMatchResult { match: boolean; - matchers: ObjectMatcher[]; + matcher: ObjectMatcher | null; } +type LabelsMatch = Map; + interface MatchingResult { matches: boolean; - details: Map; - labelsMatch: Map; + labelsMatch: LabelsMatch; } -// check if every matcher returns "true" for the set of labels -function matchLabels(matchers: ObjectMatcher[], labels: Label[]): MatchingResult { - const details = new Map(); +// returns a match results for given set of matchers (from a policy for instance) and a set of labels +export function matchLabels(matchers: ObjectMatcher[], labels: Label[]): MatchingResult { + const matches = matchLabelsSet(matchers, labels); - // If a policy has no matchers it still can be a match, hence matchers can be empty and match can be true - // So we cannot use empty array of matchers as an indicator of no match - const labelsMatch = new Map( - labels.map((label) => [label, { match: false, matchers: [] }]) - ); + // create initial map of label => match result + const labelsMatch: LabelsMatch = new Map(labels.map((label) => [label, { match: false, matcher: null }])); - const matches = matchers.every((matcher) => { - const matchingLabels = labels.filter((label) => isLabelMatch(matcher, label)); + // for each matcher, check which label it matched for + matchers.forEach((matcher) => { + const matchingLabel = labels.find((label) => isLabelMatch(matcher, label)); - matchingLabels.forEach((label) => { - const labelMatch = labelsMatch.get(label); - // The condition is just to satisfy TS. The map should have all the labels due to the previous map initialization - if (labelMatch) { - labelMatch.match = true; - labelMatch.matchers.push(matcher); - } - }); - - if (matchingLabels.length === 0) { - return false; + // record that matcher for the label + if (matchingLabel) { + labelsMatch.set(matchingLabel, { + match: true, + matcher, + }); } - - details.set(matcher, matchingLabels); - return matchingLabels.length > 0; }); - return { matches, details, labelsMatch }; + return { matches, labelsMatch }; +} + +// Compare set of matchers to set of label +export function matchLabelsSet(matchers: ObjectMatcher[], labels: Label[]): boolean { + for (const matcher of matchers) { + if (!isLabelMatchInSet(matcher, labels)) { + return false; + } + } + return true; } export interface AlertInstanceMatch { instance: Labels; - matchDetails: Map; - labelsMatch: Map; + labelsMatch: LabelsMatch; } export interface RouteMatchResult { route: T; - details: Map; - labelsMatch: Map; + labelsMatch: LabelsMatch; } // Match does a depth-first left-to-right search through the route tree // and returns the matching routing nodes. // If the current node is not a match, return nothing -// const normalizedMatchers = normalizeMatchers(root); // Normalization should have happened earlier in the code -function findMatchingRoutes(root: T, labels: Label[]): Array> { - let matches: Array> = []; +function findMatchingRoutes(route: T, labels: Label[]): Array> { + let childMatches: Array> = []; // If the current node is not a match, return nothing - const matchResult = matchLabels(root.object_matchers ?? [], labels); + const matchResult = matchLabels(route.object_matchers ?? [], labels); if (!matchResult.matches) { return []; } // If the current node matches, recurse through child nodes - if (root.routes) { - for (let index = 0; index < root.routes.length; index++) { - let child = root.routes[index]; + if (route.routes) { + for (const child of route.routes) { let matchingChildren = findMatchingRoutes(child, labels); // TODO how do I solve this typescript thingy? It looks correct to me /shrug // @ts-ignore - matches = matches.concat(matchingChildren); + childMatches = childMatches.concat(matchingChildren); // we have matching children and we don't want to continue, so break here if (matchingChildren.length && !child.continue) { break; @@ -124,11 +97,11 @@ function findMatchingRoutes(root: T, labels: Label[]): Array(parent: T): T { }; } -export { findMatchingAlertGroups, findMatchingRoutes, getInheritedProperties }; +type OperatorPredicate = (labelValue: string, matcherValue: string) => boolean; +const OperatorFunctions: Record = { + [MatcherOperator.equal]: (lv, mv) => lv === mv, + [MatcherOperator.notEqual]: (lv, mv) => lv !== mv, + [MatcherOperator.regex]: (lv, mv) => new RegExp(mv).test(lv), + [MatcherOperator.notRegex]: (lv, mv) => !new RegExp(mv).test(lv), +}; + +function isLabelMatchInSet(matcher: ObjectMatcher, labels: Label[]): boolean { + const [matcherKey, operator, matcherValue] = matcher; + + let labelValue = ''; // matchers that have no labels are treated as empty string label values + const labelForMatcher = Object.fromEntries(labels)[matcherKey]; + if (labelForMatcher) { + labelValue = labelForMatcher; + } + + const matchFunction = OperatorFunctions[operator]; + if (!matchFunction) { + throw new Error(`no such operator: ${operator}`); + } + + return matchFunction(labelValue, matcherValue); +} + +// ⚠️ DO NOT USE THIS FUNCTION FOR ROUTE SELECTION ALGORITHM +// for route selection algorithm, always compare a single matcher to the entire label set +// see "matchLabelsSet" +function isLabelMatch(matcher: ObjectMatcher, label: Label): boolean { + let [labelKey, labelValue] = label; + const [matcherKey, operator, matcherValue] = matcher; + + if (labelKey !== matcherKey) { + return false; + } + + const matchFunction = OperatorFunctions[operator]; + if (!matchFunction) { + throw new Error(`no such operator: ${operator}`); + } + + return matchFunction(labelValue, matcherValue); +} + +export { findMatchingAlertGroups, findMatchingRoutes, getInheritedProperties, isLabelMatchInSet };