Skip to content

Commit 691f73a

Browse files
ChrisJBurnsclaude
ChrisJBurns
and
claude
authored
Add TelemetryConfigRef support to MCPRemoteProxy (#4719)
* Add TelemetryConfigRef field and condition constants to MCPRemoteProxy Brings MCPRemoteProxy to parity with MCPServer's telemetry API by adding a TelemetryConfigRef field for referencing shared MCPTelemetryConfig resources. Includes CEL mutual exclusivity validation with the deprecated inline Telemetry field, TelemetryConfigHash in status for change detection, and condition type/reason constants. Part of #4620 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add GetTelemetryConfigForMCPRemoteProxy helper Namespace-scoped fetch for MCPTelemetryConfig referenced by an MCPRemoteProxy. Returns (nil, nil) when the ref is nil or the resource is not found, matching the MCPServer getTelemetryConfigForMCPServer contract. Part of #4620 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add handleTelemetryConfig and MCPTelemetryConfig watch for MCPRemoteProxy Wire up the reconciler to validate referenced MCPTelemetryConfig resources, track config hashes in status, and reconcile when the underlying MCPTelemetryConfig changes. Follows the same handler pattern as handleToolConfig and the MCPServer telemetry handler. Part of #4620 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Prefer TelemetryConfigRef over inline Telemetry in MCPRemoteProxy RunConfig When building the RunConfig, resolve telemetry from the referenced MCPTelemetryConfig first and fall back to the deprecated inline Telemetry field. Adds ctx parameter to createRunConfigFromMCPRemoteProxy to support the API fetch, matching the MCPServer runconfig pattern. Part of #4620 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Regenerate deepcopy, CRD manifests, and API docs Auto-generated from MCPRemoteProxy type changes: adds telemetryConfigRef to CRD schema with CEL mutual exclusivity validation, telemetryConfigHash to status, and updated API reference docs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix lint issues and extract helpers to reduce cyclomatic complexity Extract resolveToolConfig and addTelemetryOptions from createRunConfigFromMCPRemoteProxy to bring cyclomatic complexity below the threshold. Rename shadowed ctx to apiCtx for clarity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address review feedback for TelemetryConfigRef support Five fixes from code review: 1. Add +kubebuilder:rbac marker for mcptelemetryconfigs on MCPRemoteProxyReconciler — required for runtime API access 2. Extend MCPTelemetryConfigReconciler.findReferencingWorkloads to scan MCPRemoteProxy in addition to MCPServer, fixing deletion protection and referencingWorkloads status. Add MCPRemoteProxy watch to SetupWithManager for the reverse-direction reconciliation 3. Mount telemetry CA bundle volumes in proxy deployment via addTelemetryCABundleVolumes — without this, caBundleRef users get file-not-found at proxy startup 4. Fix condition removal not persisted when TelemetryConfigHash is already empty — check for stale condition before deciding whether to call Status().Update() 5. Fix test assertions to re-fetch persisted state from fakeClient instead of reading in-memory pointers. Add recovery-from-False and stale-condition-removal test cases Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add integration tests for MCPRemoteProxy telemetry config ref Two new integration tests in the mcp-telemetry-config suite: 1. MCPRemoteProxy tracked in ReferencingWorkloads — creates a proxy referencing an MCPTelemetryConfig and asserts the proxy appears in status.referencingWorkloads via the MCPRemoteProxy watch handler. 2. Deletion protection for proxy-only references — verifies that the finalizer blocks deletion while an MCPRemoteProxy references the config, and allows deletion after the proxy is removed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 570615e commit 691f73a

File tree

14 files changed

+995
-46
lines changed

14 files changed

+995
-46
lines changed

‎cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ type HeaderFromSecret struct {
3838
// MCPRemoteProxySpec defines the desired state of MCPRemoteProxy
3939
//
4040
// +kubebuilder:validation:XValidation:rule="!(has(self.oidcConfig) && has(self.oidcConfigRef))",message="oidcConfig and oidcConfigRef are mutually exclusive; use oidcConfigRef to reference a shared MCPOIDCConfig"
41+
// +kubebuilder:validation:XValidation:rule="!(has(self.telemetry) && has(self.telemetryConfigRef))",message="telemetry and telemetryConfigRef are mutually exclusive; migrate to telemetryConfigRef"
4142
//
4243
//nolint:lll // CEL validation rules exceed line length limit
4344
type MCPRemoteProxySpec struct {
@@ -102,7 +103,17 @@ type MCPRemoteProxySpec struct {
102103
// +optional
103104
ToolConfigRef *ToolConfigRef `json:"toolConfigRef,omitempty"`
104105

105-
// Telemetry defines observability configuration for the proxy
106+
// TelemetryConfigRef references an MCPTelemetryConfig resource for shared telemetry configuration.
107+
// The referenced MCPTelemetryConfig must exist in the same namespace as this MCPRemoteProxy.
108+
// Cross-namespace references are not supported for security and isolation reasons.
109+
// Mutually exclusive with the deprecated inline Telemetry field.
110+
// +optional
111+
TelemetryConfigRef *MCPTelemetryConfigReference `json:"telemetryConfigRef,omitempty"`
112+
113+
// Telemetry defines inline observability configuration for the proxy.
114+
// Deprecated: Use TelemetryConfigRef to reference a shared MCPTelemetryConfig resource instead.
115+
// This field will be removed in a future release. Setting both telemetry and telemetryConfigRef
116+
// is rejected by CEL validation.
106117
// +optional
107118
Telemetry *TelemetryConfig `json:"telemetry,omitempty"`
108119

@@ -174,6 +185,10 @@ type MCPRemoteProxyStatus struct {
174185
// +optional
175186
ToolConfigHash string `json:"toolConfigHash,omitempty"`
176187

188+
// TelemetryConfigHash stores the hash of the referenced MCPTelemetryConfig for change detection
189+
// +optional
190+
TelemetryConfigHash string `json:"telemetryConfigHash,omitempty"`
191+
177192
// ExternalAuthConfigHash is the hash of the referenced MCPExternalAuthConfig spec
178193
// +optional
179194
ExternalAuthConfigHash string `json:"externalAuthConfigHash,omitempty"`
@@ -227,6 +242,9 @@ const (
227242
// ConditionTypeMCPRemoteProxyToolConfigValidated indicates whether the ToolConfigRef is valid
228243
ConditionTypeMCPRemoteProxyToolConfigValidated = "ToolConfigValidated"
229244

245+
// ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated indicates whether the TelemetryConfigRef is valid
246+
ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated = "TelemetryConfigRefValidated"
247+
230248
// ConditionTypeMCPRemoteProxyExternalAuthConfigValidated indicates whether the ExternalAuthConfigRef is valid
231249
ConditionTypeMCPRemoteProxyExternalAuthConfigValidated = "ExternalAuthConfigValidated"
232250

@@ -278,6 +296,18 @@ const (
278296
// ConditionReasonMCPRemoteProxyToolConfigFetchError indicates an error occurred fetching the MCPToolConfig
279297
ConditionReasonMCPRemoteProxyToolConfigFetchError = "ToolConfigFetchError"
280298

299+
// ConditionReasonMCPRemoteProxyTelemetryConfigRefValid indicates the TelemetryConfigRef is valid
300+
ConditionReasonMCPRemoteProxyTelemetryConfigRefValid = "TelemetryConfigRefValid"
301+
302+
// ConditionReasonMCPRemoteProxyTelemetryConfigRefNotFound indicates the referenced MCPTelemetryConfig was not found
303+
ConditionReasonMCPRemoteProxyTelemetryConfigRefNotFound = "TelemetryConfigRefNotFound"
304+
305+
// ConditionReasonMCPRemoteProxyTelemetryConfigRefInvalid indicates the referenced MCPTelemetryConfig is invalid
306+
ConditionReasonMCPRemoteProxyTelemetryConfigRefInvalid = "TelemetryConfigRefInvalid"
307+
308+
// ConditionReasonMCPRemoteProxyTelemetryConfigRefFetchError indicates an error occurred fetching the MCPTelemetryConfig
309+
ConditionReasonMCPRemoteProxyTelemetryConfigRefFetchError = "TelemetryConfigRefFetchError"
310+
281311
// ConditionReasonMCPRemoteProxyExternalAuthConfigValid indicates the ExternalAuthConfigRef is valid
282312
ConditionReasonMCPRemoteProxyExternalAuthConfigValid = "ExternalAuthConfigValid"
283313

‎cmd/thv-operator/api/v1alpha1/zz_generated.deepcopy.go‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎cmd/thv-operator/controllers/mcpremoteproxy_controller.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ type MCPRemoteProxyReconciler struct {
4848
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptoolconfigs,verbs=get;list;watch
4949
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs,verbs=get;list;watch
5050
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs,verbs=get;list;watch
51+
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcptelemetryconfigs,verbs=get;list;watch
5152
// +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpoidcconfigs/status,verbs=get;update;patch
5253
// +kubebuilder:rbac:groups="",resources=configmaps,verbs=create;delete;get;list;patch;update;watch
5354
// +kubebuilder:rbac:groups="",resources=services,verbs=create;delete;get;list;patch;update;watch
@@ -128,6 +129,16 @@ func (r *MCPRemoteProxyReconciler) validateAndHandleConfigs(ctx context.Context,
128129
return err
129130
}
130131

132+
// Handle MCPTelemetryConfig
133+
if err := r.handleTelemetryConfig(ctx, proxy); err != nil {
134+
ctxLogger.Error(err, "Failed to handle MCPTelemetryConfig")
135+
proxy.Status.Phase = mcpv1alpha1.MCPRemoteProxyPhaseFailed
136+
if statusErr := r.Status().Update(ctx, proxy); statusErr != nil {
137+
ctxLogger.Error(statusErr, "Failed to update MCPRemoteProxy status after MCPTelemetryConfig error")
138+
}
139+
return err
140+
}
141+
131142
// Handle MCPExternalAuthConfig
132143
if err := r.handleExternalAuthConfig(ctx, proxy); err != nil {
133144
ctxLogger.Error(err, "Failed to handle MCPExternalAuthConfig")
@@ -641,6 +652,97 @@ func (r *MCPRemoteProxyReconciler) handleToolConfig(ctx context.Context, proxy *
641652
return nil
642653
}
643654

655+
// handleTelemetryConfig validates and tracks the hash of the referenced MCPTelemetryConfig.
656+
// It updates the MCPRemoteProxy status when the telemetry configuration changes.
657+
func (r *MCPRemoteProxyReconciler) handleTelemetryConfig(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error {
658+
ctxLogger := log.FromContext(ctx)
659+
660+
if proxy.Spec.TelemetryConfigRef == nil {
661+
// No MCPTelemetryConfig referenced, clear any stored hash and condition.
662+
condType := mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated
663+
condRemoved := meta.FindStatusCondition(proxy.Status.Conditions, condType) != nil
664+
meta.RemoveStatusCondition(&proxy.Status.Conditions, condType)
665+
if condRemoved || proxy.Status.TelemetryConfigHash != "" {
666+
proxy.Status.TelemetryConfigHash = ""
667+
if err := r.Status().Update(ctx, proxy); err != nil {
668+
return fmt.Errorf("failed to clear MCPTelemetryConfig hash from status: %w", err)
669+
}
670+
}
671+
return nil
672+
}
673+
674+
// Get the referenced MCPTelemetryConfig
675+
telemetryConfig, err := ctrlutil.GetTelemetryConfigForMCPRemoteProxy(ctx, r.Client, proxy)
676+
if err != nil {
677+
// Transient API error (not a NotFound)
678+
meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{
679+
Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated,
680+
Status: metav1.ConditionFalse,
681+
Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyTelemetryConfigRefFetchError,
682+
Message: err.Error(),
683+
ObservedGeneration: proxy.Generation,
684+
})
685+
return err
686+
}
687+
688+
if telemetryConfig == nil {
689+
// Resource genuinely does not exist
690+
meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{
691+
Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated,
692+
Status: metav1.ConditionFalse,
693+
Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyTelemetryConfigRefNotFound,
694+
Message: fmt.Sprintf("MCPTelemetryConfig %s not found", proxy.Spec.TelemetryConfigRef.Name),
695+
ObservedGeneration: proxy.Generation,
696+
})
697+
return fmt.Errorf("MCPTelemetryConfig %s not found", proxy.Spec.TelemetryConfigRef.Name)
698+
}
699+
700+
// Validate that the MCPTelemetryConfig is valid (has Valid=True condition)
701+
if err := telemetryConfig.Validate(); err != nil {
702+
meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{
703+
Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated,
704+
Status: metav1.ConditionFalse,
705+
Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyTelemetryConfigRefInvalid,
706+
Message: fmt.Sprintf("MCPTelemetryConfig %s is invalid: %v", proxy.Spec.TelemetryConfigRef.Name, err),
707+
ObservedGeneration: proxy.Generation,
708+
})
709+
return fmt.Errorf("MCPTelemetryConfig %s is invalid: %w", proxy.Spec.TelemetryConfigRef.Name, err)
710+
}
711+
712+
// Detect whether the condition is transitioning to True (e.g. recovering from
713+
// a transient error). Without this check the status update is skipped when the
714+
// hash is unchanged, leaving a stale False condition.
715+
condType := mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated
716+
prevCondition := meta.FindStatusCondition(proxy.Status.Conditions, condType)
717+
needsUpdate := prevCondition == nil || prevCondition.Status != metav1.ConditionTrue
718+
719+
meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{
720+
Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyTelemetryConfigRefValidated,
721+
Status: metav1.ConditionTrue,
722+
Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyTelemetryConfigRefValid,
723+
Message: fmt.Sprintf("MCPTelemetryConfig %s is valid", proxy.Spec.TelemetryConfigRef.Name),
724+
ObservedGeneration: proxy.Generation,
725+
})
726+
727+
if proxy.Status.TelemetryConfigHash != telemetryConfig.Status.ConfigHash {
728+
ctxLogger.Info("MCPTelemetryConfig has changed, updating MCPRemoteProxy",
729+
"proxy", proxy.Name,
730+
"telemetryConfig", telemetryConfig.Name,
731+
"oldHash", proxy.Status.TelemetryConfigHash,
732+
"newHash", telemetryConfig.Status.ConfigHash)
733+
proxy.Status.TelemetryConfigHash = telemetryConfig.Status.ConfigHash
734+
needsUpdate = true
735+
}
736+
737+
if needsUpdate {
738+
if err := r.Status().Update(ctx, proxy); err != nil {
739+
return fmt.Errorf("failed to update MCPTelemetryConfig status: %w", err)
740+
}
741+
}
742+
743+
return nil
744+
}
745+
644746
// handleExternalAuthConfig validates and tracks the hash of the referenced MCPExternalAuthConfig
645747
func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error {
646748
ctxLogger := log.FromContext(ctx)
@@ -1390,6 +1492,37 @@ func (r *MCPRemoteProxyReconciler) mapOIDCConfigToMCPRemoteProxy(
13901492
return requests
13911493
}
13921494

1495+
// mapTelemetryConfigToMCPRemoteProxy maps MCPTelemetryConfig changes to MCPRemoteProxy reconciliation requests.
1496+
func (r *MCPRemoteProxyReconciler) mapTelemetryConfigToMCPRemoteProxy(
1497+
ctx context.Context, obj client.Object,
1498+
) []reconcile.Request {
1499+
telemetryConfig, ok := obj.(*mcpv1alpha1.MCPTelemetryConfig)
1500+
if !ok {
1501+
return nil
1502+
}
1503+
1504+
proxyList := &mcpv1alpha1.MCPRemoteProxyList{}
1505+
if err := r.List(ctx, proxyList, client.InNamespace(telemetryConfig.Namespace)); err != nil {
1506+
log.FromContext(ctx).Error(err, "Failed to list MCPRemoteProxies for MCPTelemetryConfig watch")
1507+
return nil
1508+
}
1509+
1510+
var requests []reconcile.Request
1511+
for _, proxy := range proxyList.Items {
1512+
if proxy.Spec.TelemetryConfigRef != nil &&
1513+
proxy.Spec.TelemetryConfigRef.Name == telemetryConfig.Name {
1514+
requests = append(requests, reconcile.Request{
1515+
NamespacedName: types.NamespacedName{
1516+
Name: proxy.Name,
1517+
Namespace: proxy.Namespace,
1518+
},
1519+
})
1520+
}
1521+
}
1522+
1523+
return requests
1524+
}
1525+
13931526
// SetupWithManager sets up the controller with the Manager
13941527
func (r *MCPRemoteProxyReconciler) SetupWithManager(mgr ctrl.Manager) error {
13951528
// Create a handler that maps MCPExternalAuthConfig changes to MCPRemoteProxy reconciliation requests
@@ -1470,5 +1603,9 @@ func (r *MCPRemoteProxyReconciler) SetupWithManager(mgr ctrl.Manager) error {
14701603
&mcpv1alpha1.MCPOIDCConfig{},
14711604
handler.EnqueueRequestsFromMapFunc(r.mapOIDCConfigToMCPRemoteProxy),
14721605
).
1606+
Watches(
1607+
&mcpv1alpha1.MCPTelemetryConfig{},
1608+
handler.EnqueueRequestsFromMapFunc(r.mapTelemetryConfigToMCPRemoteProxy),
1609+
).
14731610
Complete(r)
14741611
}

‎cmd/thv-operator/controllers/mcpremoteproxy_deployment.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ func (r *MCPRemoteProxyReconciler) deploymentForMCPRemoteProxy(
3030
// Build deployment components using helper functions
3131
args := r.buildContainerArgs()
3232
volumeMounts, volumes := r.buildVolumesForProxy(proxy)
33+
r.addTelemetryCABundleVolumes(ctx, proxy, &volumes, &volumeMounts)
3334
env := r.buildEnvVarsForProxy(ctx, proxy)
3435

3536
// Add embedded auth server volumes and env vars. AuthServerRef takes precedence;
@@ -143,6 +144,29 @@ func (*MCPRemoteProxyReconciler) buildVolumesForProxy(
143144
return volumeMounts, volumes
144145
}
145146

147+
// addTelemetryCABundleVolumes appends CA bundle volumes for the referenced MCPTelemetryConfig.
148+
// Must be called from deploymentForMCPRemoteProxy where the client is available.
149+
func (r *MCPRemoteProxyReconciler) addTelemetryCABundleVolumes(
150+
ctx context.Context,
151+
proxy *mcpv1alpha1.MCPRemoteProxy,
152+
volumes *[]corev1.Volume,
153+
volumeMounts *[]corev1.VolumeMount,
154+
) {
155+
if proxy.Spec.TelemetryConfigRef == nil {
156+
return
157+
}
158+
telCfg, err := ctrlutil.GetTelemetryConfigForMCPRemoteProxy(ctx, r.Client, proxy)
159+
if err != nil {
160+
log.FromContext(ctx).Error(err, "Failed to fetch MCPTelemetryConfig for CA bundle volume")
161+
return
162+
}
163+
if telCfg != nil {
164+
caVolumes, caMounts := ctrlutil.AddTelemetryCABundleVolumes(telCfg)
165+
*volumes = append(*volumes, caVolumes...)
166+
*volumeMounts = append(*volumeMounts, caMounts...)
167+
}
168+
}
169+
146170
// buildEnvVarsForProxy builds environment variables for the proxy container
147171
func (r *MCPRemoteProxyReconciler) buildEnvVarsForProxy(
148172
ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy,

0 commit comments

Comments
 (0)