Fix(target-allocator): accept prometheus receiver with only target_allocator block#5085
Open
sashetov wants to merge 1 commit into
Open
Fix(target-allocator): accept prometheus receiver with only target_allocator block#5085sashetov wants to merge 1 commit into
sashetov wants to merge 1 commit into
Conversation
|
|
Contributor
|
I don't think you need to set You should change this in some our e2e test configs - if they pass, it'll confirm that your change is valid. Finally, you'll need to use an email address in your commit metadata that is linked to your GH account and sign the CLA with said account. |
Signed-off-by: Alex Vassilevski <alexander@vassilevski.com>
23eb5c8 to
5581200
Compare
Contributor
E2E Test Results 38 files 263 suites 2h 42m 15s ⏱️ Results for commit 5581200. ♻️ This comment has been updated with latest results. |
Author
|
@swiatekm I dropped the |
Contributor
The TargetAllocator CRD tests are not passing, can you have a look? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix(target-allocator): accept prometheus receiver with only target_allocator block
Closes #2998
Summary
When an
OpenTelemetryCollectorCR has a prometheus receiver containing only atarget_allocator:block (noconfig:/scrape_configs:), reconciliation fails with"no prometheusConfig available as part of the configuration". this is wrong - that shape is a legitimate "TA-only" mode where the target allocator supplies scrape configuration externally (typically from discovered PrometheusCR objects, or a configmap mounted on the TA, outside the operator's surface).Note: a 2025-09-17 comment on the issue claimed this can't be reproduced, but I can't confirm that — the bug report shape (receiver with
target_allocator:only, noconfig:at all) still triggers the error onmain. unit-level repro below.What Changed
internal/manifests/targetallocator/adapters/config_to_prom_config.go-AddTAConfigToPromConfignow skips the scrape_configs strip whenconfig:is absent instead of erroring; whenconfig:is present the existing strip still runs.ValidateTargetAllocatorConfigreturnsnilwhenconfig:is absent (TA-only mode signal), instead of cascading the missing-config error fromGetScrapeConfigsFromPromConfig.internal/manifests/targetallocator/adapters/config_to_prom_config_test.go- the previously-asserts-bug sub-case ("missing config property"→ expects the old error) is removed from the negative test, and two positive sub-cases are added:"TA-only mode: no config block, only target_allocator set"and"TA-only mode: no config block, no target_allocator block". both assert no spuriousconfig:key is inserted on the way out.TestValidateTargetAllocatorConfiggets its"receiver config empty and PrometheusCR disabled"row flipped from "expect error" to "expect nil", plus a new explicit"no config block but target_allocator set, PrometheusCR disabled"row.tests/e2e-targetallocator/targetallocator-no-promconfig/- new chainsaw e2e: deploys anOpenTelemetryCollectorwithreceivers: { prometheus: {} }andtargetAllocator.enabled: true+prometheusCR.enabled: true, then asserts the collector and TA come up Ready and that the rendered collector configmap contains the operator-injectedtarget_allocatorblock underprometheus:with noconfig:key..chloggen/fix_target-allocator-only-config.yaml- bug_fix entry.Reproduction
a throwaway test (not committed) calls both functions with the exact config from the bug report. at HEAD^ both fail with the bug's error; at HEAD both pass:
The regression coverage that lands in this PR is in
config_to_prom_config_test.goand the new chainsaw test undertests/e2e-targetallocator/targetallocator-no-promconfig/; the throwaway above is just to demonstrate the before-state in a single test file at the same commit.Design
config: {}map so downstream code could keep writing into it. swiatekm pointed out the prometheus receiver doesn't require aconfig:block at all, so the synthesis is unnecessary. the current diff just guards thedelete(prometheusCfg, "scrape_configs")behind the presence check — if there's noconfig:, there are no scrape_configs to strip and nothing else inAddTAConfigToPromConfigtouchesprometheusCfg. the rendered collector config stays minimal (just the operator-injectedtarget_allocatorblock) and the chainsaw assert pins that shape.config:is absent and TA is enabled (this whole code path runs only when TA is enabled), there is genuinely nothing to validate locally. the TA owns its scrape-config source.target_allocator.endpointuser-supplied value is still overwritten by the operator (unchanged). that's existing behavior; this PR doesn't change endpoint semantics, only the validation/strip path.Tests
make precommitnot run end-to-end locally (envtest binaries download is slow on first run); the targetedgo testruns above cover the same packages. the new chainsaw test runs under the operator's e2e-targetallocator suite in CI.Notes
make ensure-update-is-noopshould be a no-op - verified by not editing anything underapis/.ValidateTargetAllocatorConfigchange out into a separate PR if maintainers prefer; both pieces are needed to fully resolve Target Allocator doesn't accept only target_allocator config in the collector config #2998 since both functions are called from the webhook (collector_webhook.go:330-337).