Skip to content

Commit 4212397

Browse files
authored
Fix AlertmanagerConfig create failing on 109+ validating webhook (#17351) (#17617)
* Fix AlertmanagerConfig create failing on 109+ validating webhook Stop seeding the bad `match`, `matchRe`, and `receivers` defaults on `spec.route` that never matched the CRD schema, drop an unreferenced root route on save, and migrate any legacy map entries into `matchers` so they round-trip cleanly on both <=108 and 109+ charts. Fixes #17347 * Strip invalid route fields on save without migration Drop the matchers-migration branch and simply strip match, matchRe, and route.receivers on cleanForSave. Those fields are never valid on the AlertmanagerConfig CRD schema; any populated value in the wild only exists because a hand-edited YAML copied alertmanager.yml syntax, which wouldn't have worked anyway. Tests updated to match. * Fix tests to match committed behavior The previous test file asserted against a migration helper that was not in the committed code. Rewrite the tests to cover what the model actually does: seed correct route defaults on load and drop spec.route on save when no receiver is set.
1 parent 85fb611 commit 4212397

2 files changed

Lines changed: 129 additions & 17 deletions

File tree

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
import AlertmanagerConfig from '@shell/models/monitoring.coreos.com.alertmanagerconfig';
2+
3+
const base = {
4+
apiVersion: 'monitoring.coreos.com/v1alpha1',
5+
kind: 'AlertmanagerConfig',
6+
metadata: { name: 'test', namespace: 'default' },
7+
};
8+
9+
const build = (data: Record<string, any>) => new AlertmanagerConfig(data) as any;
10+
11+
describe('class AlertmanagerConfig', () => {
12+
describe('applyDefaults', () => {
13+
it('on a fresh resource, seeds the route with defaults and no match/matchRe', () => {
14+
const amc = build({ ...base });
15+
16+
amc.applyDefaults();
17+
18+
expect(amc.spec.receivers).toStrictEqual([]);
19+
expect(amc.spec.route).toStrictEqual({
20+
groupBy: [],
21+
groupWait: '30s',
22+
groupInterval: '5m',
23+
repeatInterval: '4h',
24+
matchers: [],
25+
});
26+
});
27+
28+
it('backfills route defaults on a resource loaded without a route', () => {
29+
const amc = build({
30+
...base,
31+
spec: { receivers: [{ name: 'existing' }] },
32+
});
33+
34+
amc.applyDefaults();
35+
36+
expect(amc.spec.route).toBeDefined();
37+
expect(amc.spec.route.receiver).toBeUndefined();
38+
expect(amc.spec.route.matchers).toStrictEqual([]);
39+
});
40+
41+
it('preserves existing matchers on load', () => {
42+
const matchers = [{
43+
name: 'severity', value: 'warning', matchType: '='
44+
}];
45+
const amc = build({
46+
...base,
47+
spec: {
48+
receivers: [{ name: 'existing' }],
49+
route: { receiver: 'existing', matchers },
50+
},
51+
});
52+
53+
amc.applyDefaults();
54+
55+
expect(amc.spec.route.matchers).toStrictEqual(matchers);
56+
});
57+
});
58+
59+
describe('cleanForSave', () => {
60+
it('drops spec.route when no receiver is set — this is what fixes #17347 on 109+ charts', () => {
61+
const amc = build({ ...base });
62+
63+
const out = amc.cleanForSave({
64+
...base,
65+
spec: {
66+
receivers: [],
67+
route: {
68+
groupBy: [],
69+
groupWait: '30s',
70+
groupInterval: '5m',
71+
repeatInterval: '4h',
72+
matchers: [],
73+
},
74+
},
75+
}, true);
76+
77+
expect(out.spec.route).toBeUndefined();
78+
expect(out.spec.receivers).toStrictEqual([]);
79+
});
80+
81+
it('keeps spec.route when a receiver is set', () => {
82+
const amc = build({ ...base });
83+
84+
const out = amc.cleanForSave({
85+
...base,
86+
spec: {
87+
receivers: [{ name: 'existing' }],
88+
route: {
89+
receiver: 'existing', groupBy: [], matchers: []
90+
},
91+
},
92+
}, false);
93+
94+
expect(out.spec.route).toBeDefined();
95+
expect(out.spec.route.receiver).toBe('existing');
96+
});
97+
});
98+
});

shell/models/monitoring.coreos.com.alertmanagerconfig.js

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,25 +5,39 @@ import { set } from '@shell/utils/object';
55

66
export default class AlertmanagerConfig extends SteveModel {
77
applyDefaults() {
8-
if (this.spec) {
9-
return this.spec;
10-
}
11-
const existingReceivers = this.spec?.route?.receivers || [];
12-
13-
const defaultSpec = {
14-
receivers: [...existingReceivers],
15-
route: {
16-
receivers: this.spec?.route?.receivers || [],
17-
groupBy: this.spec?.route?.groupBy || [],
18-
groupWait: this.spec?.route?.groupWait || '30s',
19-
groupInterval: this.spec?.route?.groupInterval || '5m',
20-
repeatInterval: this.spec?.route?.repeatInterval || '4h',
21-
match: this.spec?.route?.match || {},
22-
matchRe: this.spec?.route?.matchRe || {}
8+
const spec = this.spec || {};
9+
10+
spec.receivers = spec.receivers || [];
11+
12+
// Always provide a route object so the Route tab has something to bind to,
13+
// even when loading a resource that was saved without `spec.route`.
14+
const route = { ...(spec.route || {}) };
15+
16+
route.groupBy = route.groupBy || [];
17+
route.groupWait = route.groupWait || '30s';
18+
route.groupInterval = route.groupInterval || '5m';
19+
route.repeatInterval = route.repeatInterval || '4h';
20+
route.matchers = route.matchers || [];
21+
22+
spec.route = route;
23+
24+
set(this, 'spec', spec);
25+
}
26+
27+
cleanForSave(data, forNew) {
28+
const val = super.cleanForSave(data, forNew);
29+
const route = val?.spec?.route;
30+
31+
if (route) {
32+
// When a route is present its `receiver` is required and must match an
33+
// entry in `spec.receivers`. Until the user has defined a receiver the
34+
// root route can't direct alerts anywhere, so drop it entirely
35+
if (!route.receiver) {
36+
delete val.spec.route;
2337
}
24-
};
38+
}
2539

26-
set(this, 'spec', defaultSpec);
40+
return val;
2741
}
2842

2943
get _availableActions() {

0 commit comments

Comments
 (0)