public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Alerting: Support for optimistic concurrency in priovisioning Tempate API (#91195)

Browse Source 
* support optimistic concurrency in template service

* update request handler to get version from query parameter

* return not found if a new template is set with version

* update PUT api to set version

* update documentation + for mute timings

---------

Co-authored-by: brendamuir <100768211+brendamuir@users.noreply.github.com>
This commit is contained in:
Yuri Tseretyan
2024-08-09 11:40:07 -04:00
committed by GitHub
co-authored by brendamuir
parent d20510a1db
commit 1108a00668
12 changed files with 371 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/ngalert/api/api_provisioning.go
+9 -7
View File
@@ -46,7 +46,7 @@ type ContactPointService interface {
type TemplateService interface {
GetTemplates(ctx context.Context, orgID int64) ([]definitions.NotificationTemplate, error)
SetTemplate(ctx context.Context, orgID int64, tmpl definitions.NotificationTemplate) (definitions.NotificationTemplate, error)
DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance) error
DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance, version string) error
}
type NotificationPolicyService interface {
@@ -221,24 +221,26 @@ func (srv *ProvisioningSrv) RouteGetTemplate(c *contextmodel.ReqContext, name st
func (srv *ProvisioningSrv) RoutePutTemplate(c *contextmodel.ReqContext, body definitions.NotificationTemplateContent, name string) response.Response {
tmpl := definitions.NotificationTemplate{
Name: name,
Template: body.Template,
Provenance: determineProvenance(c),
Name: name,
Template: body.Template,
Provenance: determineProvenance(c),
ResourceVersion: body.ResourceVersion,
}
modified, err := srv.templates.SetTemplate(c.Req.Context(), c.SignedInUser.GetOrgID(), tmpl)
if err != nil {
if errors.Is(err, provisioning.ErrValidation) {
return ErrResp(http.StatusBadRequest, err, "")
}
return ErrResp(http.StatusInternalServerError, err, "")
return response.ErrOrFallback(http.StatusInternalServerError, "", err)
}
return response.JSON(http.StatusAccepted, modified)
}
func (srv *ProvisioningSrv) RouteDeleteTemplate(c *contextmodel.ReqContext, name string) response.Response {
err := srv.templates.DeleteTemplate(c.Req.Context(), c.SignedInUser.GetOrgID(), name, determineProvenance(c))
version := c.Query("version")
err := srv.templates.DeleteTemplate(c.Req.Context(), c.SignedInUser.GetOrgID(), name, determineProvenance(c), version)
if err != nil {
return ErrResp(http.StatusInternalServerError, err, "")
return response.ErrOrFallback(http.StatusInternalServerError, "", err)
}
return response.JSON(http.StatusNoContent, nil)
}
pkg/services/ngalert/api/tooling/api.json
+25 -1
View File
@@ -2264,6 +2264,9 @@
},
"template": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
@@ -2272,6 +2275,9 @@
"properties": {
"template": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
@@ -6183,16 +6189,28 @@
"operationId": "RouteDeleteTemplate",
"parameters": [
{
"description": "Template Name",
"description": "Template name",
"in": "path",
"name": "name",
"required": true,
"type": "string"
},
{
"description": "Version of template to use for optimistic concurrency. Leave empty to disable validation",
"in": "query",
"name": "version",
"type": "string"
}
],
"responses": {
"204": {
"description": " The template was deleted successfully."
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
},
"summary": "Delete a template.",
@@ -6265,6 +6283,12 @@
"schema": {
"$ref": "#/definitions/ValidationError"
}
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
},
"summary": "Updates an existing notification template.",
pkg/services/ngalert/api/tooling/definitions/provisioning_templates.go
+19 -4
View File
@@ -26,6 +26,7 @@ package definitions
// Responses:
// 202: NotificationTemplate
// 400: ValidationError
// 409: GenericPublicError
// swagger:route DELETE /v1/provisioning/templates/{name} provisioning stable RouteDeleteTemplate
//
@@ -33,6 +34,7 @@ package definitions
//
// Responses:
// 204: description: The template was deleted successfully.
// 409: GenericPublicError
// swagger:parameters RouteGetTemplate RoutePutTemplate RouteDeleteTemplate
type RouteGetTemplateParam struct {
@@ -41,18 +43,31 @@ type RouteGetTemplateParam struct {
Name string `json:"name"`
}
// swagger:parameters stable RouteDeleteTemplate
type RouteDeleteTemplateParam struct {
// Template name
// in:path
Name string `json:"name"`
// Version of template to use for optimistic concurrency. Leave empty to disable validation
// in:query
Version string `json:"version"`
}
// swagger:model
type NotificationTemplate struct {
Name string `json:"name"`
Template string `json:"template"`
Provenance Provenance `json:"provenance,omitempty"`
Name string `json:"name"`
Template string `json:"template"`
Provenance Provenance `json:"provenance,omitempty"`
ResourceVersion string `json:"version,omitempty"`
}
// swagger:model
type NotificationTemplates []NotificationTemplate
type NotificationTemplateContent struct {
Template string `json:"template"`
Template string `json:"template"`
ResourceVersion string `json:"version,omitempty"`
}
// swagger:parameters RoutePutTemplate
pkg/services/ngalert/api/tooling/post.json
+25 -1
View File
@@ -2264,6 +2264,9 @@
},
"template": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
@@ -2272,6 +2275,9 @@
"properties": {
"template": {
"type": "string"
},
"version": {
"type": "string"
}
},
"type": "object"
@@ -8395,16 +8401,28 @@
"operationId": "RouteDeleteTemplate",
"parameters": [
{
"description": "Template Name",
"description": "Template name",
"in": "path",
"name": "name",
"required": true,
"type": "string"
},
{
"description": "Version of template to use for optimistic concurrency. Leave empty to disable validation",
"in": "query",
"name": "version",
"type": "string"
}
],
"responses": {
"204": {
"description": " The template was deleted successfully."
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
},
"summary": "Delete a template.",
@@ -8477,6 +8495,12 @@
"schema": {
"$ref": "#/definitions/ValidationError"
}
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
},
"summary": "Updates an existing notification template.",
pkg/services/ngalert/api/tooling/spec.json
+25 -1
View File
@@ -3393,6 +3393,12 @@
"schema": {
"$ref": "#/definitions/ValidationError"
}
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
}
},
@@ -3406,15 +3412,27 @@
"parameters": [
{
"type": "string",
"description": "Template Name",
"description": "Template name",
"name": "name",
"in": "path",
"required": true
},
{
"type": "string",
"description": "Version of template to use for optimistic concurrency. Leave empty to disable validation",
"name": "version",
"in": "query"
}
],
"responses": {
"204": {
"description": " The template was deleted successfully."
},
"409": {
"description": "GenericPublicError",
"schema": {
"$ref": "#/definitions/GenericPublicError"
}
}
}
}
@@ -5880,6 +5898,9 @@
},
"template": {
"type": "string"
},
"version": {
"type": "string"
}
}
},
@@ -5888,6 +5909,9 @@
"properties": {
"template": {
"type": "string"
},
"version": {
"type": "string"
}
}
},
pkg/services/ngalert/provisioning/errors.go
+2
View File
@@ -18,6 +18,8 @@ var (
ErrTimeIntervalInvalid = errutil.BadRequest("alerting.notifications.time-intervals.invalidFormat").MustTemplate("Invalid format of the submitted time interval", errutil.WithPublic("Time interval is in invalid format. Correct the payload and try again."))
ErrTimeIntervalInUse = errutil.Conflict("alerting.notifications.time-intervals.used").MustTemplate("Time interval is used")
ErrTemplateNotFound = errutil.NotFound("alerting.notifications.templates.notFound")
ErrContactPointReferenced = errutil.Conflict("alerting.notifications.contact-points.referenced", errutil.WithPublicMessage("Contact point is currently referenced by a notification policy."))
ErrContactPointUsedInRule = errutil.Conflict("alerting.notifications.contact-points.used-by-rule", errutil.WithPublicMessage("Contact point is currently used in the notification settings of one or many alert rules."))
)
pkg/services/ngalert/provisioning/templates.go
+49 -5
View File
@@ -3,6 +3,8 @@ package provisioning
import (
"context"
"fmt"
"hash/fnv"
"unsafe"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
@@ -37,8 +39,9 @@ func (t *TemplateService) GetTemplates(ctx context.Context, orgID int64) ([]defi
templates := make([]definitions.NotificationTemplate, 0, len(revision.Config.TemplateFiles))
for name, tmpl := range revision.Config.TemplateFiles {
tmpl := definitions.NotificationTemplate{
Name: name,
Template: tmpl,
Name: name,
Template: tmpl,
ResourceVersion: calculateTemplateFingerprint(tmpl),
}
provenance, err := t.provenanceStore.GetProvenance(ctx, &tmpl, orgID)
@@ -80,6 +83,16 @@ func (t *TemplateService) SetTemplate(ctx context.Context, orgID int64, tmpl def
}
}
existing, ok := revision.Config.TemplateFiles[tmpl.Name]
if ok {
err = t.checkOptimisticConcurrency(tmpl.Name, existing, models.Provenance(tmpl.Provenance), tmpl.ResourceVersion, "update")
if err != nil {
return definitions.NotificationTemplate{}, err
}
} else if tmpl.ResourceVersion != "" { // if version is set then it's an update operation. Fail because resource does not exist anymore
return definitions.NotificationTemplate{}, ErrTemplateNotFound.Errorf("")
}
revision.Config.TemplateFiles[tmpl.Name] = tmpl.Template
err = t.xact.InTransaction(ctx, func(ctx context.Context) error {
@@ -92,10 +105,15 @@ func (t *TemplateService) SetTemplate(ctx context.Context, orgID int64, tmpl def
return definitions.NotificationTemplate{}, err
}
return tmpl, nil
return definitions.NotificationTemplate{
Name: tmpl.Name,
Template: tmpl.Template,
Provenance: tmpl.Provenance,
ResourceVersion: calculateTemplateFingerprint(tmpl.Template),
}, nil
}
func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance) error {
func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name string, provenance definitions.Provenance, version string) error {
revision, err := t.configStore.Get(ctx, orgID)
if err != nil {
return err
@@ -105,11 +123,16 @@ func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name
return nil
}
_, ok := revision.Config.TemplateFiles[name]
existing, ok := revision.Config.TemplateFiles[name]
if !ok {
return nil
}
err = t.checkOptimisticConcurrency(name, existing, models.Provenance(provenance), version, "delete")
if err != nil {
return err
}
// check that provenance is not changed in an invalid way
storedProvenance, err := t.provenanceStore.GetProvenance(ctx, &definitions.NotificationTemplate{Name: name}, orgID)
if err != nil {
@@ -131,3 +154,24 @@ func (t *TemplateService) DeleteTemplate(ctx context.Context, orgID int64, name
return t.provenanceStore.DeleteProvenance(ctx, &tgt, orgID)
})
}
func (t *TemplateService) checkOptimisticConcurrency(name, currentContent string, provenance models.Provenance, desiredVersion string, action string) error {
if desiredVersion == "" {
if provenance != models.ProvenanceFile {
// if version is not specified and it's not a file provisioning, emit a log message to reflect that optimistic concurrency is disabled for this request
t.log.Debug("ignoring optimistic concurrency check because version was not provided", "template", name, "operation", action)
}
return nil
}
currentVersion := calculateTemplateFingerprint(currentContent)
if currentVersion != desiredVersion {
return ErrVersionConflict.Errorf("provided version %s of template %s does not match current version %s", desiredVersion, name, currentVersion)
}
return nil
}
func calculateTemplateFingerprint(t string) string {
sum := fnv.New64()
_, _ = sum.Write(unsafe.Slice(unsafe.StringData(t), len(t))) //nolint:gosec
return fmt.Sprintf("%016x", sum.Sum64())
}
pkg/services/ngalert/provisioning/templates_test.go
+78 -9
View File
@@ -127,6 +127,45 @@ func TestTemplateService(t *testing.T) {
require.ErrorIs(t, err, expectedErr)
})
t.Run("rejects existing templates if version is not right", func(t *testing.T) {
mockStore := &legacy_storage.MockAMConfigStore{}
sut := createTemplateServiceSut(legacy_storage.NewAlertmanagerConfigStore(mockStore))
mockStore.EXPECT().
GetsConfig(models.AlertConfiguration{
AlertmanagerConfiguration: configWithTemplates,
})
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceNone, nil)
template := definitions.NotificationTemplate{
Name: "a",
Template: "asdf-new",
ResourceVersion: "bad-version",
Provenance: definitions.Provenance(models.ProvenanceNone),
}
_, err := sut.SetTemplate(context.Background(), 1, template)
require.ErrorIs(t, err, ErrVersionConflict)
})
t.Run("rejects new template if version is set", func(t *testing.T) {
mockStore := &legacy_storage.MockAMConfigStore{}
sut := createTemplateServiceSut(legacy_storage.NewAlertmanagerConfigStore(mockStore))
tmpl := createNotificationTemplate()
tmpl.ResourceVersion = "test"
mockStore.EXPECT().
GetsConfig(models.AlertConfiguration{
AlertmanagerConfiguration: configWithTemplates,
})
mockStore.EXPECT().SaveSucceeds()
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceNone, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
_, err := sut.SetTemplate(context.Background(), 1, tmpl)
require.ErrorIs(t, err, ErrTemplateNotFound)
})
t.Run("propagates errors", func(t *testing.T) {
t.Run("when unable to read config", func(t *testing.T) {
mockStore := &legacy_storage.MockAMConfigStore{}
@@ -336,7 +375,7 @@ func TestTemplateService(t *testing.T) {
Return(nil, fmt.Errorf("failed"))
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI), "")
require.Error(t, err)
})
@@ -350,7 +389,7 @@ func TestTemplateService(t *testing.T) {
})
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI), "")
require.Truef(t, legacy_storage.ErrBadAlertmanagerConfiguration.Base.Is(err), "expected ErrBadAlertmanagerConfiguration but got %s", err.Error())
})
@@ -363,7 +402,7 @@ func TestTemplateService(t *testing.T) {
Return(nil, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "template", definitions.Provenance(models.ProvenanceAPI), "")
require.Truef(t, legacy_storage.ErrNoAlertmanagerConfiguration.Is(err), "expected ErrNoAlertmanagerConfiguration but got %s", err.Error())
})
@@ -381,7 +420,7 @@ func TestTemplateService(t *testing.T) {
DeleteProvenance(mock.Anything, mock.Anything, mock.Anything).
Return(fmt.Errorf("failed to save provenance"))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI), "")
require.ErrorContains(t, err, "failed to save provenance")
})
@@ -399,7 +438,7 @@ func TestTemplateService(t *testing.T) {
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI), "")
require.ErrorContains(t, err, "failed to save config")
})
@@ -416,7 +455,23 @@ func TestTemplateService(t *testing.T) {
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI), "")
require.NoError(t, err)
})
t.Run("deletes template from config file on success ignoring optimistic concurrency", func(t *testing.T) {
mockStore := &legacy_storage.MockAMConfigStore{}
sut := createTemplateServiceSut(legacy_storage.NewAlertmanagerConfigStore(mockStore))
mockStore.EXPECT().
GetsConfig(models.AlertConfiguration{
AlertmanagerConfiguration: configWithTemplates,
})
mockStore.EXPECT().SaveSucceeds()
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI), "b26e328af4bb9aaf")
require.NoError(t, err)
})
@@ -432,7 +487,7 @@ func TestTemplateService(t *testing.T) {
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
err := sut.DeleteTemplate(context.Background(), 1, "does not exist", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "does not exist", definitions.Provenance(models.ProvenanceAPI), "")
require.NoError(t, err)
})
@@ -448,7 +503,7 @@ func TestTemplateService(t *testing.T) {
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceAPI, nil)
sut.provenanceStore.(*MockProvisioningStore).EXPECT().SaveSucceeds()
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceAPI), "")
require.NoError(t, err)
})
@@ -469,10 +524,24 @@ func TestTemplateService(t *testing.T) {
return expectedErr
}
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceNone))
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceNone), "")
require.ErrorIs(t, err, expectedErr)
})
t.Run("errors if version is not right", func(t *testing.T) {
mockStore := &legacy_storage.MockAMConfigStore{}
sut := createTemplateServiceSut(legacy_storage.NewAlertmanagerConfigStore(mockStore))
mockStore.EXPECT().
GetsConfig(models.AlertConfiguration{
AlertmanagerConfiguration: configWithTemplates,
})
sut.provenanceStore.(*MockProvisioningStore).EXPECT().GetProvenance(mock.Anything, mock.Anything, mock.Anything).Return(models.ProvenanceNone, nil)
err := sut.DeleteTemplate(context.Background(), 1, "a", definitions.Provenance(models.ProvenanceNone), "bad-version")
require.ErrorIs(t, err, ErrVersionConflict)
})
})
}
pkg/services/provisioning/alerting/text_templates.go
+1 -1
View File
@@ -45,7 +45,7 @@ func (c *defaultTextTemplateProvisioner) Unprovision(ctx context.Context,
files []*AlertingFile) error {
for _, file := range files {
for _, deleteTemplate := range file.DeleteTemplates {
err := c.templateService.DeleteTemplate(ctx, deleteTemplate.OrgID, deleteTemplate.Name, definitions.Provenance(models.ProvenanceFile))
err := c.templateService.DeleteTemplate(ctx, deleteTemplate.OrgID, deleteTemplate.Name, definitions.Provenance(models.ProvenanceFile), "")
if err != nil {
return err
}