From ea26377c50bf55bfda05e74fa07df4086c9e1b56 Mon Sep 17 00:00:00 2001 From: Adesh Deshmukh Date: Wed, 26 Aug 2026 05:37:52 +0530 Subject: [PATCH] Fix metrics-collector panic on invalid regular expression filters GetFilterRegexpList discarded the error from regexp.Compile, so an invalid user-provided filter produced a nil *regexp.Regexp that panicked the metrics-collector sidecar when matched against log lines, leaving the Trial without reported metrics. Handle the compile error and fail fast with a descriptive message identifying the invalid filter. Filters are also now compiled once at startup instead of on every log line in the watch loop. Fixes #2707 Signed-off-by: Adesh Deshmukh --- .../v1beta1/file-metricscollector/main.go | 10 +++-- .../file-metricscollector.go | 17 ++++++--- .../file-metricscollector_test.go | 37 +++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/cmd/metricscollector/v1beta1/file-metricscollector/main.go b/cmd/metricscollector/v1beta1/file-metricscollector/main.go index 5730df39150..cdc23563ccc 100644 --- a/cmd/metricscollector/v1beta1/file-metricscollector/main.go +++ b/cmd/metricscollector/v1beta1/file-metricscollector/main.go @@ -44,7 +44,6 @@ import ( "fmt" "os" "path/filepath" - "regexp" "strconv" "strings" "time" @@ -175,6 +174,12 @@ func watchMetricsFile(mFile string, stopRules stopRulesFlag, filters []string, f klog.Fatalf("Failed to create new Process from pid %v, error: %v", mainProcPid, err) } + // Get list of regural expressions from filters. + metricRegList, err := filemc.GetFilterRegexpList(filters) + if err != nil { + klog.Fatalf("Invalid metric filter: %v", err) + } + // Start watch log lines. t, _ := tail.TailFile(mFile, tail.Config{Follow: true}) for line := range t.Lines { @@ -184,9 +189,6 @@ func watchMetricsFile(mFile string, stopRules stopRulesFlag, filters []string, f switch fileFormat { case commonv1beta1.TextFormat: - // Get list of regural expressions from filters. - var metricRegList []*regexp.Regexp - metricRegList = filemc.GetFilterRegexpList(filters) // Check if log line contains metric from stop rules. isRuleLine := false diff --git a/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go b/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go index a9ddc9ba2cd..89f7f51d18b 100644 --- a/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go +++ b/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector.go @@ -70,7 +70,10 @@ func CollectObservationLog(fileName string, metrics []string, filters []string, } func parseLogsInTextFormat(logs []string, metrics []string, filters []string) (*v1beta1.ObservationLog, error) { - metricRegList := GetFilterRegexpList(filters) + metricRegList, err := GetFilterRegexpList(filters) + if err != nil { + return nil, err + } mlogs := make([]*v1beta1.MetricLog, 0, len(logs)) for _, logline := range logs { @@ -238,15 +241,19 @@ func parseTimestamp(timestamp interface{}) string { } } -// GetFilterRegexpList returns Regexp array from filters string array -func GetFilterRegexpList(filters []string) []*regexp.Regexp { +// GetFilterRegexpList returns Regexp array from filters string array. +// Returns an error if any filter is not a valid regular expression. +func GetFilterRegexpList(filters []string) ([]*regexp.Regexp, error) { regexpList := make([]*regexp.Regexp, 0, len(filters)) if len(filters) == 0 { filters = append(filters, common.DefaultFilter) } for _, filter := range filters { - reg, _ := regexp.Compile(filter) + reg, err := regexp.Compile(filter) + if err != nil { + return nil, fmt.Errorf("invalid metric filter %q: %v", filter, err) + } regexpList = append(regexpList, reg) } - return regexpList + return regexpList, nil } diff --git a/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go b/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go index 7e8042ef66b..bb3d2df3e0e 100644 --- a/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go +++ b/pkg/metricscollector/v1beta1/file-metricscollector/file-metricscollector_test.go @@ -19,6 +19,7 @@ package sidecarmetricscollector import ( "os" "path/filepath" + "strings" "testing" "time" @@ -327,3 +328,39 @@ invalid INFO {metricName: loss, metricValue: 0.3634}`, }) } } + +func TestGetFilterRegexpList(t *testing.T) { + // Invalid filters must return a descriptive error instead of a nil regexp which panics later. + invalidFilter := `{metricName: ([\w|-]+, metricValue: (.*)}` + regList, err := GetFilterRegexpList([]string{invalidFilter}) + if err == nil { + t.Errorf("Expected error for invalid metric filter %v", invalidFilter) + } else if !strings.Contains(err.Error(), "invalid metric filter") || !strings.Contains(err.Error(), invalidFilter) { + t.Errorf("Unexpected error for invalid metric filter: %v", err) + } + if regList != nil { + t.Errorf("Expected nil regexp list for invalid metric filter, got: %v", regList) + } + + // Empty filters must fall back to the default filter. + regList, err = GetFilterRegexpList([]string{}) + if err != nil { + t.Errorf("Unexpected error for empty filters: %v", err) + } + if len(regList) != 1 { + t.Errorf("Expected default filter regexp, got: %v", regList) + } + + // Valid filters must compile without error. + validFilters := []string{ + `{metricName: ([\w|-]+), metricValue: ((-?\d+)(\.\d+)?)}`, + "loss=([+-]?\\d*(\\.\\d+)?([Ee][+-]?\\d+)?)", + } + regList, err = GetFilterRegexpList(validFilters) + if err != nil { + t.Errorf("Unexpected error for valid filters: %v", err) + } + if len(regList) != len(validFilters) { + t.Errorf("Expected %v regexps, got: %v", len(validFilters), len(regList)) + } +}