From ae93bae88fed884f59adef604ea059deff81650b Mon Sep 17 00:00:00 2001
From: Julius Volz <julius.volz@gmail.com>
Date: Fri, 27 Dec 2019 10:32:19 +0100
Subject: [PATCH] Fix: exclude metric name in labels.MatchLabels() (#6523)

This function is only used in one place to format an error message when
encountering multiple matches on the "one" side of a
one-to-many/many-to-one vector match, which is probably why nobodoy has
noticed this before. The hashing is already done correctly and excludes
the metric name label when using the "ignoring" matching modifier.

Signed-off-by: Julius Volz <julius.volz@gmail.com>
---
 pkg/labels/labels.go      |   2 +-
 pkg/labels/labels_test.go | 137 +++++++++++++++++++++++++-------------
 2 files changed, 93 insertions(+), 46 deletions(-)

diff --git a/pkg/labels/labels.go b/pkg/labels/labels.go
index 93c0a338b..da6f60bd1 100644
--- a/pkg/labels/labels.go
+++ b/pkg/labels/labels.go
@@ -109,7 +109,7 @@ func (ls Labels) MatchLabels(on bool, names ...string) Labels {
 	}
 
 	for _, v := range ls {
-		if _, ok := nameSet[v.Name]; on == ok {
+		if _, ok := nameSet[v.Name]; on == ok && (on || v.Name != MetricName) {
 			matchedLabels = append(matchedLabels, v)
 		}
 	}
diff --git a/pkg/labels/labels_test.go b/pkg/labels/labels_test.go
index 3651ae5f1..173288aff 100644
--- a/pkg/labels/labels_test.go
+++ b/pkg/labels/labels_test.go
@@ -15,6 +15,8 @@ package labels
 
 import (
 	"testing"
+
+	"github.com/prometheus/prometheus/util/testutil"
 )
 
 func TestLabels_MatchLabels(t *testing.T) {
@@ -45,60 +47,105 @@ func TestLabels_MatchLabels(t *testing.T) {
 		},
 	}
 
-	providedNames := []string{
-		"__name__",
-		"alertname",
-		"alertstate",
-		"instance",
-	}
-
-	got := labels.MatchLabels(true, providedNames...)
-	expected := Labels{
+	tests := []struct {
+		providedNames []string
+		on            bool
+		expected      Labels
+	}{
+		// on = true, explicitly including metric name in matching.
 		{
-			Name:  "__name__",
-			Value: "ALERTS",
+			providedNames: []string{
+				"__name__",
+				"alertname",
+				"alertstate",
+				"instance",
+			},
+			on: true,
+			expected: Labels{
+				{
+					Name:  "__name__",
+					Value: "ALERTS",
+				},
+				{
+					Name:  "alertname",
+					Value: "HTTPRequestRateLow",
+				},
+				{
+					Name:  "alertstate",
+					Value: "pending",
+				},
+				{
+					Name:  "instance",
+					Value: "0",
+				},
+			},
 		},
+		// on = false, explicitly excluding metric name from matching.
 		{
-			Name:  "alertname",
-			Value: "HTTPRequestRateLow",
+			providedNames: []string{
+				"__name__",
+				"alertname",
+				"alertstate",
+				"instance",
+			},
+			on: false,
+			expected: Labels{
+				{
+					Name:  "job",
+					Value: "app-server",
+				},
+				{
+					Name:  "severity",
+					Value: "critical",
+				},
+			},
 		},
+		// on = true, explicitly excluding metric name from matching.
 		{
-			Name:  "alertstate",
-			Value: "pending",
+			providedNames: []string{
+				"alertname",
+				"alertstate",
+				"instance",
+			},
+			on: true,
+			expected: Labels{
+				{
+					Name:  "alertname",
+					Value: "HTTPRequestRateLow",
+				},
+				{
+					Name:  "alertstate",
+					Value: "pending",
+				},
+				{
+					Name:  "instance",
+					Value: "0",
+				},
+			},
 		},
+		// on = false, implicitly excluding metric name from matching.
 		{
-			Name:  "instance",
-			Value: "0",
+			providedNames: []string{
+				"alertname",
+				"alertstate",
+				"instance",
+			},
+			on: false,
+			expected: Labels{
+				{
+					Name:  "job",
+					Value: "app-server",
+				},
+				{
+					Name:  "severity",
+					Value: "critical",
+				},
+			},
 		},
 	}
 
-	assertSlice(t, got, expected)
-
-	// Now try with 'on' set to false.
-	got = labels.MatchLabels(false, providedNames...)
-
-	expected = Labels{
-		{
-			Name:  "job",
-			Value: "app-server",
-		},
-		{
-			Name:  "severity",
-			Value: "critical",
-		},
-	}
-
-	assertSlice(t, got, expected)
-}
-
-func assertSlice(t *testing.T, got, expected Labels) {
-	if len(expected) != len(got) {
-		t.Errorf("expected the length of matched label names to be %d, but got %d", len(expected), len(got))
-	}
-
-	for i, expectedLabel := range expected {
-		if expectedLabel.Name != got[i].Name {
-			t.Errorf("expected to get Label with name %s, but got %s instead", expectedLabel.Name, got[i].Name)
-		}
+	for i, test := range tests {
+		got := labels.MatchLabels(test.on, test.providedNames...)
+		testutil.Equals(t, test.expected, got, "unexpected labelset for test case %d", i)
 	}
 }