From dd0a2d4cff212798e08156bebd88279ba80c9907 Mon Sep 17 00:00:00 2001 From: Moustafa Baiou Date: Mon, 10 Nov 2025 14:40:35 -0500 Subject: [PATCH] Alerting: Add validation to check updates on rule groups (#113669) This moves some of the validation logic for rule groups from the legacy storage layer to the validator. --- .../rules/pkg/app/alertrule/validator.go | 31 +++-- .../rules/pkg/app/recordingrule/validator.go | 30 ++--- .../rules/pkg/app/util/group_validation.go | 59 ++++++++++ .../pkg/app/util/group_validation_test.go | 109 ++++++++++++++++++ .../rules/alertrule/legacy_storage.go | 14 --- .../rules/recordingrule/legacy_storage.go | 9 -- 6 files changed, 198 insertions(+), 54 deletions(-) create mode 100644 apps/alerting/rules/pkg/app/util/group_validation.go create mode 100644 apps/alerting/rules/pkg/app/util/group_validation_test.go diff --git a/apps/alerting/rules/pkg/app/alertrule/validator.go b/apps/alerting/rules/pkg/app/alertrule/validator.go index e86d17cab48..636c1b4a323 100644 --- a/apps/alerting/rules/pkg/app/alertrule/validator.go +++ b/apps/alerting/rules/pkg/app/alertrule/validator.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "slices" - "strconv" "time" "github.com/grafana/grafana-app-sdk/app" @@ -16,6 +15,19 @@ import ( prom_model "github.com/prometheus/common/model" ) +// validateGroupLabels now delegates to util.ValidateGroupLabels for shared logic. +func validateGroupLabels(r *model.AlertRule, oldObject resource.Object, action resource.AdmissionAction) error { + var oldLabels map[string]string + if oldObject != nil { + if oldRule, ok := oldObject.(*model.AlertRule); ok { + oldLabels = oldRule.Labels + } else { + return fmt.Errorf("old object is not of type *v0alpha1.AlertRule") + } + } + return util.ValidateGroupLabels(r.Labels, oldLabels, action) +} + func NewValidator(cfg config.RuntimeConfig) *simple.Validator { return &simple.Validator{ ValidateFunc: func(ctx context.Context, req *app.AdmissionRequest) error { @@ -24,7 +36,6 @@ func NewValidator(cfg config.RuntimeConfig) *simple.Validator { if !ok { return fmt.Errorf("object is not of type *v0alpha1.AlertRule") } - // 1) Validate provenance status annotation sourceProv := r.GetProvenanceStatus() if !slices.Contains(model.AcceptedProvenanceStatuses, sourceProv) { @@ -32,20 +43,8 @@ func NewValidator(cfg config.RuntimeConfig) *simple.Validator { } // 2) Validate group labels rules - group := r.Labels[model.GroupLabelKey] - groupIndexStr := r.Labels[model.GroupIndexLabelKey] - if req.Action == resource.AdmissionActionCreate { - if group != "" || groupIndexStr != "" { - return fmt.Errorf("cannot set group when creating alert rule") - } - } - if group != "" { // if group is set, group-index must be set and numeric - if groupIndexStr == "" { - return fmt.Errorf("%s must be set when %s is set", model.GroupIndexLabelKey, model.GroupLabelKey) - } - if _, err := strconv.Atoi(groupIndexStr); err != nil { - return fmt.Errorf("invalid %s: %w", model.GroupIndexLabelKey, err) - } + if err := validateGroupLabels(r, req.OldObject, req.Action); err != nil { + return err } // 3) Validate folder is set and exists diff --git a/apps/alerting/rules/pkg/app/recordingrule/validator.go b/apps/alerting/rules/pkg/app/recordingrule/validator.go index 8354b9d7501..ea9e8f207d6 100644 --- a/apps/alerting/rules/pkg/app/recordingrule/validator.go +++ b/apps/alerting/rules/pkg/app/recordingrule/validator.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "slices" - "strconv" "github.com/grafana/grafana-app-sdk/app" "github.com/grafana/grafana-app-sdk/resource" @@ -15,6 +14,19 @@ import ( prom_model "github.com/prometheus/common/model" ) +// validateGroupLabels now delegates to util.ValidateGroupLabels for shared logic. +func validateGroupLabels(r *model.RecordingRule, oldObject resource.Object, action resource.AdmissionAction) error { + var oldLabels map[string]string + if oldObject != nil { + if oldRule, ok := oldObject.(*model.RecordingRule); ok { + oldLabels = oldRule.Labels + } else { + return fmt.Errorf("old object is not of type *v0alpha1.RecordingRule") + } + } + return util.ValidateGroupLabels(r.Labels, oldLabels, action) +} + func NewValidator(cfg config.RuntimeConfig) *simple.Validator { return &simple.Validator{ ValidateFunc: func(ctx context.Context, req *app.AdmissionRequest) error { @@ -29,20 +41,8 @@ func NewValidator(cfg config.RuntimeConfig) *simple.Validator { return fmt.Errorf("invalid provenance status: %s", sourceProv) } - group := r.Labels[model.GroupLabelKey] - groupIndexStr := r.Labels[model.GroupIndexLabelKey] - if req.Action == resource.AdmissionActionCreate { - if group != "" || groupIndexStr != "" { - return fmt.Errorf("cannot set group when creating recording rule") - } - } - if group != "" { - if groupIndexStr == "" { - return fmt.Errorf("%s must be set when %s is set", model.GroupIndexLabelKey, model.GroupLabelKey) - } - if _, err := strconv.Atoi(groupIndexStr); err != nil { - return fmt.Errorf("invalid %s: %w", model.GroupIndexLabelKey, err) - } + if err := validateGroupLabels(r, req.OldObject, req.Action); err != nil { + return err } folderUID := "" diff --git a/apps/alerting/rules/pkg/app/util/group_validation.go b/apps/alerting/rules/pkg/app/util/group_validation.go new file mode 100644 index 00000000000..8624ef816e8 --- /dev/null +++ b/apps/alerting/rules/pkg/app/util/group_validation.go @@ -0,0 +1,59 @@ +package util + +import ( + "fmt" + "strconv" + + "github.com/grafana/grafana-app-sdk/resource" + model "github.com/grafana/grafana/apps/alerting/rules/pkg/apis/alerting/v0alpha1" +) + +// ValidateGroupLabels enforces the cross-field rules for group-related labels. +// +// Rules enforced: +// - On create, group-related labels must not be set. +// - If one of group or group-index is set, the other must also be set. +// - group-index must be an integer. +// - On update, group/group-index can only be present if they were present on the old object. +// +// Pass current labels and, when available, the previous object's labels. The previous labels +// may be nil when there is no old object (e.g., create operations). +func ValidateGroupLabels(labels map[string]string, oldLabels map[string]string, action resource.AdmissionAction) error { + groupStr, groupExists := labels[model.GroupLabelKey] + groupIndexStr, groupIndexStrExists := labels[model.GroupIndexLabelKey] + + if groupExists || groupIndexStrExists { + if action == resource.AdmissionActionCreate { + return fmt.Errorf("cannot set group when creating a new rule") + } + if groupExists && !groupIndexStrExists { + return fmt.Errorf("%s must be set when %s is set", model.GroupIndexLabelKey, model.GroupLabelKey) + } + if groupIndexStrExists && !groupExists { + return fmt.Errorf("%s must be set when %s is set", model.GroupLabelKey, model.GroupIndexLabelKey) + } + // Disallow empty values when labels are present + if groupExists && groupStr == "" { + return fmt.Errorf("%s cannot be empty", model.GroupLabelKey) + } + if groupIndexStrExists && groupIndexStr == "" { + return fmt.Errorf("%s cannot be empty", model.GroupIndexLabelKey) + } + if _, err := strconv.Atoi(groupIndexStr); err != nil { + return fmt.Errorf("invalid %s: %w", model.GroupIndexLabelKey, err) + } + + // On updates, ensure that group and group-index are only set if the old object had them set + if oldLabels != nil { + _, oldGroupExists := oldLabels[model.GroupLabelKey] + _, oldGroupIndexExists := oldLabels[model.GroupIndexLabelKey] + if groupExists && !oldGroupExists { + return fmt.Errorf("cannot set group when updating un-grouped rule") + } + if groupIndexStrExists && !oldGroupIndexExists { + return fmt.Errorf("cannot set group-index when updating un-grouped rule") + } + } + } + return nil +} diff --git a/apps/alerting/rules/pkg/app/util/group_validation_test.go b/apps/alerting/rules/pkg/app/util/group_validation_test.go new file mode 100644 index 00000000000..c2b44980b93 --- /dev/null +++ b/apps/alerting/rules/pkg/app/util/group_validation_test.go @@ -0,0 +1,109 @@ +package util + +import ( + "testing" + + "github.com/grafana/grafana-app-sdk/resource" + model "github.com/grafana/grafana/apps/alerting/rules/pkg/apis/alerting/v0alpha1" +) + +func TestValidateGroupLabels(t *testing.T) { + group := model.GroupLabelKey + groupIdx := model.GroupIndexLabelKey + + tests := []struct { + name string + labels map[string]string + oldLabels map[string]string + action resource.AdmissionAction + wantErr bool + }{ + { + name: "update empty group value", + labels: map[string]string{group: "", groupIdx: "1"}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "update empty group-index value", + labels: map[string]string{group: "g1", groupIdx: ""}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "create empty group value disallowed", + labels: map[string]string{group: ""}, + action: resource.AdmissionActionCreate, + wantErr: true, + }, + { + name: "create no labels allowed", + labels: nil, + action: resource.AdmissionActionCreate, + wantErr: false, + }, + { + name: "create with group disallowed", + labels: map[string]string{group: "g1"}, + action: resource.AdmissionActionCreate, + wantErr: true, + }, + { + name: "create with group-index disallowed", + labels: map[string]string{groupIdx: "1"}, + action: resource.AdmissionActionCreate, + wantErr: true, + }, + { + name: "update missing paired index", + labels: map[string]string{group: "g1"}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "update missing paired group", + labels: map[string]string{groupIdx: "1"}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "update invalid index format", + labels: map[string]string{group: "g1", groupIdx: "x"}, + oldLabels: map[string]string{group: "g1", groupIdx: "0"}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "update cannot add group when previously ungrouped", + labels: map[string]string{group: "g1", groupIdx: "0"}, + oldLabels: map[string]string{}, + action: resource.AdmissionActionUpdate, + wantErr: true, + }, + { + name: "update allowed when previously grouped", + labels: map[string]string{group: "g1", groupIdx: "2"}, + oldLabels: map[string]string{group: "g1", groupIdx: "1"}, + action: resource.AdmissionActionUpdate, + wantErr: false, + }, + { + name: "update no labels remains allowed", + labels: nil, + action: resource.AdmissionActionUpdate, + wantErr: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := ValidateGroupLabels(tc.labels, tc.oldLabels, tc.action) + if tc.wantErr && err == nil { + t.Fatalf("expected error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/pkg/registry/apps/alerting/rules/alertrule/legacy_storage.go b/pkg/registry/apps/alerting/rules/alertrule/legacy_storage.go index 1e2de84b784..13a7d0b01f7 100644 --- a/pkg/registry/apps/alerting/rules/alertrule/legacy_storage.go +++ b/pkg/registry/apps/alerting/rules/alertrule/legacy_storage.go @@ -126,11 +126,6 @@ func (s *legacyStorage) Create(ctx context.Context, obj runtime.Object, createVa if p.GenerateName != "" { return nil, fmt.Errorf("generate-name is not supported in legacy storage mode") } - // TODO: move this to the validation function - if p.Labels[model.GroupLabelKey] != "" || p.Labels[model.GroupIndexLabelKey] != "" { - return nil, k8serrors.NewBadRequest("cannot set group when creating alert rule") - } - model, provenance, err := convertToDomainModel(info.OrgID, p) if err != nil { return nil, err @@ -160,12 +155,6 @@ func (s *legacyStorage) Update(ctx context.Context, name string, objInfo rest.Up return old, false, err } - current, ok := old.(*model.AlertRule) - if !ok { - // this shouldn't really be possible - return nil, false, k8serrors.NewBadRequest("expected valid alert rule object") - } - obj, err := objInfo.UpdatedObject(ctx, old) if err != nil { return old, false, err @@ -180,9 +169,6 @@ func (s *legacyStorage) Update(ctx context.Context, name string, objInfo rest.Up if !ok { return nil, false, k8serrors.NewBadRequest("expected valid alert rule object") } - if current.Labels[model.GroupLabelKey] == "" && new.Labels[model.GroupLabelKey] != "" { - return nil, false, k8serrors.NewBadRequest("cannot set group label when updating un-grouped alert rule") - } model, provenance, err := convertToDomainModel(info.OrgID, new) if err != nil { diff --git a/pkg/registry/apps/alerting/rules/recordingrule/legacy_storage.go b/pkg/registry/apps/alerting/rules/recordingrule/legacy_storage.go index df1b57db523..cb69a51ce19 100644 --- a/pkg/registry/apps/alerting/rules/recordingrule/legacy_storage.go +++ b/pkg/registry/apps/alerting/rules/recordingrule/legacy_storage.go @@ -162,11 +162,6 @@ func (s *legacyStorage) Update(ctx context.Context, name string, objInfo rest.Up return nil, false, err } - current, ok := old.(*model.RecordingRule) - if !ok { - return nil, false, k8serrors.NewBadRequest("expected valid recording rule object") - } - obj, err := objInfo.UpdatedObject(ctx, old) if err != nil { return old, false, err @@ -185,10 +180,6 @@ func (s *legacyStorage) Update(ctx context.Context, name string, objInfo rest.Up if new.Name != "" { new.UID = types.UID(new.Name) } - // TODO: move to validation function - if current.Labels[model.GroupLabelKey] == "" && new.Labels[model.GroupLabelKey] != "" { - return nil, false, k8serrors.NewBadRequest("cannot set group label when updating un-grouped recording rule") - } model, provenance, err := convertToDomainModel(info.OrgID, new) if err != nil {