From 5d68ebb2076f2776e5c553b56f92b57e99a8fe61 Mon Sep 17 00:00:00 2001 From: johncming Date: Sat, 21 Mar 2020 21:34:00 +0800 Subject: [PATCH] pkg/rulefmt: fix bug of validate. Signed-off-by: johncming --- model/rulefmt/rulefmt.go | 18 +- .../record_and_alert.both_set.bad.yaml | 6 + .../record_and_alert.both_unset.bad.yaml | 4 + pkg/rulefmt/rulefmt_test.go | 175 ++++++++++++++++++ 4 files changed, 191 insertions(+), 12 deletions(-) create mode 100644 model/rulefmt/testdata/record_and_alert.both_set.bad.yaml create mode 100644 model/rulefmt/testdata/record_and_alert.both_unset.bad.yaml create mode 100644 pkg/rulefmt/rulefmt_test.go diff --git a/model/rulefmt/rulefmt.go b/model/rulefmt/rulefmt.go index 30b3face0d..6be93f6428 100644 --- a/model/rulefmt/rulefmt.go +++ b/model/rulefmt/rulefmt.go @@ -173,17 +173,11 @@ func (r *RuleNode) Validate() (nodes []WrappedError) { }) } if r.Record.Value == "" && r.Alert.Value == "" { - if r.Record.Value == "0" { - nodes = append(nodes, WrappedError{ - err: fmt.Errorf("one of 'record' or 'alert' must be set"), - node: &r.Alert, - }) - } else { - nodes = append(nodes, WrappedError{ - err: fmt.Errorf("one of 'record' or 'alert' must be set"), - node: &r.Record, - }) - } + nodes = append(nodes, WrappedError{ + err: fmt.Errorf("one of 'record' or 'alert' must be set"), + node: &r.Record, + nodeAlt: &r.Alert, + }) } if r.Expr.Value == "" { @@ -250,7 +244,7 @@ func (r *RuleNode) Validate() (nodes []WrappedError) { nodes = append(nodes, WrappedError{err: err}) } - return + return nodes } // testTemplateParsing checks if the templates used in labels and annotations diff --git a/model/rulefmt/testdata/record_and_alert.both_set.bad.yaml b/model/rulefmt/testdata/record_and_alert.both_set.bad.yaml new file mode 100644 index 0000000000..0ba81b7423 --- /dev/null +++ b/model/rulefmt/testdata/record_and_alert.both_set.bad.yaml @@ -0,0 +1,6 @@ +groups: +- name: yolo + rules: + - record: Hi + alert: Hello + expr: 1 diff --git a/model/rulefmt/testdata/record_and_alert.both_unset.bad.yaml b/model/rulefmt/testdata/record_and_alert.both_unset.bad.yaml new file mode 100644 index 0000000000..64d2e8f20d --- /dev/null +++ b/model/rulefmt/testdata/record_and_alert.both_unset.bad.yaml @@ -0,0 +1,4 @@ +groups: + - name: yolo + rules: + - expr: 1 diff --git a/pkg/rulefmt/rulefmt_test.go b/pkg/rulefmt/rulefmt_test.go new file mode 100644 index 0000000000..58b5d5ad36 --- /dev/null +++ b/pkg/rulefmt/rulefmt_test.go @@ -0,0 +1,175 @@ +// Copyright 2017 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package rulefmt + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/prometheus/prometheus/util/testutil" +) + +func TestParseFileSuccess(t *testing.T) { + if _, errs := ParseFile("testdata/test.yaml"); len(errs) > 0 { + t.Errorf("unexpected errors parsing file") + for _, err := range errs { + t.Error(err) + } + } +} + +func TestParseFileFailure(t *testing.T) { + table := []struct { + filename string + errMsg string + }{ + { + filename: "duplicate_grp.bad.yaml", + errMsg: "groupname: \"yolo\" is repeated in the same file", + }, + { + filename: "bad_expr.bad.yaml", + errMsg: "parse error", + }, + { + filename: "record_and_alert.both_set.bad.yaml", + errMsg: "'alert' and 'record' cannot be set at the same time", + }, + { + filename: "record_and_alert.both_unset.bad.yaml", + errMsg: "one of 'record' or 'alert' must be set", + }, + { + filename: "no_rec_alert.bad.yaml", + errMsg: "one of 'record' or 'alert' must be set", + }, + { + filename: "noexpr.bad.yaml", + errMsg: "field 'expr' must be set in rule", + }, + { + filename: "bad_lname.bad.yaml", + errMsg: "invalid label name", + }, + { + filename: "bad_annotation.bad.yaml", + errMsg: "invalid annotation name", + }, + { + filename: "invalid_record_name.bad.yaml", + errMsg: "invalid recording rule name", + }, + { + filename: "bad_field.bad.yaml", + errMsg: "field annotation not found", + }, + { + filename: "invalid_label_name.bad.yaml", + errMsg: "invalid label name", + }, + } + + for _, c := range table { + _, errs := ParseFile(filepath.Join("testdata", c.filename)) + if errs == nil { + t.Errorf("Expected error parsing %s but got none", c.filename) + continue + } + if !strings.Contains(errs[0].Error(), c.errMsg) { + t.Errorf("Expected error for %s to contain %q but got: %s", c.filename, c.errMsg, errs) + } + } + +} + +func TestTemplateParsing(t *testing.T) { + tests := []struct { + ruleString string + shouldPass bool + }{ + { + ruleString: ` +groups: +- name: example + rules: + - alert: InstanceDown + expr: up == 0 + for: 5m + labels: + severity: "page" + annotations: + summary: "Instance {{ $labels.instance }} down" +`, + shouldPass: true, + }, + { + // `$label` instead of `$labels`. + ruleString: ` +groups: +- name: example + rules: + - alert: InstanceDown + expr: up == 0 + for: 5m + labels: + severity: "page" + annotations: + summary: "Instance {{ $label.instance }} down" +`, + shouldPass: false, + }, + { + // `$this_is_wrong`. + ruleString: ` +groups: +- name: example + rules: + - alert: InstanceDown + expr: up == 0 + for: 5m + labels: + severity: "{{$this_is_wrong}}" + annotations: + summary: "Instance {{ $labels.instance }} down" +`, + shouldPass: false, + }, + { + // `$labels.quantile * 100`. + ruleString: ` +groups: +- name: example + rules: + - alert: InstanceDown + expr: up == 0 + for: 5m + labels: + severity: "page" + annotations: + summary: "Instance {{ $labels.instance }} down" + description: "{{$labels.quantile * 100}}" +`, + shouldPass: false, + }, + } + + for _, tst := range tests { + rgs, errs := Parse([]byte(tst.ruleString)) + testutil.Assert(t, rgs != nil, "Rule parsing, rule=\n"+tst.ruleString) + passed := (tst.shouldPass && len(errs) == 0) || (!tst.shouldPass && len(errs) > 0) + testutil.Assert(t, passed, "Rule validation failed, rule=\n"+tst.ruleString) + } + +}