From b6643512849d8adcf590b64d58ce73d44c9884ec Mon Sep 17 00:00:00 2001 From: yongruilin Date: Fri, 11 Oct 2024 16:45:33 -0700 Subject: [PATCH 1/2] fix: Remove unnecessary lock in metrics parsing allow-list manifest --- .../src/k8s.io/component-base/metrics/opts.go | 2 - .../component-base/metrics/opts_test.go | 70 +++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 59994e3db9d..1633a8558ce 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -363,8 +363,6 @@ func SetLabelAllowListFromCLI(allowListMapping map[string]string) { } func SetLabelAllowListFromManifest(manifest string) { - allowListLock.Lock() - defer allowListLock.Unlock() allowListMapping := make(map[string]string) data, err := os.ReadFile(filepath.Clean(manifest)) if err != nil { diff --git a/staging/src/k8s.io/component-base/metrics/opts_test.go b/staging/src/k8s.io/component-base/metrics/opts_test.go index 46d5bade21f..eb61a6fbd1d 100644 --- a/staging/src/k8s.io/component-base/metrics/opts_test.go +++ b/staging/src/k8s.io/component-base/metrics/opts_test.go @@ -17,6 +17,7 @@ limitations under the License. package metrics import ( + "os" "reflect" "testing" @@ -138,3 +139,72 @@ func TestConstrainLabelMap(t *testing.T) { }) } } + +func TestSetLabelAllowListFromManifest(t *testing.T) { + tests := []struct { + name string + manifest string + manifestExist bool + expectlabelValueAllowLists map[string]*MetricLabelAllowList + }{ + { + name: "successfully parse manifest", + manifestExist: true, + manifest: `metric1,label1: v1,v2 +metric2,label2: v3`, + expectlabelValueAllowLists: map[string]*MetricLabelAllowList{ + "metric1": { + labelToAllowList: map[string]sets.String{ + "label1": sets.NewString("v1", "v2"), + }, + }, + "metric2": { + labelToAllowList: map[string]sets.String{ + "label2": sets.NewString("v3"), + }, + }, + }, + }, + { + name: "failed to read manifest file", + manifestExist: false, + expectlabelValueAllowLists: map[string]*MetricLabelAllowList{}, + }, + { + name: "failed to parse manifest", + manifestExist: true, + manifest: `allow-list: +- metric1,label1:v1 +- metric2,label2:v2,v3`, + expectlabelValueAllowLists: map[string]*MetricLabelAllowList{}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + labelValueAllowLists = map[string]*MetricLabelAllowList{} + manifestFilePath := "/non-existent-file.yaml" + if tc.manifestExist { + tempFile, err := os.CreateTemp("", "allow-list-test") + if err != nil { + t.Fatalf("failed to create temp file: %v", err) + } + defer func() { + if err := os.Remove(tempFile.Name()); err != nil { + t.Errorf("failed to remove temp file: %v", err) + } + }() + + if _, err := tempFile.WriteString(tc.manifest); err != nil { + t.Fatalf("failed to write to temp file: %v", err) + } + manifestFilePath = tempFile.Name() + } + + SetLabelAllowListFromManifest(manifestFilePath) + if !reflect.DeepEqual(labelValueAllowLists, tc.expectlabelValueAllowLists) { + t.Errorf("labelValueAllowLists = %+v, want %+v", labelValueAllowLists, tc.expectlabelValueAllowLists) + } + }) + } +} From c9979e6fbc00f94be28f1c85dd8d21d9f01f0ec1 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Wed, 16 Oct 2024 14:29:06 -0700 Subject: [PATCH 2/2] chore: Use generic sets for metrics The sets.String has been deprecated. --- .../src/k8s.io/component-base/metrics/opts.go | 6 +++--- .../k8s.io/component-base/metrics/opts_test.go | 16 ++++++++-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 1633a8558ce..4f69b2611e0 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -315,7 +315,7 @@ func (o *SummaryOpts) toPromSummaryOpts() prometheus.SummaryOpts { } type MetricLabelAllowList struct { - labelToAllowList map[string]sets.String + labelToAllowList map[string]sets.Set[string] } func (allowList *MetricLabelAllowList) ConstrainToAllowedList(labelNameList, labelValueList []string) { @@ -347,13 +347,13 @@ func SetLabelAllowListFromCLI(allowListMapping map[string]string) { for metricLabelName, labelValues := range allowListMapping { metricName := strings.Split(metricLabelName, ",")[0] labelName := strings.Split(metricLabelName, ",")[1] - valueSet := sets.NewString(strings.Split(labelValues, ",")...) + valueSet := sets.New[string](strings.Split(labelValues, ",")...) allowList, ok := labelValueAllowLists[metricName] if ok { allowList.labelToAllowList[labelName] = valueSet } else { - labelToAllowList := make(map[string]sets.String) + labelToAllowList := make(map[string]sets.Set[string]) labelToAllowList[labelName] = valueSet labelValueAllowLists[metricName] = &MetricLabelAllowList{ labelToAllowList, diff --git a/staging/src/k8s.io/component-base/metrics/opts_test.go b/staging/src/k8s.io/component-base/metrics/opts_test.go index eb61a6fbd1d..b2e0fb0295c 100644 --- a/staging/src/k8s.io/component-base/metrics/opts_test.go +++ b/staging/src/k8s.io/component-base/metrics/opts_test.go @@ -65,8 +65,8 @@ func TestDefaultStabilityLevel(t *testing.T) { func TestConstrainToAllowedList(t *testing.T) { allowList := &MetricLabelAllowList{ - labelToAllowList: map[string]sets.String{ - "label_a": sets.NewString("allow_value1", "allow_value2"), + labelToAllowList: map[string]sets.Set[string]{ + "label_a": sets.New[string]("allow_value1", "allow_value2"), }, } labelNameList := []string{"label_a", "label_b"} @@ -98,8 +98,8 @@ func TestConstrainToAllowedList(t *testing.T) { func TestConstrainLabelMap(t *testing.T) { allowList := &MetricLabelAllowList{ - labelToAllowList: map[string]sets.String{ - "label_a": sets.NewString("allow_value1", "allow_value2"), + labelToAllowList: map[string]sets.Set[string]{ + "label_a": sets.New[string]("allow_value1", "allow_value2"), }, } var tests = []struct { @@ -154,13 +154,13 @@ func TestSetLabelAllowListFromManifest(t *testing.T) { metric2,label2: v3`, expectlabelValueAllowLists: map[string]*MetricLabelAllowList{ "metric1": { - labelToAllowList: map[string]sets.String{ - "label1": sets.NewString("v1", "v2"), + labelToAllowList: map[string]sets.Set[string]{ + "label1": sets.New[string]("v1", "v2"), }, }, "metric2": { - labelToAllowList: map[string]sets.String{ - "label2": sets.NewString("v3"), + labelToAllowList: map[string]sets.Set[string]{ + "label2": sets.New[string]("v3"), }, }, },