Skip to content

Commit

Permalink
Fix nil pointer exceptions in certain broken dependency scenarios
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
LloydW93 committed Mar 13, 2024
1 parent 7cea6a7 commit c3716b0
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 3 deletions.
6 changes: 3 additions & 3 deletions internal/checks/rule_dependency.go
Original file line number Diff line number Diff line change
Expand Up @@ -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("- `")
Expand Down
28 changes: 28 additions & 0 deletions internal/checks/rule_dependency_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit c3716b0

Please sign in to comment.