From ddeac3d62e90a5c203b8dc23bb5a5cfe96caece7 Mon Sep 17 00:00:00 2001 From: Jacob Aronoff Date: Thu, 23 Jun 2022 19:09:31 +0200 Subject: [PATCH] Fix parameter encoding issue in TA (#930) * logging * Fixed encoding issue of params * Do string replacement * Another test * Removed dockerfile changes * Fix broken test * Newline * Change placement of the log import --- .../allocation/allocator_test.go | 17 ++- cmd/otel-allocator/allocation/http_test.go | 118 ++++++++++++++++++ cmd/otel-allocator/main.go | 12 +- pkg/collector/reconcile/config_replace.go | 4 +- pkg/collector/reconcile/configmap_test.go | 52 ++++++++ .../http_sd_config_servicemonitor_test.yaml | 25 ++++ 6 files changed, 211 insertions(+), 17 deletions(-) create mode 100644 cmd/otel-allocator/allocation/http_test.go create mode 100644 pkg/collector/testdata/http_sd_config_servicemonitor_test.yaml diff --git a/cmd/otel-allocator/allocation/allocator_test.go b/cmd/otel-allocator/allocation/allocator_test.go index 99b5f34f14..893422a3b4 100644 --- a/cmd/otel-allocator/allocation/allocator_test.go +++ b/cmd/otel-allocator/allocation/allocator_test.go @@ -4,15 +4,16 @@ import ( "math" "testing" - "github.com/go-logr/logr" "github.com/prometheus/common/model" "github.com/stretchr/testify/assert" + logf "sigs.k8s.io/controller-runtime/pkg/log" ) +var logger = logf.Log.WithName("unit-tests") + // Tests least connection - The expected collector after running findNextCollector should be the collector with the least amount of workload func TestFindNextCollector(t *testing.T) { - var log logr.Logger - s := NewAllocator(log) + s := NewAllocator(logger) defaultCol := collector{Name: "default-col", NumTargets: 1} maxCol := collector{Name: "max-col", NumTargets: 2} @@ -25,9 +26,7 @@ func TestFindNextCollector(t *testing.T) { } func TestSetCollectors(t *testing.T) { - - var log logr.Logger - s := NewAllocator(log) + s := NewAllocator(logger) cols := []string{"col-1", "col-2", "col-3"} s.SetCollectors(cols) @@ -42,8 +41,7 @@ func TestSetCollectors(t *testing.T) { func TestAddingAndRemovingTargets(t *testing.T) { // prepare allocator with initial targets and collectors - var log logr.Logger - s := NewAllocator(log) + s := NewAllocator(logger) cols := []string{"col-1", "col-2", "col-3"} s.SetCollectors(cols) @@ -88,8 +86,7 @@ func TestAddingAndRemovingTargets(t *testing.T) { func TestCollectorBalanceWhenAddingAndRemovingAtRandom(t *testing.T) { // prepare allocator with 3 collectors and 'random' amount of targets - var log logr.Logger - s := NewAllocator(log) + s := NewAllocator(logger) cols := []string{"col-1", "col-2", "col-3"} s.SetCollectors(cols) diff --git a/cmd/otel-allocator/allocation/http_test.go b/cmd/otel-allocator/allocation/http_test.go new file mode 100644 index 0000000000..c239a9c68b --- /dev/null +++ b/cmd/otel-allocator/allocation/http_test.go @@ -0,0 +1,118 @@ +package allocation + +import ( + "github.com/prometheus/common/model" + "reflect" + "testing" +) + +func TestGetAllTargetsByCollectorAndJob(t *testing.T) { + baseAllocator := NewAllocator(logger) + baseAllocator.SetCollectors([]string{"test-collector"}) + statefulAllocator := NewAllocator(logger) + statefulAllocator.SetCollectors([]string{"test-collector-0"}) + type args struct { + collector string + job string + cMap map[string][]TargetItem + allocator *Allocator + } + var tests = []struct { + name string + args args + want []targetGroupJSON + }{ + { + name: "Empty target map", + args: args{ + collector: "test-collector", + job: "test-job", + cMap: map[string][]TargetItem{}, + allocator: baseAllocator, + }, + want: nil, + }, + { + name: "Single entry target map", + args: args{ + collector: "test-collector", + job: "test-job", + cMap: map[string][]TargetItem{ + "test-collectortest-job": { + TargetItem{ + JobName: "test-job", + Label: model.LabelSet{ + "test-label": "test-value", + }, + TargetURL: "test-url", + Collector: &collector{ + Name: "test-collector", + NumTargets: 1, + }, + }, + }, + }, + allocator: baseAllocator, + }, + want: []targetGroupJSON{ + { + Targets: []string{"test-url"}, + Labels: map[model.LabelName]model.LabelValue{ + "test-label": "test-value", + }, + }, + }, + }, + { + name: "Multiple entry target map", + args: args{ + collector: "test-collector", + job: "test-job", + cMap: map[string][]TargetItem{ + "test-collectortest-job": { + TargetItem{ + JobName: "test-job", + Label: model.LabelSet{ + "test-label": "test-value", + }, + TargetURL: "test-url", + Collector: &collector{ + Name: "test-collector", + NumTargets: 1, + }, + }, + }, + "test-collectortest-job2": { + TargetItem{ + JobName: "test-job2", + Label: model.LabelSet{ + "test-label": "test-value", + }, + TargetURL: "test-url", + Collector: &collector{ + Name: "test-collector", + NumTargets: 1, + }, + }, + }, + }, + allocator: baseAllocator, + }, + want: []targetGroupJSON{ + { + Targets: []string{"test-url"}, + Labels: map[model.LabelName]model.LabelValue{ + "test-label": "test-value", + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := GetAllTargetsByCollectorAndJob(tt.args.collector, tt.args.job, tt.args.cMap, tt.args.allocator); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetAllTargetsByCollectorAndJob() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/cmd/otel-allocator/main.go b/cmd/otel-allocator/main.go index 1b6a6d6a9c..7978034afd 100644 --- a/cmd/otel-allocator/main.go +++ b/cmd/otel-allocator/main.go @@ -185,18 +185,18 @@ func (s *server) TargetsHandler(w http.ResponseWriter, r *http.Request) { compareMap[v.Collector.Name+v.JobName] = append(compareMap[v.Collector.Name+v.JobName], *v) } params := mux.Vars(r) + jobId, err := url.QueryUnescape(params["job_id"]) + if err != nil { + errorHandler(err, w, r) + return + } if len(q) == 0 { - jobId, err := url.QueryUnescape(params["job_id"]) - if err != nil { - errorHandler(err, w, r) - return - } displayData := allocation.GetAllTargetsByJob(jobId, compareMap, s.allocator) jsonHandler(w, r, displayData) } else { - tgs := allocation.GetAllTargetsByCollectorAndJob(q[0], params["job_id"], compareMap, s.allocator) + tgs := allocation.GetAllTargetsByCollectorAndJob(q[0], jobId, compareMap, s.allocator) // Displays empty list if nothing matches if len(tgs) == 0 { jsonHandler(w, r, []interface{}{}) diff --git a/pkg/collector/reconcile/config_replace.go b/pkg/collector/reconcile/config_replace.go index ece080549d..3d18d2d15f 100644 --- a/pkg/collector/reconcile/config_replace.go +++ b/pkg/collector/reconcile/config_replace.go @@ -16,6 +16,7 @@ package reconcile import ( "fmt" + "net/url" "github.com/mitchellh/mapstructure" promconfig "github.com/prometheus/prometheus/config" @@ -61,9 +62,10 @@ func ReplaceConfig(params Params) (string, error) { } 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(params.Instance), cfg.PromConfig.ScrapeConfigs[i].JobName), + URL: fmt.Sprintf("http://%s:80/jobs/%s/targets?collector_id=$POD_NAME", naming.TAService(params.Instance), escapedJob), }, } } diff --git a/pkg/collector/reconcile/configmap_test.go b/pkg/collector/reconcile/configmap_test.go index b55c322607..05df6d79a3 100644 --- a/pkg/collector/reconcile/configmap_test.go +++ b/pkg/collector/reconcile/configmap_test.go @@ -128,6 +128,58 @@ service: }) + t.Run("should return expected escaped collector config map with http_sd_config", func(t *testing.T) { + expectedLables["app.kubernetes.io/component"] = "opentelemetry-collector" + expectedLables["app.kubernetes.io/name"] = "test-collector" + expectedLables["app.kubernetes.io/version"] = "latest" + + expectedData := map[string]string{ + "collector.yaml": `exporters: + logging: null +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 + http_sd_configs: + - follow_redirects: false + url: http://test-targetallocator:80/jobs/serviceMonitor%2Ftest%2Ftest%2F0/targets?collector_id=$POD_NAME +service: + pipelines: + metrics: + exporters: + - logging + processors: [] + receivers: + - prometheus +`, + } + + param, err := newParams("test/test-img", "../testdata/http_sd_config_servicemonitor_test.yaml") + assert.NoError(t, err) + param.Instance.Spec.TargetAllocator.Enabled = true + actual := desiredConfigMap(context.Background(), param) + + assert.Equal(t, "test-collector", actual.Name) + assert.Equal(t, expectedLables, actual.Labels) + assert.Equal(t, expectedData, actual.Data) + + // Reset the value + expectedLables["app.kubernetes.io/version"] = "0.47.0" + + }) + t.Run("should return expected target allocator config map", func(t *testing.T) { expectedLables["app.kubernetes.io/component"] = "opentelemetry-targetallocator" expectedLables["app.kubernetes.io/name"] = "test-targetallocator" diff --git a/pkg/collector/testdata/http_sd_config_servicemonitor_test.yaml b/pkg/collector/testdata/http_sd_config_servicemonitor_test.yaml new file mode 100644 index 0000000000..840385d3e7 --- /dev/null +++ b/pkg/collector/testdata/http_sd_config_servicemonitor_test.yaml @@ -0,0 +1,25 @@ +processors: +receivers: + prometheus: + config: + scrape_configs: + - job_name: serviceMonitor/test/test/0 + + static_configs: + - targets: ["prom.domain:1001", "prom.domain:1002", "prom.domain:1003"] + labels: + my: label + + file_sd_configs: + - files: + - file2.json + +exporters: + logging: + +service: + pipelines: + metrics: + receivers: [prometheus] + processors: [] + exporters: [logging]