From c3716b0e3c5088e299a3fca3ee752cf5772d0013 Mon Sep 17 00:00:00 2001
From: Lloyd Wallis <lloyd@cloudflare.com>
Date: Wed, 13 Mar 2024 13:02:17 +0000
Subject: [PATCH] Fix nil pointer exceptions in certain broken dependency
 scenarios

When there are one or more broken dependency problems, the final
value of `dep` when evaluating all entries is the one used when
preparing the problem item.

However, it is possible that after a problem is identified,
subsequent entries are recording or alerting rules which either
do not have any vector selectors, or none that match.

In this situation, the final value of `dep` is `nil`, which causes
a crash.

This change fixes this by using the first broken dependency entry
recorded for building this part of the string.
---
 internal/checks/rule_dependency.go      |  6 +++---
 internal/checks/rule_dependency_test.go | 28 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/internal/checks/rule_dependency.go b/internal/checks/rule_dependency.go
index d9381ba9..ab4af887 100644
--- a/internal/checks/rule_dependency.go
+++ b/internal/checks/rule_dependency.go
@@ -90,12 +90,12 @@ func (c RuleDependencyCheck) Check(_ context.Context, _ string, rule parser.Rule
 
 	var details strings.Builder
 	details.WriteString("If you remove the ")
-	details.WriteString(dep.kind)
+	details.WriteString(broken[0].kind)
 	details.WriteString(" rule generating `")
-	details.WriteString(dep.metric)
+	details.WriteString(broken[0].metric)
 	details.WriteString("`, and there is no other source of this metric, then any other rule depending on it will break.\n")
 	details.WriteString("List of found rules that are using `")
-	details.WriteString(dep.metric)
+	details.WriteString(broken[0].metric)
 	details.WriteString("`:\n\n")
 	for _, b := range broken {
 		details.WriteString("- `")
diff --git a/internal/checks/rule_dependency_test.go b/internal/checks/rule_dependency_test.go
index bc03979b..6dba089a 100644
--- a/internal/checks/rule_dependency_test.go
+++ b/internal/checks/rule_dependency_test.go
@@ -285,6 +285,34 @@ func TestRuleDependencyCheck(t *testing.T) {
 `, discovery.Noop, "foo.yaml", "foo.yaml")[0],
 			},
 		},
+		{
+			description: "warns about removed dependency without crashing if it is not the last rule",
+			content:     "- record: foo\n  expr: sum(foo)\n",
+			checker: func(_ *promapi.FailoverGroup) checks.RuleChecker {
+				return checks.NewRuleDependencyCheck()
+			},
+			prometheus: newSimpleProm,
+			problems: func(_ string) []checks.Problem {
+				return []checks.Problem{
+					{
+						Anchor: checks.AnchorBefore,
+						Lines: parser.LineRange{
+							First: 1,
+							Last:  2,
+						},
+						Reporter: checks.RuleDependencyCheckName,
+						Text:     textDependencyRule(1),
+						Details:  detailsDependencyRule("recording", "foo", "- `alert` at `foo.yaml:2`\n"),
+						Severity: checks.Warning,
+					},
+				}
+			},
+			entries: []discovery.Entry{
+				parseWithState("- record: foo\n  expr: sum(foo)\n", discovery.Removed, "foo.yaml", "foo.yaml")[0],
+				parseWithState("- alert: alert\n  expr: foo == 0\n", discovery.Noop, "foo.yaml", "foo.yaml")[0],
+				parseWithState("- record: bar\n  expr: vector(0)\n", discovery.Noop, "foo.yaml", "foo.yaml")[0],
+			},
+		},
 	}
 
 	runTests(t, testCases)