Repository navigation
fix(api): honour Signals.Metrics/Logs/Traces toggles - #5
Merged
Merged
Conversation
Only Signals.Profiles was read; the other three toggles were accepted and ignored. Each signal now gates its *_backend key, its API bind block, and its container/Service ports. Disabling every signal is rejected in renderConfig, so it surfaces as a Degraded condition like the other spec validations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Bug
SignalsSpecdeclares four toggles, but onlyProfileswas ever read(
profilesEnabled).renderConfigemittedmetrics_backend,traces_backend,logs_backendand theprometheus/tempo/lokibind blocks unconditionally,and
apiPorts()did not look at the spec at all.Impact
signals: {metrics: false}was accepted by the API server, stored in the CR, anddid nothing — the backend, the Prometheus API bind, the remote-write port and the
Service port all stayed wired, with no indication anywhere that the field was
ignored.
Fix
internal/controller/signals.go(new):metricsEnabled/logsEnabled/tracesEnabled/profilesEnabledshare onesignalEnabled(*bool)helper (nil = enabled, unchangeddefault), plus
validateSignals.renderConfiggates each*_backendkey together with its API bind block on thematching toggle, exactly as
profilesalready did.health_checkstays unconditional.apiPortsnow takes the CR and dropsprom-rw/prom-http,tempo-http,loki-httpand
pyroscopefor disabled signals, so both the container port list and the clientService lose them. OTLP ingest, self-metrics, health check and the peer port are
always exposed. Port names moved to constants in
naming.go.renderConfig, matching theexisting
storage.s3.bucket is requiredcheck, so it propagates throughbuildConfigMap→reconcile→ theDegradedcondition the controller already sets.No CEL/webhook was added: the repo has no
XValidationmarkers and no webhook package,so this follows existing practice.
+kubebuilder:default=truemarkers werealready correct); README's "
profilesis the meaningful toggle" note updated.Tests in
internal/controller/signals_test.gocover each signal individually disabled(backend key, bind block, container ports, Service ports all gone, other signals intact),
nil toggles keeping everything, each signal working as the only enabled one, and
all-disabled being rejected at
validateSignals,renderConfigandbuildConfigMap.make testandmake lint(custom golangci-lint build) both pass.Closes #2
🤖 Generated with Claude Code