From d0e7765c6a244d93ad856345c86e433c4a46dffa Mon Sep 17 00:00:00 2001 From: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com> Date: Fri, 23 Sep 2022 13:04:41 +0300 Subject: [PATCH] Annotation: Optionally allow storing longer annotation tags (#54754) * Annotation: Optionally allow longer annotation tags * Do not accept configuration lower than today's default (500) * Apply suggestion from code review --- conf/defaults.ini | 4 ++ conf/sample.ini | 4 ++ .../setup-grafana/configure-grafana/_index.md | 4 ++ pkg/api/annotations.go | 8 ++-- pkg/services/annotations/annotations.go | 4 +- .../annotationsimpl/annotations.go | 9 ++-- .../annotations/annotationsimpl/xorm_store.go | 46 +++++++++++++++---- .../annotationsimpl/xorm_store_test.go | 19 +++++++- .../sqlstore/migrations/annotation_mig.go | 4 ++ pkg/setting/setting.go | 21 ++++++++- 10 files changed, 101 insertions(+), 22 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index 630679839e2..fc6452d21fe 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -945,6 +945,10 @@ max_annotations_to_keep = # Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations. cleanupjob_batchsize = 100 +# Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 inclusive (which is the respective's column length). Default value is 500. +# Setting it to a higher value would impact performance therefore is not recommended. +tags_length = 500 + [annotations.dashboard] # Dashboard annotations means that annotations are associated with the dashboard they are created on. diff --git a/conf/sample.ini b/conf/sample.ini index 67cd5c164a5..ac660f1d066 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -914,6 +914,10 @@ # Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations. ;cleanupjob_batchsize = 100 +# Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 inclusive (which is the respective's column length). Default value is 500. +# Setting it to a higher value would impact performance therefore is not recommended. +;tags_length = 500 + [annotations.dashboard] # Dashboard annotations means that annotations are associated with the dashboard they are created on. diff --git a/docs/sources/setup-grafana/configure-grafana/_index.md b/docs/sources/setup-grafana/configure-grafana/_index.md index 59a9a7c32ae..7002dcc30dd 100644 --- a/docs/sources/setup-grafana/configure-grafana/_index.md +++ b/docs/sources/setup-grafana/configure-grafana/_index.md @@ -1387,6 +1387,10 @@ Configures max number of alert annotations that Grafana stores. Default value is Configures the batch size for the annotation clean-up job. This setting is used for dashboard, API, and alert annotations. +### tags_length + +Enforces the maximum allowed length of the tags for any newly introduced annotations. It can be between 500 and 4096 (inclusive). Default value is 500. Setting it to a higher value would impact performance therefore is not recommended. + ## [annotations.dashboard] Dashboard annotations means that annotations are associated with the dashboard they are created on. diff --git a/pkg/api/annotations.go b/pkg/api/annotations.go index 0b981d91b54..f41bbc89093 100644 --- a/pkg/api/annotations.go +++ b/pkg/api/annotations.go @@ -154,7 +154,7 @@ func (hs *HTTPServer) PostAnnotation(c *models.ReqContext) response.Response { if errors.Is(err, annotations.ErrTimerangeMissing) { return response.Error(400, "Failed to save annotation", err) } - return response.Error(500, "Failed to save annotation", err) + return response.ErrOrFallback(500, "Failed to save annotation", err) } startID := item.Id @@ -229,7 +229,7 @@ func (hs *HTTPServer) PostGraphiteAnnotation(c *models.ReqContext) response.Resp } if err := hs.annotationsRepo.Save(c.Req.Context(), &item); err != nil { - return response.Error(500, "Failed to save Graphite annotation", err) + return response.ErrOrFallback(500, "Failed to save Graphite annotation", err) } return response.JSON(http.StatusOK, util.DynMap{ @@ -281,7 +281,7 @@ func (hs *HTTPServer) UpdateAnnotation(c *models.ReqContext) response.Response { } if err := hs.annotationsRepo.Update(c.Req.Context(), &item); err != nil { - return response.Error(500, "Failed to update annotation", err) + return response.ErrOrFallback(500, "Failed to update annotation", err) } return response.Success("Annotation updated") @@ -347,7 +347,7 @@ func (hs *HTTPServer) PatchAnnotation(c *models.ReqContext) response.Response { } if err := hs.annotationsRepo.Update(c.Req.Context(), &existing); err != nil { - return response.Error(500, "Failed to update annotation", err) + return response.ErrOrFallback(500, "Failed to update annotation", err) } return response.Success("Annotation patched") diff --git a/pkg/services/annotations/annotations.go b/pkg/services/annotations/annotations.go index 57c9eb33d8d..b41816def93 100644 --- a/pkg/services/annotations/annotations.go +++ b/pkg/services/annotations/annotations.go @@ -5,10 +5,12 @@ import ( "errors" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util/errutil" ) var ( - ErrTimerangeMissing = errors.New("missing timerange") + ErrTimerangeMissing = errors.New("missing timerange") + ErrBaseTagLimitExceeded = errutil.NewBase(errutil.StatusBadRequest, "annotations.tag-limit-exceeded", errutil.WithPublicMessage("Tags length exceeds the maximum allowed.")) ) type Repository interface { diff --git a/pkg/services/annotations/annotationsimpl/annotations.go b/pkg/services/annotations/annotationsimpl/annotations.go index cbed01db047..527807906a5 100644 --- a/pkg/services/annotations/annotationsimpl/annotations.go +++ b/pkg/services/annotations/annotationsimpl/annotations.go @@ -17,10 +17,11 @@ type RepositoryImpl struct { func ProvideService(db db.DB, cfg *setting.Cfg, tagService tag.Service) *RepositoryImpl { return &RepositoryImpl{ store: &xormRepositoryImpl{ - cfg: cfg, - db: db, - log: log.New("annotations"), - tagService: tagService, + cfg: cfg, + db: db, + log: log.New("annotations"), + tagService: tagService, + maximumTagsLength: cfg.AnnotationMaximumTagsLength, }, } } diff --git a/pkg/services/annotations/annotationsimpl/xorm_store.go b/pkg/services/annotations/annotationsimpl/xorm_store.go index 6e529b487c2..e64dfc5fa0c 100644 --- a/pkg/services/annotations/annotationsimpl/xorm_store.go +++ b/pkg/services/annotations/annotationsimpl/xorm_store.go @@ -41,10 +41,11 @@ func validateTimeRange(item *annotations.Item) error { } type xormRepositoryImpl struct { - cfg *setting.Cfg - db db.DB - log log.Logger - tagService tag.Service + cfg *setting.Cfg + db db.DB + log log.Logger + maximumTagsLength int64 + tagService tag.Service } func (r *xormRepositoryImpl) Add(ctx context.Context, item *annotations.Item) error { @@ -55,7 +56,7 @@ func (r *xormRepositoryImpl) Add(ctx context.Context, item *annotations.Item) er if item.Epoch == 0 { item.Epoch = item.Created } - if err := validateTimeRange(item); err != nil { + if err := r.validateItem(item); err != nil { return err } @@ -106,10 +107,6 @@ func (r *xormRepositoryImpl) Update(ctx context.Context, item *annotations.Item) existing.EpochEnd = item.EpochEnd } - if err := validateTimeRange(existing); err != nil { - return err - } - if item.Tags != nil { tags, err := r.tagService.EnsureTagsExist(ctx, tag.ParseTagPairs(item.Tags)) if err != nil { @@ -127,6 +124,10 @@ func (r *xormRepositoryImpl) Update(ctx context.Context, item *annotations.Item) existing.Tags = item.Tags + if err := r.validateItem(existing); err != nil { + return err + } + _, err = sess.Table("annotation").ID(existing.Id).Cols("epoch", "text", "epoch_end", "updated", "tags").Update(existing) return err }) @@ -379,6 +380,33 @@ func (r *xormRepositoryImpl) GetTags(ctx context.Context, query *annotations.Tag return annotations.FindTagsResult{Tags: tags}, nil } +func (r *xormRepositoryImpl) validateItem(item *annotations.Item) error { + if err := validateTimeRange(item); err != nil { + return err + } + + if err := r.validateTagsLength(item); err != nil { + return err + } + return nil +} + +func (r *xormRepositoryImpl) validateTagsLength(item *annotations.Item) error { + estimatedTagsLength := 1 // leading: [ + for i, t := range item.Tags { + if i == 0 { + estimatedTagsLength += len(t) + 2 // quotes + } else { + estimatedTagsLength += len(t) + 3 // leading comma and quotes + } + } + estimatedTagsLength += 1 // trailing: ] + if estimatedTagsLength > int(r.maximumTagsLength) { + return annotations.ErrBaseTagLimitExceeded.Errorf("tags length (%d) exceeds the maximum allowed (%d): modify the configuration to increase it", estimatedTagsLength, r.maximumTagsLength) + } + return nil +} + func (r *xormRepositoryImpl) CleanAnnotations(ctx context.Context, cfg setting.AnnotationCleanupSettings, annotationType string) (int64, error) { var totalAffected int64 if cfg.MaxAge > 0 { diff --git a/pkg/services/annotations/annotationsimpl/xorm_store_test.go b/pkg/services/annotations/annotationsimpl/xorm_store_test.go index e668112f0c4..f79768f54ed 100644 --- a/pkg/services/annotations/annotationsimpl/xorm_store_test.go +++ b/pkg/services/annotations/annotationsimpl/xorm_store_test.go @@ -3,6 +3,7 @@ package annotationsimpl import ( "context" "fmt" + "strings" "testing" "github.com/grafana/grafana/pkg/infra/log" @@ -28,7 +29,8 @@ func TestIntegrationAnnotations(t *testing.T) { t.Skip("skipping integration test") } sql := sqlstore.InitTestDB(t) - repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql)} + var maximumTagsLength int64 = 60 + repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql), maximumTagsLength: maximumTagsLength} testUser := &user.SignedInUser{ OrgID: 1, @@ -148,6 +150,18 @@ func TestIntegrationAnnotations(t *testing.T) { assert.Equal(t, items[0].Updated, items[0].Created) }) + badAnnotation := &annotations.Item{ + OrgId: 1, + UserId: 1, + Text: "rollback", + Type: "", + Epoch: 17, + Tags: []string{strings.Repeat("a", int(maximumTagsLength+1))}, + } + err = repo.Add(context.Background(), badAnnotation) + require.Error(t, err) + require.ErrorIs(t, err, annotations.ErrBaseTagLimitExceeded) + t.Run("Can query for annotation by id", func(t *testing.T) { items, err := repo.Get(context.Background(), &annotations.ItemQuery{ OrgId: 1, @@ -394,7 +408,8 @@ func TestIntegrationAnnotationListingWithRBAC(t *testing.T) { t.Skip("skipping integration test") } sql := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{}) - repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql)} + var maximumTagsLength int64 = 60 + repo := xormRepositoryImpl{db: sql, cfg: setting.NewCfg(), log: log.New("annotation.test"), tagService: tagimpl.ProvideService(sql), maximumTagsLength: maximumTagsLength} dashboardStore := dashboardstore.ProvideDashboardStore(sql, featuremgmt.WithFeatures(), tagimpl.ProvideService(sql)) testDashboard1 := models.SaveDashboardCommand{ diff --git a/pkg/services/sqlstore/migrations/annotation_mig.go b/pkg/services/sqlstore/migrations/annotation_mig.go index 96c62ceb56f..f66a63f06c0 100644 --- a/pkg/services/sqlstore/migrations/annotation_mig.go +++ b/pkg/services/sqlstore/migrations/annotation_mig.go @@ -178,6 +178,10 @@ func addAnnotationMig(mg *Migrator) { mg.AddMigration("Add index for alert_id on annotation table", NewAddIndexMigration(table, &Index{ Cols: []string{"alert_id"}, Type: IndexType, })) + + mg.AddMigration("Increase tags column to length 4096", NewRawSQLMigration(""). + Postgres("ALTER TABLE annotation ALTER COLUMN tags TYPE VARCHAR(4096);"). + Mysql("ALTER TABLE annotation MODIFY tags VARCHAR(4096);")) } type AddMakeRegionSingleRowMigration struct { diff --git a/pkg/setting/setting.go b/pkg/setting/setting.go index 2777bfea0a4..d447cd98024 100644 --- a/pkg/setting/setting.go +++ b/pkg/setting/setting.go @@ -374,6 +374,7 @@ type Cfg struct { // Annotations AnnotationCleanupJobBatchSize int64 + AnnotationMaximumTagsLength int64 AlertingAnnotationCleanupSetting AnnotationCleanupSettings DashboardAnnotationCleanupSettings AnnotationCleanupSettings APIAnnotationCleanupSettings AnnotationCleanupSettings @@ -601,9 +602,20 @@ func (cfg *Cfg) readGrafanaEnvironmentMetrics() error { return nil } -func (cfg *Cfg) readAnnotationSettings() { +func (cfg *Cfg) readAnnotationSettings() error { section := cfg.Raw.Section("annotations") cfg.AnnotationCleanupJobBatchSize = section.Key("cleanupjob_batchsize").MustInt64(100) + cfg.AnnotationMaximumTagsLength = section.Key("tags_length").MustInt64(500) + switch { + case cfg.AnnotationMaximumTagsLength > 4096: + // ensure that the configuration does not exceed the respective column size + return fmt.Errorf("[annotations.tags_length] configuration exceeds the maximum allowed (4096)") + case cfg.AnnotationMaximumTagsLength > 500: + cfg.Logger.Info("[annotations.tags_length] has been increased from its default value; this may affect the performance", "tagLength", cfg.AnnotationMaximumTagsLength) + case cfg.AnnotationMaximumTagsLength < 500: + cfg.Logger.Warn("[annotations.tags_length] is too low; the minimum allowed (500) is enforced") + cfg.AnnotationMaximumTagsLength = 500 + } dashboardAnnotation := cfg.Raw.Section("annotations.dashboard") apiIAnnotation := cfg.Raw.Section("annotations.api") @@ -624,6 +636,8 @@ func (cfg *Cfg) readAnnotationSettings() { cfg.AlertingAnnotationCleanupSetting = newAnnotationCleanupSettings(alertingSection, "max_annotation_age") cfg.DashboardAnnotationCleanupSettings = newAnnotationCleanupSettings(dashboardAnnotation, "max_age") cfg.APIAnnotationCleanupSettings = newAnnotationCleanupSettings(apiIAnnotation, "max_age") + + return nil } func (cfg *Cfg) readExpressionsSettings() { @@ -1020,7 +1034,10 @@ func (cfg *Cfg) Load(args CommandLineArgs) error { cfg.readSessionConfig() cfg.readSmtpSettings() cfg.readQuotaSettings() - cfg.readAnnotationSettings() + if err := cfg.readAnnotationSettings(); err != nil { + return err + } + cfg.readExpressionsSettings() if err := cfg.readGrafanaEnvironmentMetrics(); err != nil { return err