fix issues related to prometheus relabel configs when target allocator is enabled

.chloggen/fix_prometheus_relabel-configs.yaml

@@ -0,0 +1,16 @@
+# 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. operator, target allocator, github action)
+component: pkg/collector, pkg/targetallocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: fix issues related to prometheus relabel configs when target allocator is enabled
+
+# One or more tracking issues related to the change
+issues: [958, 1622, 1623]
+
+# (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:

pkg/collector/reconcile/config_replace.go

@@ -16,12 +16,9 @@ package reconcile
 
 import (
 	"fmt"
-	"net/url"
 	"time"
 
 	promconfig "github.com/prometheus/prometheus/config"
-	"github.com/prometheus/prometheus/discovery"
-	"github.com/prometheus/prometheus/discovery/http"
 	_ "github.com/prometheus/prometheus/discovery/install" // Package install has the side-effect of registering all builtin.
 	"gopkg.in/yaml.v2"
 
@@ -44,55 +41,67 @@ type Config struct {
 }
 
 func ReplaceConfig(instance v1alpha1.OpenTelemetryCollector) (string, error) {
+	// Check if TargetAllocator is enabled, if not, return the original config
 	if !instance.Spec.TargetAllocator.Enabled {
 		return instance.Spec.Config, nil
 	}
-	config, getStringErr := adapters.ConfigFromString(instance.Spec.Config)
-	if getStringErr != nil {
-		return "", getStringErr
-	}
 
-	promCfgMap, getCfgPromErr := ta.ConfigToPromConfig(instance.Spec.Config)
-	if getCfgPromErr != nil {
-		return "", getCfgPromErr
+	config, err := adapters.ConfigFromString(instance.Spec.Config)
+	if err != nil {
+		return "", err
 	}
 
-	// yaml marshaling/unsmarshaling is preferred because of the problems associated with the conversion of map to a struct using mapstructure
-	promCfg, marshalErr := yaml.Marshal(promCfgMap)
-	if marshalErr != nil {
-		return "", marshalErr
-	}
+	if featuregate.EnableTargetAllocatorRewrite.IsEnabled() {
+		promCfgMap, getCfgPromErr := ta.ConfigToPromConfig(instance.Spec.Config)
+		if getCfgPromErr != nil {
 
-	var cfg Config
-	if marshalErr = yaml.UnmarshalStrict(promCfg, &cfg); marshalErr != nil {
-		return "", fmt.Errorf("error unmarshaling YAML: %w", marshalErr)
-	}
+			return "", getCfgPromErr
+		}
 
-	for i := range cfg.PromConfig.ScrapeConfigs {
-		escapedJob := url.QueryEscape(cfg.PromConfig.ScrapeConfigs[i].JobName)
-		cfg.PromConfig.ScrapeConfigs[i].ServiceDiscoveryConfigs = discovery.Configs{
-			&http.SDConfig{
-				URL: fmt.Sprintf("http://%s:80/jobs/%s/targets?collector_id=$POD_NAME", naming.TAService(instance), escapedJob),
-			},
+		// yaml marshaling/unsmarshaling is preferred because of the problems associated with the conversion of map to a struct using mapstructure
+		promCfg, marshalErr := yaml.Marshal(promCfgMap)
+		if marshalErr != nil {
+			return "", marshalErr
+		}
+
+		var cfg Config
+		if err = yaml.UnmarshalStrict(promCfg, &cfg); err != nil {
+			return "", fmt.Errorf("error unmarshaling YAML: %w", err)
 		}
-	}
 
-	if featuregate.EnableTargetAllocatorRewrite.IsEnabled() {
 		cfg.TargetAllocConfig = &targetAllocator{
 			Endpoint:    fmt.Sprintf("http://%s:80", naming.TAService(instance)),
 			Interval:    30 * time.Second,
 			CollectorID: "${POD_NAME}",
 		}
 		// we don't need the scrape configs here anymore with target allocator enabled
 		cfg.PromConfig.ScrapeConfigs = []*promconfig.ScrapeConfig{}
+
+		// type coercion checks are handled in the ConfigToPromConfig method above
+		config["receivers"].(map[interface{}]interface{})["prometheus"] = cfg
+
+		out, updCfgMarshalErr := yaml.Marshal(config)
+		if updCfgMarshalErr != nil {
+			return "", updCfgMarshalErr
+		}
+
+		return string(out), nil
+	}
+
+	// To avoid issues caused by Prometheus validation logic, which fails regex validation when it encounters
+	// $$ in the prom config, we update the YAML file directly without marshaling and unmarshalling.
+	promCfgMap, err := ta.AddHTTPSDConfigToPromConfig(instance.Spec.Config, naming.TAService(instance))
+	if err != nil {
+		return "", err
 	}
 
 	// type coercion checks are handled in the ConfigToPromConfig method above
-	config["receivers"].(map[interface{}]interface{})["prometheus"] = cfg
+	config["receivers"].(map[interface{}]interface{})["prometheus"] = promCfgMap
 
 	out, err := yaml.Marshal(config)
 	if err != nil {
 		return "", err
 	}
+
 	return string(out), nil
 }

pkg/collector/reconcile/config_replace_test.go

@@ -15,6 +15,7 @@
 package reconcile
 
 import (
+	"os"
 	"testing"
 	"time"
 
@@ -128,3 +129,33 @@ func TestPrometheusParser(t *testing.T) {
 	})
 
 }
+
+func TestReplaceConfig(t *testing.T) {
+	param, err := newParams("test/test-img", "../testdata/relabel_config_original.yaml")
+	assert.NoError(t, err)
+
+	t.Run("should not modify config when TargetAllocator is disabled", func(t *testing.T) {
+		param.Instance.Spec.TargetAllocator.Enabled = false
+		expectedConfigBytes, err := os.ReadFile("../testdata/relabel_config_original.yaml")
+		assert.NoError(t, err)
+		expectedConfig := string(expectedConfigBytes)
+
+		actualConfig, err := ReplaceConfig(param.Instance)
+		assert.NoError(t, err)
+
+		assert.Equal(t, expectedConfig, actualConfig)
+	})
+
+	t.Run("should rewrite scrape configs with SD config when TargetAllocator is enabled and feature flag is not set", func(t *testing.T) {
+		param.Instance.Spec.TargetAllocator.Enabled = true
+
+		expectedConfigBytes, err := os.ReadFile("../testdata/relabel_config_expected_with_sd_config.yaml")
+		assert.NoError(t, err)
+		expectedConfig := string(expectedConfigBytes)
+
+		actualConfig, err := ReplaceConfig(param.Instance)
+		assert.NoError(t, err)
+
+		assert.Equal(t, expectedConfig, actualConfig)
+	})
+}

pkg/collector/reconcile/configmap.go

@@ -103,7 +103,7 @@ func desiredTAConfigMap(params Params) (corev1.ConfigMap, error) {
 		labels["app.kubernetes.io/version"] = "latest"
 	}
 
-	promConfig, err := ta.ConfigToPromConfig(params.Instance.Spec.Config)
+	promConfig, err := ta.UnescapeDollarSignsInPromConfig(params.Instance.Spec.Config)
 	if err != nil {
 		return corev1.ConfigMap{}, err
 	}

pkg/collector/reconcile/configmap_test.go

@@ -94,23 +94,11 @@ receivers:
       grpc: null
   prometheus:
     config:
-      global:
-        scrape_interval: 1m
-        scrape_timeout: 10s
-        evaluation_interval: 1m
       scrape_configs:
-      - job_name: otel-collector
-        honor_timestamps: true
+      - http_sd_configs:
+        - url: http://test-targetallocator:80/jobs/otel-collector/targets?collector_id=$POD_NAME
+        job_name: otel-collector
         scrape_interval: 10s
-        scrape_timeout: 10s
-        metrics_path: /metrics
-        scheme: http
-        follow_redirects: true
-        enable_http2: true
-        http_sd_configs:
-        - follow_redirects: false
-          enable_http2: false
-          url: http://test-targetallocator:80/jobs/otel-collector/targets?collector_id=$POD_NAME
 service:
   pipelines:
     metrics:
@@ -145,23 +133,10 @@ processors: null
 receivers:
   prometheus:
     config:
-      global:
-        scrape_interval: 1m
-        scrape_timeout: 10s
-        evaluation_interval: 1m
       scrape_configs:
-      - job_name: serviceMonitor/test/test/0
-        honor_timestamps: true
-        scrape_interval: 1m
-        scrape_timeout: 10s
-        metrics_path: /metrics
-        scheme: http
-        follow_redirects: true
-        enable_http2: true
-        http_sd_configs:
-        - follow_redirects: false
-          enable_http2: false
-          url: http://test-targetallocator:80/jobs/serviceMonitor%2Ftest%2Ftest%2F0/targets?collector_id=$POD_NAME
+      - http_sd_configs:
+        - url: http://test-targetallocator:80/jobs/serviceMonitor%2Ftest%2Ftest%2F0/targets?collector_id=$POD_NAME
+        job_name: serviceMonitor/test/test/0
 service:
   pipelines:
     metrics:

pkg/collector/testdata/relabel_config_expected_with_sd_config.yaml

@@ -0,0 +1,57 @@
+exporters: null
+logging: null
+processors: null
+receivers:
+  prometheus:
+    config:
+      global:
+        evaluation_interval: 1m
+        scrape_interval: 1m
+        scrape_timeout: 10s
+      scrape_configs:
+      - honor_labels: true
+        http_sd_configs:
+        - url: http://test-targetallocator:80/jobs/service-x/targets?collector_id=$POD_NAME
+        job_name: service-x
+        metric_relabel_configs:
+        - action: keep
+          regex: (.*)
+          separator: ;
+          source_labels:
+          - label1
+        - action: labelmap
+          regex: (.*)
+          separator: ;
+          source_labels:
+          - label4
+        metrics_path: /metrics
+        relabel_configs:
+        - action: keep
+          regex: (.*)
+          source_labels:
+          - label1
+        - action: replace
+          regex: (.*)
+          replacement: $$1_$2
+          separator: ;
+          source_labels:
+          - label2
+          target_label: label3
+        - action: labelmap
+          regex: (.*)
+          separator: ;
+          source_labels:
+          - label4
+        - action: labeldrop
+          regex: foo_.*
+        scheme: http
+        scrape_interval: 1m
+        scrape_timeout: 10s
+service:
+  pipelines:
+    metrics:
+      exporters:
+      - logging
+      processors: []
+      receivers:
+      - prometheus

pkg/collector/testdata/relabel_config_original.yaml

@@ -0,0 +1,51 @@
+exporters:
+logging:
+processors:
+receivers:
+  prometheus:
+    config:
+      global:
+        evaluation_interval: 1m
+        scrape_interval: 1m
+        scrape_timeout: 10s
+      scrape_configs:
+      - job_name: service-x
+        metrics_path: /metrics
+        scheme: http
+        scrape_interval: 1m
+        scrape_timeout: 10s
+        honor_labels: true
+        relabel_configs:
+        - source_labels: [label1]
+          action: keep
+          regex: (.*)
+        - target_label: label3
+          source_labels: [label2]
+          action: replace
+          regex: (.*)
+          replacement: "$$1_$2"
+          separator: ";"
+        - source_labels: [label4]
+          action: labelmap
+          regex: (.*)
+          separator: ";"
+        - regex: foo_.*
+          action: labeldrop
+        metric_relabel_configs:
+        - source_labels: [label1]
+          action: keep
+          regex: (.*)
+          separator: ";"
+        - regex: (.*)
+          action: labelmap
+          separator: ";"
+          source_labels: [label4]
+
+service:
+  pipelines:
+    metrics:
+      exporters:
+      - logging
+      receivers:
+      - prometheus
+      processors: []