Skip to content

Commit ebac7e9

Browse files
ChrisJBurnsclaude
andcommitted
Move endpoint validation to Validate(), fix smart quote corruption
The gci linter was replacing ASCII quotes with Unicode smart quotes (U+201C/U+201D) in kubebuilder annotations, breaking controller-gen CRD generation. Additionally, the CEL rule for endpoint validation generated double-quoted YAML that the Helm wrapper mishandled. Move the endpoint-requires-signals validation from a CEL rule to the programmatic Validate() method, which avoids both issues and provides clearer error messages. Add test case for this validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 7f4ede1 commit ebac7e9

4 files changed

Lines changed: 40 additions & 7 deletions

File tree

cmd/thv-operator/api/v1alpha1/mcptelemetryconfig_types.go

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ type SensitiveHeader struct {
3131
// - Adds ResourceAttributes for shared OTel resource attributes
3232
//
3333
// +kubebuilder:validation:XValidation:rule="!has(self.headers) || !has(self.sensitiveHeaders) || self.sensitiveHeaders.all(sh, !(sh.name in self.headers))",message="a header name cannot appear in both headers and sensitiveHeaders"
34-
// +kubebuilder:validation:XValidation:rule=”!has(self.endpoint) || size(self.endpoint) == 0 || (has(self.tracing) && self.tracing.enabled) || (has(self.metrics) && self.metrics.enabled)”,message=”endpoint requires at least one of tracing or metrics to be enabled”
3534
//
3635
//nolint:lll // CEL validation rules exceed line length limit
3736
type MCPTelemetryOTelConfig struct {
@@ -179,9 +178,30 @@ type MCPTelemetryConfigReference struct {
179178
// CEL catches issues at API admission time, but this method also validates
180179
// stored objects to catch any that bypassed CEL or were stored before CEL rules were added.
181180
func (r *MCPTelemetryConfig) Validate() error {
181+
if err := r.validateEndpointRequiresSignals(); err != nil {
182+
return err
183+
}
182184
return r.validateSensitiveHeaders()
183185
}
184186

187+
// validateEndpointRequiresSignals rejects an endpoint when neither tracing nor metrics is enabled.
188+
// Without this check the config would pass CRD validation but fail at runtime in telemetry.NewProvider.
189+
func (r *MCPTelemetryConfig) validateEndpointRequiresSignals() error {
190+
if r.Spec.OpenTelemetry == nil {
191+
return nil
192+
}
193+
otel := r.Spec.OpenTelemetry
194+
if otel.Endpoint == "" {
195+
return nil
196+
}
197+
tracingEnabled := otel.Tracing != nil && otel.Tracing.Enabled
198+
metricsEnabled := otel.Metrics != nil && otel.Metrics.Enabled
199+
if !tracingEnabled && !metricsEnabled {
200+
return fmt.Errorf("endpoint requires at least one of tracing or metrics to be enabled")
201+
}
202+
return nil
203+
}
204+
185205
// validateSensitiveHeaders validates sensitive header entries and checks for overlap with plaintext headers.
186206
func (r *MCPTelemetryConfig) validateSensitiveHeaders() error {
187207
if r.Spec.OpenTelemetry == nil {

cmd/thv-operator/controllers/mcptelemetryconfig_controller_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,19 @@ func TestMCPTelemetryConfig_Validate(t *testing.T) {
498498
},
499499
expectError: true,
500500
},
501+
{
502+
name: "invalid endpoint without tracing or metrics",
503+
config: &mcpv1alpha1.MCPTelemetryConfig{
504+
Spec: mcpv1alpha1.MCPTelemetryConfigSpec{
505+
OpenTelemetry: &mcpv1alpha1.MCPTelemetryOTelConfig{
506+
Enabled: true,
507+
Endpoint: "otel-collector:4317",
508+
// No Tracing or Metrics configured
509+
},
510+
},
511+
},
512+
expectError: true,
513+
},
501514
{
502515
name: "invalid empty secret ref name",
503516
config: &mcpv1alpha1.MCPTelemetryConfig{

deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcptelemetryconfigs.yaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ spec:
2222
- jsonPath: .spec.openTelemetry.endpoint
2323
name: Endpoint
2424
type: string
25+
- jsonPath: .status.conditions[?(@.type=='Valid')].status
26+
name: Ready
27+
type: string
2528
- jsonPath: .spec.openTelemetry.tracing.enabled
2629
name: Tracing
2730
type: boolean
@@ -163,9 +166,6 @@ spec:
163166
- message: a header name cannot appear in both headers and sensitiveHeaders
164167
rule: '!has(self.headers) || !has(self.sensitiveHeaders) || self.sensitiveHeaders.all(sh,
165168
!(sh.name in self.headers))'
166-
- message: endpoint requires at least one of tracing or metrics to be enabled
167-
rule: '!has(self.endpoint) || size(self.endpoint) == 0 || (has(self.tracing)
168-
&& self.tracing.enabled) || (has(self.metrics) && self.metrics.enabled)'
169169
prometheus:
170170
description: Prometheus defines Prometheus-specific configuration
171171
properties:

deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcptelemetryconfigs.yaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ spec:
2525
- jsonPath: .spec.openTelemetry.endpoint
2626
name: Endpoint
2727
type: string
28+
- jsonPath: .status.conditions[?(@.type=='Valid')].status
29+
name: Ready
30+
type: string
2831
- jsonPath: .spec.openTelemetry.tracing.enabled
2932
name: Tracing
3033
type: boolean
@@ -166,9 +169,6 @@ spec:
166169
- message: a header name cannot appear in both headers and sensitiveHeaders
167170
rule: '!has(self.headers) || !has(self.sensitiveHeaders) || self.sensitiveHeaders.all(sh,
168171
!(sh.name in self.headers))'
169-
- message: endpoint requires at least one of tracing or metrics to be enabled
170-
rule: '!has(self.endpoint) || size(self.endpoint) == 0 || (has(self.tracing)
171-
&& self.tracing.enabled) || (has(self.metrics) && self.metrics.enabled)'
172172
prometheus:
173173
description: Prometheus defines Prometheus-specific configuration
174174
properties:

0 commit comments

Comments
 (0)