Fix a metric relabel config unescaping bug (#2872)

.chloggen/fix-unescaping.yaml

@@ -0,0 +1,19 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix a metric relabel config unescaping bug
+
+# One or more tracking issues related to the change
+issues: [2867]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  If only metric relabel configs were present, without target relabel configs, unescaping wouldn't be applied, leading
+  to invalid Target Allocator configuration.
+

internal/manifests/collector/targetallocator_test.go

@@ -376,6 +376,43 @@ func TestGetScrapeConfigs(t *testing.T) {
 				{Object: map[string]interface{}{"job": "somejob"}},
 			},
 		},
+		{
+			name: "regex substitution",
+			input: v1beta1.Config{
+				Receivers: v1beta1.AnyConfig{
+					Object: map[string]interface{}{
+						"prometheus": map[string]any{
+							"config": map[string]any{
+								"scrape_configs": []any{
+									map[string]any{
+										"job": "somejob",
+										"metric_relabel_configs": []map[string]any{
+											{
+												"action":      "labelmap",
+												"regex":       "label_(.+)",
+												"replacement": "$$1",
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			want: []v1beta1.AnyConfig{
+				{Object: map[string]interface{}{
+					"job": "somejob",
+					"metric_relabel_configs": []any{
+						map[any]any{
+							"action":      "labelmap",
+							"regex":       "label_(.+)",
+							"replacement": "$1",
+						},
+					},
+				}},
+			},
+		},
 	}
 
 	for _, testCase := range testCases {

internal/manifests/targetallocator/adapters/config_to_prom_config.go

@@ -134,62 +134,58 @@ func UnescapeDollarSignsInPromConfig(cfg string) (map[interface{}]interface{}, e
 
 	for i, scrapeConfig := range scrapeConfigs {
 
-		relabelConfigsProperty, ok := scrapeConfig["relabel_configs"]
-		if !ok {
-			continue
-		}
-
-		relabelConfigs, ok := relabelConfigsProperty.([]interface{})
-		if !ok {
-			return nil, errorNotAListAtIndex("relabel_configs", i)
-		}
-
-		for i, rc := range relabelConfigs {
-			relabelConfig, rcErr := rc.(map[interface{}]interface{})
-			if !rcErr {
-				return nil, errorNotAMapAtIndex("relabel_config", i)
+		relabelConfigsProperty, found := scrapeConfig["relabel_configs"]
+		if found {
+			relabelConfigs, ok := relabelConfigsProperty.([]interface{})
+			if !ok {
+				return nil, errorNotAListAtIndex("relabel_configs", i)
 			}
 
-			replacementProperty, rcErr := relabelConfig["replacement"]
-			if !rcErr {
-				continue
-			}
+			for i, rc := range relabelConfigs {
+				relabelConfig, rcErr := rc.(map[interface{}]interface{})
+				if !rcErr {
+					return nil, errorNotAMapAtIndex("relabel_config", i)
+				}
 
-			replacement, rcErr := replacementProperty.(string)
-			if !rcErr {
-				return nil, errorNotAStringAtIndex("replacement", i)
-			}
+				replacementProperty, rcErr := relabelConfig["replacement"]
+				if !rcErr {
+					continue
+				}
 
-			relabelConfig["replacement"] = strings.ReplaceAll(replacement, "$$", "$")
-		}
+				replacement, rcErr := replacementProperty.(string)
+				if !rcErr {
+					return nil, errorNotAStringAtIndex("replacement", i)
+				}
 
-		metricRelabelConfigsProperty, ok := scrapeConfig["metric_relabel_configs"]
-		if !ok {
-			continue
-		}
-
-		metricRelabelConfigs, ok := metricRelabelConfigsProperty.([]interface{})
-		if !ok {
-			return nil, errorNotAListAtIndex("metric_relabel_configs", i)
+				relabelConfig["replacement"] = strings.ReplaceAll(replacement, "$$", "$")
+			}
 		}
 
-		for i, rc := range metricRelabelConfigs {
-			relabelConfig, ok := rc.(map[interface{}]interface{})
+		metricRelabelConfigsProperty, found := scrapeConfig["metric_relabel_configs"]
+		if found {
+			metricRelabelConfigs, ok := metricRelabelConfigsProperty.([]interface{})
 			if !ok {
-				return nil, errorNotAMapAtIndex("metric_relabel_config", i)
+				return nil, errorNotAListAtIndex("metric_relabel_configs", i)
 			}
 
-			replacementProperty, ok := relabelConfig["replacement"]
-			if !ok {
-				continue
-			}
+			for i, rc := range metricRelabelConfigs {
+				relabelConfig, ok := rc.(map[interface{}]interface{})
+				if !ok {
+					return nil, errorNotAMapAtIndex("metric_relabel_config", i)
+				}
 
-			replacement, ok := replacementProperty.(string)
-			if !ok {
-				return nil, errorNotAStringAtIndex("replacement", i)
-			}
+				replacementProperty, ok := relabelConfig["replacement"]
+				if !ok {
+					continue
+				}
 
-			relabelConfig["replacement"] = strings.ReplaceAll(replacement, "$$", "$")
+				replacement, ok := replacementProperty.(string)
+				if !ok {
+					return nil, errorNotAStringAtIndex("replacement", i)
+				}
+
+				relabelConfig["replacement"] = strings.ReplaceAll(replacement, "$$", "$")
+			}
 		}
 	}
 

internal/manifests/targetallocator/adapters/config_to_prom_config_test.go

@@ -117,7 +117,85 @@ func TestExtractPromConfigFromNullConfig(t *testing.T) {
 }
 
 func TestUnescapeDollarSignsInPromConfig(t *testing.T) {
-	actual := `
+	testCases := []struct {
+		description string
+		input       string
+		expected    string
+	}{
+		{
+			description: "no scrape configs",
+			input: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs: []
+`,
+			expected: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs: []
+`,
+		},
+		{
+			description: "only metric relabellings",
+			input: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs:
+      - job_name: 'example'
+        metric_relabel_configs:
+        - source_labels: ['job']
+          target_label: 'job'
+          replacement: '$$1_$2'
+`,
+			expected: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs:
+      - job_name: 'example'
+        metric_relabel_configs:
+        - source_labels: ['job']
+          target_label: 'job'
+          replacement: '$1_$2'
+`,
+		},
+		{
+			description: "only target relabellings",
+			input: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs:
+      - job_name: 'example'
+        relabel_configs:
+        - source_labels: ['__meta_service_id']
+          target_label: 'job'
+          replacement: 'my_service_$$1'
+        - source_labels: ['__meta_service_name']
+          target_label: 'instance'
+          replacement: '$1'
+`,
+			expected: `
+receivers:
+  prometheus:
+    config:
+      scrape_configs:
+      - job_name: 'example'
+        relabel_configs:
+        - source_labels: ['__meta_service_id']
+          target_label: 'job'
+          replacement: 'my_service_$1'
+        - source_labels: ['__meta_service_name']
+          target_label: 'instance'
+          replacement: '$1'
+`,
+		},
+		{
+			description: "full",
+			input: `
 receivers:
   prometheus:
     config:
@@ -134,8 +212,8 @@ receivers:
         - source_labels: ['job']
           target_label: 'job'
           replacement: '$$1_$2'
-`
-	expected := `
+`,
+			expected: `
 receivers:
   prometheus:
     config:
@@ -152,20 +230,26 @@ receivers:
         - source_labels: ['job']
           target_label: 'job'
           replacement: '$1_$2'
-`
-
-	config, err := ta.UnescapeDollarSignsInPromConfig(actual)
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-
-	expectedConfig, err := ta.UnescapeDollarSignsInPromConfig(expected)
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
+`,
+		},
 	}
-
-	if !reflect.DeepEqual(config, expectedConfig) {
-		t.Errorf("unexpected config: got %v, want %v", config, expectedConfig)
+	for _, testCase := range testCases {
+		testCase := testCase
+		t.Run(testCase.description, func(t *testing.T) {
+			config, err := ta.UnescapeDollarSignsInPromConfig(testCase.input)
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+
+			expectedConfig, err := ta.UnescapeDollarSignsInPromConfig(testCase.expected)
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+
+			if !reflect.DeepEqual(config, expectedConfig) {
+				t.Errorf("unexpected config: got %v, want %v", config, expectedConfig)
+			}
+		})
 	}
 }
 

tests/e2e-targetallocator/targetallocator-features/00-install.yaml

@@ -45,15 +45,34 @@ kind: OpenTelemetryCollector
 metadata:
   name: stateful
 spec:
-  config: "receivers:\n  jaeger:\n      protocols:\n        grpc:\n\n  # Collect own
-    metrics\n  prometheus:\n    config:\n      scrape_configs:\n      - job_name:
-    'otel-collector'\n        scrape_interval: 10s\n        static_configs:\n        -
-    targets: [ '0.0.0.0:8888' ]\n        relabel_configs:\n        - regex: __meta_kubernetes_node_label_(.+)\n
-    \         action: labelmap\n          replacement: $$1\n        - regex: test_.*\n
-    \         action: labeldrop  \n        - regex: 'metrica_*|metricb.*'\n          action:
-    labelkeep\n          replacement: $$1\n\nprocessors:\n\nexporters:\n  debug:\nservice:\n
-    \ pipelines:\n    traces:\n      receivers: [jaeger]\n      processors: []\n      exporters:
-    [debug]\n"
+  config: |
+    receivers:
+      # Collect own metrics
+      prometheus:
+        config:
+          scrape_configs:
+          - job_name: 'otel-collector'
+            scrape_interval: 10s
+            static_configs:        
+            - targets: [ '0.0.0.0:8888' ]
+            metric_relabel_configs:
+            - action: labeldrop
+              regex: (id|name)
+            - action: labelmap
+              regex: label_(.+)
+              replacement: $$1
+    
+    processors:
+    
+    exporters:
+      debug:
+    service:
+      pipelines:
+        metrics:
+          receivers: [prometheus]
+          processors: []
+          exporters: [debug]
+
   mode: statefulset
   targetAllocator:
     affinity: