diff --git a/.chloggen/fix_target-allocator-only-config.yaml b/.chloggen/fix_target-allocator-only-config.yaml new file mode 100644 index 0000000000..835c1c270a --- /dev/null +++ b/.chloggen/fix_target-allocator-only-config.yaml @@ -0,0 +1,21 @@ +# 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: Accept a prometheus receiver that only declares `target_allocator:` without a `config:` block. + +# One or more tracking issues related to the change +issues: [2998] + +# (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: | + When the prometheus receiver is configured with only a `target_allocator:` block and no `config:`, + reconciliation previously failed with `no prometheusConfig available as part of the configuration`. + The target allocator supplies scrape configuration externally in this mode (e.g. via discovered + PrometheusCR objects), so the operator now skips the scrape_configs cleanup when no `config:` + block is present. The webhook validator likewise permits this shape. diff --git a/internal/manifests/targetallocator/adapters/config_to_prom_config.go b/internal/manifests/targetallocator/adapters/config_to_prom_config.go index eab3ef9c99..d548e49cf8 100644 --- a/internal/manifests/targetallocator/adapters/config_to_prom_config.go +++ b/internal/manifests/targetallocator/adapters/config_to_prom_config.go @@ -282,14 +282,16 @@ func WithCollectorTargetReloadInterval(interval string) TAOption { // If the `EnableTargetAllocatorRewrite` feature flag for the target allocator is enabled, this function // removes the existing scrape_configs from the collector's Prometheus configuration as it's not required. func AddTAConfigToPromConfig(prometheus map[any]any, taServiceName string, taOpts ...TAOption) (map[any]any, error) { - prometheusConfigProperty, ok := prometheus["config"] - if !ok { - return nil, errorNoComponent("prometheusConfig") - } - - prometheusCfg, ok := prometheusConfigProperty.(map[any]any) - if !ok { - return nil, errorNotAMap("prometheusConfig") + // When the receiver has a `config:` block, strip any user-provided scrape_configs: + // they will be supplied by the target allocator at runtime. When the block is absent + // (TA-only mode: scrape configs come from the TA itself, e.g. a mounted configmap or + // PrometheusCR objects), there is nothing to strip. + if prometheusConfigProperty, ok := prometheus["config"]; ok { + prometheusCfg, ok := prometheusConfigProperty.(map[any]any) + if !ok { + return nil, errorNotAMap("prometheusConfig") + } + delete(prometheusCfg, "scrape_configs") } // Create the TargetAllocConfig dynamically if it doesn't exist @@ -313,9 +315,6 @@ func AddTAConfigToPromConfig(prometheus map[any]any, taServiceName string, taOpt } } - // Remove the scrape_configs key from the map - delete(prometheusCfg, "scrape_configs") - return prometheus, nil } @@ -344,6 +343,12 @@ func ValidateTargetAllocatorConfig(targetAllocatorPrometheusCR bool, promReceive if targetAllocatorPrometheusCR { return nil } + // TA-only mode: when the receiver has no `config:` block, the user is delegating + // scrape configuration to the target allocator itself (loaded from an external + // source at runtime). Permit this — we have nothing to validate locally. + if _, hasConfig := promReceiverConfig["config"]; !hasConfig { + return nil + } // if PrometheusCR isn't enabled, we need at least one scrape config scrapeConfigs, err := GetScrapeConfigsFromPromConfig(promReceiverConfig) if err != nil { 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 c219147ec4..3596ab9a0e 100644 --- a/internal/manifests/targetallocator/adapters/config_to_prom_config_test.go +++ b/internal/manifests/targetallocator/adapters/config_to_prom_config_test.go @@ -339,17 +339,12 @@ func TestAddTAConfigToPromConfig(t *testing.T) { assert.Equal(t, expectedResult, result) }) - t.Run("missing or invalid prometheusConfig property, returns error", func(t *testing.T) { + t.Run("invalid prometheusConfig property, returns error", func(t *testing.T) { testCases := []struct { name string cfg map[any]any errText string }{ - { - name: "missing config property", - cfg: map[any]any{}, - errText: "no prometheusConfig available as part of the configuration", - }, { name: "invalid config property", cfg: map[any]any{ @@ -370,6 +365,47 @@ func TestAddTAConfigToPromConfig(t *testing.T) { }) } }) + + t.Run("TA-only mode: no config block, only target_allocator set", func(t *testing.T) { + // Regression test for #2998. The user supplied only a `target_allocator:` block + // on the prometheus receiver (no `config:`). Reconciliation must not fail, and + // no spurious `config:` block should be inserted — the prometheus receiver itself + // does not require one. + cfg := map[any]any{ + "target_allocator": map[any]any{ + "endpoint": "user-supplied-endpoint", + }, + } + taServiceName := "test-targetallocator" + + result, err := ta.AddTAConfigToPromConfig(cfg, taServiceName) + + assert.NoError(t, err) + assert.NotNil(t, result) + taSection, ok := result["target_allocator"].(map[any]any) + assert.True(t, ok) + // Operator-managed endpoint replaces the user-supplied one. + assert.Equal(t, "http://test-targetallocator:80", taSection["endpoint"]) + // No `config:` block should be added when the user didn't supply one. + _, hasConfig := result["config"] + assert.False(t, hasConfig) + }) + + t.Run("TA-only mode: no config block, no target_allocator block", func(t *testing.T) { + // Edge case for #2998: an entirely empty receiver succeeds once TA is enabled. + // The operator inserts the target_allocator block but leaves `config:` absent. + cfg := map[any]any{} + taServiceName := "test-targetallocator" + + result, err := ta.AddTAConfigToPromConfig(cfg, taServiceName) + + assert.NoError(t, err) + assert.NotNil(t, result) + _, hasConfig := result["config"] + assert.False(t, hasConfig) + _, hasTA := result["target_allocator"] + assert.True(t, hasTA) + }) } func TestValidatePromConfig(t *testing.T) { @@ -481,10 +517,24 @@ func TestValidateTargetAllocatorConfig(t *testing.T) { expectedError: nil, }, { + // TA-only mode (#2998): receiver has no `config:` block, so the user is + // delegating scrape configuration to the target allocator. Validator permits. description: "receiver config empty and PrometheusCR disabled", config: map[any]any{}, targetAllocatorPrometheusCR: false, - expectedError: fmt.Errorf("no %s available as part of the configuration", "prometheusConfig"), + expectedError: nil, + }, + { + // TA-only mode with explicit target_allocator block — same as the empty case, + // permitted because no `config:` means scrape configs come from the TA itself. + description: "no config block but target_allocator set, PrometheusCR disabled", + config: map[any]any{ + "target_allocator": map[any]any{ + "endpoint": "http://my-ta:80", + }, + }, + targetAllocatorPrometheusCR: false, + expectedError: nil, }, { description: "scrape configs empty and PrometheusCR disabled", diff --git a/internal/manifests/targetallocator/configmap.go b/internal/manifests/targetallocator/configmap.go index 6c69e00fe0..534e9ee250 100644 --- a/internal/manifests/targetallocator/configmap.go +++ b/internal/manifests/targetallocator/configmap.go @@ -223,6 +223,13 @@ func getGlobalConfigFromOtelConfig(otelConfig v1beta1.Config) (v1beta1.AnyConfig func getScrapeConfigsFromOtelConfig(otelcolConfig string) ([]v1beta1.AnyConfig, error) { // Collector supports environment variable substitution, but the TA does not. // TA Scrape Configs should have a single "$", as it does not support env var substitution + promConfig, err := adapters.ConfigToPromConfig(otelcolConfig) + if err != nil { + return nil, err + } + if _, hasConfig := promConfig["config"]; !hasConfig { + return []v1beta1.AnyConfig{}, nil + } prometheusReceiverConfig, err := adapters.UnescapeDollarSignsInPromConfig(otelcolConfig) if err != nil { return nil, err diff --git a/internal/manifests/targetallocator/configmap_test.go b/internal/manifests/targetallocator/configmap_test.go index 836b7ef76b..ddba79e4a3 100644 --- a/internal/manifests/targetallocator/configmap_test.go +++ b/internal/manifests/targetallocator/configmap_test.go @@ -556,6 +556,17 @@ func TestGetScrapeConfigsFromOtelConfig(t *testing.T) { }, wantErr: errors.New("no scrape_configs available as part of the configuration"), }, + { + name: "no prom config key", + input: v1beta1.Config{ + Receivers: v1beta1.AnyConfig{ + Object: map[string]any{ + "prometheus": map[string]any{}, + }, + }, + }, + want: []v1beta1.AnyConfig{}, + }, { name: "one scrape config", input: v1beta1.Config{ diff --git a/tests/e2e-targetallocator/targetallocator-no-promconfig/00-assert.yaml b/tests/e2e-targetallocator/targetallocator-no-promconfig/00-assert.yaml new file mode 100644 index 0000000000..d0177064b3 --- /dev/null +++ b/tests/e2e-targetallocator/targetallocator-no-promconfig/00-assert.yaml @@ -0,0 +1,61 @@ +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: no-promconfig-collector +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: no-promconfig-targetallocator +status: + observedGeneration: 1 + readyReplicas: 1 + replicas: 1 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: no-promconfig-targetallocator +--- +# The rendered collector config must contain the operator-injected +# target_allocator block but no `config:` key under prometheus (the user +# didn't supply one and the operator should not synthesise one). +apiVersion: v1 +data: + collector.yaml: | + exporters: + prometheus: + endpoint: 0.0.0.0:9090 + receivers: + prometheus: + target_allocator: + collector_id: ${POD_NAME} + endpoint: http://no-promconfig-targetallocator:80 + interval: 30s + service: + pipelines: + metrics: + exporters: + - prometheus + receivers: + - prometheus + telemetry: + metrics: + readers: + - pull: + exporter: + prometheus: + host: 0.0.0.0 + port: 8888 + without_scope_info: false + without_type_suffix: false + without_units: false +kind: ConfigMap +metadata: + labels: + app.kubernetes.io/component: opentelemetry-collector + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: no-promconfig-collector diff --git a/tests/e2e-targetallocator/targetallocator-no-promconfig/00-install.yaml b/tests/e2e-targetallocator/targetallocator-no-promconfig/00-install.yaml new file mode 100644 index 0000000000..f49dc0043a --- /dev/null +++ b/tests/e2e-targetallocator/targetallocator-no-promconfig/00-install.yaml @@ -0,0 +1,80 @@ +apiVersion: v1 +automountServiceAccountToken: true +kind: ServiceAccount +metadata: + name: collector +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: (join('-', ['collector', $namespace])) +rules: +- apiGroups: + - "" + resources: + - pods + - nodes + - nodes/metrics + - services + - endpoints + - namespaces + verbs: + - get + - watch + - list +- apiGroups: + - networking.k8s.io + resources: + - ingresses + verbs: + - get + - watch + - list +- nonResourceURLs: + - /metrics + - /metrics/cadvisor + verbs: + - get +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: (join('-', ['collector', $namespace])) +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: (join('-', ['collector', $namespace])) +subjects: +- kind: ServiceAccount + name: collector + namespace: ($namespace) +--- +# Regression manifest for #2998: prometheus receiver has no `config:` block. +# Scrape configuration is delegated entirely to the target allocator, which +# loads it from PrometheusCR objects discovered in-cluster. The webhook used +# to reject this shape; reconciliation used to fail on it. +apiVersion: opentelemetry.io/v1beta1 +kind: OpenTelemetryCollector +metadata: + name: no-promconfig +spec: + config: + receivers: + prometheus: {} + exporters: + nop: {} + service: + pipelines: + metrics: + receivers: [prometheus] + exporters: [nop] + mode: statefulset + serviceAccount: collector + targetAllocator: + enabled: true + prometheusCR: + enabled: true + scrapeInterval: 1s + serviceMonitorSelector: {} + podMonitorSelector: {} + serviceAccount: ta diff --git a/tests/e2e-targetallocator/targetallocator-no-promconfig/chainsaw-test.yaml b/tests/e2e-targetallocator/targetallocator-no-promconfig/chainsaw-test.yaml new file mode 100644 index 0000000000..d7ad65efa8 --- /dev/null +++ b/tests/e2e-targetallocator/targetallocator-no-promconfig/chainsaw-test.yaml @@ -0,0 +1,27 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + creationTimestamp: null + name: targetallocator-no-promconfig +spec: + steps: + - name: Setup Target Allocator RBAC + use: + template: ../../step-templates/target-allocator-rbac-comprehensive.yaml + with: + bindings: + - name: clusterRoleName + value: targetallocator-no-promconfig + - name: clusterRoleBindingName + value: (join('-', ['ta', $namespace])) + - name: step-00 + try: + - apply: + template: true + file: 00-install.yaml + - assert: + file: 00-assert.yaml + catch: + - podLogs: + selector: app.kubernetes.io/managed-by=opentelemetry-operator