Fix metrics-collector panic on invalid regular expression filters

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

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
 }

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))
+	}
+}