diff --git a/.chloggen/fix-unescaping.yaml b/.chloggen/fix-unescaping.yaml new file mode 100755 index 0000000000..ddde9521a9 --- /dev/null +++ b/.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. + diff --git a/internal/manifests/collector/targetallocator_test.go b/internal/manifests/collector/targetallocator_test.go index c73a3240bb..76a51b66ef 100644 --- a/internal/manifests/collector/targetallocator_test.go +++ b/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 { diff --git a/internal/manifests/targetallocator/adapters/config_to_prom_config.go b/internal/manifests/targetallocator/adapters/config_to_prom_config.go index 21932e5f49..e0d7cd38e2 100644 --- a/internal/manifests/targetallocator/adapters/config_to_prom_config.go +++ b/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, "$$", "$") + } } } diff --git a/internal/manifests/targetallocator/adapters/config_to_prom_config_test.go b/internal/manifests/targetallocator/adapters/config_to_prom_config_test.go index a64ae54eda..2ad7b741c6 100644 --- a/internal/manifests/targetallocator/adapters/config_to_prom_config_test.go +++ b/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) + } + }) } } diff --git a/tests/e2e-targetallocator/targetallocator-features/00-install.yaml b/tests/e2e-targetallocator/targetallocator-features/00-install.yaml index bf45fa150a..8366133eab 100644 --- a/tests/e2e-targetallocator/targetallocator-features/00-install.yaml +++ b/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: