From ffa75c423d9b24636ea1048ea655037319becd31 Mon Sep 17 00:00:00 2001 From: Roman Khavronenko Date: Sat, 6 Jun 2020 21:27:09 +0100 Subject: [PATCH] vmalert-521: allow to disable rules expression validation. (#536) This feature may be useful for using `vmalert` with PromQL compatible datasources like Loki. --- app/vmalert/README.md | 4 +- app/vmalert/config/config.go | 27 ++++----- app/vmalert/config/config_test.go | 92 +++++++++++++++++++++++++++++-- app/vmalert/group_test.go | 2 +- app/vmalert/main.go | 11 ++-- app/vmalert/manager.go | 8 +-- app/vmalert/manager_test.go | 10 ++-- 7 files changed, 120 insertions(+), 34 deletions(-) diff --git a/app/vmalert/README.md b/app/vmalert/README.md index c783ab08e..0f054b834 100644 --- a/app/vmalert/README.md +++ b/app/vmalert/README.md @@ -106,8 +106,10 @@ Usage of vmalert: -rule /path/to/file. Path to a single file with alerting rules -rule dir/*.yaml -rule /*.yaml. Relative path to all .yaml files in "dir" folder, absolute path to all .yaml files in root. + -rule.validateExpressions + Whether to validate rules expressions via MetricsQL engine (default true) -rule.validateTemplates - Indicates to validate annotation and label templates (default true) + Whether to validate annotation and label templates (default true) ``` Pass `-help` to `vmalert` in order to see the full list of supported diff --git a/app/vmalert/config/config.go b/app/vmalert/config/config.go index ee05f03e0..9002ed270 100644 --- a/app/vmalert/config/config.go +++ b/app/vmalert/config/config.go @@ -25,7 +25,7 @@ type Group struct { } // Validate check for internal Group or Rule configuration errors -func (g *Group) Validate(validateAnnotations bool) error { +func (g *Group) Validate(validateAnnotations, validateExpressions bool) error { if g.Name == "" { return fmt.Errorf("group name must be set") } @@ -45,14 +45,18 @@ func (g *Group) Validate(validateAnnotations bool) error { if err := r.Validate(); err != nil { return fmt.Errorf("invalid rule %q.%q: %s", g.Name, ruleName, err) } - if !validateAnnotations { - continue + if validateExpressions { + if _, err := metricsql.Parse(r.Expr); err != nil { + return fmt.Errorf("invalid expression for rule %q.%q: %s", g.Name, ruleName, err) + } } - if err := notifier.ValidateTemplates(r.Annotations); err != nil { - return fmt.Errorf("invalid annotations for rule %q.%q: %s", g.Name, ruleName, err) - } - if err := notifier.ValidateTemplates(r.Labels); err != nil { - return fmt.Errorf("invalid labels for rule %q.%q: %s", g.Name, ruleName, err) + if validateAnnotations { + if err := notifier.ValidateTemplates(r.Annotations); err != nil { + return fmt.Errorf("invalid annotations for rule %q.%q: %s", g.Name, ruleName, err) + } + if err := notifier.ValidateTemplates(r.Labels); err != nil { + return fmt.Errorf("invalid labels for rule %q.%q: %s", g.Name, ruleName, err) + } } } return checkOverflow(g.XXX, fmt.Sprintf("group %q", g.Name)) @@ -77,14 +81,11 @@ func (r *Rule) Validate() error { if r.Expr == "" { return fmt.Errorf("expression can't be empty") } - if _, err := metricsql.Parse(r.Expr); err != nil { - return fmt.Errorf("invalid expression: %w", err) - } return nil } // Parse parses rule configs from given file patterns -func Parse(pathPatterns []string, validateAnnotations bool) ([]Group, error) { +func Parse(pathPatterns []string, validateAnnotations, validateExpressions bool) ([]Group, error) { var fp []string for _, pattern := range pathPatterns { matches, err := filepath.Glob(pattern) @@ -101,7 +102,7 @@ func Parse(pathPatterns []string, validateAnnotations bool) ([]Group, error) { return nil, fmt.Errorf("failed to parse file %q: %w", file, err) } for _, g := range gr { - if err := g.Validate(validateAnnotations); err != nil { + if err := g.Validate(validateAnnotations, validateExpressions); err != nil { return nil, fmt.Errorf("invalid group %q in file %q: %s", g.Name, file, err) } if _, ok := uniqueGroups[g.Name]; ok { diff --git a/app/vmalert/config/config_test.go b/app/vmalert/config/config_test.go index 26b3f9762..ff81553f5 100644 --- a/app/vmalert/config/config_test.go +++ b/app/vmalert/config/config_test.go @@ -16,7 +16,7 @@ func TestMain(m *testing.M) { } func TestParseGood(t *testing.T) { - if _, err := Parse([]string{"testdata/*good.rules", "testdata/dir/*good.*"}, true); err != nil { + if _, err := Parse([]string{"testdata/*good.rules", "testdata/dir/*good.*"}, true, true); err != nil { t.Errorf("error parsing files %s", err) } } @@ -56,7 +56,7 @@ func TestParseBad(t *testing.T) { }, } for _, tc := range testCases { - _, err := Parse(tc.path, true) + _, err := Parse(tc.path, true, true) if err == nil { t.Errorf("expected to get error") return @@ -74,10 +74,90 @@ func TestRule_Validate(t *testing.T) { if err := (&Rule{Alert: "alert"}).Validate(); err == nil { t.Errorf("exptected empty expr error") } - if err := (&Rule{Alert: "alert", Expr: "test{"}).Validate(); err == nil { - t.Errorf("exptected invalid expr error") - } if err := (&Rule{Alert: "alert", Expr: "test>0"}).Validate(); err != nil { - t.Errorf("exptected valid rule got %s", err) + t.Errorf("exptected valid rule; got %s", err) + } +} + +func TestGroup_Validate(t *testing.T) { + testCases := []struct { + group *Group + rules []Rule + validateAnnotations bool + validateExpressions bool + expErr string + }{ + { + group: &Group{}, + expErr: "group name must be set", + }, + { + group: &Group{Name: "test"}, + expErr: "contain no rules", + }, + { + group: &Group{Name: "test", + Rules: []Rule{ + { + Record: "record", + Expr: "up | 0", + }, + }, + }, + expErr: "", + }, + { + group: &Group{Name: "test", + Rules: []Rule{ + { + Record: "record", + Expr: "up | 0", + }, + }, + }, + expErr: "invalid expression", + validateExpressions: true, + }, + { + group: &Group{Name: "test", + Rules: []Rule{ + { + Alert: "alert", + Expr: "up == 1", + Labels: map[string]string{ + "summary": "{{ value|query }}", + }, + }, + }, + }, + expErr: "", + }, + { + group: &Group{Name: "test", + Rules: []Rule{ + { + Alert: "alert", + Expr: "up == 1", + Labels: map[string]string{ + "summary": "{{ value|query }}", + }, + }, + }, + }, + expErr: "error parsing annotation", + validateAnnotations: true, + }, + } + for _, tc := range testCases { + err := tc.group.Validate(tc.validateAnnotations, tc.validateExpressions) + if err == nil { + if tc.expErr != "" { + t.Errorf("expected to get err %q; got nil insted", tc.expErr) + } + continue + } + if !strings.Contains(err.Error(), tc.expErr) { + t.Errorf("expected err to contain %q; got %q instead", tc.expErr, err) + } } } diff --git a/app/vmalert/group_test.go b/app/vmalert/group_test.go index c3c213cdc..dc2b53039 100644 --- a/app/vmalert/group_test.go +++ b/app/vmalert/group_test.go @@ -133,7 +133,7 @@ func TestUpdateWith(t *testing.T) { func TestGroupStart(t *testing.T) { // TODO: make parsing from string instead of file - groups, err := config.Parse([]string{"config/testdata/rules1-good.rules"}, true) + groups, err := config.Parse([]string{"config/testdata/rules1-good.rules"}, true, true) if err != nil { t.Fatalf("failed to parse rules: %s", err) } diff --git a/app/vmalert/main.go b/app/vmalert/main.go index 526e78312..1686e6884 100644 --- a/app/vmalert/main.go +++ b/app/vmalert/main.go @@ -30,8 +30,11 @@ Examples: -rule /path/to/file. Path to a single file with alerting rules -rule dir/*.yaml -rule /*.yaml. Relative path to all .yaml files in "dir" folder, absolute path to all .yaml files in root.`) - validateTemplates = flag.Bool("rule.validateTemplates", true, "Indicates to validate annotation and label templates") - httpListenAddr = flag.String("httpListenAddr", ":8880", "Address to listen for http connections") + + validateTemplates = flag.Bool("rule.validateTemplates", true, "Whether to validate annotation and label templates") + validateExpressions = flag.Bool("rule.validateExpressions", true, "Whether to validate rules expressions via MetricsQL engine") + + httpListenAddr = flag.String("httpListenAddr", ":8880", "Address to listen for http connections") datasourceURL = flag.String("datasource.url", "", "Victoria Metrics or VMSelect url. Required parameter."+ " E.g. http://127.0.0.1:8428") @@ -100,7 +103,7 @@ func main() { manager.rr = datasource.NewVMStorage(*remoteReadURL, *remoteReadUsername, *remoteReadPassword, &http.Client{}) } - if err := manager.start(ctx, *rulePath, *validateTemplates); err != nil { + if err := manager.start(ctx, *rulePath, *validateTemplates, *validateExpressions); err != nil { logger.Fatalf("failed to start: %s", err) } @@ -113,7 +116,7 @@ func main() { <-sigHup configReloads.Inc() logger.Infof("SIGHUP received. Going to reload rules %q ...", *rulePath) - if err := manager.update(ctx, *rulePath, *validateTemplates, false); err != nil { + if err := manager.update(ctx, *rulePath, *validateTemplates, *validateExpressions, false); err != nil { configReloadErrors.Inc() configSuccess.Set(0) logger.Errorf("error while reloading rules: %s", err) diff --git a/app/vmalert/manager.go b/app/vmalert/manager.go index cad0278a2..30928f0cb 100644 --- a/app/vmalert/manager.go +++ b/app/vmalert/manager.go @@ -48,8 +48,8 @@ func (m *manager) AlertAPI(gID, aID uint64) (*APIAlert, error) { return nil, fmt.Errorf("can't find alert with id %q in group %q", aID, g.Name) } -func (m *manager) start(ctx context.Context, path []string, validate bool) error { - return m.update(ctx, path, validate, true) +func (m *manager) start(ctx context.Context, path []string, validateTpl, validateExpr bool) error { + return m.update(ctx, path, validateTpl, validateExpr, true) } func (m *manager) close() { @@ -79,9 +79,9 @@ func (m *manager) startGroup(ctx context.Context, group *Group, restore bool) { m.groups[id] = group } -func (m *manager) update(ctx context.Context, path []string, validate, restore bool) error { +func (m *manager) update(ctx context.Context, path []string, validateTpl, validateExpr, restore bool) error { logger.Infof("reading rules configuration file from %q", strings.Join(path, ";")) - groupsCfg, err := config.Parse(path, validate) + groupsCfg, err := config.Parse(path, validateTpl, validateExpr) if err != nil { return fmt.Errorf("cannot parse configuration file: %s", err) } diff --git a/app/vmalert/manager_test.go b/app/vmalert/manager_test.go index 21239818f..6eb0ad8e1 100644 --- a/app/vmalert/manager_test.go +++ b/app/vmalert/manager_test.go @@ -22,7 +22,7 @@ func TestMain(m *testing.M) { func TestManagerUpdateError(t *testing.T) { m := &manager{groups: make(map[uint64]*Group)} path := []string{"foo/bar"} - err := m.update(context.Background(), path, true, false) + err := m.update(context.Background(), path, true, true, false) if err == nil { t.Fatalf("expected to have err; got nil instead") } @@ -51,7 +51,7 @@ func TestManagerUpdateConcurrent(t *testing.T) { "config/testdata/rules2-good.rules", } *evaluationInterval = time.Millisecond - if err := m.start(context.Background(), []string{paths[0]}, true); err != nil { + if err := m.start(context.Background(), []string{paths[0]}, true, true); err != nil { t.Fatalf("failed to start: %s", err) } @@ -65,7 +65,7 @@ func TestManagerUpdateConcurrent(t *testing.T) { for i := 0; i < iterations; i++ { rnd := rand.Intn(len(paths)) path := []string{paths[rnd]} - _ = m.update(context.Background(), path, true, false) + _ = m.update(context.Background(), path, true, true, false) } }() } @@ -186,12 +186,12 @@ func TestManagerUpdate(t *testing.T) { ctx, cancel := context.WithCancel(context.TODO()) m := &manager{groups: make(map[uint64]*Group), storage: &fakeQuerier{}} path := []string{tc.initPath} - if err := m.update(ctx, path, true, false); err != nil { + if err := m.update(ctx, path, true, true, false); err != nil { t.Fatalf("failed to complete initial rules update: %s", err) } path = []string{tc.updatePath} - _ = m.update(ctx, path, true, false) + _ = m.update(ctx, path, true, true, false) if len(tc.want) != len(m.groups) { t.Fatalf("\nwant number of groups: %d;\ngot: %d ", len(tc.want), len(m.groups)) }