IntenseWebs/grafana
3
0
Fork 0
mirror of https://github.com/grafana/grafana.git synced 2025-02-25 18:55:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

Alerting: Fix rule API to accept 0 duration of field For (#50992)

Browse Source 
* make 'for' pointer to distinguish between missing field and 0
* set 'for' to -1 if the value is missing but not allow negative in the request + path -1 with the value from original rule
* update store validation to not allow negative 'for'
* update usages to use pointer
This commit is contained in:
Yuriy Tseretyan 2022-06-30 11:46:26 -04:00 committed by GitHub
parent 0e7a495829
commit 8b3b667a47
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 104 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
pkg/services/ngalert/api/api_ruler.go
View File

@ -484,8 +484,9 @@ func toGettableExtendedRuleNode(r ngmodels.AlertRule, namespaceID int64, provena
Provenance: provenance,
},
}
forDuration := model.Duration(r.For)
gettableExtendedRuleNode.ApiRuleNode = &apimodels.ApiRuleNode{
For: model.Duration(r.For),
For: &forDuration,
Annotations: r.Annotations,
Labels: r.Labels,
}

23
pkg/services/ngalert/api/api_ruler_validation.go
View File

@ -110,8 +110,13 @@ func validateRuleNode(
ExecErrState: errorState,
}
var err error
newAlertRule.For, err = validateForInterval(ruleNode)
if err != nil {
return nil, err
}
if ruleNode.ApiRuleNode != nil {
newAlertRule.For = time.Duration(ruleNode.ApiRuleNode.For)
newAlertRule.Annotations = ruleNode.ApiRuleNode.Annotations
newAlertRule.Labels = ruleNode.ApiRuleNode.Labels
@ -131,10 +136,24 @@ func validateRuleNode(
newAlertRule.PanelID = &panelIDValue
}
}
return &newAlertRule, nil
}
// validateForInterval validates ApiRuleNode.For and converts it to time.Duration. If the field is not specified returns 0 if GrafanaManagedAlert.UID is empty and -1 if it is not.
func validateForInterval(ruleNode *apimodels.PostableExtendedRuleNode) (time.Duration, error) {
if ruleNode.ApiRuleNode == nil || ruleNode.ApiRuleNode.For == nil {
if ruleNode.GrafanaManagedAlert.UID != "" {
return -1, nil // will be patched later with the real value of the current version of the rule
}
return 0, nil // if it's a new rule, use the 0 as the default
}
duration := time.Duration(*ruleNode.ApiRuleNode.For)
if duration < 0 {
return 0, fmt.Errorf("field `for` cannot be negative [%v]. 0 or any positive duration are allowed", *ruleNode.ApiRuleNode.For)
}
return duration, nil
}
// validateRuleGroup validates API model (definitions.PostableRuleGroupConfig) and converts it to a collection of models.AlertRule.
// Returns a slice that contains all rules described by API model or error if either group specification or an alert definition is not valid.
func validateRuleGroup(

5
pkg/services/ngalert/api/api_ruler_validation_test.go
View File

@ -42,9 +42,10 @@ func config(t *testing.T) *setting.UnifiedAlertingSettings {
}
func validRule() apimodels.PostableExtendedRuleNode {
forDuration := model.Duration(rand.Int63n(1000))
return apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: model.Duration(rand.Int63n(1000)),
For: &forDuration,
Labels: map[string]string{
"test-label": "data",
},
@ -240,7 +241,7 @@ func TestValidateRuleNode_NoUID(t *testing.T) {
require.Equal(t, name, alert.RuleGroup)
require.Equal(t, models.NoDataState(api.GrafanaManagedAlert.NoDataState), alert.NoDataState)
require.Equal(t, models.ExecutionErrorState(api.GrafanaManagedAlert.ExecErrState), alert.ExecErrState)
require.Equal(t, time.Duration(api.ApiRuleNode.For), alert.For)
require.Equal(t, time.Duration(*api.ApiRuleNode.For), alert.For)
require.Equal(t, api.ApiRuleNode.Annotations, alert.Annotations)
require.Equal(t, api.ApiRuleNode.Labels, alert.Labels)
},

2
pkg/services/ngalert/api/tooling/definitions/cortex-ruler.go
View File

@ -257,7 +257,7 @@ type ApiRuleNode struct {
Record string `yaml:"record,omitempty" json:"record,omitempty"`
Alert string `yaml:"alert,omitempty" json:"alert,omitempty"`
Expr string `yaml:"expr" json:"expr"`
For model.Duration `yaml:"for,omitempty" json:"for,omitempty"`
For *model.Duration `yaml:"for,omitempty" json:"for,omitempty"`
Labels map[string]string `yaml:"labels,omitempty" json:"labels,omitempty"`
Annotations map[string]string `yaml:"annotations,omitempty" json:"annotations,omitempty"`
}

4
pkg/services/ngalert/api/tooling/definitions/cortex-ruler_test.go
View File

@ -50,7 +50,7 @@ func Test_Rule_Marshaling(t *testing.T) {
desc: "grafana with for, annotation and label properties",
input: PostableExtendedRuleNode{
ApiRuleNode: &ApiRuleNode{
For: dur,
For: &dur,
Annotations: map[string]string{"foo": "bar"},
Labels: map[string]string{"label1": "val1"}},
GrafanaManagedAlert: &PostableGrafanaRule{},
@ -136,7 +136,7 @@ func Test_Rule_Group_Marshaling(t *testing.T) {
Rules: []PostableExtendedRuleNode{
{
ApiRuleNode: &ApiRuleNode{
For: dur,
For: &dur,
Annotations: map[string]string{"foo": "bar"},
Labels: map[string]string{"label1": "val1"},
},

2
pkg/services/ngalert/models/alert_rule.go
View File

@ -388,7 +388,7 @@ func PatchPartialAlertRule(existingRule *AlertRule, ruleToPatch *AlertRule) {
if ruleToPatch.NoDataState == "" {
ruleToPatch.NoDataState = existingRule.NoDataState
}
if ruleToPatch.For == 0 {
if ruleToPatch.For == -1 {
ruleToPatch.For = existingRule.For
}
}

4
pkg/services/ngalert/models/alert_rule_test.go
View File

@ -113,9 +113,9 @@ func TestPatchPartialAlertRule(t *testing.T) {
},
},
{
name: "For is 0",
name: "For is -1",
mutator: func(r *AlertRule) {
r.For = 0
r.For = -1
},
},
}

3
pkg/services/ngalert/store/alert_rule.go
View File

@ -479,5 +479,8 @@ func (st DBstore) validateAlertRule(alertRule ngmodels.AlertRule) error {
return err
}
if alertRule.For < 0 {
return fmt.Errorf("%w: field `for` cannot be negative", ngmodels.ErrAlertRuleFailedValidation)
}
return nil
}

2
pkg/tests/api/alerting/api_admin_configuration_test.go
View File

@ -161,7 +161,7 @@ func TestAdminConfiguration_SendingToExternalAlertmanagers(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},

29
pkg/tests/api/alerting/api_alertmanager_test.go
View File

@ -624,7 +624,7 @@ func TestRulerAccess(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -865,7 +865,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: "arulegroup",
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -881,7 +881,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: "arulegroup",
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -911,7 +911,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: "arulegroup",
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -941,7 +941,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: getLongString(t, ngstore.AlertRuleMaxTitleLength+1),
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -972,7 +972,7 @@ func TestAlertRuleCRUD(t *testing.T) {
interval: invalidInterval,
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -1002,7 +1002,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: "arulegroup",
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -1032,7 +1032,7 @@ func TestAlertRuleCRUD(t *testing.T) {
rulegroup: "arulegroup",
rule: apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -1090,7 +1090,7 @@ func TestAlertRuleCRUD(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -1219,6 +1219,7 @@ func TestAlertRuleCRUD(t *testing.T) {
},
{
"expr":"",
"for": "0s",
"grafana_alert":{
"id":2,
"orgId":1,
@ -1269,7 +1270,7 @@ func TestAlertRuleCRUD(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{
"label1": "val42",
"foo": "bar",
@ -1342,7 +1343,7 @@ func TestAlertRuleCRUD(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{
"label1": "val42",
"foo": "bar",
@ -1376,7 +1377,7 @@ func TestAlertRuleCRUD(t *testing.T) {
},
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{
"label1": "val42",
"foo": "bar",
@ -1448,7 +1449,7 @@ func TestAlertRuleCRUD(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: forValue,
For: &forValue,
Labels: map[string]string{
// delete foo label
"label1": "val1", // update label value
@ -1575,7 +1576,7 @@ func TestAlertRuleCRUD(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: forValue,
For: &forValue,
},
GrafanaManagedAlert: &apimodels.PostableGrafanaRule{
UID: ruleUID, // Including the UID in the payload makes the endpoint update the existing rule.

6
pkg/tests/api/alerting/api_prometheus_test.go
View File

@ -87,7 +87,7 @@ func TestPrometheusRules(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -163,7 +163,7 @@ func TestPrometheusRules(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{},
Annotations: map[string]string{"__panelId__": "1"},
},
@ -351,7 +351,7 @@ func TestPrometheusRulesFilterByDashboard(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{},
Annotations: map[string]string{
"__dashboardUid__": dashboardUID,

53
pkg/tests/api/alerting/api_ruler_test.go
View File

@ -274,14 +274,14 @@ func createRule(t *testing.T, client apiClient, folder string) {
interval, err := model.ParseDuration("1m")
require.NoError(t, err)
doubleInterval := 2 * interval
rules := apimodels.PostableRuleGroupConfig{
Name: "arulegroup",
Interval: interval,
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: 2 * interval,
For: &doubleInterval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -414,7 +414,7 @@ func TestRulerRulesFilterByDashboard(t *testing.T) {
Rules: []apimodels.PostableExtendedRuleNode{
{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{},
Annotations: map[string]string{
"__dashboardUid__": dashboardUID,
@ -513,6 +513,7 @@ func TestRulerRulesFilterByDashboard(t *testing.T) {
}
}, {
"expr": "",
"for":"0s",
"grafana_alert": {
"id": 2,
"orgId": 1,
@ -814,13 +815,55 @@ func TestRuleGroupSequence(t *testing.T) {
})
}
func TestRuleUpdate(t *testing.T) {
// Setup Grafana and its Database
dir, path := testinfra.CreateGrafDir(t, testinfra.GrafanaOpts{
DisableLegacyAlerting: true,
EnableUnifiedAlerting: true,
DisableAnonymous: true,
AppModeProduction: true,
})
grafanaListedAddr, store := testinfra.StartGrafana(t, dir, path)
// Create a user to make authenticated requests
createUser(t, store, user.CreateUserCommand{
DefaultOrgRole: string(models.ROLE_EDITOR),
Password: "password",
Login: "grafana",
})
client := newAlertingApiClient(grafanaListedAddr, "grafana", "password")
folder1Title := "folder1"
client.CreateFolder(t, util.GenerateShortUID(), folder1Title)
t.Run("should be able to reset 'for' to 0", func(t *testing.T) {
group := generateAlertRuleGroup(1, alertRuleGen())
expected := model.Duration(10 * time.Second)
group.Rules[0].ApiRuleNode.For = &expected
status, body := client.PostRulesGroup(t, folder1Title, &group)
require.Equalf(t, http.StatusAccepted, status, "failed to post rule group. Response: %s", body)
getGroup := client.GetRulesGroup(t, folder1Title, group.Name)
require.Equal(t, expected, *getGroup.Rules[0].ApiRuleNode.For)
group = convertGettableRuleGroupToPostable(getGroup.GettableRuleGroupConfig)
expected = 0
group.Rules[0].ApiRuleNode.For = &expected
status, body = client.PostRulesGroup(t, folder1Title, &group)
require.Equalf(t, http.StatusAccepted, status, "failed to post rule group. Response: %s", body)
getGroup = client.GetRulesGroup(t, folder1Title, group.Name)
require.Equal(t, expected, *getGroup.Rules[0].ApiRuleNode.For)
})
}
func newTestingRuleConfig(t *testing.T) apimodels.PostableRuleGroupConfig {
interval, err := model.ParseDuration("1m")
require.NoError(t, err)
firstRule := apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},
@ -847,7 +890,7 @@ func newTestingRuleConfig(t *testing.T) apimodels.PostableRuleGroupConfig {
}
secondRule := apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: interval,
For: &interval,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},

3
pkg/tests/api/alerting/testing.go
View File

@ -87,9 +87,10 @@ func getBody(t *testing.T, body io.ReadCloser) string {
func alertRuleGen() func() apimodels.PostableExtendedRuleNode {
return func() apimodels.PostableExtendedRuleNode {
forDuration := model.Duration(10 * time.Second)
return apimodels.PostableExtendedRuleNode{
ApiRuleNode: &apimodels.ApiRuleNode{
For: model.Duration(10 * time.Second),
For: &forDuration,
Labels: map[string]string{"label1": "val1"},
Annotations: map[string]string{"annotation1": "val1"},
},