Add TelemetryConfigRef support to VirtualMCPServer CRD#4801
Add TelemetryConfigRef support to VirtualMCPServer CRD#4801ChrisJBurns merged 5 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4801 +/- ##
==========================================
- Coverage 69.01% 68.93% -0.08%
==========================================
Files 517 518 +1
Lines 54829 54980 +151
==========================================
+ Hits 37840 37901 +61
- Misses 14071 14154 +83
- Partials 2918 2925 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
ChrisJBurns
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: kubernetes-expert, code-reviewer, toolhive-expert, go-security-reviewer, go-expert-developer
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | MCPTelemetryConfig controller missing VirtualMCPServer in findReferencingWorkloads | 8/10 | HIGH | Fix |
| 2 | Deployment builder silently swallows telemetry fetch errors | 8/10 | MEDIUM | Discuss |
| 3 | Redundant MCPTelemetryConfig API calls (3x per reconcile) | 9/10 | MEDIUM | Fix (or follow-up) |
| 4 | Deprecation log at wrong level + fires every reconciliation | 8/10 | MEDIUM | Fix |
| 5 | normalizeTelemetry returns nil,nil when ref set but config not found | 7/10 | LOW | Fix |
| 6 | Architecture docs not updated (Mermaid diagram + "Referenced by") | 8/10 | MEDIUM | Fix |
| 7 | Missing condition cleanup when TelemetryConfigRef is removed | 7/10 | MEDIUM | Fix |
| 8 | Double error wrapping in normalizeTelemetry | 7/10 | LOW | Fix |
Overall
The implementation is well-structured and closely follows established MCPServer/MCPRemoteProxy telemetry config ref patterns. The CRD types, CEL validation, controller handler, watch mapper, status management, and converter are all consistent with existing conventions. The test coverage (unit + integration) is solid and covers the key scenarios.
The main gap is that the MCPTelemetryConfig controller's findReferencingWorkloads was not updated to recognize VirtualMCPServer as a referencing workload. This means deletion protection via finalizer won't block deletion of an MCPTelemetryConfig that a VirtualMCPServer still references — a functional gap in the feature being added. The remaining findings are non-blocking improvements: reducing redundant API calls (3x per reconcile), fixing the deprecation log level/frequency, cleaning up stale conditions on ref removal, and updating architecture docs.
Documentation
docs/arch/09-operator-architecture.md— MissingVMCP -.->|telemetryConfigRef| TelCfgedge in the Mermaid diagram (line ~131). The "Referenced by" text for MCPTelemetryConfig (line ~246) must now include VirtualMCPServer.docs/arch/10-virtual-mcp-architecture.md— No mention oftelemetryConfigRefsupport. Add a brief note.
Generated with Claude Code
cmd/thv-operator/controllers/virtualmcpserver_telemetryconfig.go
Outdated
Show resolved
Hide resolved
Fixes from code review on PR #4801 for issue #4792: - Add VirtualMCPServer to MCPTelemetryConfig controller's findReferencingWorkloads, watch handler, and RBAC markers for deletion protection parity - Eliminate redundant MCPTelemetryConfig API calls (3x per reconcile) by fetching once in handleTelemetryConfig and threading the config through to converter and deployment builder - Change normalizeTelemetry to accept pre-fetched config instead of re-fetching - Fix deprecation log level from Info to V(1) debug to avoid log flooding - Remove stale TelemetryConfigRefValidated condition when TelemetryConfigRef is nil - Update architecture docs with VirtualMCPServer telemetryConfigRef edges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
VirtualMCPServer can now reference shared MCPTelemetryConfig resources via spec.telemetryConfigRef, following the same pattern used by MCPServer and MCPRemoteProxy. This enables shared telemetry configuration, Kubernetes-native secret references for OTLP auth headers, CA bundle ConfigMap references, and per-server serviceName overrides. Implements changes for issue #4792: - Add TelemetryConfigRef field and CEL mutual exclusivity validation - Add TelemetryConfigHash to status for change detection - Add telemetry handler with batched statusManager pattern - Add MCPTelemetryConfig watch in SetupWithManager - Update converter to prefer TelemetryConfigRef over inline telemetry - Add CA bundle volumes and sensitive header env vars to deployment - Regenerate CRD manifests, RBAC, deepcopy, and mocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add deprecation comments to config.Config.Telemetry field (for operator context) and emit a warning log when inline telemetry is used during conversion, guiding operators to migrate to spec.telemetryConfigRef. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add envtest-based integration tests for VirtualMCPServer TelemetryConfigRef: hash tracking, config change detection, missing ref condition, and CEL mutual exclusivity validation. Update observability docs to show the preferred telemetryConfigRef pattern for VirtualMCPServer and mark inline config.telemetry as deprecated. Add example manifest demonstrating shared MCPTelemetryConfig with VirtualMCPServer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fixes from code review on PR #4801 for issue #4792: - Add VirtualMCPServer to MCPTelemetryConfig controller's findReferencingWorkloads, watch handler, and RBAC markers for deletion protection parity - Eliminate redundant MCPTelemetryConfig API calls (3x per reconcile) by fetching once in handleTelemetryConfig and threading the config through to converter and deployment builder - Change normalizeTelemetry to accept pre-fetched config instead of re-fetching - Fix deprecation log level from Info to V(1) debug to avoid log flooding - Remove stale TelemetryConfigRefValidated condition when TelemetryConfigRef is nil - Update architecture docs with VirtualMCPServer telemetryConfigRef edges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
a3bf1f3 to
aad9236
Compare
Summary
VirtualMCPServer only supported inline telemetry configuration via
spec.config.telemetry, while MCPServer and MCPRemoteProxy already support referencing sharedMCPTelemetryConfigresources. This gap means VirtualMCPServer users cannot leverage Kubernetes-native secret references for OTLP auth headers, CA bundle ConfigMap references for internal PKI, or per-serverserviceNameoverrides for observability isolation.spec.telemetryConfigReffield to reference sharedMCPTelemetryConfigresourcesconfig.telemetryTelemetryConfigHashto status for change detection and rolling updatesTelemetryConfigRefover inline telemetryCloses #4792
Type of change
Test plan
task test)task lint-fix)Changes
api/v1alpha1/virtualmcpserver_types.goTelemetryConfigReffield, CEL validation, status hash, condition constantspkg/virtualmcpserverstatus/types.goSetTelemetryConfigHashandSetTelemetryConfigRefValidatedConditionto interfacepkg/virtualmcpserverstatus/collector.goUpdateStatuspkg/controllerutil/config.goGetTelemetryConfigForVirtualMCPServer()helpercontrollers/virtualmcpserver_telemetryconfig.gocontrollers/virtualmcpserver_controller.gohandleConfigRefsextraction, MCPTelemetryConfig watchcontrollers/virtualmcpserver_deployment.gopkg/vmcpconfig/converter.goTelemetryConfigRefvs inline, extracted helperscontrollers/virtualmcpserver_telemetryconfig_test.gopkg/controllerutil/config_test.goGetTelemetryConfigForVirtualMCPServerpkg/virtualmcpserverstatus/collector_test.gopkg/vmcpconfig/converter_test.goConvertwithTelemetryConfigRefDoes this introduce a user-facing change?
Yes. VirtualMCPServer now supports
spec.telemetryConfigRefto reference sharedMCPTelemetryConfigresources, consistent with MCPServer and MCPRemoteProxy. The existing inlineconfig.telemetryfield continues to work but is deprecated.Implementation plan
Approved implementation plan
See the full plan at: #4792
Key design decisions:
spec.telemetryConfigRef(not nested underspec.config) for consistency with MCPServer/MCPRemoteProxyStatusManagerpattern (not directr.Status().Update()) matching VirtualMCPServer conventionshandleTelemetryConfig, so it's guaranteed to existAddTelemetryCABundleVolumes,GenerateOpenTelemetryEnvVarsFromRef,NormalizeMCPTelemetryConfigGenerated with Claude Code
Large PR Justification
This is a single feature (TelemetryConfigRef for VirtualMCPServer) that requires coordinated changes across CRD types, controller handler, converter, deployment builder, MCPTelemetryConfig controller (deletion protection), status management, integration tests, documentation, and example manifests. Splitting into smaller PRs would produce incomplete/non-functional intermediate states — e.g., adding the CRD field without the controller handler leaves a field that does nothing, or adding the handler without the MCPTelemetryConfig controller update leaves a deletion protection gap.