From 09e954f87586ab9c2575994dd62c28fa6c5f953a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20=C5=9Awi=C4=85tek?= Date: Mon, 17 Jun 2024 16:10:28 +0200 Subject: [PATCH] Prepare Collector reconciliation for TargetAllocator CRD --- controllers/builder_test.go | 2 +- controllers/common.go | 5 +++-- .../opentelemetrycollector_controller.go | 4 +--- internal/manifests/collector/config_replace.go | 15 +++++++++------ .../manifests/collector/config_replace_test.go | 17 +++++------------ internal/manifests/collector/configmap.go | 2 +- internal/manifests/collector/suite_test.go | 9 +++++++-- internal/manifests/params.go | 2 +- internal/naming/main.go | 4 ++-- pkg/sidecar/pod.go | 2 +- 10 files changed, 31 insertions(+), 31 deletions(-) diff --git a/controllers/builder_test.go b/controllers/builder_test.go index d3dadfd3db..8870910990 100644 --- a/controllers/builder_test.go +++ b/controllers/builder_test.go @@ -1988,7 +1988,7 @@ prometheus_cr: } targetAllocator, err := collector.TargetAllocator(params) require.NoError(t, err) - params.TargetAllocator = *targetAllocator + params.TargetAllocator = targetAllocator if len(tt.featuregates) > 0 { fg := strings.Join(tt.featuregates, ",") flagset := featuregate.Flags(colfeaturegate.GlobalRegistry()) diff --git a/controllers/common.go b/controllers/common.go index 865ee91b98..31a84bfd39 100644 --- a/controllers/common.go +++ b/controllers/common.go @@ -59,7 +59,8 @@ func BuildCollector(params manifests.Params) ([]client.Object, error) { } resources = append(resources, objs...) } - if params.OtelCol.Spec.TargetAllocator.Enabled { + // TODO: Remove this after TargetAllocator CRD is reconciled + if params.TargetAllocator != nil { taParams := targetallocator.Params{ Client: params.Client, Scheme: params.Scheme, @@ -67,7 +68,7 @@ func BuildCollector(params manifests.Params) ([]client.Object, error) { Log: params.Log, Config: params.Config, Collector: params.OtelCol, - TargetAllocator: params.TargetAllocator, + TargetAllocator: *params.TargetAllocator, } taResources, err := BuildTargetAllocator(taParams) if err != nil { diff --git a/controllers/opentelemetrycollector_controller.go b/controllers/opentelemetrycollector_controller.go index b005728199..ea6f8908d6 100644 --- a/controllers/opentelemetrycollector_controller.go +++ b/controllers/opentelemetrycollector_controller.go @@ -183,9 +183,7 @@ func (r *OpenTelemetryCollectorReconciler) getParams(instance v1beta1.OpenTeleme if err != nil { return p, err } - if targetAllocator != nil { - p.TargetAllocator = *targetAllocator - } + p.TargetAllocator = targetAllocator return p, nil } diff --git a/internal/manifests/collector/config_replace.go b/internal/manifests/collector/config_replace.go index 85f38bf6ae..6ea55dc44d 100644 --- a/internal/manifests/collector/config_replace.go +++ b/internal/manifests/collector/config_replace.go @@ -21,6 +21,7 @@ import ( _ "github.com/prometheus/prometheus/discovery/install" // Package install has the side-effect of registering all builtin. "gopkg.in/yaml.v3" + "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" "github.com/open-telemetry/opentelemetry-operator/apis/v1beta1" "github.com/open-telemetry/opentelemetry-operator/internal/manifests/collector/adapters" ta "github.com/open-telemetry/opentelemetry-operator/internal/manifests/targetallocator/adapters" @@ -41,13 +42,15 @@ type Config struct { TargetAllocConfig *targetAllocator `yaml:"target_allocator,omitempty"` } -func ReplaceConfig(instance v1beta1.OpenTelemetryCollector) (string, error) { - cfgStr, err := instance.Spec.Config.Yaml() +func ReplaceConfig(otelcol v1beta1.OpenTelemetryCollector, targetAllocator *v1alpha1.TargetAllocator) (string, error) { + collectorSpec := otelcol.Spec + taEnabled := targetAllocator != nil + cfgStr, err := collectorSpec.Config.Yaml() if err != nil { return "", err } - // Check if TargetAllocator is enabled, if not, return the original config - if !instance.Spec.TargetAllocator.Enabled { + // Check if TargetAllocator is present, if not, return the original config + if !taEnabled { return cfgStr, nil } @@ -61,14 +64,14 @@ func ReplaceConfig(instance v1beta1.OpenTelemetryCollector) (string, error) { return "", getCfgPromErr } - validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, instance.Spec.TargetAllocator.Enabled) + validateCfgPromErr := ta.ValidatePromConfig(promCfgMap, taEnabled) if validateCfgPromErr != nil { return "", validateCfgPromErr } // 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. - updPromCfgMap, getCfgPromErr := ta.AddTAConfigToPromConfig(promCfgMap, naming.TAService(instance.Name)) + updPromCfgMap, getCfgPromErr := ta.AddTAConfigToPromConfig(promCfgMap, naming.TAService(targetAllocator.Name)) if getCfgPromErr != nil { return "", getCfgPromErr } diff --git a/internal/manifests/collector/config_replace_test.go b/internal/manifests/collector/config_replace_test.go index 2a97f8a008..c57c95efec 100644 --- a/internal/manifests/collector/config_replace_test.go +++ b/internal/manifests/collector/config_replace_test.go @@ -31,8 +31,7 @@ func TestPrometheusParser(t *testing.T) { t.Run("should update config with targetAllocator block if block not present", func(t *testing.T) { // Set up the test scenario - param.OtelCol.Spec.TargetAllocator.Enabled = true - actualConfig, err := ReplaceConfig(param.OtelCol) + actualConfig, err := ReplaceConfig(param.OtelCol, param.TargetAllocator) assert.NoError(t, err) // Verify the expected changes in the config @@ -57,9 +56,7 @@ func TestPrometheusParser(t *testing.T) { paramTa, err := newParams("test/test-img", "testdata/http_sd_config_ta_test.yaml") require.NoError(t, err) - paramTa.OtelCol.Spec.TargetAllocator.Enabled = true - - actualConfig, err := ReplaceConfig(paramTa.OtelCol) + actualConfig, err := ReplaceConfig(paramTa.OtelCol, param.TargetAllocator) assert.NoError(t, err) // Verify the expected changes in the config @@ -80,9 +77,7 @@ func TestPrometheusParser(t *testing.T) { }) t.Run("should not update config with http_sd_config", func(t *testing.T) { - param.OtelCol.Spec.TargetAllocator.Enabled = false - - actualConfig, err := ReplaceConfig(param.OtelCol) + actualConfig, err := ReplaceConfig(param.OtelCol, nil) assert.NoError(t, err) // prepare @@ -120,25 +115,23 @@ func TestReplaceConfig(t *testing.T) { assert.NoError(t, err) t.Run("should not modify config when TargetAllocator is disabled", func(t *testing.T) { - param.OtelCol.Spec.TargetAllocator.Enabled = false expectedConfigBytes, err := os.ReadFile("testdata/relabel_config_original.yaml") assert.NoError(t, err) expectedConfig := string(expectedConfigBytes) - actualConfig, err := ReplaceConfig(param.OtelCol) + actualConfig, err := ReplaceConfig(param.OtelCol, nil) assert.NoError(t, err) assert.YAMLEq(t, expectedConfig, actualConfig) }) t.Run("should remove scrape configs if TargetAllocator is enabled", func(t *testing.T) { - param.OtelCol.Spec.TargetAllocator.Enabled = true expectedConfigBytes, err := os.ReadFile("testdata/config_expected_targetallocator.yaml") assert.NoError(t, err) expectedConfig := string(expectedConfigBytes) - actualConfig, err := ReplaceConfig(param.OtelCol) + actualConfig, err := ReplaceConfig(param.OtelCol, param.TargetAllocator) assert.NoError(t, err) assert.YAMLEq(t, expectedConfig, actualConfig) diff --git a/internal/manifests/collector/configmap.go b/internal/manifests/collector/configmap.go index 74f90f28d5..934dd2fe36 100644 --- a/internal/manifests/collector/configmap.go +++ b/internal/manifests/collector/configmap.go @@ -32,7 +32,7 @@ func ConfigMap(params manifests.Params) (*corev1.ConfigMap, error) { collectorName := naming.Collector(params.OtelCol.Name) labels := manifestutils.Labels(params.OtelCol.ObjectMeta, collectorName, params.OtelCol.Spec.Image, ComponentOpenTelemetryCollector, []string{}) - replacedConf, err := ReplaceConfig(params.OtelCol) + replacedConf, err := ReplaceConfig(params.OtelCol, params.TargetAllocator) if err != nil { params.Log.V(2).Info("failed to update prometheus config to use sharded targets: ", "err", err) return nil, err diff --git a/internal/manifests/collector/suite_test.go b/internal/manifests/collector/suite_test.go index 1f2654ba60..b3aa3b5359 100644 --- a/internal/manifests/collector/suite_test.go +++ b/internal/manifests/collector/suite_test.go @@ -129,7 +129,7 @@ func newParams(taContainerImage string, file string, options ...config.Option) ( } cfg := config.New(append(defaultOptions, options...)...) - return manifests.Params{ + params := manifests.Params{ Config: cfg, OtelCol: v1beta1.OpenTelemetryCollector{ TypeMeta: metav1.TypeMeta{ @@ -168,5 +168,10 @@ func newParams(taContainerImage string, file string, options ...config.Option) ( }, }, Log: logger, - }, nil + } + targetAllocator, err := TargetAllocator(params) + if err == nil { + params.TargetAllocator = targetAllocator + } + return params, nil } diff --git a/internal/manifests/params.go b/internal/manifests/params.go index 7e12a74b4d..69be71fb0b 100644 --- a/internal/manifests/params.go +++ b/internal/manifests/params.go @@ -32,7 +32,7 @@ type Params struct { Scheme *runtime.Scheme Log logr.Logger OtelCol v1beta1.OpenTelemetryCollector - TargetAllocator v1alpha1.TargetAllocator + TargetAllocator *v1alpha1.TargetAllocator OpAMPBridge v1alpha1.OpAMPBridge Config config.Config } diff --git a/internal/naming/main.go b/internal/naming/main.go index 7ebd21d9ba..f4c6dc3389 100644 --- a/internal/naming/main.go +++ b/internal/naming/main.go @@ -142,8 +142,8 @@ func ClusterRoleBinding(otelcol, namespace string) string { } // TAService returns the name to use for the TargetAllocator service. -func TAService(otelcol string) string { - return DNSName(Truncate("%s-targetallocator", 63, otelcol)) +func TAService(taName string) string { + return DNSName(Truncate("%s-targetallocator", 63, taName)) } // OpAMPBridgeService returns the name to use for the OpAMPBridge service. diff --git a/pkg/sidecar/pod.go b/pkg/sidecar/pod.go index 1f98e22170..d7db13484c 100644 --- a/pkg/sidecar/pod.go +++ b/pkg/sidecar/pod.go @@ -34,7 +34,7 @@ const ( // add a new sidecar container to the given pod, based on the given OpenTelemetryCollector. func add(cfg config.Config, logger logr.Logger, otelcol v1beta1.OpenTelemetryCollector, pod corev1.Pod, attributes []corev1.EnvVar) (corev1.Pod, error) { - otelColCfg, err := collector.ReplaceConfig(otelcol) + otelColCfg, err := collector.ReplaceConfig(otelcol, nil) if err != nil { return pod, err }