alertmanager: honor optional MSTeams webhook secrets#8448
alertmanager: honor optional MSTeams webhook secrets#8448GauravJ3 wants to merge 2 commits intoprometheus-operator:mainprometheus-operator/prometheus-operator:mainfrom GauravJ3:codex/fix-optional-msteams-webhookGauravJ3/prometheus-operator:codex/fix-optional-msteams-webhookCopy head branch name to clipboard
Conversation
AlertmanagerConfig currently treats optional MSTeams webhook secret references as mandatory, which prevents config validation and rendering when the secret or key is absent. Teach the shared secret loader to honor SecretKeySelector.optional and skip MSTeams receiver entries whose optional webhook secret is missing. Add regression tests for store lookups, AlertmanagerConfig validation, and rendered Alertmanager config output. Closes prometheus-operator#7264 Signed-off-by: Gaurav Joshi <gauravjoshi12379@gmail.com>
|
Thanks for taking a look. I checked the failing CI job, and the only failing check is the ThanosRuler e2e job:
If this looks flaky on your side as well, could someone please rerun the failed e2e job? |
Fresa
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks great! I just had some small comments.
I'm not an owner/maintainer of this repo just as a FYI.
| }, | ||
| golden: "CR_with_MSTeams_Receiver_Partial_Conf.golden", | ||
| }, | ||
| { |
There was a problem hiding this comment.
Maybe we should add a test for v2 as well?
There was a problem hiding this comment.
Added MSTeamsV2 coverage in the follow-up commit so the config-generation path now has the same optional-missing-secret regression test as v1. Thanks for the catch.
| }, | ||
| ok: false, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Added the matching MSTeamsV2 validation coverage as well, so both the operator-side validation test and the config-generation test now cover v2.
Add validation and config-generation coverage for optional missing MSTeamsV2 webhook secrets so the regression tests match the v1 MSTeams coverage added in the previous commit. Signed-off-by: Gaurav Joshi <gauravjoshi12379@gmail.com>
|
Thanks for looking at this! As I wrote in #7264 (comment)
We have a chicken-and-egg problem because 1) the secret doesn't exist before the pods are created and 2) Alertmanager won't start if the msteams receiver has no webhook URL. In the best case, I assume that the pods will fail to start until the operator reads the secret but I haven't checked in real life. The limitation is also not restricted to the msteams receiver but applies to all AlertmanagerConfig fields of type |
Description
Honor
SecretKeySelector.optionalfor Microsoft Teams webhook secrets inAlertmanagerConfigprocessing.This change makes optional missing webhook secrets and keys non-fatal in the
shared secret loader and skips the affected MSTeams receiver entry when
rendering Alertmanager configuration. That keeps the rest of the
AlertmanagerConfig valid instead of failing reconciliation.
Closes: #7264
Type of change
CHANGE(fix or feature that would cause existing functionality to not work as expected)FEATURE(non-breaking change which adds functionality)BUGFIX(non-breaking change which fixes an issue)ENHANCEMENT(non-breaking change which improves existing functionality)NONE(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Verification
shared assets store.
webhook secrets.
when its optional webhook secret is missing.
go test ./pkg/assets ./pkg/alertmanager -run 'TestGetSecretKey|TestCheckAlertmanagerConfig|TestGenerateConfig|TestInitializeFromAlertmanagerConfig'Changelog entry