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

Alerting: Add validation to check updates on rule groups (#113669)

Browse Source 
This moves some of the validation logic for rule groups from the legacy storage layer to the validator.
This commit is contained in:
Moustafa Baiou
2025-11-10 14:40:35 -05:00
committed by GitHub
parent 142340e0ff
commit dd0a2d4cff
6 changed files with 198 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/alerting/rules/pkg/app/alertrule/validator.go
+15 -16
View File
@@ -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
apps/alerting/rules/pkg/app/recordingrule/validator.go
+15 -15
View File
@@ -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 := ""
apps/alerting/rules/pkg/app/util/group_validation.go
+59
View File
@@ -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
}
apps/alerting/rules/pkg/app/util/group_validation_test.go
+109
View File
@@ -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)
}
})
}
}
pkg/registry/apps/alerting/rules/alertrule/legacy_storage.go
-14
View File
@@ -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 {
pkg/registry/apps/alerting/rules/recordingrule/legacy_storage.go
-9
View File
@@ -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 {