From d1c46fc2766cb663cb1ac2e52cc48574ab654e54 Mon Sep 17 00:00:00 2001 From: tdakkota Date: Tue, 28 Jul 2026 13:39:44 +0300 Subject: [PATCH 1/2] fix(api): honour Signals.Metrics/Logs/Traces toggles 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) --- README.md | 2 +- api/v1alpha1/oteldbcluster_types.go | 13 +- internal/controller/config.go | 34 ++--- internal/controller/naming.go | 14 ++ internal/controller/resources.go | 53 +++++--- internal/controller/signals.go | 55 ++++++++ internal/controller/signals_test.go | 201 ++++++++++++++++++++++++++++ 7 files changed, 335 insertions(+), 37 deletions(-) create mode 100644 internal/controller/signals.go create mode 100644 internal/controller/signals_test.go diff --git a/README.md b/README.md index bca2fb0..12b15ff 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ for a fuller example including the S3 backend. | `cluster.peerPort` | Peer replication port (default 7946). | | `cluster.etcdPrefix` | etcd key prefix (storage "root", default `/oteldb`). | | `cluster.staticZone` | Fixed failure-domain label for the cluster's nodes (ring zone-spreading). | -| `signals` | Which signals to serve (all default on; `profiles` is the meaningful toggle). | +| `signals` | Which signals to serve (all default on). Disabling one drops its backend, its API bind and its ports; disabling all is rejected. | | `engine` | Storage engine tuning: `flushInterval`, `readCacheSize`, `decodeCacheSize`, `decodeMemoryLimit`, `aggregateStats`. | | `service.type` / `annotations` | Client Service exposing the query/ingest APIs. | | `resources`, `nodeSelector`, `affinity`, `tolerations`, `topologySpreadConstraints`, `podSecurityContext`, `securityContext`, `podAnnotations`, `podLabels`, `serviceAccountName` | Standard pod scheduling/security knobs. | diff --git a/api/v1alpha1/oteldbcluster_types.go b/api/v1alpha1/oteldbcluster_types.go index 9f0a987..353f4e6 100644 --- a/api/v1alpha1/oteldbcluster_types.go +++ b/api/v1alpha1/oteldbcluster_types.go @@ -70,7 +70,7 @@ type OtelDBClusterSpec struct { Cluster ClusterSpec `json:"cluster,omitempty"` // Signals selects which telemetry signals the cluster serves (all default to enabled and are - // served from the embedded clustered storage engine). + // served from the embedded clustered storage engine). Disabling every signal is rejected. // +optional Signals SignalsSpec `json:"signals,omitempty"` @@ -263,17 +263,26 @@ type ClusterSpec struct { StaticZone string `json:"staticZone,omitempty"` } -// SignalsSpec selects which signals the cluster serves. +// SignalsSpec selects which signals the cluster serves. Disabling a signal drops its backend and +// its API bind from the rendered config, and its ports from the pods and the client Service. At +// least one signal must stay enabled. Unset means enabled. type SignalsSpec struct { + // Metrics serves the Prometheus query API and remote-write ingest. // +kubebuilder:default=true // +optional Metrics *bool `json:"metrics,omitempty"` + + // Logs serves the Loki query API. // +kubebuilder:default=true // +optional Logs *bool `json:"logs,omitempty"` + + // Traces serves the Tempo query API. // +kubebuilder:default=true // +optional Traces *bool `json:"traces,omitempty"` + + // Profiles serves the Pyroscope query and ingest API. // +kubebuilder:default=true // +optional Profiles *bool `json:"profiles,omitempty"` diff --git a/internal/controller/config.go b/internal/controller/config.go index 5fe5153..226d967 100644 --- a/internal/controller/config.go +++ b/internal/controller/config.go @@ -30,17 +30,28 @@ import ( // zone) is intentionally omitted — it is injected per pod via environment variables so the whole // StatefulSet can share this single ConfigMap. func renderConfig(cr *dbv1alpha1.OtelDBCluster, etcdEndpoints []string) (string, error) { + if err := validateSignals(cr); err != nil { + return "", err + } + cfg := map[string]any{ - // Serve every signal from the embedded clustered storage engine; no ClickHouse. - "metrics_backend": valStorage, - "traces_backend": valStorage, - "logs_backend": valStorage, - "tempo": map[string]any{keyBind: "0.0.0.0:3200"}, - "prometheus": map[string]any{keyBind: "0.0.0.0:9090"}, - "loki": map[string]any{keyBind: "0.0.0.0:3100"}, - "health_check": map[string]any{keyBind: "0.0.0.0:13133"}, + "health_check": map[string]any{keyBind: "0.0.0.0:13133"}, } + // Every enabled signal is served from the embedded clustered storage engine; no ClickHouse. + // A disabled signal drops both its backend and its API bind, so the API is not served at all. + if metricsEnabled(cr) { + cfg["metrics_backend"] = valStorage + cfg["prometheus"] = map[string]any{keyBind: "0.0.0.0:9090"} + } + if tracesEnabled(cr) { + cfg["traces_backend"] = valStorage + cfg["tempo"] = map[string]any{keyBind: "0.0.0.0:3200"} + } + if logsEnabled(cr) { + cfg["logs_backend"] = valStorage + cfg["loki"] = map[string]any{keyBind: "0.0.0.0:3100"} + } if profilesEnabled(cr) { cfg["profiles_backend"] = valStorage cfg["pyroscope"] = map[string]any{keyBind: "0.0.0.0:4040"} @@ -127,13 +138,6 @@ func renderConfig(cr *dbv1alpha1.OtelDBCluster, etcdEndpoints []string) (string, return string(out), nil } -func profilesEnabled(cr *dbv1alpha1.OtelDBCluster) bool { - if p := cr.Spec.Signals.Profiles; p != nil { - return *p - } - return true // default on -} - func backendOf(cr *dbv1alpha1.OtelDBCluster) dbv1alpha1.StorageBackend { if b := cr.Spec.Storage.Backend; b != "" { return b diff --git a/internal/controller/naming.go b/internal/controller/naming.go index 587b712..6e57fca 100644 --- a/internal/controller/naming.go +++ b/internal/controller/naming.go @@ -51,6 +51,20 @@ const ( portPeer = 7946 ) +// Port names, shared by the container ports, the Services and the probes. +const ( + portNameOTLPGRPC = "otlp-grpc" + portNameOTLPHTTP = "otlp-http" + portNamePromRW = "prom-rw" + portNamePromHTTP = "prom-http" + portNameTempoHTTP = "tempo-http" + portNameLokiHTTP = "loki-http" + portNamePyroscope = "pyroscope" + portNameSelfMetric = "metrics" + portNameHealth = "health-check" + portNamePeer = "peer" +) + const ( configVolumeName = "config" dataVolumeName = "data" diff --git a/internal/controller/resources.go b/internal/controller/resources.go index 956947e..4220be9 100644 --- a/internal/controller/resources.go +++ b/internal/controller/resources.go @@ -36,19 +36,32 @@ type namedPort struct { port int32 } -// apiPorts are the client-facing API ports served by every node. -func apiPorts() []namedPort { - return []namedPort{ - {"otlp-grpc", portOTLPGRPC}, - {"otlp-http", portOTLPHTTP}, - {"prom-rw", portPromRW}, - {"prom-http", portPromHTTP}, - {"tempo-http", portTempoHTTP}, - {"loki-http", portLokiHTTP}, - {"pyroscope", portPyroscope}, - {"metrics", portSelfMetric}, - {"health-check", portHealth}, +// apiPorts are the client-facing API ports served by every node. Ports belonging to a disabled +// signal are omitted; OTLP ingest, self-metrics and the health check are always exposed. +func apiPorts(cr *dbv1alpha1.OtelDBCluster) []namedPort { + ports := []namedPort{ + {portNameOTLPGRPC, portOTLPGRPC}, + {portNameOTLPHTTP, portOTLPHTTP}, } + if metricsEnabled(cr) { + ports = append(ports, + namedPort{portNamePromRW, portPromRW}, + namedPort{portNamePromHTTP, portPromHTTP}, + ) + } + if tracesEnabled(cr) { + ports = append(ports, namedPort{portNameTempoHTTP, portTempoHTTP}) + } + if logsEnabled(cr) { + ports = append(ports, namedPort{portNameLokiHTTP, portLokiHTTP}) + } + if profilesEnabled(cr) { + ports = append(ports, namedPort{portNamePyroscope, portPyroscope}) + } + return append(ports, + namedPort{portNameSelfMetric, portSelfMetric}, + namedPort{portNameHealth, portHealth}, + ) } // buildConfigMap renders the shared oteldb config into a ConfigMap. @@ -83,7 +96,7 @@ func buildPeerService(cr *dbv1alpha1.OtelDBCluster) *corev1.Service { PublishNotReadyAddresses: true, // peers must resolve each other before readiness Selector: selectorLabels(cr), Ports: []corev1.ServicePort{{ - Name: "peer", + Name: portNamePeer, Port: peerPortOf(cr), TargetPort: intstr.FromInt32(peerPortOf(cr)), Protocol: corev1.ProtocolTCP, @@ -99,8 +112,9 @@ func buildClientService(cr *dbv1alpha1.OtelDBCluster) *corev1.Service { if svcType == "" { svcType = corev1.ServiceTypeClusterIP } - ports := make([]corev1.ServicePort, 0, len(apiPorts())) - for _, p := range apiPorts() { + api := apiPorts(cr) + ports := make([]corev1.ServicePort, 0, len(api)) + for _, p := range api { ports = append(ports, corev1.ServicePort{ Name: p.name, Port: p.port, @@ -247,18 +261,19 @@ func podEnv(cr *dbv1alpha1.OtelDBCluster) []corev1.EnvVar { } func containerPorts(cr *dbv1alpha1.OtelDBCluster) []corev1.ContainerPort { - ports := make([]corev1.ContainerPort, 0, len(apiPorts())+1) - for _, p := range apiPorts() { + api := apiPorts(cr) + ports := make([]corev1.ContainerPort, 0, len(api)+1) + for _, p := range api { ports = append(ports, corev1.ContainerPort{Name: p.name, ContainerPort: p.port, Protocol: corev1.ProtocolTCP}) } - ports = append(ports, corev1.ContainerPort{Name: "peer", ContainerPort: peerPortOf(cr), Protocol: corev1.ProtocolTCP}) + ports = append(ports, corev1.ContainerPort{Name: portNamePeer, ContainerPort: peerPortOf(cr), Protocol: corev1.ProtocolTCP}) return ports } func httpProbe(path string) *corev1.Probe { return &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ - HTTPGet: &corev1.HTTPGetAction{Path: path, Port: intstr.FromString("health-check")}, + HTTPGet: &corev1.HTTPGetAction{Path: path, Port: intstr.FromString(portNameHealth)}, }, PeriodSeconds: 10, TimeoutSeconds: 3, diff --git a/internal/controller/signals.go b/internal/controller/signals.go new file mode 100644 index 0000000..d2e046d --- /dev/null +++ b/internal/controller/signals.go @@ -0,0 +1,55 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "fmt" + + dbv1alpha1 "github.com/oteldb/operator/api/v1alpha1" +) + +// signalEnabled reads a signal toggle; an unset toggle means enabled. +func signalEnabled(toggle *bool) bool { + if toggle != nil { + return *toggle + } + return true +} + +func metricsEnabled(cr *dbv1alpha1.OtelDBCluster) bool { + return signalEnabled(cr.Spec.Signals.Metrics) +} + +func logsEnabled(cr *dbv1alpha1.OtelDBCluster) bool { + return signalEnabled(cr.Spec.Signals.Logs) +} + +func tracesEnabled(cr *dbv1alpha1.OtelDBCluster) bool { + return signalEnabled(cr.Spec.Signals.Traces) +} + +func profilesEnabled(cr *dbv1alpha1.OtelDBCluster) bool { + return signalEnabled(cr.Spec.Signals.Profiles) +} + +// validateSignals rejects a cluster that would serve no signal at all. +func validateSignals(cr *dbv1alpha1.OtelDBCluster) error { + if metricsEnabled(cr) || logsEnabled(cr) || tracesEnabled(cr) || profilesEnabled(cr) { + return nil + } + return fmt.Errorf("spec.signals: at least one signal must be enabled") +} diff --git a/internal/controller/signals_test.go b/internal/controller/signals_test.go new file mode 100644 index 0000000..71da96e --- /dev/null +++ b/internal/controller/signals_test.go @@ -0,0 +1,201 @@ +/* +Copyright 2026. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "slices" + "testing" + + "k8s.io/utils/ptr" + "sigs.k8s.io/yaml" + + dbv1alpha1 "github.com/oteldb/operator/api/v1alpha1" +) + +// signalCase describes what a single signal contributes to the config and the port lists. +type signalCase struct { + name string + disable func(*dbv1alpha1.SignalsSpec) + cfgKeys []string // config keys that must vanish when disabled + portName []string // container/Service port names that must vanish when disabled +} + +func signalCases() []signalCase { + return []signalCase{ + { + name: "metrics", + disable: func(s *dbv1alpha1.SignalsSpec) { s.Metrics = ptr.To(false) }, + cfgKeys: []string{"metrics_backend", "prometheus"}, + portName: []string{"prom-rw", "prom-http"}, + }, + { + name: "logs", + disable: func(s *dbv1alpha1.SignalsSpec) { s.Logs = ptr.To(false) }, + cfgKeys: []string{"logs_backend", "loki"}, + portName: []string{"loki-http"}, + }, + { + name: "traces", + disable: func(s *dbv1alpha1.SignalsSpec) { s.Traces = ptr.To(false) }, + cfgKeys: []string{"traces_backend", "tempo"}, + portName: []string{"tempo-http"}, + }, + { + name: "profiles", + disable: func(s *dbv1alpha1.SignalsSpec) { s.Profiles = ptr.To(false) }, + cfgKeys: []string{"profiles_backend", "pyroscope"}, + portName: []string{"pyroscope"}, + }, + } +} + +func renderedConfig(t *testing.T, cr *dbv1alpha1.OtelDBCluster) map[string]any { + t.Helper() + out, err := renderConfig(cr, cr.Spec.Etcd.Endpoints) + if err != nil { + t.Fatalf("renderConfig: %v", err) + } + var cfg map[string]any + if err := yaml.Unmarshal([]byte(out), &cfg); err != nil { + t.Fatalf("unmarshal rendered config: %v\n%s", err, out) + } + return cfg +} + +func containerPortNames(cr *dbv1alpha1.OtelDBCluster) []string { + ports := buildStatefulSet(cr, "hash").Spec.Template.Spec.Containers[0].Ports + names := make([]string, 0, len(ports)) + for _, p := range ports { + names = append(names, p.Name) + } + return names +} + +func servicePortNames(cr *dbv1alpha1.OtelDBCluster) []string { + ports := buildClientService(cr).Spec.Ports + names := make([]string, 0, len(ports)) + for _, p := range ports { + names = append(names, p.Name) + } + return names +} + +// A disabled signal must lose its backend key, its API bind and its ports, and nothing else. +func TestSignalDisabled(t *testing.T) { + for _, tc := range signalCases() { + t.Run(tc.name, func(t *testing.T) { + cr := testCluster() + tc.disable(&cr.Spec.Signals) + + cfg := renderedConfig(t, cr) + for _, k := range tc.cfgKeys { + if _, present := cfg[k]; present { + t.Errorf("config key %q must be absent when %s is disabled", k, tc.name) + } + } + // Other signals are untouched. + for _, other := range signalCases() { + if other.name == tc.name { + continue + } + for _, k := range other.cfgKeys { + if _, present := cfg[k]; !present { + t.Errorf("config key %q must stay when only %s is disabled", k, tc.name) + } + } + } + if _, present := cfg["health_check"]; !present { + t.Errorf("health_check must always be configured") + } + + for _, want := range tc.portName { + if names := containerPortNames(cr); slices.Contains(names, want) { + t.Errorf("container port %q must be absent when %s is disabled: %v", want, tc.name, names) + } + if names := servicePortNames(cr); slices.Contains(names, want) { + t.Errorf("service port %q must be absent when %s is disabled: %v", want, tc.name, names) + } + } + // Ingest, self-metrics, health and peer ports survive regardless. + for _, want := range []string{portNameOTLPGRPC, portNameOTLPHTTP, portNameSelfMetric, portNameHealth, portNamePeer} { + if names := containerPortNames(cr); !slices.Contains(names, want) { + t.Errorf("container port %q must stay when %s is disabled: %v", want, tc.name, names) + } + } + }) + } +} + +// Nil toggles keep every signal enabled, as documented by the kubebuilder defaults. +func TestSignalsNilKeepsEverything(t *testing.T) { + cr := testCluster() + cfg := renderedConfig(t, cr) + for _, tc := range signalCases() { + for _, k := range tc.cfgKeys { + if _, present := cfg[k]; !present { + t.Errorf("config key %q must be present with unset toggles", k) + } + } + for _, want := range tc.portName { + if names := containerPortNames(cr); !slices.Contains(names, want) { + t.Errorf("container port %q must be present with unset toggles: %v", want, names) + } + if names := servicePortNames(cr); !slices.Contains(names, want) { + t.Errorf("service port %q must be present with unset toggles: %v", want, names) + } + } + } +} + +func TestSignalsAllDisabledRejected(t *testing.T) { + cr := testCluster() + for _, tc := range signalCases() { + tc.disable(&cr.Spec.Signals) + } + if err := validateSignals(cr); err == nil { + t.Errorf("expected an error when every signal is disabled") + } + if _, err := renderConfig(cr, cr.Spec.Etcd.Endpoints); err == nil { + t.Errorf("renderConfig must reject a cluster serving no signal") + } + if _, err := buildConfigMap(cr, cr.Spec.Etcd.Endpoints); err == nil { + t.Errorf("buildConfigMap must propagate the validation error") + } +} + +func TestSignalsSingleEnabledAccepted(t *testing.T) { + for _, tc := range signalCases() { + t.Run(tc.name, func(t *testing.T) { + cr := testCluster() + // Disable everything but this one. + for _, other := range signalCases() { + if other.name != tc.name { + other.disable(&cr.Spec.Signals) + } + } + if err := validateSignals(cr); err != nil { + t.Fatalf("only %s enabled must be valid: %v", tc.name, err) + } + cfg := renderedConfig(t, cr) + for _, k := range tc.cfgKeys { + if _, present := cfg[k]; !present { + t.Errorf("config key %q must be present when %s is the only enabled signal", k, tc.name) + } + } + }) + } +} From 70f5f8bc8039f25207b71e6b8d91a16b53da859a Mon Sep 17 00:00:00 2001 From: tdakkota Date: Tue, 28 Jul 2026 13:39:45 +0300 Subject: [PATCH 2/2] chore(crd): regenerate manifests for signals docs Co-Authored-By: Claude Opus 5 (1M context) --- config/crd/bases/db.oteldb.io_oteldbclusters.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/config/crd/bases/db.oteldb.io_oteldbclusters.yaml b/config/crd/bases/db.oteldb.io_oteldbclusters.yaml index a8fc4e5..61c4ca9 100644 --- a/config/crd/bases/db.oteldb.io_oteldbclusters.yaml +++ b/config/crd/bases/db.oteldb.io_oteldbclusters.yaml @@ -1639,19 +1639,24 @@ spec: signals: description: |- Signals selects which telemetry signals the cluster serves (all default to enabled and are - served from the embedded clustered storage engine). + served from the embedded clustered storage engine). Disabling every signal is rejected. properties: logs: default: true + description: Logs serves the Loki query API. type: boolean metrics: default: true + description: Metrics serves the Prometheus query API and remote-write + ingest. type: boolean profiles: default: true + description: Profiles serves the Pyroscope query and ingest API. type: boolean traces: default: true + description: Traces serves the Tempo query API. type: boolean type: object storage: