Introduce the rules lint command

This command lints a rules file. The linter's aim is not verify correctness but just YAML and PromQL expression formatting within the rule file. It is meant to always edit in place but has a flag to perform a dry-run.

This closes #39 and allows us to have a more consistent diff whilst running `rules prepare`

Author: gotjosh

README.md

@@ -60,6 +60,12 @@ This command will load each rule group in the specified files and load them into
 
     cortextool rules load ./example_rules_one.yaml ./example_rules_two.yaml  ...
 
+#### Rules Lint
+
+This command lints a rules file. The linter's aim is not verify correctness but just YAML and PromQL expression formatting within the rule file. This command always edits in place, you can use the dry run flag (`-n`) if you'd like to perform a trial run that does not make any changes.
+
+    cortextool rules lint -n ./example_rules_one.yaml ./example_rules_two.yaml ...
+
 #### Rules Prepare
 
 This command prepares a rules file for upload to Cortex. It lints all your PromQL expressions and adds an specific label to your PromQL query aggregations in the file. Unlike, the previous command this one does not interact with your Cortex cluster.

pkg/commands/rules.go

@@ -17,7 +17,7 @@ import (
 	"github.com/grafana/cortextool/pkg/rules"
 	log "github.com/sirupsen/logrus"
 	"gopkg.in/alecthomas/kingpin.v2"
-	"gopkg.in/yaml.v2"
+	yamlv3 "gopkg.in/yaml.v3"
 )
 
 const (
@@ -60,6 +60,9 @@ type RuleCommand struct {
 	InPlaceEdit      bool
 	AggregationLabel string
 
+	// Lint Rules Config
+	LintDryRun bool
+
 	DisableColor bool
 }
 
@@ -93,6 +96,9 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
 	prepareCmd := rulesCmd.
 		Command("prepare", "modifies a set of rules by including an specific label in aggregations.").
 		Action(r.prepare)
+	lintCmd := rulesCmd.
+		Command("lint", "formats a set of rule files. It reorders keys alphabetically, uses 4 spaces as indentantion, and formats PromQL expressions to a single line.").
+		Action(r.lint)
 
 	// Require Cortex cluster address and tentant ID on all these commands
 	for _, c := range []*kingpin.CmdClause{listCmd, printRulesCmd, getRuleGroupCmd, deleteRuleGroupCmd, loadRulesCmd, diffRulesCmd, syncRulesCmd} {
@@ -151,6 +157,15 @@ func (r *RuleCommand) Register(app *kingpin.Application) {
 		"edits the rule file in place",
 	).Short('i').BoolVar(&r.InPlaceEdit)
 	prepareCmd.Flag("label", "label to include as part of the aggregations.").Default(defaultPrepareAggregationLabel).Short('l').StringVar(&r.AggregationLabel)
+
+	// Lint Command
+	lintCmd.Arg("rule-files", "The rule files to check.").Required().ExistingFilesVar(&r.RuleFilesList)
+	lintCmd.Flag("rule-files", "The rule files to check. Flag can be reused to load multiple files.").StringVar(&r.RuleFiles)
+	lintCmd.Flag(
+		"rule-dirs",
+		"Comma seperated list of paths to directories containing rules yaml files. Each file in a directory with a .yml or .yaml suffix will be parsed.",
+	).StringVar(&r.RuleFilesPath)
+	lintCmd.Flag("dry-run", "Performs a trial run that doesn't make any changes and (mostly) produces the same outpupt as a real run.").Short('n').BoolVar(&r.LintDryRun)
 }
 
 func (r *RuleCommand) setup(k *kingpin.ParseContext) error {
@@ -498,24 +513,67 @@ func (r *RuleCommand) prepare(k *kingpin.ParseContext) error {
 	}
 
 	// now, save all the files
-	for _, ns := range namespaces {
-		payload, err := yaml.Marshal(ns)
+	if err := save(namespaces, r.InPlaceEdit); err != nil {
+		return err
+	}
+
+	log.Infof("SUCCESS: %d rules found, %d modified expressions", count, mod)
+
+	return nil
+}
+
+func (r *RuleCommand) lint(k *kingpin.ParseContext) error {
+	err := r.setupFiles()
+	if err != nil {
+		return errors.Wrap(err, "prepare operation unsuccessful, unable to load rules files")
+	}
+
+	namespaces, err := rules.ParseFiles(r.RuleFilesList)
+	if err != nil {
+		return errors.Wrap(err, "prepare operation unsuccessful, unable to parse rules files")
+	}
+
+	var count, mod int
+	for _, ruleNamespace := range namespaces {
+		c, m, err := ruleNamespace.LintPromQLExpressions()
+		if err != nil {
+			return err
+		}
+
+		count += c
+		mod += m
+	}
+
+	if !r.LintDryRun {
+		// linting will always in-place edit unless is a dry-run.
+		if err := save(namespaces, true); err != nil {
+			return err
+		}
+	}
+
+	log.Infof("SUCCESS: %d rules found, %d linted expressions", count, mod)
+
+	return nil
+}
+
+// save saves a set of rule files to to disk. You can specify whenever you want the
+// file(s) to be edited in-place.
+func save(nss map[string]rules.RuleNamespace, i bool) error {
+	for _, ns := range nss {
+		payload, err := yamlv3.Marshal(ns)
 		if err != nil {
 			return err
 		}
 
 		filepath := ns.Filepath
-		if !r.InPlaceEdit {
+		if !i {
 			filepath = filepath + ".result"
 		}
 
-		err = ioutil.WriteFile(filepath, payload, 0644)
-		if err != nil {
+		if err := ioutil.WriteFile(filepath, payload, 0644); err != nil {
 			return err
 		}
 	}
 
-	log.Infof("SUCESS: %d rules found, %d modified expressions", count, mod)
-
 	return nil
 }

pkg/rules/rules.go

@@ -20,7 +20,38 @@ type RuleNamespace struct {
 	Groups []rulefmt.RuleGroup `yaml:"groups"`
 }
 
-// AggregateBy Modifies the aggregation rules in groups to include a given Label.
+// LintPromQLExpressions runs the `expr` from a rule through the PromQL parser and
+// compares its output. if it differs from the parser, it uses the parser's instead.
+func (r RuleNamespace) LintPromQLExpressions() (int, int, error) {
+	// `count` represents the number of rules we evalated.
+	// `mod` represents the number of rules linted.
+	var count, mod int
+	for i, group := range r.Groups {
+		for j, rule := range group.Rules {
+			log.WithFields(log.Fields{"rule": getRuleName(rule)}).Debugf("linting PromQL")
+			exp, err := promql.ParseExpr(rule.Expr)
+			if err != nil {
+				return count, mod, err
+			}
+
+			count++
+			if rule.Expr != exp.String() {
+				log.WithFields(log.Fields{
+					"rule":        getRuleName(rule),
+					"currentExpr": rule.Expr,
+					"afterExpr":   exp.String(),
+				}).Debugf("expression differs")
+
+				mod++
+				r.Groups[i].Rules[j].Expr = exp.String()
+			}
+		}
+	}
+
+	return count, mod, nil
+}
+
+// AggregateBy modifies the aggregation rules in groups to include a given Label.
 func (r RuleNamespace) AggregateBy(label string) (int, int, error) {
 	// `count` represents the number of rules we evalated.
 	// `mod` represents the number of rules we modified - a modification can either be a lint or adding the
@@ -42,7 +73,12 @@ func (r RuleNamespace) AggregateBy(label string) (int, int, error) {
 			promql.Inspect(exp, f)
 
 			// Only modify the ones that actually changed.
-			if r.Groups[i].Rules[j].Expr != exp.String() {
+			if rule.Expr != exp.String() {
+				log.WithFields(log.Fields{
+					"rule":        getRuleName(rule),
+					"currentExpr": rule.Expr,
+					"afterExpr":   exp.String(),
+				}).Debugf("expression differs")
 				mod++
 				r.Groups[i].Rules[j].Expr = exp.String()
 			}

pkg/rules/rules_test.go

@@ -1,6 +1,7 @@
 package rules
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/prometheus/prometheus/pkg/rulefmt"
@@ -95,3 +96,63 @@ func TestAggregateBy(t *testing.T) {
 		})
 	}
 }
+
+func TestLintPromQLExpressions(t *testing.T) {
+	tt := []struct {
+		name            string
+		expr            string
+		expected        string
+		err             error
+		count, modified int
+	}{
+		{
+			name:     "it lints simple expressions",
+			expr:     "up                                   != 1",
+			expected: "up != 1",
+			count:    1, modified: 1,
+			err: nil,
+		},
+		{
+			name:     "it lints aggregations expressions",
+			expr:     "avg (rate(prometheus_notifications_queue_capacity[5m])) by (cluster, job)",
+			expected: "avg by(cluster, job) (rate(prometheus_notifications_queue_capacity[5m]))",
+			count:    1, modified: 1,
+			err: nil,
+		},
+		{
+			name:     "with no opinion",
+			expr:     "build_tag_info > 1",
+			expected: "build_tag_info > 1",
+			count:    1, modified: 0,
+			err: nil,
+		},
+		{
+			name:     "with an invalid expression",
+			expr:     "it fails",
+			expected: "it fails",
+			count:    0, modified: 0,
+			err: errors.New(`parse error at char 4: could not parse remaining input "fails"...`),
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			r := RuleNamespace{Groups: []rulefmt.RuleGroup{{Rules: []rulefmt.Rule{
+				{Alert: "AName", Expr: tc.expr},
+			}}}}
+
+			c, m, err := r.LintPromQLExpressions()
+			rexpr := r.Groups[0].Rules[0].Expr
+
+			require.Equal(t, tc.count, c)
+			require.Equal(t, tc.modified, m)
+			require.Equal(t, tc.expected, rexpr)
+
+			if tc.err == nil {
+				require.NoError(t, err)
+			} else {
+				require.EqualError(t, err, tc.err.Error())
+			}
+		})
+	}
+}