Merge pull request #128012 from yongruilin/fix-allow-metrics

fix: Remove unnecessary lock in metrics parsing allow-list manifest

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,
@@ -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 {

staging/src/k8s.io/component-base/metrics/opts_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package metrics
 
 import (
+	"os"
 	"reflect"
 	"testing"
 
@@ -64,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"}
@@ -97,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 {
@@ -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.Set[string]{
+						"label1": sets.New[string]("v1", "v2"),
+					},
+				},
+				"metric2": {
+					labelToAllowList: map[string]sets.Set[string]{
+						"label2": sets.New[string]("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)
+			}
+		})
+	}
+}