-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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.
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
Alerting: Fix provisioned templates being ignored by alertmanager #69485
Conversation
Template provisioning sets the template in cfg.TemplateFiles while a recent change made it so that alertmanager reads cfg.AlertmanagerConfig.Templates instead. This change fixes the issue on both ends, by having provisioning set boths fields and reverts the change on the alertmanager side so that it uses cfg.TemplateFiles.
@@ -53,6 +53,7 @@ func (t *TemplateService) SetTemplate(ctx context.Context, orgID int64, tmpl def | |||
revision.cfg.TemplateFiles = map[string]string{} | |||
} | |||
revision.cfg.TemplateFiles[tmpl.Name] = tmpl.Template | |||
revision.cfg.AlertmanagerConfig.Templates = append(revision.cfg.AlertmanagerConfig.Templates, tmpl.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also append each time a provisioned template is updated:
curl -H "Content-Type: application/json" -u admin:admin http://127.0.0.1:3000/api/v1/provisioning/templates/test -X PUT -d '{"name":"test","template":"hello1"}'
curl -H "Content-Type: application/json" -u admin:admin http://127.0.0.1:3000/api/v1/provisioning/templates/test -X PUT -d '{"name":"test","template":"hello2"}'
"templates": [
"test",
"test"
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we might as well overwrite cfg.AlertmanagerConfig.Templates
here with the current state of TemplateFiles
since we'll do it during applyConfig
anyways, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think so! I've noticed that sometimes deleted templates are removed from template_files
and not templates
, so it should fix both!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the relationship between this and cfg.AlertmanagerConfig.Templates = paths
on Line 269 of pkg/services/ngalert/notifier/alertmanager.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not technically required since we now do the same thing in pkg/services/ngalert/notifier/alertmanager.go
, figured we might preempt future issues by just making this do both anyways. I can remove it if that's the preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this in:
- Grafana 9.5.2 ✅
- This branch with broken AM configuration (provisioned template missing from
templates
) ✅ - This branch with fixed AM configuration (provisioned template now present in
templates
) ✅
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-69485-to-v10.0.x origin/v10.0.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x c16f1f5e99def9e0136170605368ab33e31e80a1
# Push it to GitHub
git push --set-upstream origin backport-69485-to-v10.0.x
git switch main
# Remove the local backport branch
git branch -D backport-69485-to-v10.0.x Then, create a pull request where the |
…9485) * Alerting: Fix provisioned templates being ignored by alertmanager Template provisioning sets the template in cfg.TemplateFiles while a recent change made it so that alertmanager reads cfg.AlertmanagerConfig.Templates instead. This change fixes the issue on both ends, by having provisioning set boths fields and reverts the change on the alertmanager side so that it uses cfg.TemplateFiles. (cherry picked from commit c16f1f5)
…anager (#69488) Alerting: Fix provisioned templates being ignored by alertmanager (#69485) * Alerting: Fix provisioned templates being ignored by alertmanager Template provisioning sets the template in cfg.TemplateFiles while a recent change made it so that alertmanager reads cfg.AlertmanagerConfig.Templates instead. This change fixes the issue on both ends, by having provisioning set boths fields and reverts the change on the alertmanager side so that it uses cfg.TemplateFiles. (cherry picked from commit c16f1f5)
…anager (#69488) Alerting: Fix provisioned templates being ignored by alertmanager (#69485) * Alerting: Fix provisioned templates being ignored by alertmanager Template provisioning sets the template in cfg.TemplateFiles while a recent change made it so that alertmanager reads cfg.AlertmanagerConfig.Templates instead. This change fixes the issue on both ends, by having provisioning set boths fields and reverts the change on the alertmanager side so that it uses cfg.TemplateFiles. (cherry picked from commit c16f1f5) (cherry picked from commit 0a6e80c)
Template provisioning sets the template in cfg.TemplateFiles while a recent change made it so that alertmanager reads cfg.AlertmanagerConfig.Templates instead.
This change fixes the issue on both ends, by having provisioning set boths fields and reverts the change on the alertmanager side so that it uses cfg.TemplateFiles.
Fixes: #69486