diff --git a/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go b/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go index f28198ad63..91b52cce34 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go @@ -184,6 +184,12 @@ const ( // ConditionTypeMCPRemoteProxyGroupRefValidated indicates whether the GroupRef is valid ConditionTypeMCPRemoteProxyGroupRefValidated = "GroupRefValidated" + + // ConditionTypeMCPRemoteProxyToolConfigValidated indicates whether the ToolConfigRef is valid + ConditionTypeMCPRemoteProxyToolConfigValidated = "ToolConfigValidated" + + // ConditionTypeMCPRemoteProxyExternalAuthConfigValidated indicates whether the ExternalAuthConfigRef is valid + ConditionTypeMCPRemoteProxyExternalAuthConfigValidated = "ExternalAuthConfigValidated" ) // Condition reasons for MCPRemoteProxy @@ -217,6 +223,24 @@ const ( // ConditionReasonMCPRemoteProxyGroupRefNotReady indicates the referenced MCPGroup is not in the Ready state ConditionReasonMCPRemoteProxyGroupRefNotReady = "GroupRefNotReady" + + // ConditionReasonMCPRemoteProxyToolConfigValid indicates the ToolConfigRef is valid + ConditionReasonMCPRemoteProxyToolConfigValid = "ToolConfigValid" + + // ConditionReasonMCPRemoteProxyToolConfigNotFound indicates the referenced MCPToolConfig was not found + ConditionReasonMCPRemoteProxyToolConfigNotFound = "ToolConfigNotFound" + + // ConditionReasonMCPRemoteProxyToolConfigFetchError indicates an error occurred fetching the MCPToolConfig + ConditionReasonMCPRemoteProxyToolConfigFetchError = "ToolConfigFetchError" + + // ConditionReasonMCPRemoteProxyExternalAuthConfigValid indicates the ExternalAuthConfigRef is valid + ConditionReasonMCPRemoteProxyExternalAuthConfigValid = "ExternalAuthConfigValid" + + // ConditionReasonMCPRemoteProxyExternalAuthConfigNotFound indicates the referenced MCPExternalAuthConfig was not found + ConditionReasonMCPRemoteProxyExternalAuthConfigNotFound = "ExternalAuthConfigNotFound" + + // ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError indicates an error occurred fetching the MCPExternalAuthConfig + ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError = "ExternalAuthConfigFetchError" ) //+kubebuilder:object:root=true diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index 4c2553f37f..0d51b2754e 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -352,6 +352,8 @@ func (r *MCPRemoteProxyReconciler) validateSpec(ctx context.Context, proxy *mcpv func (r *MCPRemoteProxyReconciler) handleToolConfig(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error { ctxLogger := log.FromContext(ctx) if proxy.Spec.ToolConfigRef == nil { + // Remove condition if ToolConfigRef is not set + meta.RemoveStatusCondition(&proxy.Status.Conditions, mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated) if proxy.Status.ToolConfigHash != "" { proxy.Status.ToolConfigHash = "" if err := r.Status().Update(ctx, proxy); err != nil { @@ -363,12 +365,36 @@ func (r *MCPRemoteProxyReconciler) handleToolConfig(ctx context.Context, proxy * toolConfig, err := ctrlutil.GetToolConfigForMCPRemoteProxy(ctx, r.Client, proxy) if err != nil { - return err + if errors.IsNotFound(err) { + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigNotFound, + Message: fmt.Sprintf("MCPToolConfig '%s' not found in namespace '%s'", + proxy.Spec.ToolConfigRef.Name, proxy.Namespace), + ObservedGeneration: proxy.Generation, + }) + return fmt.Errorf("MCPToolConfig '%s' not found in namespace '%s'", + proxy.Spec.ToolConfigRef.Name, proxy.Namespace) + } + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigFetchError, + Message: "Failed to fetch MCPToolConfig", + ObservedGeneration: proxy.Generation, + }) + return fmt.Errorf("failed to fetch MCPToolConfig: %w", err) } - if toolConfig == nil { - return fmt.Errorf("MCPToolConfig %s not found", proxy.Spec.ToolConfigRef.Name) - } + // ToolConfig found and valid + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigValid, + Message: fmt.Sprintf("MCPToolConfig '%s' is valid", toolConfig.Name), + ObservedGeneration: proxy.Generation, + }) if proxy.Status.ToolConfigHash != toolConfig.Status.ConfigHash { ctxLogger.Info("MCPToolConfig has changed, updating MCPRemoteProxy", @@ -390,6 +416,8 @@ func (r *MCPRemoteProxyReconciler) handleToolConfig(ctx context.Context, proxy * func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error { ctxLogger := log.FromContext(ctx) if proxy.Spec.ExternalAuthConfigRef == nil { + // Remove condition if ExternalAuthConfigRef is not set + meta.RemoveStatusCondition(&proxy.Status.Conditions, mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) if proxy.Status.ExternalAuthConfigHash != "" { proxy.Status.ExternalAuthConfigHash = "" if err := r.Status().Update(ctx, proxy); err != nil { @@ -401,12 +429,36 @@ func (r *MCPRemoteProxyReconciler) handleExternalAuthConfig(ctx context.Context, externalAuthConfig, err := ctrlutil.GetExternalAuthConfigForMCPRemoteProxy(ctx, r.Client, proxy) if err != nil { - return err + if errors.IsNotFound(err) { + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigNotFound, + Message: fmt.Sprintf("MCPExternalAuthConfig '%s' not found in namespace '%s'", + proxy.Spec.ExternalAuthConfigRef.Name, proxy.Namespace), + ObservedGeneration: proxy.Generation, + }) + return fmt.Errorf("MCPExternalAuthConfig '%s' not found in namespace '%s'", + proxy.Spec.ExternalAuthConfigRef.Name, proxy.Namespace) + } + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError, + Message: "Failed to fetch MCPExternalAuthConfig", + ObservedGeneration: proxy.Generation, + }) + return fmt.Errorf("failed to fetch MCPExternalAuthConfig: %w", err) } - if externalAuthConfig == nil { - return fmt.Errorf("MCPExternalAuthConfig %s not found", proxy.Spec.ExternalAuthConfigRef.Name) - } + // ExternalAuthConfig found and valid + meta.SetStatusCondition(&proxy.Status.Conditions, metav1.Condition{ + Type: mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigValid, + Message: fmt.Sprintf("MCPExternalAuthConfig '%s' is valid", externalAuthConfig.Name), + ObservedGeneration: proxy.Generation, + }) if proxy.Status.ExternalAuthConfigHash != externalAuthConfig.Status.ConfigHash { ctxLogger.Info("MCPExternalAuthConfig has changed, updating MCPRemoteProxy", diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index df53deb0e2..7cd487b1e6 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -24,12 +24,14 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" @@ -278,11 +280,15 @@ func TestHandleToolConfig(t *testing.T) { t.Parallel() tests := []struct { - name string - proxy *mcpv1alpha1.MCPRemoteProxy - toolConfig *mcpv1alpha1.MCPToolConfig - expectError bool - errContains string + name string + proxy *mcpv1alpha1.MCPRemoteProxy + toolConfig *mcpv1alpha1.MCPToolConfig + interceptorFuncs *interceptor.Funcs + expectError bool + errContains string + expectCondition bool + expectedCondStatus metav1.ConditionStatus + expectedCondReason string }{ { name: "no tool config reference", @@ -295,7 +301,8 @@ func TestHandleToolConfig(t *testing.T) { RemoteURL: "https://mcp.example.com", }, }, - expectError: false, + expectError: false, + expectCondition: false, // Condition should be removed when no reference }, { name: "valid tool config reference", @@ -323,7 +330,10 @@ func TestHandleToolConfig(t *testing.T) { ConfigHash: "abc123", }, }, - expectError: false, + expectError: false, + expectCondition: true, + expectedCondStatus: metav1.ConditionTrue, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigValid, }, { name: "tool config hash update", @@ -354,7 +364,10 @@ func TestHandleToolConfig(t *testing.T) { ConfigHash: "new-hash", }, }, - expectError: false, + expectError: false, + expectCondition: true, + expectedCondStatus: metav1.ConditionTrue, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigValid, }, { name: "tool config reference removed", @@ -370,7 +383,8 @@ func TestHandleToolConfig(t *testing.T) { ToolConfigHash: "old-hash", }, }, - expectError: false, + expectError: false, + expectCondition: false, // Condition should be removed when reference is removed }, { name: "tool config not found", @@ -386,8 +400,39 @@ func TestHandleToolConfig(t *testing.T) { }, }, }, - expectError: true, - errContains: "failed to get MCPToolConfig", + expectError: true, + errContains: "not found in namespace", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigNotFound, + }, + { + name: "tool config fetch error", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "error-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ToolConfigRef: &mcpv1alpha1.ToolConfigRef{ + Name: "tool-config", + }, + }, + }, + interceptorFuncs: &interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*mcpv1alpha1.MCPToolConfig); ok { + return fmt.Errorf("simulated API server error") + } + return c.Get(ctx, key, obj, opts...) + }, + }, + expectError: true, + errContains: "failed to fetch MCPToolConfig", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyToolConfigFetchError, }, } @@ -401,11 +446,14 @@ func TestHandleToolConfig(t *testing.T) { objects = append(objects, tt.toolConfig) } - fakeClient := fake.NewClientBuilder(). + builder := fake.NewClientBuilder(). WithScheme(scheme). WithRuntimeObjects(objects...). - WithStatusSubresource(&mcpv1alpha1.MCPRemoteProxy{}). - Build() + WithStatusSubresource(&mcpv1alpha1.MCPRemoteProxy{}) + if tt.interceptorFuncs != nil { + builder = builder.WithInterceptorFuncs(*tt.interceptorFuncs) + } + fakeClient := builder.Build() reconciler := &MCPRemoteProxyReconciler{ Client: fakeClient, @@ -419,6 +467,19 @@ func TestHandleToolConfig(t *testing.T) { if tt.errContains != "" { assert.Contains(t, err.Error(), tt.errContains) } + + // Verify condition on in-memory object for error cases + if tt.expectCondition { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated) + assert.NotNil(t, cond, "ToolConfigValidated condition should be set") + if cond != nil { + assert.Equal(t, tt.expectedCondStatus, cond.Status, + "Condition status should match expected") + assert.Equal(t, tt.expectedCondReason, cond.Reason, + "Condition reason should match expected") + } + } } else { assert.NoError(t, err) @@ -439,6 +500,23 @@ func TestHandleToolConfig(t *testing.T) { assert.Empty(t, updatedProxy.Status.ToolConfigHash, "Status hash should be cleared when reference is removed") } + + // Verify condition (check in-memory object since conditions are set there) + if tt.expectCondition { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated) + assert.NotNil(t, cond, "ToolConfigValidated condition should be set") + if cond != nil { + assert.Equal(t, tt.expectedCondStatus, cond.Status, + "Condition status should match expected") + assert.Equal(t, tt.expectedCondReason, cond.Reason, + "Condition reason should match expected") + } + } else { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyToolConfigValidated) + assert.Nil(t, cond, "ToolConfigValidated condition should not be set when no reference") + } } }) } @@ -449,11 +527,15 @@ func TestHandleExternalAuthConfig(t *testing.T) { t.Parallel() tests := []struct { - name string - proxy *mcpv1alpha1.MCPRemoteProxy - externalAuth *mcpv1alpha1.MCPExternalAuthConfig - expectError bool - errContains string + name string + proxy *mcpv1alpha1.MCPRemoteProxy + externalAuth *mcpv1alpha1.MCPExternalAuthConfig + interceptorFuncs *interceptor.Funcs + expectError bool + errContains string + expectCondition bool + expectedCondStatus metav1.ConditionStatus + expectedCondReason string }{ { name: "no external auth reference", @@ -466,7 +548,8 @@ func TestHandleExternalAuthConfig(t *testing.T) { RemoteURL: "https://mcp.example.com", }, }, - expectError: false, + expectError: false, + expectCondition: false, // Condition should be removed when no reference }, { name: "valid external auth reference", @@ -503,7 +586,10 @@ func TestHandleExternalAuthConfig(t *testing.T) { ConfigHash: "xyz789", }, }, - expectError: false, + expectError: false, + expectCondition: true, + expectedCondStatus: metav1.ConditionTrue, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigValid, }, { name: "external auth config hash update", @@ -543,7 +629,10 @@ func TestHandleExternalAuthConfig(t *testing.T) { ConfigHash: "new-hash", }, }, - expectError: false, + expectError: false, + expectCondition: true, + expectedCondStatus: metav1.ConditionTrue, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigValid, }, { name: "external auth config reference removed", @@ -559,7 +648,8 @@ func TestHandleExternalAuthConfig(t *testing.T) { ExternalAuthConfigHash: "old-hash", }, }, - expectError: false, + expectError: false, + expectCondition: false, // Condition should be removed when reference is removed }, { name: "external auth config not found", @@ -575,8 +665,39 @@ func TestHandleExternalAuthConfig(t *testing.T) { }, }, }, - expectError: true, - errContains: "failed to get MCPExternalAuthConfig", + expectError: true, + errContains: "not found in namespace", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigNotFound, + }, + { + name: "external auth config fetch error", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "error-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "auth-config", + }, + }, + }, + interceptorFuncs: &interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*mcpv1alpha1.MCPExternalAuthConfig); ok { + return fmt.Errorf("simulated API server error") + } + return c.Get(ctx, key, obj, opts...) + }, + }, + expectError: true, + errContains: "failed to fetch MCPExternalAuthConfig", + expectCondition: true, + expectedCondStatus: metav1.ConditionFalse, + expectedCondReason: mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError, }, } @@ -590,11 +711,14 @@ func TestHandleExternalAuthConfig(t *testing.T) { objects = append(objects, tt.externalAuth) } - fakeClient := fake.NewClientBuilder(). + builder := fake.NewClientBuilder(). WithScheme(scheme). WithRuntimeObjects(objects...). - WithStatusSubresource(&mcpv1alpha1.MCPRemoteProxy{}). - Build() + WithStatusSubresource(&mcpv1alpha1.MCPRemoteProxy{}) + if tt.interceptorFuncs != nil { + builder = builder.WithInterceptorFuncs(*tt.interceptorFuncs) + } + fakeClient := builder.Build() reconciler := &MCPRemoteProxyReconciler{ Client: fakeClient, @@ -608,6 +732,19 @@ func TestHandleExternalAuthConfig(t *testing.T) { if tt.errContains != "" { assert.Contains(t, err.Error(), tt.errContains) } + + // Verify condition on in-memory object for error cases + if tt.expectCondition { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) + assert.NotNil(t, cond, "ExternalAuthConfigValidated condition should be set") + if cond != nil { + assert.Equal(t, tt.expectedCondStatus, cond.Status, + "Condition status should match expected") + assert.Equal(t, tt.expectedCondReason, cond.Reason, + "Condition reason should match expected") + } + } } else { assert.NoError(t, err) @@ -628,6 +765,23 @@ func TestHandleExternalAuthConfig(t *testing.T) { assert.Empty(t, updatedProxy.Status.ExternalAuthConfigHash, "Status hash should be cleared when reference is removed") } + + // Verify condition (check in-memory object since conditions are set there) + if tt.expectCondition { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) + assert.NotNil(t, cond, "ExternalAuthConfigValidated condition should be set") + if cond != nil { + assert.Equal(t, tt.expectedCondStatus, cond.Status, + "Condition status should match expected") + assert.Equal(t, tt.expectedCondReason, cond.Reason, + "Condition reason should match expected") + } + } else { + cond := meta.FindStatusCondition(tt.proxy.Status.Conditions, + mcpv1alpha1.ConditionTypeMCPRemoteProxyExternalAuthConfigValidated) + assert.Nil(t, cond, "ExternalAuthConfigValidated condition should not be set when no reference") + } } }) }