From 758914e1fbb1a9ac386a086135d03ea487ec7fe3 Mon Sep 17 00:00:00 2001 From: Nick Moore Date: Fri, 3 Feb 2023 08:44:29 +0000 Subject: [PATCH 1/2] Fix SanitizeLabelName for certain invalid labels SanitizeLabelName was not correctly sanitizing label names that: 1. Started with a digit (0-9) 2. Were empty This commit changes the santization code to catch both of these edge cases and adds new tests to validate it works correctly in them both. In the first case, a leading digit will be replaced with an underscore, and in the latter, the function will return a single underscore. Signed-off-by: Nick Moore --- util/strutil/strconv.go | 22 +++++++++++++++------- util/strutil/strconv_test.go | 8 ++++++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/util/strutil/strconv.go b/util/strutil/strconv.go index 5d62bac9c..28875fc7f 100644 --- a/util/strutil/strconv.go +++ b/util/strutil/strconv.go @@ -16,12 +16,9 @@ package strutil import ( "fmt" "net/url" - - "github.com/grafana/regexp" + "strings" ) -var invalidLabelCharRE = regexp.MustCompile(`[^a-zA-Z0-9_]`) - // TableLinkForExpression creates an escaped relative link to the table view of // the provided expression. func TableLinkForExpression(expr string) string { @@ -36,8 +33,19 @@ func GraphLinkForExpression(expr string) string { return fmt.Sprintf("/graph?g0.expr=%s&g0.tab=0", escapedExpression) } -// SanitizeLabelName replaces anything that doesn't match -// client_label.LabelNameRE with an underscore. +// SanitizeLabelName replaces any invalid character with an underscore, and if +// given an empty string, returns a string containing a single underscore. func SanitizeLabelName(name string) string { - return invalidLabelCharRE.ReplaceAllString(name, "_") + if len(name) == 0 { + return "_" + } + var validSb strings.Builder + for i, b := range name { + if !((b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '_' || (b >= '0' && b <= '9' && i > 0)) { + validSb.WriteRune('_') + } else { + validSb.WriteRune(b) + } + } + return validSb.String() } diff --git a/util/strutil/strconv_test.go b/util/strutil/strconv_test.go index 05c3eb498..686adbeb9 100644 --- a/util/strutil/strconv_test.go +++ b/util/strutil/strconv_test.go @@ -58,4 +58,12 @@ func TestSanitizeLabelName(t *testing.T) { actual = SanitizeLabelName("barClient.LABEL$$##") expected = "barClient_LABEL____" require.Equal(t, expected, actual, "SanitizeLabelName failed for label (%s)", expected) + + actual = SanitizeLabelName("0zerothClient1LABEL") + expected = "_zerothClient1LABEL" + require.Equal(t, expected, actual, "SanitizeLabelName failed for label (%s)", expected) + + actual = SanitizeLabelName("") + expected = "_" + require.Equal(t, expected, actual, "SanitizeLabelName failed for the empty label") } From c05ebef306f485cb0791f835b6ee254d3092178c Mon Sep 17 00:00:00 2001 From: Nick Moore Date: Fri, 3 Feb 2023 15:23:58 +0000 Subject: [PATCH 2/2] Implement SanitizeLabelName and Full variant Rather than removing the previous implementation of SanitizeLabelName, offer another version named SanitizeFullLabelName that achieved the desired requirements, without breaking existing Prometheus code. Update testing to validate correctness of new variant. Signed-off-by: Nick Moore --- util/strutil/strconv.go | 17 +++++++++++++++-- util/strutil/strconv_test.go | 18 ++++++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/util/strutil/strconv.go b/util/strutil/strconv.go index 28875fc7f..8cdd7d483 100644 --- a/util/strutil/strconv.go +++ b/util/strutil/strconv.go @@ -17,8 +17,12 @@ import ( "fmt" "net/url" "strings" + + "github.com/grafana/regexp" ) +var invalidLabelCharRE = regexp.MustCompile(`[^a-zA-Z0-9_]`) + // TableLinkForExpression creates an escaped relative link to the table view of // the provided expression. func TableLinkForExpression(expr string) string { @@ -33,9 +37,18 @@ func GraphLinkForExpression(expr string) string { return fmt.Sprintf("/graph?g0.expr=%s&g0.tab=0", escapedExpression) } -// SanitizeLabelName replaces any invalid character with an underscore, and if -// given an empty string, returns a string containing a single underscore. +// SanitizeLabelName replaces anything that doesn't match +// client_label.LabelNameRE with an underscore. +// Note: this does not handle all Prometheus label name restrictions (such as +// not starting with a digit 0-9), and hence should only be used if the label +// name is prefixed with a known valid string. func SanitizeLabelName(name string) string { + return invalidLabelCharRE.ReplaceAllString(name, "_") +} + +// SanitizeFullLabelName replaces any invalid character with an underscore, and +// if given an empty string, returns a string containing a single underscore. +func SanitizeFullLabelName(name string) string { if len(name) == 0 { return "_" } diff --git a/util/strutil/strconv_test.go b/util/strutil/strconv_test.go index 686adbeb9..f09e7ffb3 100644 --- a/util/strutil/strconv_test.go +++ b/util/strutil/strconv_test.go @@ -58,12 +58,22 @@ func TestSanitizeLabelName(t *testing.T) { actual = SanitizeLabelName("barClient.LABEL$$##") expected = "barClient_LABEL____" require.Equal(t, expected, actual, "SanitizeLabelName failed for label (%s)", expected) +} + +func TestSanitizeFullLabelName(t *testing.T) { + actual := SanitizeFullLabelName("fooClientLABEL") + expected := "fooClientLABEL" + require.Equal(t, expected, actual, "SanitizeFullLabelName failed for label (%s)", expected) - actual = SanitizeLabelName("0zerothClient1LABEL") + actual = SanitizeFullLabelName("barClient.LABEL$$##") + expected = "barClient_LABEL____" + require.Equal(t, expected, actual, "SanitizeFullLabelName failed for label (%s)", expected) + + actual = SanitizeFullLabelName("0zerothClient1LABEL") expected = "_zerothClient1LABEL" - require.Equal(t, expected, actual, "SanitizeLabelName failed for label (%s)", expected) + require.Equal(t, expected, actual, "SanitizeFullLabelName failed for label (%s)", expected) - actual = SanitizeLabelName("") + actual = SanitizeFullLabelName("") expected = "_" - require.Equal(t, expected, actual, "SanitizeLabelName failed for the empty label") + require.Equal(t, expected, actual, "SanitizeFullLabelName failed for the empty label") }