Repository: kubernetes/kubernetes (language: go)

Issue:
Components hang forever due to a bug in parsing allow metric labels manifest
### What happened?

The components hang forever when initializing with `allow-metric-labels-manifest` is set.

For example, kube-scheduler set the flag as follows
```
root@dev-control-plane:/etc/kubernetes/manifests# cat kube-scheduler.yaml |grep -A8 command
  - command:
    - kube-scheduler
    - --authentication-kubeconfig=/etc/kubernetes/scheduler.conf
    - --authorization-kubeconfig=/etc/kubernetes/scheduler.conf
    - --bind-address=127.0.0.1
    - --kubeconfig=/etc/kubernetes/scheduler.conf
    - --leader-elect=true
    - --allow-metric-labels-manifest=/etc/kubernetes/allow-list.yaml
    image: registry.k8s.io/kube-scheduler:v1.30.0
```
It hangs forever
```
root@dev-control-plane:/etc/kubernetes# kubectl logs -n kube-system kube-scheduler-dev-control-plane -f
I1011 00:10:55.222334       1 serving.go:380] Generated self-signed cert in-memory



^C
```

It should be due to the bug in function [SetLabelAllowListFromManifest](https://github.com/kubernetes/kubernetes/blob/c15581b277e9e42d830898357fdbd92dcf7981c6/staging/src/k8s.io/component-base/metrics/opts.go#L366) which locks the `allowListLock` and tries to double lock in [here](https://github.com/kubernetes/kubernetes/blob/c15581b277e9e42d830898357fdbd92dcf7981c6/staging/src/k8s.io/component-base/metrics/opts.go#L379)

### What did you expect to happen?

The component start and run properly. e.g.
```
root@dev-control-plane:/etc/kubernetes/manifests# kubectl logs -n kube-system kube-scheduler-dev-control-plane -f
I1011 00:20:14.946492       1 serving.go:380] Generated self-signed cert in-memory
I1011 00:20:15.109004       1 server.go:154] "Starting Kubernetes Scheduler" version="v1.30.0"
I1011 00:20:15.109046       1 server.go:156] "Golang settings" GOGC="" GOMAXPROCS="" GOTRACEBACK=""
I1011 00:20:15.113642       1 configmap_cafile_content.go:202] "Starting controller" name="client-ca::kube-system::extension-apiserver-authentication::client-ca-file"
I1011 00:20:15.113671       1 configmap_cafile_content.go:202] "Starting controller" name="client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file"
I1011 00:20:15.113643       1 requestheader_controller.go:169] Starting RequestHeaderAuthRequestController
I1011 00:20:15.113762       1 secure_serving.go:213] Serving securely on 127.0.0.1:10259
I1011 00:20:15.113802       1 shared_informer.go:313] Waiting for caches to sync for RequestHeaderAuthRequestController
I1011 00:20:15.113869       1 tlsconfig.go:240] "Starting DynamicServingCertificateController"
I1011 00:20:15.113668       1 shared_informer.go:313] Waiting for caches to sync for client-ca::kube-system::extension-apiserver-authentication::client-ca-file
I1011 00:20:15.113707       1 shared_informer.go:313] Waiting for caches to sync for client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file
I1011 00:20:15.214278       1 shared_informer.go:320] Caches are synced for client-ca::kube-system::extension-apiserver-authentication::client-ca-file
I1011 00:20:15.214325       1 leaderelection.go:250] attempting to acquire leader lease kube-system/kube-scheduler...
I1011 00:20:15.214474       1 shared_informer.go:320] Caches are synced for client-ca::kube-system::extension-apiserver-authentication::requestheader-client-ca-file
I1011 00:20:15.214495       1 shared_informer.go:320] Caches are synced for RequestHeaderAuthRequestController
I1011 00:20:32.976351       1 leaderelection.go:260] successfully acquired lease kube-system/kube-scheduler
```

### How can we reproduce it (as minimally and precisely as possible)?

Mount the correct format allow-list manifest to the pod and set the `--allow-metric-label-manfiests` flag
e.g. static kube-scheduler manfiest
```
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    component: kube-scheduler
    tier: control-plane
  name: kube-scheduler
  namespace: kube-system
spec:
  containers:
  - command:
    - kube-scheduler
    - --authentication-kubeconfig=/etc/kubernetes/scheduler.conf
    - --authorization-kubeconfig=/etc/kubernetes/scheduler.conf
    - --bind-address=127.0.0.1
    - --kubeconfig=/etc/kubernetes/scheduler.conf
    - --leader-elect=true
    image: registry.k8s.io/kube-scheduler:v1.30.0
    imagePullPolicy: IfNotPresent
    livenessProbe:
      failureThreshold: 8
      httpGet:
        host: 127.0.0.1
        path: /healthz
        port: 10259
        scheme: HTTPS
      initialDelaySeconds: 10
      periodSeconds: 10
      timeoutSeconds: 15
    name: kube-scheduler
    resources:
      requests:
        cpu: 100m
    startupProbe:
      failureThreshold: 24
      httpGet:
        host: 127.0.0.1
        path: /healthz
        port: 10259
        scheme: HTTPS
      initialDelaySeconds: 10
      periodSeconds: 10
      timeoutSeconds: 15
    volumeMounts:
    - mountPath: /etc/kubernetes/scheduler.conf
      name: kubeconfig
      readOnly: true
    - mountPath: /etc/kubernetes/allow-list.yaml
      name: allowlist
      readOnly: true
  hostNetwork: true
  priority: 2000001000
  priorityClassName: system-node-critical
  securityContext:
    seccompProfile:
      type: RuntimeDefault
  volumes:
  - hostPath:
      path: /etc/kubernetes/scheduler.conf
      type: FileOrCreate
    name: kubeconfig
  - hostPath:
      path: /etc/kubernetes/allow-list.yaml
      type: File
    name: allowlist
```
Then check the pod's log
```
root@dev-control-plane:/etc/kubernetes/manifests# kubectl logs -n kube-system kube-scheduler-dev-control-plane -f
```

### Anything else we need to know?

It should be an easy fix by removing the locking logic https://github.com/kubernetes/kubernetes/blob/c15581b277e9e42d830898357fdbd92dcf7981c6/staging/src/k8s.io/component-base/metrics/opts.go#L366 and adding more test to cover this.

Also, I've sent a PR to update the documentation to apply the correct manifest format 
https://github.com/kubernetes/website/pull/48283

### Kubernetes version

<details>

```console
$ kubectl version
# paste output here
Client Version: v1.30.0
```

</details>


### Cloud provider

<details>

</details>


### OS version

<details>

```console
# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here
```

</details>


### Install tools

<details>

</details>


### Container runtime (CRI) and version (if applicable)

<details>

</details>


### Related plugins (CNI, CSI, ...) and versions (if applicable)

<details>

</details>



Discussion / hints:
This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the `triage/accepted` label and provide further guidance.

The `triage/accepted` label can be added by org members by writing `/triage accepted` in a comment.


<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
/sig instrumentation
/assign

Fix patch:
diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go
index 59994e3db9d7b..4f69b2611e067 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,
@@ -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 {


Test patch:
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 46d5bade21f12..b2e0fb0295cdb 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"
 
@@ -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)
+			}
+		})
+	}
+}