From 10f04e19f0e90b16551236a5f5ee0cf2b0ddf3e7 Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Tue, 7 Feb 2023 19:29:45 +0100 Subject: [PATCH] [v9.3.x] Alerting: Fix template validation in provisioning api (#62824) * Alerting: Fix template validation in provisioning api (#62530) * Alerting: Fix template validation in provisioning api Fix issue where provisioning API accepts a malformed template having extra text outside of definition block and template name matching definition name. (cherry picked from commit f9ec16e74ffbf47263feb8cca70d17835f43add7) --------- Co-authored-by: Matthew Jacobson --- .../definitions/alertmanager_validation.go | 23 ++-- .../alertmanager_validation_test.go | 114 ++++++++++++++++++ 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go index 86c8a57cc93..8621f938315 100644 --- a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go +++ b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation.go @@ -5,6 +5,7 @@ import ( tmplhtml "html/template" "regexp" "strings" + tmpltext "text/template" "time" "github.com/prometheus/alertmanager/template" @@ -65,13 +66,6 @@ func (t *MessageTemplate) Validate() error { return fmt.Errorf("template must have content") } - tmpl := tmplhtml.New("").Option("missingkey=zero") - tmpl.Funcs(tmplhtml.FuncMap(template.DefaultFuncs)) - _, err := tmpl.Parse(t.Template) - if err != nil { - return fmt.Errorf("invalid template: %w", err) - } - content := strings.TrimSpace(t.Template) found, err := regexp.MatchString(`\{\{\s*define`, content) if err != nil { @@ -87,6 +81,21 @@ func (t *MessageTemplate) Validate() error { } t.Template = content + // Validate template contents. We try to stick as close to what will actually happen when the templates are parsed + // by the alertmanager as possible. That means parsing with both the text and html parsers and making sure we set + // the template name and options. + ttext := tmpltext.New(t.Name).Option("missingkey=zero") + ttext.Funcs(tmpltext.FuncMap(template.DefaultFuncs)) + if _, err := ttext.Parse(t.Template); err != nil { + return fmt.Errorf("invalid template: %w", err) + } + + thtml := tmplhtml.New(t.Name).Option("missingkey=zero") + thtml.Funcs(tmplhtml.FuncMap(template.DefaultFuncs)) + if _, err := thtml.Parse(t.Template); err != nil { + return fmt.Errorf("invalid template: %w", err) + } + return nil } diff --git a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation_test.go b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation_test.go index d0d3de03719..78ee41b8466 100644 --- a/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation_test.go +++ b/pkg/services/ngalert/api/tooling/definitions/alertmanager_validation_test.go @@ -1,6 +1,7 @@ package definitions import ( + "errors" "testing" "github.com/prometheus/alertmanager/config" @@ -365,3 +366,116 @@ func TestValidateMuteTimeInterval(t *testing.T) { } }) } + +func TestValidateNotificationTemplates(t *testing.T) { + tc := []struct { + name string + template MessageTemplate + expContent string + expError error + }{ + { + name: "Same template name as definition", + template: MessageTemplate{ + Name: "Same name as definition", + Template: `{{ define "Same name as definition" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + Provenance: "test", + }, + expContent: `{{ define "Same name as definition" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + expError: nil, + }, + { + name: "Different template name than definition", + template: MessageTemplate{ + Name: "Different name than definition", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + Provenance: "test", + }, + expContent: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + expError: nil, + }, + { + name: "Fix template - missing both {{ define }} and {{ end }}", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `Firing: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}`, + Provenance: "test", + }, + expContent: "{{ define \"Alert Instance Template\" }}\n Firing: {{ .Labels.alertname }}\\nSilence: {{ .SilenceURL }}\n{{ end }}", + expError: nil, + }, + { + name: "Multiple definitions", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}{{ define "Alert Instance Template2" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + Provenance: "test", + }, + expContent: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}{{ define "Alert Instance Template2" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + expError: nil, + }, + { + name: "Malformed template - missing {{ define }}", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + Provenance: "test", + }, + expError: errors.New("invalid template: template: Alert Instance Template:3: unexpected {{end}}"), + }, + { + name: "Malformed template - missing {{ end }}", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n`, + Provenance: "test", + }, + expError: errors.New("invalid template: template: Alert Instance Template:1: unexpected EOF"), + }, + { + name: "Malformed template - multiple definitions duplicate name", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}\n{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}`, + Provenance: "test", + }, + expError: errors.New("invalid template: template: Different name than definition:1: template: multiple definition of template \"Alert Instance Template\""), + }, + { + // This is fine as long as the template name is different from the definition, it just ignores the extra text. + name: "Extra text outside definition block - different template name and definition", + template: MessageTemplate{ + Name: "Different name than definition", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}[what is this?]`, + Provenance: "test", + }, + expContent: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}[what is this?]`, + expError: nil, + }, + { + // This is NOT fine as the template name is the same as the definition. + // GO template parser will treat it as if it's wrapped in {{ define "Alert Instance Template" }}, thus creating a duplicate definition. + name: "Extra text outside definition block - same template name and definition", + template: MessageTemplate{ + Name: "Alert Instance Template", + Template: `{{ define "Alert Instance Template" }}\nFiring: {{ .Labels.alertname }}\nSilence: {{ .SilenceURL }}\n{{ end }}[what is this?]`, + Provenance: "test", + }, + expError: errors.New("invalid template: template: Alert Instance Template:1: template: multiple definition of template \"Alert Instance Template\""), + }, + } + + for _, tt := range tc { + t.Run(tt.name, func(t *testing.T) { + err := tt.template.Validate() + if tt.expError == nil { + require.NoError(t, err) + } else { + require.Error(t, err) + return + } + + require.Equal(t, tt.expContent, tt.template.Template) + }) + } +}