Merge pull request #890 from cloudflare/tags

Validate value tags
cloudflare · Mar 1, 2024 · 244dfc7 · 244dfc7
2 parents 24ccae9 + 1453b81
commit 244dfc7
Show file tree

Hide file tree

Showing 5 changed files with 323 additions and 57 deletions.
diff --git a/cmd/pint/tests/0077_strict_error_owner.txt b/cmd/pint/tests/0077_strict_error_owner.txt
@@ -6,20 +6,17 @@ cmp stderr stderr.txt
 level=INFO msg="Finding all rules to check" paths=["rules"]
 rules/strict.yml:4-7 Bug: `rule/owner` comments are required in all files, please add a `# pint file/owner $owner` somewhere in this file and/or `# pint rule/owner $owner` on top of each rule. (rule/owner)
  4 |   - alert: foo bar
- 5 |     expr: 0
+ 5 |     expr: up == 0
  6 |     annotations:
  7 |       foo: bar
 
-rules/strict.yml:5 Warning: Alert query doesn't have any condition, it will always fire if the metric exists. (alerts/comparison)
- 5 |     expr: 0
-
-level=INFO msg="Problems found" Bug=1 Warning=1
+level=INFO msg="Problems found" Bug=1
 level=ERROR msg="Fatal error" err="found 1 problem(s) with severity Bug or higher"
 -- rules/strict.yml --
 groups:
 - name: foo
   rules:
   - alert: foo bar
-    expr: 0
+    expr: up == 0
     annotations:
       foo: bar
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -7,6 +7,27 @@
 - [rule/dependency](checks/rule/dependency.md) check will now warn if an alerting rule
   that's being removed in a pull request is being used inside `ALERTS{alertname="..."}`
   or  `ALERTS_FOR_STATE{alertname="..."}` queries.
+- pint will now perform extra validation of YAML files to ensure that all values
+  are mappings and strings.
+  This will error on rules where, for example, label value is unquoted number:
+
+  Bad rule:
+
+  ```yaml
+  - alert: DeadMansSwitch
+    expr: vector(1)
+    labels:
+      priority: 5
+  ```
+
+  Good rule:
+
+  ```yaml
+  - alert: DeadMansSwitch
+    expr: vector(1)
+    labels:
+      priority: "5"
+  ```
 
 ### Changed
 

diff --git a/internal/parser/models.go b/internal/parser/models.go
@@ -26,6 +26,13 @@ func nodeLines(node *yaml.Node, offset int) (lr LineRange) {
 	return lr
 }
 
+func nodeValue(node *yaml.Node) string {
+	if node.Alias != nil {
+		return node.Alias.Value
+	}
+	return node.Value
+}
+
 func mergeComments(node *yaml.Node) (comments []string) {
 	if node.HeadComment != "" {
 		comments = append(comments, node.HeadComment)
@@ -63,10 +70,7 @@ func (yn *YamlNode) IsIdentical(b *YamlNode) bool {
 func newYamlNode(node *yaml.Node, offset int) *YamlNode {
 	n := YamlNode{
 		Lines: nodeLines(node, offset),
-		Value: node.Value,
-	}
-	if node.Alias != nil {
-		n.Value = node.Alias.Value
+		Value: nodeValue(node),
 	}
 	return &n
 }
@@ -79,10 +83,7 @@ func newYamlNodeWithKey(key, node *yaml.Node, offset int) *YamlNode {
 			First: min(keyLines.First, valLines.First),
 			Last:  max(keyLines.Last, valLines.Last),
 		},
-		Value: node.Value,
-	}
-	if node.Alias != nil {
-		n.Value = node.Alias.Value
+		Value: nodeValue(node),
 	}
 	return &n
 }

diff --git a/internal/parser/parser.go b/internal/parser/parser.go
@@ -122,6 +122,16 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool,
 	var keepFiringForPart *YamlNode
 	var annotationsPart *YamlMap
 
+	var recordNode *yaml.Node
+	var alertNode *yaml.Node
+	var exprNode *yaml.Node
+	var forNode *yaml.Node
+	var keepFiringForNode *yaml.Node
+	var labelsNode *yaml.Node
+	var annotationsNode *yaml.Node
+	labelsNodes := map[*yaml.Node]*yaml.Node{}
+	annotationsNodes := map[*yaml.Node]*yaml.Node{}
+
 	var key *yaml.Node
 	unknownKeys := []*yaml.Node{}
 
@@ -158,47 +168,55 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool,
 			switch key.Value {
 			case recordKey:
 				if recordPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, recordKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, recordKey)
 				}
+				recordNode = part
 				recordPart = newYamlNodeWithKey(key, part, offset)
 				lines.Last = max(lines.Last, recordPart.Lines.Last)
 			case alertKey:
 				if alertPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, alertKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, alertKey)
 				}
+				alertNode = part
 				alertPart = newYamlNodeWithKey(key, part, offset)
 				lines.Last = max(lines.Last, alertPart.Lines.Last)
 			case exprKey:
 				if exprPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, exprKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, exprKey)
 				}
+				exprNode = part
 				exprPart = newPromQLExpr(key, part, offset)
 				lines.Last = max(lines.Last, exprPart.Value.Lines.Last)
 			case forKey:
 				if forPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, forKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, forKey)
 				}
+				forNode = part
 				forPart = newYamlNodeWithKey(key, part, offset)
 				lines.Last = max(lines.Last, forPart.Lines.Last)
+			case keepFiringForKey:
+				if keepFiringForPart != nil {
+					return duplicatedKeyError(lines, part.Line+offset, keepFiringForKey)
+				}
+				keepFiringForNode = part
+				keepFiringForPart = newYamlNodeWithKey(key, part, offset)
+				lines.Last = max(lines.Last, keepFiringForPart.Lines.Last)
 			case labelsKey:
 				if labelsPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, labelsKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, labelsKey)
 				}
+				labelsNode = part
+				labelsNodes = mappingNodes(part)
 				labelsPart = newYamlMap(key, part, offset)
 				lines.Last = max(lines.Last, labelsPart.Lines.Last)
 			case annotationsKey:
 				if annotationsPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, annotationsKey, nil)
+					return duplicatedKeyError(lines, part.Line+offset, annotationsKey)
 				}
+				annotationsNode = part
+				annotationsNodes = mappingNodes(part)
 				annotationsPart = newYamlMap(key, part, offset)
 				lines.Last = max(lines.Last, annotationsPart.Lines.Last)
-
-			case keepFiringForKey:
-				if keepFiringForPart != nil {
-					return duplicatedKeyError(lines, part.Line+offset, keepFiringForKey, nil)
-				}
-				keepFiringForPart = newYamlNodeWithKey(key, part, offset)
-				lines.Last = max(lines.Last, keepFiringForPart.Lines.Last)
 			default:
 				unknownKeys = append(unknownKeys, key)
 			}
@@ -345,6 +363,37 @@ func parseRule(content []byte, node *yaml.Node, offset int) (rule Rule, _ bool,
 		}
 	}
 
+	for key, part := range map[string]*yaml.Node{
+		recordKey:        recordNode,
+		alertKey:         alertNode,
+		exprKey:          exprNode,
+		forKey:           forNode,
+		keepFiringForKey: keepFiringForNode,
+	} {
+		if part != nil && !isTag(part.ShortTag(), "!!str") {
+			return invalidValueError(lines, part.Line+offset, key, "string", describeTag(part.ShortTag()))
+		}
+	}
+	for key, part := range map[string]*yaml.Node{
+		labelsKey:      labelsNode,
+		annotationsKey: annotationsNode,
+	} {
+		if part != nil && !isTag(part.ShortTag(), "!!map") {
+			return invalidValueError(lines, part.Line+offset, key, "mapping", describeTag(part.ShortTag()))
+		}
+	}
+
+	for section, parts := range map[string]map[*yaml.Node]*yaml.Node{
+		labelsKey:      labelsNodes,
+		annotationsKey: annotationsNodes,
+	} {
+		for key, value := range parts {
+			if !isTag(value.ShortTag(), "!!str") {
+				return invalidValueError(lines, value.Line+offset, fmt.Sprintf("%s %s", section, nodeValue(key)), "string", describeTag(value.ShortTag()))
+			}
+		}
+	}
+
 	if recordPart != nil && exprPart != nil {
 		rule = Rule{
 			Lines: lines,
@@ -381,7 +430,7 @@ func unpackNodes(node *yaml.Node) []*yaml.Node {
 	nodes := make([]*yaml.Node, 0, len(node.Content))
 	var isMerge bool
 	for _, part := range node.Content {
-		if part.Tag == "!!merge" && part.Value == "<<" {
+		if part.ShortTag() == "!!merge" && part.Value == "<<" {
 			isMerge = true
 		}
 
@@ -482,13 +531,64 @@ func resolveMapAlias(part, parent *yaml.Node) *yaml.Node {
 	return &node
 }
 
-func duplicatedKeyError(lines LineRange, line int, key string, err error) (Rule, bool, error) {
+func duplicatedKeyError(lines LineRange, line int, key string) (Rule, bool, error) {
 	rule := Rule{
 		Lines: lines,
 		Error: ParseError{
 			Line: line,
 			Err:  fmt.Errorf("duplicated %s key", key),
 		},
 	}
-	return rule, false, err
+	return rule, false, nil
+}
+
+func invalidValueError(lines LineRange, line int, key, expectedKind, gotKind string) (Rule, bool, error) {
+	rule := Rule{
+		Lines: lines,
+		Error: ParseError{
+			Line: line,
+			Err:  fmt.Errorf("%s value must be a YAML %s, got %s instead", key, expectedKind, gotKind),
+		},
+	}
+	return rule, false, nil
+}
+
+func isTag(tag, expected string) bool {
+	if tag == "!!null" {
+		return true
+	}
+	return tag == expected
+}
+
+func describeTag(tag string) string {
+	switch tag {
+	case "":
+		return "unknown type"
+	case "!!str":
+		return "string"
+	case "!!int":
+		return "integer"
+	case "!!seq":
+		return "list"
+	case "!!map":
+		return "mapping"
+	case "!!binary":
+		return "binary data"
+	default:
+		return strings.TrimLeft(tag, "!")
+	}
+}
+
+func mappingNodes(node *yaml.Node) map[*yaml.Node]*yaml.Node {
+	m := make(map[*yaml.Node]*yaml.Node, len(node.Content))
+	var key *yaml.Node
+	for _, child := range node.Content {
+		if key != nil {
+			m[key] = child
+			key = nil
+		} else {
+			key = child
+		}
+	}
+	return m
 }