From e893c89333d250e4061b857d1876d2f2d6caee71 Mon Sep 17 00:00:00 2001
From: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>
Date: Wed, 14 Jun 2017 15:07:54 +0530
Subject: [PATCH] Validate labels and annotations

Signed-off-by: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>
---
 pkg/rulefmt/rulefmt.go                       | 18 ++++++++++++++++++
 pkg/rulefmt/rulefmt_test.go                  |  8 ++++++++
 pkg/rulefmt/testdata/bad_annotation.bad.yaml |  8 ++++++++
 pkg/rulefmt/testdata/bad_lname.bad.yaml      |  8 ++++++++
 pkg/rulefmt/testdata/test.yaml               | 18 ++++++++++++------
 rules/manager.go                             |  4 +---
 6 files changed, 55 insertions(+), 9 deletions(-)
 create mode 100644 pkg/rulefmt/testdata/bad_annotation.bad.yaml
 create mode 100644 pkg/rulefmt/testdata/bad_lname.bad.yaml

diff --git a/pkg/rulefmt/rulefmt.go b/pkg/rulefmt/rulefmt.go
index cd922fd4f..8fef3a1c8 100644
--- a/pkg/rulefmt/rulefmt.go
+++ b/pkg/rulefmt/rulefmt.go
@@ -18,6 +18,7 @@ import (
 
 	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
+	"github.com/prometheus/common/model"
 	"github.com/prometheus/prometheus/promql"
 )
 
@@ -111,6 +112,23 @@ func (r *Rule) Validate() (errs []error) {
 			errs = append(errs, errors.Errorf("invalid field 'for' in recording rule"))
 		}
 	}
+
+	for k, v := range r.Labels {
+		if !model.LabelName(k).IsValid() {
+			errs = append(errs, errors.Errorf("invalid label name: %s", k))
+		}
+
+		if !model.LabelValue(v).IsValid() {
+			errs = append(errs, errors.Errorf("invalid label value: %s", v))
+		}
+	}
+
+	for k := range r.Annotations {
+		if !model.LabelName(k).IsValid() {
+			errs = append(errs, errors.Errorf("invalid annotation name: %s", k))
+		}
+	}
+
 	return errs
 }
 
diff --git a/pkg/rulefmt/rulefmt_test.go b/pkg/rulefmt/rulefmt_test.go
index fd914e839..95d088f18 100644
--- a/pkg/rulefmt/rulefmt_test.go
+++ b/pkg/rulefmt/rulefmt_test.go
@@ -57,6 +57,14 @@ func TestParseFileFailure(t *testing.T) {
 			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",
+		},
 	}
 
 	for _, c := range table {
diff --git a/pkg/rulefmt/testdata/bad_annotation.bad.yaml b/pkg/rulefmt/testdata/bad_annotation.bad.yaml
new file mode 100644
index 000000000..523da2322
--- /dev/null
+++ b/pkg/rulefmt/testdata/bad_annotation.bad.yaml
@@ -0,0 +1,8 @@
+Version: 1
+Groups:
+  - name: yolo
+    rules:
+    - alert: hola
+      expr: 1
+      annotations:
+        ins-tance: localhost 
diff --git a/pkg/rulefmt/testdata/bad_lname.bad.yaml b/pkg/rulefmt/testdata/bad_lname.bad.yaml
new file mode 100644
index 000000000..3ece3cf44
--- /dev/null
+++ b/pkg/rulefmt/testdata/bad_lname.bad.yaml
@@ -0,0 +1,8 @@
+Version: 1
+Groups:
+  - name: yolo
+    rules:
+      - record: hola
+        expr: 1
+        labels:
+          ins-tance: localhost
diff --git a/pkg/rulefmt/testdata/test.yaml b/pkg/rulefmt/testdata/test.yaml
index 70bb267ee..d7cf526b7 100644
--- a/pkg/rulefmt/testdata/test.yaml
+++ b/pkg/rulefmt/testdata/test.yaml
@@ -6,7 +6,8 @@ groups:
   - alert: HighErrors
     expr: |
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
     for: 5m
     labels:
       severity: critical
@@ -17,7 +18,8 @@ groups:
   - record: "new_metric"
     expr: |
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
     labels:
       abc: edf
       uvw: xyz
@@ -25,7 +27,8 @@ groups:
   - alert: HighErrors
     expr: | 
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
     for: 5m
     labels:
       severity: critical
@@ -38,7 +41,8 @@ groups:
   - alert: HighErrors
     expr: | 
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
     for: 5m
     labels:
       severity: critical
@@ -46,12 +50,14 @@ groups:
   - record: "new_metric"
     expr: |
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
 
   - alert: HighErrors
     expr: | 
       sum without(instance) (rate(errors_total[5m]))
-      / sum without(instance) (rate(requests_total[5m]))
+      / 
+      sum without(instance) (rate(requests_total[5m]))
     for: 5m
     labels:
       severity: critical
diff --git a/rules/manager.go b/rules/manager.go
index 9720c3829..ec8812c80 100644
--- a/rules/manager.go
+++ b/rules/manager.go
@@ -267,9 +267,7 @@ func typeForRule(r Rule) ruleType {
 	panic(fmt.Errorf("unknown rule type: %T", r))
 }
 
-// Eval runs a single evaluation cycle in which all rules are evaluated in parallel.
-// In the future a single group will be evaluated sequentially to properly handle
-// rule dependency.
+// Eval runs a single evaluation cycle in which all rules are evaluated sequentially.
 func (g *Group) Eval(ts time.Time) {
 	for i, rule := range g.rules {
 		rtyp := string(typeForRule(rule))