Repository: VictoriaMetrics/VictoriaMetrics
Pull request #536 (2020-06-06 10:26:21)

vmalert-521: allow to disable rules expression validation.
This feature may be useful for using `vmalert` with PromQL
compatible datasources like Loki.
#521 

codecov[bot]: # [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=h1) Report
> Merging [#536](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=desc) into [master](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/commit/ff624c9125c45ba00bc6dec57cc5c784e319262b&el=desc) will **decrease** coverage by `0.03%`.
> The diff coverage is `88.23%`.

[![Impacted file tree graph](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/graphs/tree.svg?width=650&height=150&src=pr&token=YND8dYVcXn)](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
- Coverage   68.17%   68.13%   -0.04%     
==========================================
  Files         151      151              
  Lines       20885    20885              
==========================================
- Hits        14239    14231       -8     
- Misses       5499     5511      +12     
+ Partials     1147     1143       -4     
```


| [Impacted Files](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [app/vmalert/main.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvbWFpbi5nbw==) | `0.00% <0.00%> (ø)` | |
| [app/vmalert/config/config.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvY29uZmlnL2NvbmZpZy5nbw==) | `79.45% <100.00%> (+8.21%)` | :arrow_up: |
| [app/vmalert/manager.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-YXBwL3ZtYWxlcnQvbWFuYWdlci5nbw==) | `82.60% <100.00%> (ø)` | |
| [lib/fs/dir\_remover.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-bGliL2ZzL2Rpcl9yZW1vdmVyLmdv) | `3.33% <0.00%> (-23.34%)` | :arrow_down: |
| [app/vmselect/promql/aggr\_incremental.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-YXBwL3Ztc2VsZWN0L3Byb21xbC9hZ2dyX2luY3JlbWVudGFsLmdv) | `90.72% <0.00%> (-1.38%)` | :arrow_down: |
| [lib/storage/partition.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-bGliL3N0b3JhZ2UvcGFydGl0aW9uLmdv) | `72.11% <0.00%> (+0.23%)` | :arrow_up: |
| [lib/encoding/int.go](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536/diff?src=pr&el=tree#diff-bGliL2VuY29kaW5nL2ludC5nbw==) | `80.76% <0.00%> (+1.28%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=footer). Last update [ff624c9...172c91b](https://codecov.io/gh/VictoriaMetrics/VictoriaMetrics/pull/536?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
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 err := notifier.ValidateTemplates(r.Annotations); err != nil {
-			return fmt.Errorf("invalid annotations for rule %q.%q: %s", g.Name, ruleName, err)
+		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.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/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)
 	}


Test patch:
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/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))
 			}