Fix for unmarshalling error for keepequal/dropequal (#2794)

* fix * add require * fixing lint errors * adding unit tests * fixing some yaml * adding few more tests * fixing lint * adding comment * Update cmd/otel-allocator/server/server.go Co-authored-by: Jacob Aronoff <[email protected]> --------- Co-authored-by: Jacob Aronoff <[email protected]>
open-telemetry · Apr 11, 2024 · bef802d · bef802d
1 parent d9588b9
commit bef802d
Show file tree

Hide file tree

Showing 6 changed files with 343 additions and 1 deletion.
diff --git a/.chloggen/fix-keepequal-dropequal.yaml b/.chloggen/fix-keepequal-dropequal.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: target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix for keepequal/dropequal action
+
+# One or more tracking issues related to the change
+issues: [2793]
+
+# (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:
diff --git a/cmd/otel-allocator/server/server.go b/cmd/otel-allocator/server/server.go
@@ -16,6 +16,7 @@ package server
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/pprof"
@@ -106,6 +107,50 @@ func (s *Server) Shutdown(ctx context.Context) error {
 	return s.server.Shutdown(ctx)
 }
 
+// RemoveRegexFromRelabelAction is needed specifically for keepequal/dropequal actions because even though the user doesn't specify the
+// regex field for these actions the unmarshalling implementations of prometheus adds back the default regex fields
+// which in turn causes the receiver to error out since the unmarshaling of the json response doesn't expect anything in the regex fields
+// for these actions. Adding this as a fix until the original issue with prometheus unmarshaling is fixed -
+// https://github.com/prometheus/prometheus/issues/12534
+func RemoveRegexFromRelabelAction(jsonConfig []byte) ([]byte, error) {
+	var jobToScrapeConfig map[string]interface{}
+	err := json.Unmarshal(jsonConfig, &jobToScrapeConfig)
+	if err != nil {
+		return nil, err
+	}
+	for _, scrapeConfig := range jobToScrapeConfig {
+		scrapeConfig := scrapeConfig.(map[string]interface{})
+		if scrapeConfig["relabel_configs"] != nil {
+			relabelConfigs := scrapeConfig["relabel_configs"].([]interface{})
+			for _, relabelConfig := range relabelConfigs {
+				relabelConfig := relabelConfig.(map[string]interface{})
+				// Dropping regex key from the map since unmarshalling this on the client(metrics_receiver.go) results in error
+				// because of the bug here - https://github.com/prometheus/prometheus/issues/12534
+				if relabelConfig["action"] == "keepequal" || relabelConfig["action"] == "dropequal" {
+					delete(relabelConfig, "regex")
+				}
+			}
+		}
+		if scrapeConfig["metric_relabel_configs"] != nil {
+			metricRelabelConfigs := scrapeConfig["metric_relabel_configs"].([]interface{})
+			for _, metricRelabelConfig := range metricRelabelConfigs {
+				metricRelabelConfig := metricRelabelConfig.(map[string]interface{})
+				// Dropping regex key from the map since unmarshalling this on the client(metrics_receiver.go) results in error
+				// because of the bug here - https://github.com/prometheus/prometheus/issues/12534
+				if metricRelabelConfig["action"] == "keepequal" || metricRelabelConfig["action"] == "dropequal" {
+					delete(metricRelabelConfig, "regex")
+				}
+			}
+		}
+	}
+
+	jsonConfigNew, err := json.Marshal(jobToScrapeConfig)
+	if err != nil {
+		return nil, err
+	}
+	return jsonConfigNew, nil
+}
+
 // UpdateScrapeConfigResponse updates the scrape config response. The target allocator first marshals these
 // configurations such that the underlying prometheus marshaling is used. After that, the YAML is converted
 // in to a JSON format for consumers to use.
@@ -120,8 +165,14 @@ func (s *Server) UpdateScrapeConfigResponse(configs map[string]*promconfig.Scrap
 	if err != nil {
 		return err
 	}
+
+	jsonConfigNew, err := RemoveRegexFromRelabelAction(jsonConfig)
+	if err != nil {
+		return err
+	}
+
 	s.mtx.Lock()
-	s.scrapeConfigResponse = jsonConfig
+	s.scrapeConfigResponse = jsonConfigNew
 	s.mtx.Unlock()
 	return nil
 }

diff --git a/cmd/otel-allocator/server/server_test.go b/cmd/otel-allocator/server/server_test.go
@@ -34,6 +34,7 @@ import (
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
 	"github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/allocation"
+	allocatorconfig "github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/config"
 	"github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/target"
 )
 
@@ -612,6 +613,63 @@ func TestServer_Readiness(t *testing.T) {
 	}
 }
 
+func TestServer_ScrapeConfigRespose(t *testing.T) {
+	tests := []struct {
+		description  string
+		filePath     string
+		expectedCode int
+	}{
+		{
+			description:  "Jobs with all actions",
+			filePath:     "./testdata/prom-config-all-actions.yaml",
+			expectedCode: http.StatusOK,
+		},
+		{
+			description:  "Jobs with config combinations",
+			filePath:     "./testdata/prom-config-test.yaml",
+			expectedCode: http.StatusOK,
+		},
+		{
+			description:  "Jobs with no config",
+			filePath:     "./testdata/prom-no-config.yaml",
+			expectedCode: http.StatusOK,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.description, func(t *testing.T) {
+			listenAddr := ":8080"
+			s := NewServer(logger, nil, listenAddr)
+
+			allocCfg := allocatorconfig.CreateDefaultConfig()
+			err := allocatorconfig.LoadFromFile(tc.filePath, &allocCfg)
+			require.NoError(t, err)
+
+			jobToScrapeConfig := make(map[string]*promconfig.ScrapeConfig)
+
+			for _, scrapeConfig := range allocCfg.PromConfig.ScrapeConfigs {
+				jobToScrapeConfig[scrapeConfig.JobName] = scrapeConfig
+			}
+
+			assert.NoError(t, s.UpdateScrapeConfigResponse(jobToScrapeConfig))
+
+			request := httptest.NewRequest("GET", "/scrape_configs", nil)
+			w := httptest.NewRecorder()
+
+			s.server.Handler.ServeHTTP(w, request)
+			result := w.Result()
+
+			assert.Equal(t, tc.expectedCode, result.StatusCode)
+			bodyBytes, err := io.ReadAll(result.Body)
+			require.NoError(t, err)
+
+			// Checking to make sure yaml unmarshaling doesn't result in errors for responses containing any supported prometheus relabel action
+			scrapeConfigs := map[string]*promconfig.ScrapeConfig{}
+			err = yaml.Unmarshal(bodyBytes, scrapeConfigs)
+			require.NoError(t, err)
+		})
+	}
+}
+
 func newLink(jobName string) target.LinkJSON {
 	return target.LinkJSON{Link: fmt.Sprintf("/jobs/%s/targets", url.QueryEscape(jobName))}
 }
diff --git a/cmd/otel-allocator/server/testdata/prom-config-all-actions.yaml b/cmd/otel-allocator/server/testdata/prom-config-all-actions.yaml
@@ -0,0 +1,175 @@
+collector_selector:
+  matchlabels:
+    app.kubernetes.io/instance: default.test
+    app.kubernetes.io/managed-by: opentelemetry-operator
+prometheus_cr:
+  scrape_interval: 60s
+config:
+  scrape_configs:
+  - job_name: job-replace
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_test]
+        action: replace
+        regex: "test$1replacement"
+        replacement: "myreplacement$$1"
+        target_label: "mytarget"
+    metric_relabel_configs:
+      - action: replace
+        source_labels: [city]
+        separator: ","
+        regex: (ci.*)(name$)
+        replacement: "test_newest_1_$1"
+        target_label: city
+  - job_name: job-lowercase
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_test]
+        action: lowercase
+        target_label: "mytarget"
+    metric_relabel_configs:
+      - action: lowercase
+        source_labels: [city]
+        target_label: city
+  - job_name: job-uppercase
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_test]
+        action: uppercase
+        target_label: "mytarget"
+    metric_relabel_configs:
+      - action: uppercase
+        source_labels: [city]
+        target_label: city
+  - job_name: job-keep
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+    metric_relabel_configs:
+      - action: keep
+        source_labels: [city]
+        separator: ","
+        regex: (ci.*)(name$)
+  - job_name: job-drop
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_droplabel]
+        action: drop
+        regex: "prometheus-reference-app"
+    metric_relabel_configs:
+      - action: drop
+        source_labels: [city]
+        regex: (ci.*)(name$)
+  - job_name: job-keepequal
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keepequal
+        target_label: __meta_kubernetes_pod_label_mylabel
+    metric_relabel_configs:
+      - action: keepequal
+        source_labels: [city]
+        target_label: city
+  - job_name: job-dropequal
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_dropequallabel]
+        action: dropequal
+        target_label: "test"
+    metric_relabel_configs:
+      - action: dropequal
+        source_labels: [citytest]
+        target_label: city
+  - job_name: job-hashmod
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_hashmod]
+        action: hashmod
+        modulus: 5
+        regex: "prometheus-reference-app"
+        target_label: city
+    metric_relabel_configs:
+      - action: hashmod
+        modulus: 5
+        source_labels: [city]
+        regex: (ci.*)(name$)
+        target_label: city
+  - job_name: job-labelmap
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_hashmod]
+        action: labelmap
+        regex: "prometheus-reference-app"
+        target_label: city
+    metric_relabel_configs:
+      - action: labelmap
+        source_labels: [city]
+        regex: (ci.*)(name$)
+        target_label: city
+  - job_name: job-labeldrop
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - action: labeldrop
+        regex: "prometheus-reference-app"
+    metric_relabel_configs:
+      - action: labeldrop
+        regex: (ci.*)(name$)
+  - job_name: job-labelkeep
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - action: labelkeep
+        regex: "prometheus-reference-app"
+    metric_relabel_configs:
+      - action: labelkeep
+        regex: (ci.*)(name$$)
diff --git a/cmd/otel-allocator/server/testdata/prom-config-test.yaml b/cmd/otel-allocator/server/testdata/prom-config-test.yaml
@@ -0,0 +1,34 @@
+collector_selector:
+  matchlabels:
+    app.kubernetes.io/instance: default.test
+    app.kubernetes.io/managed-by: opentelemetry-operator
+prometheus_cr:
+  scrape_interval: 60s
+config:
+  scrape_configs:
+  - job_name: job-no-target-relabel
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    metric_relabel_configs:
+      - action: replace
+        source_labels: [city]
+        separator: ","
+        regex: (ci.*)(name$)
+        replacement: "test_newest_1_$1"
+        target_label: city
+  - job_name: job-no-metric-relabel
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
+    relabel_configs:
+      - source_labels: [__meta_kubernetes_pod_label_app]
+        action: keep
+        regex: "prometheus-reference-app"
+      - source_labels: [__meta_kubernetes_pod_label_test]
+        action: lowercase
+        target_label: "mytarget"
+  - job_name: job-no-relabel
+    scheme: http
+    kubernetes_sd_configs:
+      - role: pod
diff --git a/cmd/otel-allocator/server/testdata/prom-no-config.yaml b/cmd/otel-allocator/server/testdata/prom-no-config.yaml
@@ -0,0 +1,8 @@
+collector_selector:
+  matchlabels:
+    app.kubernetes.io/instance: default.test
+    app.kubernetes.io/managed-by: opentelemetry-operator
+prometheus_cr:
+  scrape_interval: 60s
+config:
+  scrape_configs: