From 69c076371eb78617b4555f2fceada88d2dc7f649 Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 9 Jan 2026 10:15:06 +0100 Subject: [PATCH 1/5] remove docs --- .../virtualmcpserver_controller.go | 167 ++++++- .../virtualmcpserver_deployment.go | 13 +- .../virtualmcpserver_vmcpconfig.go | 24 +- .../virtualmcpserver_vmcpconfig_test.go | 163 ++++++ cmd/vmcp/app/commands.go | 30 +- pkg/vmcp/aggregator/discoverer.go | 54 ++ pkg/vmcp/config/config.go | 32 ++ pkg/vmcp/config/zz_generated.deepcopy.go | 29 ++ .../virtualmcp/virtualmcp_lifecycle_test.go | 464 ++++++++++++++++++ 9 files changed, 956 insertions(+), 20 deletions(-) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 9e1a194c2a..ffb8f4ab82 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -496,14 +496,28 @@ func (r *VirtualMCPServerReconciler) ensureAllResources( return nil } -// ensureRBACResources ensures that the RBAC resources are in place for the VirtualMCPServer +// ensureRBACResources ensures that the RBAC resources are in place for the VirtualMCPServer. +// RBAC resources are only created in dynamic mode (source: discovered) where vMCP needs K8s API access +// to discover backends at runtime. In static mode (source: inline), no RBAC is needed. func (r *VirtualMCPServerReconciler) ensureRBACResources( ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, ) error { + // Determine the outgoing auth source mode + source := OutgoingAuthSourceDiscovered // default + if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { + source = vmcp.Spec.OutgoingAuth.Source + } + + // Static mode (inline): No RBAC needed, cleanup any existing RBAC resources + if source == OutgoingAuthSourceInline { + return r.cleanupRBACResources(ctx, vmcp) + } + + // Dynamic mode (discovered): Ensure RBAC resources exist serviceAccountName := vmcpServiceAccountName(vmcp.Name) - // Ensure Role with minimal permissions + // Ensure Role with permissions to discover backends and update status if err := ctrlutil.EnsureRBACResource(ctx, r.Client, r.Scheme, vmcp, "Role", func() client.Object { return &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ @@ -551,6 +565,54 @@ func (r *VirtualMCPServerReconciler) ensureRBACResources( }) } +// cleanupRBACResources removes RBAC resources when switching from dynamic to static mode. +// This is needed when a VirtualMCPServer switches from "discovered" to "inline" source mode. +func (r *VirtualMCPServerReconciler) cleanupRBACResources( + ctx context.Context, + vmcp *mcpv1alpha1.VirtualMCPServer, +) error { + ctxLogger := log.FromContext(ctx) + serviceAccountName := vmcpServiceAccountName(vmcp.Name) + + // Delete RoleBinding + roleBinding := &rbacv1.RoleBinding{} + roleBindingKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} + if err := r.Get(ctx, roleBindingKey, roleBinding); err == nil { + ctxLogger.Info("Deleting RoleBinding for static mode", "name", serviceAccountName) + if err := r.Delete(ctx, roleBinding); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete RoleBinding: %w", err) + } + } else if !errors.IsNotFound(err) { + return fmt.Errorf("failed to get RoleBinding: %w", err) + } + + // Delete Role + role := &rbacv1.Role{} + roleKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} + if err := r.Get(ctx, roleKey, role); err == nil { + ctxLogger.Info("Deleting Role for static mode", "name", serviceAccountName) + if err := r.Delete(ctx, role); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete Role: %w", err) + } + } else if !errors.IsNotFound(err) { + return fmt.Errorf("failed to get Role: %w", err) + } + + // Delete ServiceAccount + serviceAccount := &corev1.ServiceAccount{} + serviceAccountKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} + if err := r.Get(ctx, serviceAccountKey, serviceAccount); err == nil { + ctxLogger.Info("Deleting ServiceAccount for static mode", "name", serviceAccountName) + if err := r.Delete(ctx, serviceAccount); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete ServiceAccount: %w", err) + } + } else if !errors.IsNotFound(err) { + return fmt.Errorf("failed to get ServiceAccount: %w", err) + } + + return nil +} + // getVmcpConfigChecksum fetches the vmcp Config ConfigMap checksum annotation. // This is used to trigger deployment rollouts when the configuration changes. // @@ -1249,6 +1311,26 @@ func vmcpServiceAccountName(vmcpName string) string { return fmt.Sprintf("%s-vmcp", vmcpName) } +// getServiceAccountNameForVmcp returns the service account name for a VirtualMCPServer +// based on its outgoing auth source mode. +// - Dynamic mode (discovered): Returns the dedicated service account name +// - Static mode (inline): Returns empty string (uses default service account) +func (*VirtualMCPServerReconciler) getServiceAccountNameForVmcp(vmcp *mcpv1alpha1.VirtualMCPServer) string { + // Determine the outgoing auth source mode + source := OutgoingAuthSourceDiscovered // default + if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { + source = vmcp.Spec.OutgoingAuth.Source + } + + // Static mode: Use default service account (no RBAC resources) + if source == OutgoingAuthSourceInline { + return "" + } + + // Dynamic mode: Use dedicated service account with K8s API permissions + return vmcpServiceAccountName(vmcp.Name) +} + // vmcpServiceName generates the service name for a VirtualMCPServer // Uses "vmcp-" prefix to distinguish from MCPServer's "mcp-{name}-proxy" pattern. // This allows VirtualMCPServer and MCPServer to coexist with the same base name. @@ -1510,6 +1592,87 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( return outgoing, nil } +// buildStaticBackends builds a list of StaticBackendConfig for static mode by discovering +// backend URLs and transport types from workloads in the MCPGroup. +// This allows vMCP to operate without K8s API access by embedding backend information in ConfigMap. +func (r *VirtualMCPServerReconciler) buildStaticBackends( + ctx context.Context, + vmcp *mcpv1alpha1.VirtualMCPServer, + typedWorkloads []workloads.TypedWorkload, +) ([]vmcpconfig.StaticBackendConfig, error) { + ctxLogger := log.FromContext(ctx) + + // Build maps of MCPServers and MCPRemoteProxies for efficient lookup + mcpServerMap, err := r.listMCPServersAsMap(ctx, vmcp.Namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPServers: %w", err) + } + + mcpRemoteProxyMap, err := r.listMCPRemoteProxiesAsMap(ctx, vmcp.Namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPRemoteProxies: %w", err) + } + + staticBackends := make([]vmcpconfig.StaticBackendConfig, 0, len(typedWorkloads)) + + for _, workload := range typedWorkloads { + var url, transport string + + switch workload.Type { + case workloads.WorkloadTypeMCPServer: + mcpServer, found := mcpServerMap[workload.Name] + if !found { + ctxLogger.V(1).Info("MCPServer not found in map, skipping", + "backend", workload.Name) + continue + } + + // Build URL from service + serviceName := mcpServer.Name + namespace := mcpServer.Namespace + port := mcpServer.Spec.ProxyPort + url = fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", serviceName, namespace, port) + + // Get transport type + transport = string(mcpServer.Spec.Transport) + + case workloads.WorkloadTypeMCPRemoteProxy: + mcpRemoteProxy, found := mcpRemoteProxyMap[workload.Name] + if !found { + ctxLogger.V(1).Info("MCPRemoteProxy not found in map, skipping", + "backend", workload.Name) + continue + } + + // Use the remote URL + url = mcpRemoteProxy.Spec.RemoteURL + + // Get transport type + transport = string(mcpRemoteProxy.Spec.Transport) + + default: + ctxLogger.V(1).Info("Unknown workload type, skipping", + "backend", workload.Name, + "type", workload.Type) + continue + } + + staticBackend := vmcpconfig.StaticBackendConfig{ + Name: workload.Name, + URL: url, + Transport: transport, + } + + staticBackends = append(staticBackends, staticBackend) + ctxLogger.V(1).Info("Built static backend config", + "name", workload.Name, + "url", url, + "transport", transport) + } + + return staticBackends, nil +} + // discoverBackends discovers all MCPServers in the referenced MCPGroup and returns // a list of DiscoveredBackend objects with their current status. // This reuses the existing workload discovery code from pkg/vmcp/workloads. diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index a159440e1f..1c79576c89 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -50,7 +50,8 @@ const ( vmcpReadinessFailures = int32(3) // consecutive failures before removing from service ) -// RBAC rules for VirtualMCPServer service account +// RBAC rules for VirtualMCPServer service account in dynamic mode +// These rules allow vMCP to discover backends and configurations at runtime var vmcpRBACRules = []rbacv1.PolicyRule{ { APIGroups: []string{""}, @@ -59,9 +60,14 @@ var vmcpRBACRules = []rbacv1.PolicyRule{ }, { APIGroups: []string{"toolhive.stacklok.dev"}, - Resources: []string{"mcpgroups", "mcpservers", "mcpremoteproxies", "mcpexternalauthconfigs"}, + Resources: []string{"mcpgroups", "mcpservers", "mcpremoteproxies", "mcpexternalauthconfigs", "mcptoolconfigs"}, Verbs: []string{"get", "list", "watch"}, }, + { + APIGroups: []string{"toolhive.stacklok.dev"}, + Resources: []string{"virtualmcpservers/status"}, + Verbs: []string{"update", "patch"}, + }, } // deploymentForVirtualMCPServer returns a VirtualMCPServer Deployment object @@ -81,6 +87,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(ls, vmcp) deploymentTemplateLabels, deploymentTemplateAnnotations := r.buildPodTemplateMetadata(ls, vmcp, vmcpConfigChecksum) podSecurityContext, containerSecurityContext := r.buildSecurityContextsForVmcp(ctx, vmcp) + serviceAccountName := r.getServiceAccountNameForVmcp(vmcp) dep := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -100,7 +107,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( Annotations: deploymentTemplateAnnotations, }, Spec: corev1.PodSpec{ - ServiceAccountName: vmcpServiceAccountName(vmcp.Name), + ServiceAccountName: serviceAccountName, Containers: []corev1.Container{{ Image: getVmcpImage(), ImagePullPolicy: corev1.PullIfNotPresent, diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go index 485e6db1a9..8d25be6d0f 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go @@ -41,18 +41,32 @@ func (r *VirtualMCPServerReconciler) ensureVmcpConfigConfigMap( return fmt.Errorf("failed to create vmcp Config from VirtualMCPServer: %w", err) } - // For dynamic mode (source: "discovered"), preserve the Source field so the vMCP pod - // can start BackendWatcher for runtime backend discovery. - // For inline mode, discover backends at reconcile time and include in ConfigMap. - if config.OutgoingAuth != nil && config.OutgoingAuth.Source != "discovered" { + // Only include backends in ConfigMap for static mode (source: inline) + // In dynamic mode (source: discovered), vMCP discovers backends at runtime via K8s API + if config.OutgoingAuth != nil && config.OutgoingAuth.Source == "inline" { + // Build OutgoingAuthConfig with full backend details for static mode discoveredAuthConfig, err := r.buildOutgoingAuthConfig(ctx, vmcp, typedWorkloads) if err != nil { - ctxLogger.V(1).Info("Failed to build discovered auth config, using spec-only config", + ctxLogger.V(1).Info("Failed to build auth config for inline mode, using spec-only config", "error", err) } else if discoveredAuthConfig != nil { + // Merge discovered config into the config + // The discovered config already includes inline overrides, so we can replace it config.OutgoingAuth = discoveredAuthConfig } + + // Build static backend configurations with URLs and transport types + // This allows vMCP to operate without K8s API access in static mode + staticBackends, err := r.buildStaticBackends(ctx, vmcp, typedWorkloads) + if err != nil { + ctxLogger.V(1).Info("Failed to build static backends, using empty list", + "error", err) + } else { + config.Backends = staticBackends + } } + // For discovered mode, keep the minimal OutgoingAuthConfig (source, defaults, overrides only) + // vMCP will discover backends and their auth configs at runtime using K8s API // Validate the vmcp Config before creating the ConfigMap validator := vmcpconfig.NewValidator() diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index a0cf000658..08c27f2f90 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -969,3 +969,166 @@ func TestVirtualMCPServerReconciler_CompositeToolRefs_NotFound(t *testing.T) { require.Error(t, err, "should fail when referenced tool doesn't exist") assert.Contains(t, err.Error(), "not found", "error should mention not found") } + +// TestConfigMapContent_DynamicMode tests that in dynamic mode (discovered), +// the ConfigMap contains minimal content without backends +func TestConfigMapContent_DynamicMode(t *testing.T) { + t.Parallel() + + ctx := context.Background() + testScheme := createRunConfigTestScheme() + + // Create MCPGroup for workload discovery + mcpGroup := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPGroupSpec{}, + Status: mcpv1alpha1.MCPGroupStatus{ + Phase: mcpv1alpha1.MCPGroupPhaseReady, + }, + } + + // Create VirtualMCPServer in dynamic mode (source: discovered) + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmcp", + Namespace: "default", + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: "test-group"}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Dynamic mode + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(vmcpServer, mcpGroup). + Build() + + reconciler := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: testScheme, + } + + // Discover workloads + workloadDiscoverer := workloads.NewK8SDiscovererWithClient(fakeClient, vmcpServer.Namespace) + workloadNames, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcpServer.Spec.Config.Group) + require.NoError(t, err) + + // Create ConfigMap + err = reconciler.ensureVmcpConfigConfigMap(ctx, vmcpServer, workloadNames) + require.NoError(t, err) + + // Verify ConfigMap was created + configMap := &corev1.ConfigMap{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: vmcpConfigMapName("test-vmcp"), + Namespace: "default", + }, configMap) + require.NoError(t, err) + + // Parse the YAML config + var config vmcpconfig.Config + err = yaml.Unmarshal([]byte(configMap.Data["config.yaml"]), &config) + require.NoError(t, err) + + // In dynamic mode, ConfigMap should have minimal content: + // - OutgoingAuth with source: discovered + // - No backends in OutgoingAuth (vMCP discovers at runtime) + require.NotNil(t, config.OutgoingAuth) + assert.Equal(t, "discovered", config.OutgoingAuth.Source, "source should be discovered") + assert.Empty(t, config.OutgoingAuth.Backends, "backends should be empty in dynamic mode") + + t.Log("✅ Dynamic mode ConfigMap contains minimal content without backends") +} + +// TestConfigMapContent_StaticMode tests that in static mode (inline), +// the ConfigMap contains full backend details +func TestConfigMapContent_StaticMode(t *testing.T) { + t.Parallel() + + ctx := context.Background() + testScheme := createRunConfigTestScheme() + + // Create MCPGroup for workload discovery + mcpGroup := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPGroupSpec{}, + Status: mcpv1alpha1.MCPGroupStatus{ + Phase: mcpv1alpha1.MCPGroupPhaseReady, + }, + } + + // Create VirtualMCPServer in static mode (source: inline) + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmcp", + Namespace: "default", + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: "test-group"}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "inline", // Static mode + Backends: map[string]mcpv1alpha1.BackendAuthConfig{ + "test-backend": { + Type: mcpv1alpha1.BackendAuthTypeDiscovered, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(vmcpServer, mcpGroup). + Build() + + reconciler := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: testScheme, + } + + // Discover workloads + workloadDiscoverer := workloads.NewK8SDiscovererWithClient(fakeClient, vmcpServer.Namespace) + workloadNames, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcpServer.Spec.Config.Group) + require.NoError(t, err) + + // Create ConfigMap + err = reconciler.ensureVmcpConfigConfigMap(ctx, vmcpServer, workloadNames) + require.NoError(t, err) + + // Verify ConfigMap was created + configMap := &corev1.ConfigMap{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: vmcpConfigMapName("test-vmcp"), + Namespace: "default", + }, configMap) + require.NoError(t, err) + + // Parse the YAML config + var config vmcpconfig.Config + err = yaml.Unmarshal([]byte(configMap.Data["config.yaml"]), &config) + require.NoError(t, err) + + // In static mode, ConfigMap should have full backend details: + // - OutgoingAuth with source: inline + // - Backends with auth configs + require.NotNil(t, config.OutgoingAuth) + assert.Equal(t, "inline", config.OutgoingAuth.Source, "source should be inline") + assert.NotEmpty(t, config.OutgoingAuth.Backends, "backends should be present in static mode") + + t.Log("✅ Static mode ConfigMap contains full backend details") +} diff --git a/cmd/vmcp/app/commands.go b/cmd/vmcp/app/commands.go index ca6060bcab..cd05918181 100644 --- a/cmd/vmcp/app/commands.go +++ b/cmd/vmcp/app/commands.go @@ -228,17 +228,27 @@ func discoverBackends(ctx context.Context, cfg *config.Config) ([]vmcp.Backend, return nil, nil, fmt.Errorf("failed to create backend client: %w", err) } - // Initialize managers for backend discovery - logger.Info("Initializing group manager") - groupsManager, err := groups.NewManager() - if err != nil { - return nil, nil, fmt.Errorf("failed to create groups manager: %w", err) - } + // Create backend discoverer based on configuration mode + var discoverer aggregator.BackendDiscoverer + if len(cfg.Backends) > 0 { + // Static mode: Use pre-configured backends from config (no K8s API access needed) + logger.Infof("Static mode: using %d pre-configured backends", len(cfg.Backends)) + discoverer = aggregator.NewUnifiedBackendDiscovererWithStaticBackends( + cfg.Backends, + cfg.OutgoingAuth, + ) + } else { + // Dynamic mode: Discover backends at runtime from K8s API + logger.Info("Dynamic mode: initializing group manager for backend discovery") + groupsManager, err := groups.NewManager() + if err != nil { + return nil, nil, fmt.Errorf("failed to create groups manager: %w", err) + } - // Create backend discoverer based on runtime environment - discoverer, err := aggregator.NewBackendDiscoverer(ctx, groupsManager, cfg.OutgoingAuth) - if err != nil { - return nil, nil, fmt.Errorf("failed to create backend discoverer: %w", err) + discoverer, err = aggregator.NewBackendDiscoverer(ctx, groupsManager, cfg.OutgoingAuth) + if err != nil { + return nil, nil, fmt.Errorf("failed to create backend discoverer: %w", err) + } } logger.Infof("Discovering backends in group: %s", cfg.Group) diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 012416f05c..79bcb660bc 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -27,6 +27,7 @@ type backendDiscoverer struct { workloadsManager workloads.Discoverer groupsManager groups.Manager authConfig *config.OutgoingAuthConfig + staticBackends []config.StaticBackendConfig // Pre-configured backends for static mode } // NewUnifiedBackendDiscoverer creates a unified backend discoverer that works with both @@ -43,6 +44,21 @@ func NewUnifiedBackendDiscoverer( workloadsManager: workloadsManager, groupsManager: groupsManager, authConfig: authConfig, + staticBackends: nil, // Dynamic mode - discover backends at runtime + } +} + +// NewUnifiedBackendDiscovererWithStaticBackends creates a backend discoverer for static mode +// with pre-configured backends, eliminating the need for K8s API access. +func NewUnifiedBackendDiscovererWithStaticBackends( + staticBackends []config.StaticBackendConfig, + authConfig *config.OutgoingAuthConfig, +) BackendDiscoverer { + return &backendDiscoverer{ + workloadsManager: nil, // Not needed in static mode + groupsManager: nil, // Not needed in static mode + authConfig: authConfig, + staticBackends: staticBackends, } } @@ -95,9 +111,21 @@ func NewBackendDiscovererWithManager( // Discover finds all backend workloads in the specified group. // Returns all accessible backends with their health status marked based on workload status. // The groupRef is the group name (e.g., "engineering-team"). +// +// In static mode (when staticBackends are configured), this returns pre-configured backends +// without any K8s API access. In dynamic mode, it discovers backends at runtime. func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) ([]vmcp.Backend, error) { logger.Infof("Discovering backends in group %s", groupRef) + // Static mode: Use pre-configured backends if available + if len(d.staticBackends) > 0 { + logger.Infof("Using %d pre-configured static backends (no K8s API access)", len(d.staticBackends)) + return d.discoverFromStaticConfig() + } + + // Dynamic mode: Discover backends from K8s API at runtime + logger.Infof("Dynamic mode: discovering backends from K8s API") + // Verify that the group exists exists, err := d.groupsManager.Exists(ctx, groupRef) if err != nil { @@ -202,3 +230,29 @@ func (d *backendDiscoverer) applyAuthConfigToBackend(backend *vmcp.Backend, back } } } + +// discoverFromStaticConfig converts pre-configured static backends into vmcp.Backend objects +// for use in static mode where no K8s API access is available. +func (d *backendDiscoverer) discoverFromStaticConfig() ([]vmcp.Backend, error) { + backends := make([]vmcp.Backend, 0, len(d.staticBackends)) + + for _, staticBackend := range d.staticBackends { + backend := vmcp.Backend{ + ID: staticBackend.Name, + Name: staticBackend.Name, + BaseURL: staticBackend.URL, + TransportType: staticBackend.Transport, + HealthStatus: vmcp.BackendHealthy, // Assume healthy, actual health check happens later + Metadata: staticBackend.Metadata, + } + + // Apply auth configuration from OutgoingAuthConfig + d.applyAuthConfigToBackend(&backend, staticBackend.Name) + + backends = append(backends, backend) + logger.Infof("Loaded static backend: %s (url=%s, transport=%s)", + staticBackend.Name, staticBackend.URL, staticBackend.Transport) + } + + return backends, nil +} diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index a6b45d8daa..d1865ba22f 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -80,6 +80,14 @@ type Config struct { // +kubebuilder:validation:Required Group string `json:"groupRef" yaml:"groupRef"` + // Backends defines pre-configured backend servers for static mode. + // When OutgoingAuth.Source is "inline", this field contains the full list of backend + // servers with their URLs and transport types, eliminating the need for K8s API access. + // When OutgoingAuth.Source is "discovered", this field is empty and backends are + // discovered at runtime via Kubernetes API. + // +optional + Backends []StaticBackendConfig `json:"backends,omitempty" yaml:"backends,omitempty"` + // IncomingAuth configures how clients authenticate to the virtual MCP server. // When using the Kubernetes operator, this is populated by the converter from // VirtualMCPServerSpec.IncomingAuth and any values set here will be superseded. @@ -196,6 +204,30 @@ type AuthzConfig struct { Policies []string `json:"policies,omitempty" yaml:"policies,omitempty"` } +// StaticBackendConfig defines a pre-configured backend server for static mode. +// This allows vMCP to operate without Kubernetes API access by embedding all backend +// information directly in the configuration. +// +kubebuilder:object:generate=true +type StaticBackendConfig struct { + // Name is the backend identifier. + // Must match the backend name from the MCPGroup for auth config resolution. + // +kubebuilder:validation:Required + Name string `json:"name" yaml:"name"` + + // URL is the backend's MCP server base URL. + // +kubebuilder:validation:Required + URL string `json:"url" yaml:"url"` + + // Transport is the MCP transport protocol: "stdio", "http", "sse", "streamable-http" + // +kubebuilder:validation:Enum=stdio;http;sse;streamable-http + // +kubebuilder:validation:Required + Transport string `json:"transport" yaml:"transport"` + + // Metadata stores additional backend information. + // +optional + Metadata map[string]string `json:"metadata,omitempty" yaml:"metadata,omitempty"` +} + // OutgoingAuthConfig configures backend authentication. // // Note: When using the Kubernetes operator (VirtualMCPServer CRD), the diff --git a/pkg/vmcp/config/zz_generated.deepcopy.go b/pkg/vmcp/config/zz_generated.deepcopy.go index 97b75415dd..b6857bc40b 100644 --- a/pkg/vmcp/config/zz_generated.deepcopy.go +++ b/pkg/vmcp/config/zz_generated.deepcopy.go @@ -138,6 +138,13 @@ func (in *CompositeToolRef) DeepCopy() *CompositeToolRef { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Config) DeepCopyInto(out *Config) { *out = *in + if in.Backends != nil { + in, out := &in.Backends, &out.Backends + *out = make([]StaticBackendConfig, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.IncomingAuth != nil { in, out := &in.IncomingAuth, &out.IncomingAuth *out = new(IncomingAuthConfig) @@ -410,6 +417,28 @@ func (in *OutputProperty) DeepCopy() *OutputProperty { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *StaticBackendConfig) DeepCopyInto(out *StaticBackendConfig) { + *out = *in + if in.Metadata != nil { + in, out := &in.Metadata, &out.Metadata + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new StaticBackendConfig. +func (in *StaticBackendConfig) DeepCopy() *StaticBackendConfig { + if in == nil { + return nil + } + out := new(StaticBackendConfig) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *StepErrorHandling) DeepCopyInto(out *StepErrorHandling) { *out = *in diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go index 47a6300e6d..c4dc69aa21 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go @@ -13,7 +13,9 @@ import ( "github.com/mark3labs/mcp-go/mcp" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -1243,3 +1245,465 @@ var _ = Describe("VirtualMCPServer K8s Manager Infrastructure", Ordered, func() }) }) }) + +// VirtualMCPServer Dynamic vs Static Mode Tests +// These tests verify the operator behavior for different outgoingAuth.source modes: +// - discovered (dynamic): vMCP discovers backends at runtime via K8s API, requires RBAC +// - inline (static): All backends are pre-configured in ConfigMap, no RBAC needed +var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { + var ( + testNamespace = "default" + mcpGroupName = "test-mode-config-group" + backend1Name = "backend-mode-fetch" + timeout = 3 * time.Minute + pollingInterval = 2 * time.Second + ) + + BeforeAll(func() { + By("Creating MCPGroup for mode configuration tests") + CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, + "Test MCP Group for mode configuration E2E tests", timeout, pollingInterval) + + By("Creating backend MCPServer") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backend1Name, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + }, + } + Expect(k8sClient.Create(ctx, backend)).To(Succeed()) + + By("Waiting for backend MCPServer to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backend1Name, + Namespace: testNamespace, + }, server) + if err != nil { + return fmt.Errorf("failed to get server: %w", err) + } + + if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { + return nil + } + return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) + }, timeout, pollingInterval).Should(Succeed(), "Backend should be ready") + }) + + AfterAll(func() { + By("Cleaning up backend MCPServer") + backend := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backend1Name, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, backend) + + By("Cleaning up MCPGroup") + group := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: mcpGroupName, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, group) + }) + + Context("Dynamic Mode (discovered)", func() { + var vmcpServerName = "test-vmcp-dynamic-mode" + + AfterEach(func() { + By("Cleaning up VirtualMCPServer") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, vmcpServer) + + By("Waiting for VirtualMCPServer deletion") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + return err != nil + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("should create RBAC resources in dynamic mode", func() { + By("Creating VirtualMCPServer with discovered source (dynamic mode)") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: mcpGroupName}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Dynamic mode - should create RBAC + }, + ServiceType: "ClusterIP", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + + serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) + + By("Verifying ServiceAccount was created") + sa := &corev1.ServiceAccount{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist in dynamic mode") + + By("Verifying Role was created with K8s API permissions") + role := &rbacv1.Role{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, role) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "Role should exist in dynamic mode") + + // Verify Role has correct permissions + Expect(role.Rules).NotTo(BeEmpty()) + + // Check for ConfigMap and Secret permissions + hasConfigMapPerms := false + hasToolHivePerms := false + hasStatusPerms := false + + for _, rule := range role.Rules { + // Check for ConfigMap/Secret read permissions + if len(rule.APIGroups) > 0 && rule.APIGroups[0] == "" { + for _, resource := range rule.Resources { + if resource == "configmaps" || resource == "secrets" { + hasConfigMapPerms = true + } + } + } + + // Check for ToolHive resource read permissions + if len(rule.APIGroups) > 0 && rule.APIGroups[0] == "toolhive.stacklok.dev" { + for _, resource := range rule.Resources { + if resource == "mcpgroups" || resource == "mcpservers" { + hasToolHivePerms = true + } + if resource == "virtualmcpservers/status" { + hasStatusPerms = true + } + } + } + } + + Expect(hasConfigMapPerms).To(BeTrue(), "Role should have ConfigMap/Secret read permissions") + Expect(hasToolHivePerms).To(BeTrue(), "Role should have ToolHive resource read permissions") + Expect(hasStatusPerms).To(BeTrue(), "Role should have status update permissions") + + By("Verifying RoleBinding was created") + rb := &rbacv1.RoleBinding{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, rb) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "RoleBinding should exist in dynamic mode") + + // Verify RoleBinding references correct ServiceAccount and Role + Expect(rb.RoleRef.Name).To(Equal(serviceAccountName)) + Expect(rb.RoleRef.Kind).To(Equal("Role")) + Expect(rb.Subjects).To(HaveLen(1)) + Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount")) + Expect(rb.Subjects[0].Name).To(Equal(serviceAccountName)) + + By("Verifying Deployment uses the ServiceAccount") + deployment := &appsv1.Deployment{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, deployment) + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + Expect(deployment.Spec.Template.Spec.ServiceAccountName).To(Equal(serviceAccountName), + "Deployment should use the created ServiceAccount in dynamic mode") + }) + + It("should have minimal ConfigMap in dynamic mode", func() { + By("Creating VirtualMCPServer with discovered source (dynamic mode)") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName + "-configmap", + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: mcpGroupName}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Dynamic mode + }, + ServiceType: "ClusterIP", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + vmcpServerName = vmcpServerName + "-configmap" // Update for cleanup + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + + By("Verifying ConfigMap contains minimal content") + configMapName := fmt.Sprintf("%s-vmcp-config", vmcpServerName) + configMap := &corev1.ConfigMap{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: configMapName, + Namespace: testNamespace, + }, configMap) + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + Expect(configMap.Data).To(HaveKey("config.yaml")) + configYAML := configMap.Data["config.yaml"] + + // In dynamic mode, ConfigMap should NOT contain backend URLs/details + // vMCP discovers these at runtime via K8s API + Expect(configYAML).To(ContainSubstring("source: discovered")) + Expect(configYAML).NotTo(ContainSubstring("url: http://"), + "Dynamic mode ConfigMap should not contain backend URLs") + }) + }) + + Context("Static Mode (inline)", func() { + var vmcpServerName = "test-vmcp-static-mode" + + AfterEach(func() { + By("Cleaning up VirtualMCPServer") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, vmcpServer) + + By("Waiting for VirtualMCPServer deletion") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + return err != nil + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("should NOT create RBAC resources in static mode", func() { + By("Creating VirtualMCPServer with inline source (static mode)") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: mcpGroupName}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "inline", // Static mode - should NOT create RBAC + }, + ServiceType: "ClusterIP", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + + serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) + + By("Verifying ServiceAccount was NOT created") + sa := &corev1.ServiceAccount{} + Consistently(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + return err != nil // Should not exist + }, 10*time.Second, 2*time.Second).Should(BeTrue(), "ServiceAccount should not exist in static mode") + + By("Verifying Role was NOT created") + role := &rbacv1.Role{} + Consistently(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, role) + return err != nil // Should not exist + }, 10*time.Second, 2*time.Second).Should(BeTrue(), "Role should not exist in static mode") + + By("Verifying RoleBinding was NOT created") + rb := &rbacv1.RoleBinding{} + Consistently(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, rb) + return err != nil // Should not exist + }, 10*time.Second, 2*time.Second).Should(BeTrue(), "RoleBinding should not exist in static mode") + + By("Verifying Deployment uses default ServiceAccount") + deployment := &appsv1.Deployment{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, deployment) + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + // In static mode, ServiceAccountName should be empty (uses default) + Expect(deployment.Spec.Template.Spec.ServiceAccountName).To(BeEmpty(), + "Deployment should use default ServiceAccount in static mode") + }) + }) + + Context("Mode Switching", func() { + var vmcpServerName = "test-vmcp-mode-switch" + + AfterEach(func() { + By("Cleaning up VirtualMCPServer") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, vmcpServer) + + By("Waiting for all resources to be cleaned up") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + return err != nil + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("should clean up RBAC when switching from dynamic to static", func() { + By("Creating VirtualMCPServer in dynamic mode") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: mcpGroupName}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Start in dynamic mode + }, + ServiceType: "ClusterIP", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready with RBAC") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + + serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) + + By("Verifying RBAC resources exist in dynamic mode") + sa := &corev1.ServiceAccount{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist before mode switch") + + By("Switching to static mode") + Eventually(func() error { + // Get latest version + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + if err != nil { + return err + } + + // Update to static mode + vmcpServer.Spec.OutgoingAuth.Source = "inline" + return k8sClient.Update(ctx, vmcpServer) + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + By("Waiting for operator to reconcile and clean up RBAC") + // Wait for RBAC resources to be deleted + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + return err != nil // Should not exist after mode switch + }, 2*time.Minute, 5*time.Second).Should(BeTrue(), "ServiceAccount should be deleted after switching to static mode") + + By("Verifying Role was also deleted") + role := &rbacv1.Role{} + Consistently(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, role) + return err != nil + }, 10*time.Second, 2*time.Second).Should(BeTrue(), "Role should not exist after mode switch") + + By("Verifying RoleBinding was also deleted") + rb := &rbacv1.RoleBinding{} + Consistently(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, rb) + return err != nil + }, 10*time.Second, 2*time.Second).Should(BeTrue(), "RoleBinding should not exist after mode switch") + + By("Verifying VirtualMCPServer is still ready after mode switch") + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + if err != nil { + return err + } + + if vmcpServer.Status.Phase != mcpv1alpha1.VirtualMCPServerPhaseReady { + return fmt.Errorf("VirtualMCPServer not ready, phase: %s", vmcpServer.Status.Phase) + } + return nil + }, 2*time.Minute, 5*time.Second).Should(Succeed(), "VirtualMCPServer should remain ready after mode switch") + }) + }) +}) From a72e7f69d572810b7245a15045859f069a3f0293 Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 9 Jan 2026 10:57:41 +0100 Subject: [PATCH 2/5] fixes from review --- .../virtualmcpserver_controller.go | 9 +- .../virtualmcpserver_vmcpconfig_test.go | 131 +++++++++++++++++- .../virtualmcp/virtualmcp_lifecycle_test.go | 22 ++- 3 files changed, 147 insertions(+), 15 deletions(-) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index ffb8f4ab82..bac0dc4483 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -1573,10 +1573,11 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( outgoing.Default = defaultStrategy } - // Discover ExternalAuthConfig from MCPServers if source is "discovered" - if source == OutgoingAuthSourceDiscovered { - r.discoverExternalAuthConfigs(ctx, vmcp, typedWorkloads, outgoing) - } + // Discover ExternalAuthConfig from MCPServers to populate backend auth configs. + // This function is called from ensureVmcpConfigConfigMap only for inline/static mode, + // where we need full backend details in the ConfigMap. For discovered/dynamic mode, + // this function is not called, keeping the ConfigMap minimal. + r.discoverExternalAuthConfigs(ctx, vmcp, typedWorkloads, outgoing) // Apply inline overrides (works for all source modes) if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Backends != nil { diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index 08c27f2f90..a5e486a28b 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -1049,9 +1049,11 @@ func TestConfigMapContent_DynamicMode(t *testing.T) { t.Log("✅ Dynamic mode ConfigMap contains minimal content without backends") } -// TestConfigMapContent_StaticMode tests that in static mode (inline), -// the ConfigMap contains full backend details -func TestConfigMapContent_StaticMode(t *testing.T) { +// TestConfigMapContent_StaticMode_InlineOverrides tests that in static mode (inline), +// explicitly specified backends in the spec are preserved in the ConfigMap. +// This tests inline overrides, not discovery. See TestConfigMapContent_StaticModeWithDiscovery +// for testing actual backend discovery from MCPServers in the group. +func TestConfigMapContent_StaticMode_InlineOverrides(t *testing.T) { t.Parallel() ctx := context.Background() @@ -1123,12 +1125,127 @@ func TestConfigMapContent_StaticMode(t *testing.T) { err = yaml.Unmarshal([]byte(configMap.Data["config.yaml"]), &config) require.NoError(t, err) - // In static mode, ConfigMap should have full backend details: + // In static mode with inline backends, ConfigMap should preserve them: // - OutgoingAuth with source: inline - // - Backends with auth configs + // - Backends from spec.outgoingAuth.backends are included require.NotNil(t, config.OutgoingAuth) assert.Equal(t, "inline", config.OutgoingAuth.Source, "source should be inline") - assert.NotEmpty(t, config.OutgoingAuth.Backends, "backends should be present in static mode") + require.NotEmpty(t, config.OutgoingAuth.Backends, "backends should be present in static mode") - t.Log("✅ Static mode ConfigMap contains full backend details") + // Verify the inline backend from spec is present + _, exists := config.OutgoingAuth.Backends["test-backend"] + assert.True(t, exists, "inline backend from spec should be present in ConfigMap") + + t.Log("✅ Static mode ConfigMap preserves inline backend overrides from spec") +} + +// TestConfigMapContent_StaticModeWithDiscovery tests that in static mode (inline), +// the ConfigMap contains discovered backend auth configs from MCPServer ExternalAuthConfigRefs +func TestConfigMapContent_StaticModeWithDiscovery(t *testing.T) { + t.Parallel() + + ctx := context.Background() + testScheme := createRunConfigTestScheme() + + // Create MCPGroup for workload discovery + mcpGroup := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-group", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPGroupSpec{}, + Status: mcpv1alpha1.MCPGroupStatus{ + Phase: mcpv1alpha1.MCPGroupPhaseReady, + }, + } + + // Create MCPExternalAuthConfig that will be referenced by MCPServer + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-auth-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeUnauthenticated, + }, + } + + // Create MCPServer with ExternalAuthConfigRef + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "discovered-backend", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: "test-group", + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: "test-auth-config", + }, + }, + } + + // Create VirtualMCPServer in static mode (source: inline) WITHOUT inline backends + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmcp", + Namespace: "default", + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: "test-group"}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "inline", // Static mode - should discover backends + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(vmcpServer, mcpGroup, mcpServer, externalAuthConfig). + Build() + + reconciler := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: testScheme, + } + + // Discover workloads + workloadDiscoverer := workloads.NewK8SDiscovererWithClient(fakeClient, vmcpServer.Namespace) + workloadNames, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcpServer.Spec.Config.Group) + require.NoError(t, err) + require.NotEmpty(t, workloadNames, "should have discovered the MCPServer") + + // Create ConfigMap + err = reconciler.ensureVmcpConfigConfigMap(ctx, vmcpServer, workloadNames) + require.NoError(t, err) + + // Verify ConfigMap was created + configMap := &corev1.ConfigMap{} + err = fakeClient.Get(ctx, types.NamespacedName{ + Name: vmcpConfigMapName("test-vmcp"), + Namespace: "default", + }, configMap) + require.NoError(t, err) + + // Parse the YAML config + var config vmcpconfig.Config + err = yaml.Unmarshal([]byte(configMap.Data["config.yaml"]), &config) + require.NoError(t, err) + + // In static mode with discovery, ConfigMap should have: + // - OutgoingAuth with source: inline + // - Backends populated from discovered MCPServer ExternalAuthConfigRefs + require.NotNil(t, config.OutgoingAuth) + assert.Equal(t, "inline", config.OutgoingAuth.Source, "source should be inline") + require.NotEmpty(t, config.OutgoingAuth.Backends, "backends should be discovered in static mode") + + // Verify the discovered backend is present + discoveredBackend, exists := config.OutgoingAuth.Backends["discovered-backend"] + require.True(t, exists, "discovered backend should be present in ConfigMap") + require.NotNil(t, discoveredBackend, "discovered backend should have auth strategy") + assert.Equal(t, "unauthenticated", discoveredBackend.Type, "backend should have correct auth type") + + t.Log("✅ Static mode ConfigMap contains discovered backend auth configs") } diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go index c4dc69aa21..5c4f0a0939 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go @@ -1450,9 +1450,11 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { It("should have minimal ConfigMap in dynamic mode", func() { By("Creating VirtualMCPServer with discovered source (dynamic mode)") + // Use local variable to avoid modifying Context-level vmcpServerName + localVmcpName := vmcpServerName + "-configmap" vmcpServer := &mcpv1alpha1.VirtualMCPServer{ ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName + "-configmap", + Name: localVmcpName, Namespace: testNamespace, }, Spec: mcpv1alpha1.VirtualMCPServerSpec{ @@ -1467,13 +1469,25 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, } Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - vmcpServerName = vmcpServerName + "-configmap" // Update for cleanup + + // Add explicit cleanup for this test's resource with the modified name + DeferCleanup(func() { + By("Cleaning up VirtualMCPServer with modified name") + _ = k8sClient.Delete(ctx, vmcpServer) + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: localVmcpName, + Namespace: testNamespace, + }, vmcpServer) + return err != nil + }, timeout, pollingInterval).Should(BeTrue()) + }) By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + WaitForVirtualMCPServerReady(ctx, k8sClient, localVmcpName, testNamespace, timeout, pollingInterval) By("Verifying ConfigMap contains minimal content") - configMapName := fmt.Sprintf("%s-vmcp-config", vmcpServerName) + configMapName := fmt.Sprintf("%s-vmcp-config", localVmcpName) configMap := &corev1.ConfigMap{} Eventually(func() error { return k8sClient.Get(ctx, types.NamespacedName{ From 8480fd229b6b7cb8dc767d927ea621a825ab20e1 Mon Sep 17 00:00:00 2001 From: taskbot Date: Fri, 9 Jan 2026 11:52:34 +0100 Subject: [PATCH 3/5] simplify code and fixes from review --- .../virtualmcpserver_controller.go | 198 +++++------------- .../virtualmcpserver_deployment.go | 2 +- .../virtualmcpserver_vmcpconfig.go | 142 ++++++++++--- .../virtualmcpserver_vmcpconfig_test.go | 139 ++++++++++-- cmd/vmcp/app/commands.go | 1 + deploy/charts/operator-crds/Chart.yaml | 2 +- deploy/charts/operator-crds/README.md | 2 +- ...olhive.stacklok.dev_virtualmcpservers.yaml | 44 ++++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 44 ++++ docs/operator/crd-api.md | 22 ++ pkg/vmcp/aggregator/discoverer.go | 14 ++ pkg/vmcp/aggregator/discoverer_test.go | 113 ++++++++++ pkg/vmcp/config/config.go | 24 ++- pkg/vmcp/discovery/middleware_test.go | 48 ++++- pkg/vmcp/workloads/k8s.go | 3 + .../virtualmcp/virtualmcp_lifecycle_test.go | 152 ++++++++++++-- test/integration/vmcp/helpers/helpers_test.go | 76 +++++++ 17 files changed, 814 insertions(+), 212 deletions(-) create mode 100644 test/integration/vmcp/helpers/helpers_test.go diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index bac0dc4483..6595e520cc 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -496,22 +496,20 @@ func (r *VirtualMCPServerReconciler) ensureAllResources( return nil } -// ensureRBACResources ensures that the RBAC resources are in place for the VirtualMCPServer. -// RBAC resources are only created in dynamic mode (source: discovered) where vMCP needs K8s API access -// to discover backends at runtime. In static mode (source: inline), no RBAC is needed. +// ensureRBACResources ensures RBAC resources for VirtualMCPServer in dynamic mode. +// In static mode, RBAC creation is skipped. Existing RBAC resources (if any) remain until +// the VirtualMCPServer is deleted - they will be garbage collected via owner references. func (r *VirtualMCPServerReconciler) ensureRBACResources( ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, ) error { // Determine the outgoing auth source mode - source := OutgoingAuthSourceDiscovered // default - if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { - source = vmcp.Spec.OutgoingAuth.Source - } + source := outgoingAuthSource(vmcp) - // Static mode (inline): No RBAC needed, cleanup any existing RBAC resources + // Static mode (inline): Skip RBAC creation + // Owner references ensure garbage collection when VirtualMCPServer is deleted if source == OutgoingAuthSourceInline { - return r.cleanupRBACResources(ctx, vmcp) + return nil } // Dynamic mode (discovered): Ensure RBAC resources exist @@ -565,54 +563,6 @@ func (r *VirtualMCPServerReconciler) ensureRBACResources( }) } -// cleanupRBACResources removes RBAC resources when switching from dynamic to static mode. -// This is needed when a VirtualMCPServer switches from "discovered" to "inline" source mode. -func (r *VirtualMCPServerReconciler) cleanupRBACResources( - ctx context.Context, - vmcp *mcpv1alpha1.VirtualMCPServer, -) error { - ctxLogger := log.FromContext(ctx) - serviceAccountName := vmcpServiceAccountName(vmcp.Name) - - // Delete RoleBinding - roleBinding := &rbacv1.RoleBinding{} - roleBindingKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} - if err := r.Get(ctx, roleBindingKey, roleBinding); err == nil { - ctxLogger.Info("Deleting RoleBinding for static mode", "name", serviceAccountName) - if err := r.Delete(ctx, roleBinding); err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete RoleBinding: %w", err) - } - } else if !errors.IsNotFound(err) { - return fmt.Errorf("failed to get RoleBinding: %w", err) - } - - // Delete Role - role := &rbacv1.Role{} - roleKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} - if err := r.Get(ctx, roleKey, role); err == nil { - ctxLogger.Info("Deleting Role for static mode", "name", serviceAccountName) - if err := r.Delete(ctx, role); err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete Role: %w", err) - } - } else if !errors.IsNotFound(err) { - return fmt.Errorf("failed to get Role: %w", err) - } - - // Delete ServiceAccount - serviceAccount := &corev1.ServiceAccount{} - serviceAccountKey := types.NamespacedName{Name: serviceAccountName, Namespace: vmcp.Namespace} - if err := r.Get(ctx, serviceAccountKey, serviceAccount); err == nil { - ctxLogger.Info("Deleting ServiceAccount for static mode", "name", serviceAccountName) - if err := r.Delete(ctx, serviceAccount); err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete ServiceAccount: %w", err) - } - } else if !errors.IsNotFound(err) { - return fmt.Errorf("failed to get ServiceAccount: %w", err) - } - - return nil -} - // getVmcpConfigChecksum fetches the vmcp Config ConfigMap checksum annotation. // This is used to trigger deployment rollouts when the configuration changes. // @@ -910,13 +860,9 @@ func (r *VirtualMCPServerReconciler) containerNeedsUpdate( } // Check if service account has changed - expectedServiceAccountName := vmcpServiceAccountName(vmcp.Name) + expectedServiceAccountName := r.serviceAccountNameForVmcp(vmcp) currentServiceAccountName := deployment.Spec.Template.Spec.ServiceAccountName - if currentServiceAccountName != "" && currentServiceAccountName != expectedServiceAccountName { - return true - } - - return false + return currentServiceAccountName != expectedServiceAccountName } // deploymentMetadataNeedsUpdate checks if deployment-level metadata has changed @@ -1311,16 +1257,21 @@ func vmcpServiceAccountName(vmcpName string) string { return fmt.Sprintf("%s-vmcp", vmcpName) } -// getServiceAccountNameForVmcp returns the service account name for a VirtualMCPServer +// outgoingAuthSource returns the outgoing auth source mode with default fallback. +// Returns OutgoingAuthSourceDiscovered if not specified. +func outgoingAuthSource(vmcp *mcpv1alpha1.VirtualMCPServer) string { + if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { + return vmcp.Spec.OutgoingAuth.Source + } + return OutgoingAuthSourceDiscovered +} + +// serviceAccountNameForVmcp returns the service account name for a VirtualMCPServer // based on its outgoing auth source mode. // - Dynamic mode (discovered): Returns the dedicated service account name // - Static mode (inline): Returns empty string (uses default service account) -func (*VirtualMCPServerReconciler) getServiceAccountNameForVmcp(vmcp *mcpv1alpha1.VirtualMCPServer) string { - // Determine the outgoing auth source mode - source := OutgoingAuthSourceDiscovered // default - if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { - source = vmcp.Spec.OutgoingAuth.Source - } +func (*VirtualMCPServerReconciler) serviceAccountNameForVmcp(vmcp *mcpv1alpha1.VirtualMCPServer) string { + source := outgoingAuthSource(vmcp) // Static mode: Use default service account (no RBAC resources) if source == OutgoingAuthSourceInline { @@ -1554,10 +1505,7 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( typedWorkloads []workloads.TypedWorkload, ) (*vmcpconfig.OutgoingAuthConfig, error) { // Determine source - default to "discovered" if not specified - source := OutgoingAuthSourceDiscovered - if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Source != "" { - source = vmcp.Spec.OutgoingAuth.Source - } + source := outgoingAuthSource(vmcp) outgoing := &vmcpconfig.OutgoingAuthConfig{ Source: source, @@ -1593,90 +1541,42 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig( return outgoing, nil } -// buildStaticBackends builds a list of StaticBackendConfig for static mode by discovering -// backend URLs and transport types from workloads in the MCPGroup. -// This allows vMCP to operate without K8s API access by embedding backend information in ConfigMap. -func (r *VirtualMCPServerReconciler) buildStaticBackends( +// convertBackendsToStaticBackends converts Backend objects to StaticBackendConfig for ConfigMap embedding. +// Preserves metadata and uses transport types from workload Specs. +// Logs warnings when backends are skipped due to missing URL or transport information. +func convertBackendsToStaticBackends( ctx context.Context, - vmcp *mcpv1alpha1.VirtualMCPServer, - typedWorkloads []workloads.TypedWorkload, -) ([]vmcpconfig.StaticBackendConfig, error) { - ctxLogger := log.FromContext(ctx) - - // Build maps of MCPServers and MCPRemoteProxies for efficient lookup - mcpServerMap, err := r.listMCPServersAsMap(ctx, vmcp.Namespace) - if err != nil { - return nil, fmt.Errorf("failed to list MCPServers: %w", err) - } - - mcpRemoteProxyMap, err := r.listMCPRemoteProxiesAsMap(ctx, vmcp.Namespace) - if err != nil { - return nil, fmt.Errorf("failed to list MCPRemoteProxies: %w", err) - } - - staticBackends := make([]vmcpconfig.StaticBackendConfig, 0, len(typedWorkloads)) - - for _, workload := range typedWorkloads { - var url, transport string - - switch workload.Type { - case workloads.WorkloadTypeMCPServer: - mcpServer, found := mcpServerMap[workload.Name] - if !found { - ctxLogger.V(1).Info("MCPServer not found in map, skipping", - "backend", workload.Name) - continue - } - - // Build URL from service - serviceName := mcpServer.Name - namespace := mcpServer.Namespace - port := mcpServer.Spec.ProxyPort - url = fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", serviceName, namespace, port) - - // Get transport type - transport = string(mcpServer.Spec.Transport) - - case workloads.WorkloadTypeMCPRemoteProxy: - mcpRemoteProxy, found := mcpRemoteProxyMap[workload.Name] - if !found { - ctxLogger.V(1).Info("MCPRemoteProxy not found in map, skipping", - "backend", workload.Name) - continue - } - - // Use the remote URL - url = mcpRemoteProxy.Spec.RemoteURL - - // Get transport type - transport = string(mcpRemoteProxy.Spec.Transport) - - default: - ctxLogger.V(1).Info("Unknown workload type, skipping", - "backend", workload.Name, - "type", workload.Type) + backends []vmcptypes.Backend, + transportMap map[string]string, +) []vmcpconfig.StaticBackendConfig { + logger := log.FromContext(ctx) + static := make([]vmcpconfig.StaticBackendConfig, 0, len(backends)) + for _, backend := range backends { + if backend.BaseURL == "" { + logger.V(1).Info("Skipping backend without URL in static mode", + "backend", backend.Name) continue } - staticBackend := vmcpconfig.StaticBackendConfig{ - Name: workload.Name, - URL: url, - Transport: transport, + transport := transportMap[backend.Name] + if transport == "" { + logger.V(1).Info("Skipping backend without transport information in static mode", + "backend", backend.Name) + continue } - staticBackends = append(staticBackends, staticBackend) - ctxLogger.V(1).Info("Built static backend config", - "name", workload.Name, - "url", url, - "transport", transport) + static = append(static, vmcpconfig.StaticBackendConfig{ + Name: backend.Name, + URL: backend.BaseURL, + Transport: transport, + Metadata: backend.Metadata, + }) } - - return staticBackends, nil + return static } // discoverBackends discovers all MCPServers in the referenced MCPGroup and returns // a list of DiscoveredBackend objects with their current status. -// This reuses the existing workload discovery code from pkg/vmcp/workloads. // //nolint:gocyclo func (r *VirtualMCPServerReconciler) discoverBackends( @@ -1685,13 +1585,9 @@ func (r *VirtualMCPServerReconciler) discoverBackends( ) ([]mcpv1alpha1.DiscoveredBackend, error) { ctxLogger := log.FromContext(ctx) - // Create groups manager using the controller's client and VirtualMCPServer's namespace groupsManager := groups.NewCRDManager(r.Client, vmcp.Namespace) - - // Create K8S workload discoverer for the VirtualMCPServer's namespace workloadDiscoverer := workloads.NewK8SDiscovererWithClient(r.Client, vmcp.Namespace) - // Get all workloads in the group typedWorkloads, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcp.Spec.Config.Group) if err != nil { return nil, fmt.Errorf("failed to list workloads in group: %w", err) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index 1c79576c89..148ffe77c9 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -87,7 +87,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( deploymentLabels, deploymentAnnotations := r.buildDeploymentMetadataForVmcp(ls, vmcp) deploymentTemplateLabels, deploymentTemplateAnnotations := r.buildPodTemplateMetadata(ls, vmcp, vmcpConfigChecksum) podSecurityContext, containerSecurityContext := r.buildSecurityContextsForVmcp(ctx, vmcp) - serviceAccountName := r.getServiceAccountNameForVmcp(vmcp) + serviceAccountName := r.serviceAccountNameForVmcp(vmcp) dep := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go index 8d25be6d0f..13567fde5c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go @@ -7,13 +7,16 @@ import ( "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/kubernetes/configmaps" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/oidc" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" - "github.com/stacklok/toolhive/cmd/thv-operator/pkg/vmcpconfig" + operatorvmcpconfig "github.com/stacklok/toolhive/cmd/thv-operator/pkg/vmcpconfig" + "github.com/stacklok/toolhive/pkg/groups" + vmcptypes "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/aggregator" + vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" "github.com/stacklok/toolhive/pkg/vmcp/workloads" ) @@ -25,14 +28,9 @@ func (r *VirtualMCPServerReconciler) ensureVmcpConfigConfigMap( vmcp *mcpv1alpha1.VirtualMCPServer, typedWorkloads []workloads.TypedWorkload, ) error { - ctxLogger := log.FromContext(ctx) - - // Create OIDC resolver to handle all OIDC types (kubernetes, configMap, inline) + // Create OIDC resolver and converter for CRD-to-config transformation oidcResolver := oidc.NewResolver(r.Client) - - // Convert CRD to vmcp config using converter with OIDC resolver and Kubernetes client - // The client is needed to fetch referenced VirtualMCPCompositeToolDefinition resources - converter, err := vmcpconfig.NewConverter(oidcResolver, r.Client) + converter, err := operatorvmcpconfig.NewConverter(oidcResolver, r.Client) if err != nil { return fmt.Errorf("failed to create vmcp converter: %w", err) } @@ -41,35 +39,45 @@ func (r *VirtualMCPServerReconciler) ensureVmcpConfigConfigMap( return fmt.Errorf("failed to create vmcp Config from VirtualMCPServer: %w", err) } - // Only include backends in ConfigMap for static mode (source: inline) - // In dynamic mode (source: discovered), vMCP discovers backends at runtime via K8s API + // Static mode (inline): Embed full backend details in ConfigMap. + // Dynamic mode (discovered): vMCP discovers backends at runtime via K8s API. if config.OutgoingAuth != nil && config.OutgoingAuth.Source == "inline" { - // Build OutgoingAuthConfig with full backend details for static mode + // Build auth config with backend details discoveredAuthConfig, err := r.buildOutgoingAuthConfig(ctx, vmcp, typedWorkloads) if err != nil { - ctxLogger.V(1).Info("Failed to build auth config for inline mode, using spec-only config", - "error", err) - } else if discoveredAuthConfig != nil { - // Merge discovered config into the config - // The discovered config already includes inline overrides, so we can replace it + return fmt.Errorf("failed to build auth config for static mode: %w", err) + } + if discoveredAuthConfig != nil { config.OutgoingAuth = discoveredAuthConfig } - // Build static backend configurations with URLs and transport types - // This allows vMCP to operate without K8s API access in static mode - staticBackends, err := r.buildStaticBackends(ctx, vmcp, typedWorkloads) + // Discover backends with metadata + backends, err := r.discoverBackendsWithMetadata(ctx, vmcp) + if err != nil { + return fmt.Errorf("failed to discover backends for static mode: %w", err) + } + + // Get transport types from workload specs + transportMap, err := r.buildTransportMap(ctx, vmcp.Namespace, typedWorkloads) if err != nil { - ctxLogger.V(1).Info("Failed to build static backends, using empty list", - "error", err) - } else { - config.Backends = staticBackends + return fmt.Errorf("failed to build transport map for static mode: %w", err) + } + + config.Backends = convertBackendsToStaticBackends(ctx, backends, transportMap) + + // Validate at least one backend exists + if len(config.Backends) == 0 { + return fmt.Errorf( + "static mode requires at least one backend with valid transport (%v), "+ + "but none were discovered in group %s", + vmcpconfig.StaticModeAllowedTransports, + config.Group, + ) } } - // For discovered mode, keep the minimal OutgoingAuthConfig (source, defaults, overrides only) - // vMCP will discover backends and their auth configs at runtime using K8s API // Validate the vmcp Config before creating the ConfigMap - validator := vmcpconfig.NewValidator() + validator := operatorvmcpconfig.NewValidator() if err := validator.Validate(ctx, config); err != nil { return fmt.Errorf("invalid vmcp Config: %w", err) } @@ -118,3 +126,83 @@ func labelsForVmcpConfig(vmcpName string) map[string]string { "toolhive.stacklok.io/managed-by": "toolhive-operator", } } + +// discoverBackendsWithMetadata discovers backends and returns full Backend objects with metadata. +// Used in static mode for ConfigMap generation to preserve backend metadata. +func (r *VirtualMCPServerReconciler) discoverBackendsWithMetadata( + ctx context.Context, + vmcp *mcpv1alpha1.VirtualMCPServer, +) ([]vmcptypes.Backend, error) { + groupsManager := groups.NewCRDManager(r.Client, vmcp.Namespace) + workloadDiscoverer := workloads.NewK8SDiscovererWithClient(r.Client, vmcp.Namespace) + + // Build auth config if OutgoingAuth is configured + var authConfig *vmcpconfig.OutgoingAuthConfig + if vmcp.Spec.OutgoingAuth != nil { + typedWorkloads, err := workloadDiscoverer.ListWorkloadsInGroup(ctx, vmcp.Spec.Config.Group) + if err != nil { + return nil, fmt.Errorf("failed to list workloads in group: %w", err) + } + + authConfig, err = r.buildOutgoingAuthConfig(ctx, vmcp, typedWorkloads) + if err != nil { + authConfig = nil // Continue without auth config on error + } + } + + backendDiscoverer := aggregator.NewUnifiedBackendDiscoverer(workloadDiscoverer, groupsManager, authConfig) + backends, err := backendDiscoverer.Discover(ctx, vmcp.Spec.Config.Group) + if err != nil { + return nil, fmt.Errorf("failed to discover backends: %w", err) + } + + return backends, nil +} + +// buildTransportMap builds a map of backend names to transport types from workload Specs. +// Used in static mode to populate transport field in ConfigMap. +func (r *VirtualMCPServerReconciler) buildTransportMap( + ctx context.Context, + namespace string, + typedWorkloads []workloads.TypedWorkload, +) (map[string]string, error) { + transportMap := make(map[string]string, len(typedWorkloads)) + + mcpServerMap, err := r.listMCPServersAsMap(ctx, namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPServers: %w", err) + } + + mcpRemoteProxyMap, err := r.listMCPRemoteProxiesAsMap(ctx, namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPRemoteProxies: %w", err) + } + + for _, workload := range typedWorkloads { + var transport string + + switch workload.Type { + case workloads.WorkloadTypeMCPServer: + if mcpServer, found := mcpServerMap[workload.Name]; found { + // Read effective transport (ProxyMode takes precedence over Transport) + // For stdio servers, ProxyMode indicates how they're proxied (sse or streamable-http) + if mcpServer.Spec.ProxyMode != "" { + transport = string(mcpServer.Spec.ProxyMode) + } else { + transport = string(mcpServer.Spec.Transport) + } + } + + case workloads.WorkloadTypeMCPRemoteProxy: + if mcpRemoteProxy, found := mcpRemoteProxyMap[workload.Name]; found { + transport = string(mcpRemoteProxy.Spec.Transport) + } + } + + if transport != "" { + transportMap[workload.Name] = transport + } + } + + return transportMap, nil +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go index a5e486a28b..8a0b378806 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig_test.go @@ -665,7 +665,7 @@ func TestYAMLMarshalingDeterminism(t *testing.T) { assert.NotEmpty(t, firstResult) assert.Greater(t, len(firstResult), 100, "YAML output should contain substantial content") - t.Logf("✅ All %d marshaling iterations produced identical output (%d bytes)", + t.Logf("All %d marshaling iterations produced identical output (%d bytes)", iterations, len(results[0])) } @@ -1041,12 +1041,14 @@ func TestConfigMapContent_DynamicMode(t *testing.T) { // In dynamic mode, ConfigMap should have minimal content: // - OutgoingAuth with source: discovered - // - No backends in OutgoingAuth (vMCP discovers at runtime) + // - No auth backends in OutgoingAuth (vMCP discovers at runtime) + // - No static backends in Backends (vMCP discovers at runtime) require.NotNil(t, config.OutgoingAuth) assert.Equal(t, "discovered", config.OutgoingAuth.Source, "source should be discovered") - assert.Empty(t, config.OutgoingAuth.Backends, "backends should be empty in dynamic mode") + assert.Empty(t, config.OutgoingAuth.Backends, "auth backends should be empty in dynamic mode") + assert.Empty(t, config.Backends, "static backends should be empty in dynamic mode") - t.Log("✅ Dynamic mode ConfigMap contains minimal content without backends") + t.Log("Dynamic mode ConfigMap contains minimal content without backends") } // TestConfigMapContent_StaticMode_InlineOverrides tests that in static mode (inline), @@ -1071,6 +1073,23 @@ func TestConfigMapContent_StaticMode_InlineOverrides(t *testing.T) { }, } + // Create MCPServer in the group so static mode has something to discover + // This is needed because static mode validates that at least one backend exists + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backend", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: "test-group", + Transport: "sse", // Required for backend discovery + }, + Status: mcpv1alpha1.MCPServerStatus{ + Phase: mcpv1alpha1.MCPServerPhaseRunning, + URL: "http://test-backend.default.svc.cluster.local:8080", + }, + } + // Create VirtualMCPServer in static mode (source: inline) vmcpServer := &mcpv1alpha1.VirtualMCPServer{ ObjectMeta: metav1.ObjectMeta{ @@ -1095,7 +1114,8 @@ func TestConfigMapContent_StaticMode_InlineOverrides(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithScheme(testScheme). - WithObjects(vmcpServer, mcpGroup). + WithObjects(vmcpServer, mcpGroup, mcpServer). + WithStatusSubresource(mcpServer). Build() reconciler := &VirtualMCPServerReconciler{ @@ -1136,7 +1156,7 @@ func TestConfigMapContent_StaticMode_InlineOverrides(t *testing.T) { _, exists := config.OutgoingAuth.Backends["test-backend"] assert.True(t, exists, "inline backend from spec should be present in ConfigMap") - t.Log("✅ Static mode ConfigMap preserves inline backend overrides from spec") + t.Log("Static mode ConfigMap preserves inline backend overrides from spec") } // TestConfigMapContent_StaticModeWithDiscovery tests that in static mode (inline), @@ -1170,18 +1190,23 @@ func TestConfigMapContent_StaticModeWithDiscovery(t *testing.T) { }, } - // Create MCPServer with ExternalAuthConfigRef + // Create MCPServer with ExternalAuthConfigRef and Status mcpServer := &mcpv1alpha1.MCPServer{ ObjectMeta: metav1.ObjectMeta{ Name: "discovered-backend", Namespace: "default", }, Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: "test-group", + GroupRef: "test-group", + Transport: "sse", // Required for static mode backend discovery ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ Name: "test-auth-config", }, }, + Status: mcpv1alpha1.MCPServerStatus{ + Phase: mcpv1alpha1.MCPServerPhaseRunning, + URL: "http://discovered-backend.default.svc.cluster.local:8080", + }, } // Create VirtualMCPServer in static mode (source: inline) WITHOUT inline backends @@ -1204,6 +1229,7 @@ func TestConfigMapContent_StaticModeWithDiscovery(t *testing.T) { fakeClient := fake.NewClientBuilder(). WithScheme(testScheme). WithObjects(vmcpServer, mcpGroup, mcpServer, externalAuthConfig). + WithStatusSubresource(mcpServer). Build() reconciler := &VirtualMCPServerReconciler{ @@ -1235,17 +1261,106 @@ func TestConfigMapContent_StaticModeWithDiscovery(t *testing.T) { require.NoError(t, err) // In static mode with discovery, ConfigMap should have: - // - OutgoingAuth with source: inline - // - Backends populated from discovered MCPServer ExternalAuthConfigRefs + // - OutgoingAuth with source: inline and auth configs + // - Backends populated with URLs and transport types for zero-K8s-access mode require.NotNil(t, config.OutgoingAuth) assert.Equal(t, "inline", config.OutgoingAuth.Source, "source should be inline") require.NotEmpty(t, config.OutgoingAuth.Backends, "backends should be discovered in static mode") - // Verify the discovered backend is present + // Verify the discovered backend auth config is present discoveredBackend, exists := config.OutgoingAuth.Backends["discovered-backend"] require.True(t, exists, "discovered backend should be present in ConfigMap") require.NotNil(t, discoveredBackend, "discovered backend should have auth strategy") assert.Equal(t, "unauthenticated", discoveredBackend.Type, "backend should have correct auth type") - t.Log("✅ Static mode ConfigMap contains discovered backend auth configs") + // Verify static backend configurations (URLs + transport) are populated + require.NotEmpty(t, config.Backends, "static backends with URLs should be populated in static mode") + + // Find the discovered backend in the static backend list + var foundBackend *vmcpconfig.StaticBackendConfig + for i := range config.Backends { + if config.Backends[i].Name == "discovered-backend" { + foundBackend = &config.Backends[i] + break + } + } + require.NotNil(t, foundBackend, "discovered backend should be in static backends list") + assert.NotEmpty(t, foundBackend.URL, "backend should have URL populated") + assert.NotEmpty(t, foundBackend.Transport, "backend should have transport type populated") + + // Verify metadata is preserved (group, tool_type, workload_type, namespace) + require.NotNil(t, foundBackend.Metadata, "backend should have metadata") + assert.Equal(t, "test-group", foundBackend.Metadata["group"], "backend should have group metadata") + assert.Equal(t, "mcp", foundBackend.Metadata["tool_type"], "backend should have tool_type metadata") + assert.Equal(t, "mcp_server", foundBackend.Metadata["workload_type"], "backend should have workload_type metadata") + assert.Equal(t, "default", foundBackend.Metadata["namespace"], "backend should have namespace metadata") + + t.Log("Static mode ConfigMap contains both auth configs, backend URLs/transports, and metadata") +} + +// TestConvertBackendsToStaticBackends_SkipsInvalidBackends tests that backends +// without URL or transport are skipped with appropriate logging +func TestConvertBackendsToStaticBackends_SkipsInvalidBackends(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + backends := []vmcp.Backend{ + { + Name: "valid-backend", + BaseURL: "http://backend1:8080", + TransportType: "sse", + Metadata: map[string]string{"key": "value"}, + }, + { + Name: "no-url-backend", + BaseURL: "", // Missing URL + TransportType: "sse", + }, + { + Name: "no-transport-backend", + BaseURL: "http://backend2:8080", + // Transport will be missing from map + }, + } + + transportMap := map[string]string{ + "valid-backend": "sse", + "no-url-backend": "streamable-http", + // "no-transport-backend" intentionally missing + } + + result := convertBackendsToStaticBackends(ctx, backends, transportMap) + + // Should only include the valid backend + assert.Len(t, result, 1, "should only include backends with URL and transport") + assert.Equal(t, "valid-backend", result[0].Name) + assert.Equal(t, "http://backend1:8080", result[0].URL) + assert.Equal(t, "sse", result[0].Transport) + assert.Equal(t, "value", result[0].Metadata["key"]) +} + +// TestStaticModeTransportConstants verifies that the transport constants match the CRD enum. +// This test ensures consistency between runtime validation and CRD schema validation. +func TestStaticModeTransportConstants(t *testing.T) { + t.Parallel() + + // Define the expected transports that should be in the CRD enum. + // If this test fails, it means the CRD enum in StaticBackendConfig.Transport + // is out of sync with vmcpconfig.StaticModeAllowedTransports. + expectedTransports := []string{vmcpconfig.TransportSSE, vmcpconfig.TransportStreamableHTTP} + + // Verify the slice matches exactly + assert.ElementsMatch(t, expectedTransports, vmcpconfig.StaticModeAllowedTransports, + "StaticModeAllowedTransports must match the transport constants") + + // Verify individual constants have expected values + assert.Equal(t, "sse", vmcpconfig.TransportSSE, "TransportSSE constant value") + assert.Equal(t, "streamable-http", vmcpconfig.TransportStreamableHTTP, "TransportStreamableHTTP constant value") + + // NOTE: When updating allowed transports: + // 1. Update the constants in pkg/vmcp/config/config.go + // 2. Update the CRD enum in StaticBackendConfig.Transport: +kubebuilder:validation:Enum=... + // 3. Run: task operator-generate && task operator-manifests + // 4. This test will verify the constants match the expected values } diff --git a/cmd/vmcp/app/commands.go b/cmd/vmcp/app/commands.go index cd05918181..36cfbe6b44 100644 --- a/cmd/vmcp/app/commands.go +++ b/cmd/vmcp/app/commands.go @@ -236,6 +236,7 @@ func discoverBackends(ctx context.Context, cfg *config.Config) ([]vmcp.Backend, discoverer = aggregator.NewUnifiedBackendDiscovererWithStaticBackends( cfg.Backends, cfg.OutgoingAuth, + cfg.Group, ) } else { // Dynamic mode: Discover backends at runtime from K8s API diff --git a/deploy/charts/operator-crds/Chart.yaml b/deploy/charts/operator-crds/Chart.yaml index 47d15cab11..5da4ebde7a 100644 --- a/deploy/charts/operator-crds/Chart.yaml +++ b/deploy/charts/operator-crds/Chart.yaml @@ -2,5 +2,5 @@ apiVersion: v2 name: toolhive-operator-crds description: A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. type: application -version: 0.0.96 +version: 0.0.97 appVersion: "0.0.1" diff --git a/deploy/charts/operator-crds/README.md b/deploy/charts/operator-crds/README.md index 8e44047b92..fc1e4ab5f4 100644 --- a/deploy/charts/operator-crds/README.md +++ b/deploy/charts/operator-crds/README.md @@ -1,6 +1,6 @@ # ToolHive Operator CRDs Helm Chart -![Version: 0.0.96](https://img.shields.io/badge/Version-0.0.96-informational?style=flat-square) +![Version: 0.0.97](https://img.shields.io/badge/Version-0.0.97-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) A Helm chart for installing the ToolHive Operator CRDs into Kubernetes. diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 0806bb46b4..175c00dd06 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -215,6 +215,50 @@ spec: data included in audit logs (in bytes). type: integer type: object + backends: + description: |- + Backends defines pre-configured backend servers for static mode. + When OutgoingAuth.Source is "inline", this field contains the full list of backend + servers with their URLs and transport types, eliminating the need for K8s API access. + When OutgoingAuth.Source is "discovered", this field is empty and backends are + discovered at runtime via Kubernetes API. + items: + description: |- + StaticBackendConfig defines a pre-configured backend server for static mode. + This allows vMCP to operate without Kubernetes API access by embedding all backend + information directly in the configuration. + properties: + metadata: + additionalProperties: + type: string + description: |- + Metadata is a custom key-value map for storing additional backend information + such as labels, tags, or other arbitrary data (e.g., "env": "prod", "region": "us-east-1"). + This is NOT Kubernetes ObjectMeta - it's a simple string map for user-defined metadata. + Reserved keys: "group" is automatically set by vMCP and any user-provided value will be overridden. + type: object + name: + description: |- + Name is the backend identifier. + Must match the backend name from the MCPGroup for auth config resolution. + type: string + transport: + description: |- + Transport is the MCP transport protocol: "sse" or "streamable-http" + Only network transports supported by vMCP client are allowed. + enum: + - sse + - streamable-http + type: string + url: + description: URL is the backend's MCP server base URL. + type: string + required: + - name + - transport + - url + type: object + type: array compositeToolRefs: description: |- CompositeToolRefs references VirtualMCPCompositeToolDefinition resources diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index 250c99f8d6..4410bf494e 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -218,6 +218,50 @@ spec: data included in audit logs (in bytes). type: integer type: object + backends: + description: |- + Backends defines pre-configured backend servers for static mode. + When OutgoingAuth.Source is "inline", this field contains the full list of backend + servers with their URLs and transport types, eliminating the need for K8s API access. + When OutgoingAuth.Source is "discovered", this field is empty and backends are + discovered at runtime via Kubernetes API. + items: + description: |- + StaticBackendConfig defines a pre-configured backend server for static mode. + This allows vMCP to operate without Kubernetes API access by embedding all backend + information directly in the configuration. + properties: + metadata: + additionalProperties: + type: string + description: |- + Metadata is a custom key-value map for storing additional backend information + such as labels, tags, or other arbitrary data (e.g., "env": "prod", "region": "us-east-1"). + This is NOT Kubernetes ObjectMeta - it's a simple string map for user-defined metadata. + Reserved keys: "group" is automatically set by vMCP and any user-provided value will be overridden. + type: object + name: + description: |- + Name is the backend identifier. + Must match the backend name from the MCPGroup for auth config resolution. + type: string + transport: + description: |- + Transport is the MCP transport protocol: "sse" or "streamable-http" + Only network transports supported by vMCP client are allowed. + enum: + - sse + - streamable-http + type: string + url: + description: URL is the backend's MCP server base URL. + type: string + required: + - name + - transport + - url + type: object + type: array compositeToolRefs: description: |- CompositeToolRefs references VirtualMCPCompositeToolDefinition resources diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index cbd532f4c7..f44aa34856 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -235,6 +235,7 @@ _Appears in:_ | --- | --- | --- | --- | | `name` _string_ | Name is the virtual MCP server name. | | | | `groupRef` _string_ | Group references an existing MCPGroup that defines backend workloads.
In Kubernetes, the referenced MCPGroup must exist in the same namespace. | | Required: \{\}
| +| `backends` _[vmcp.config.StaticBackendConfig](#vmcpconfigstaticbackendconfig) array_ | Backends defines pre-configured backend servers for static mode.
When OutgoingAuth.Source is "inline", this field contains the full list of backend
servers with their URLs and transport types, eliminating the need for K8s API access.
When OutgoingAuth.Source is "discovered", this field is empty and backends are
discovered at runtime via Kubernetes API. | | | | `incomingAuth` _[vmcp.config.IncomingAuthConfig](#vmcpconfigincomingauthconfig)_ | IncomingAuth configures how clients authenticate to the virtual MCP server.
When using the Kubernetes operator, this is populated by the converter from
VirtualMCPServerSpec.IncomingAuth and any values set here will be superseded. | | | | `outgoingAuth` _[vmcp.config.OutgoingAuthConfig](#vmcpconfigoutgoingauthconfig)_ | OutgoingAuth configures how the virtual MCP server authenticates to backends.
When using the Kubernetes operator, this is populated by the converter from
VirtualMCPServerSpec.OutgoingAuth and any values set here will be superseded. | | | | `aggregation` _[vmcp.config.AggregationConfig](#vmcpconfigaggregationconfig)_ | Aggregation defines tool aggregation and conflict resolution strategies.
Supports ToolConfigRef for Kubernetes-native MCPToolConfig resource references. | | | @@ -440,6 +441,27 @@ _Appears in:_ | `default` _[pkg.json.Any](#pkgjsonany)_ | Default is the fallback value if template expansion fails.
Type coercion is applied to match the declared Type. | | Schemaless: \{\}
| +#### vmcp.config.StaticBackendConfig + + + +StaticBackendConfig defines a pre-configured backend server for static mode. +This allows vMCP to operate without Kubernetes API access by embedding all backend +information directly in the configuration. + + + +_Appears in:_ +- [vmcp.config.Config](#vmcpconfigconfig) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `name` _string_ | Name is the backend identifier.
Must match the backend name from the MCPGroup for auth config resolution. | | Required: \{\}
| +| `url` _string_ | URL is the backend's MCP server base URL. | | Required: \{\}
| +| `transport` _string_ | Transport is the MCP transport protocol: "sse" or "streamable-http"
Only network transports supported by vMCP client are allowed. | | Enum: [sse streamable-http]
Required: \{\}
| +| `metadata` _object (keys:string, values:string)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | + + #### vmcp.config.StepErrorHandling diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 79bcb660bc..367eb187cc 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -28,6 +28,7 @@ type backendDiscoverer struct { groupsManager groups.Manager authConfig *config.OutgoingAuthConfig staticBackends []config.StaticBackendConfig // Pre-configured backends for static mode + groupRef string // Group reference for static mode metadata } // NewUnifiedBackendDiscoverer creates a unified backend discoverer that works with both @@ -53,12 +54,14 @@ func NewUnifiedBackendDiscoverer( func NewUnifiedBackendDiscovererWithStaticBackends( staticBackends []config.StaticBackendConfig, authConfig *config.OutgoingAuthConfig, + groupRef string, ) BackendDiscoverer { return &backendDiscoverer{ workloadsManager: nil, // Not needed in static mode groupsManager: nil, // Not needed in static mode authConfig: authConfig, staticBackends: staticBackends, + groupRef: groupRef, } } @@ -249,6 +252,17 @@ func (d *backendDiscoverer) discoverFromStaticConfig() ([]vmcp.Backend, error) { // Apply auth configuration from OutgoingAuthConfig d.applyAuthConfigToBackend(&backend, staticBackend.Name) + // Set group metadata (reserved key, always overridden) + if backend.Metadata == nil { + backend.Metadata = make(map[string]string) + } + // Warn if user provided a conflicting group value + if existingGroup, exists := backend.Metadata["group"]; exists && existingGroup != d.groupRef { + logger.Warnf("Backend %s has user-provided group metadata '%s' which will be overridden with '%s'", + staticBackend.Name, existingGroup, d.groupRef) + } + backend.Metadata["group"] = d.groupRef + backends = append(backends, backend) logger.Infof("Loaded static backend: %s (url=%s, transport=%s)", staticBackend.Name, staticBackend.URL, staticBackend.Transport) diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 9f75120f9e..2ce3341506 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -1168,3 +1168,116 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { assert.Equal(t, "default-fallback-token", backend.AuthConfig.HeaderInjection.HeaderValue) }) } + +// TestStaticBackendDiscoverer_MetadataGroupOverride verifies that the "group" metadata key +// is always overridden with the groupRef value, even if user provides a different value. +func TestStaticBackendDiscoverer_MetadataGroupOverride(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + staticBackends []config.StaticBackendConfig + groupRef string + expectedGroupVals []string + }{ + { + name: "user-provided group metadata is overridden", + staticBackends: []config.StaticBackendConfig{ + { + Name: "backend1", + URL: "http://backend1:8080", + Transport: "sse", + Metadata: map[string]string{ + "group": "wrong-group", // User provided conflicting value + "env": "prod", + }, + }, + }, + groupRef: "correct-group", + expectedGroupVals: []string{"correct-group"}, + }, + { + name: "group metadata added when not present", + staticBackends: []config.StaticBackendConfig{ + { + Name: "backend2", + URL: "http://backend2:8080", + Transport: "streamable-http", + Metadata: map[string]string{ + "env": "dev", + }, + }, + }, + groupRef: "test-group", + expectedGroupVals: []string{"test-group"}, + }, + { + name: "group metadata added when metadata is nil", + staticBackends: []config.StaticBackendConfig{ + { + Name: "backend3", + URL: "http://backend3:8080", + Transport: "sse", + Metadata: nil, // No metadata at all + }, + }, + groupRef: "my-group", + expectedGroupVals: []string{"my-group"}, + }, + { + name: "multiple backends all get correct group", + staticBackends: []config.StaticBackendConfig{ + { + Name: "backend1", + URL: "http://backend1:8080", + Transport: "sse", + Metadata: map[string]string{"group": "wrong1"}, + }, + { + Name: "backend2", + URL: "http://backend2:8080", + Transport: "streamable-http", + Metadata: map[string]string{"env": "prod"}, + }, + }, + groupRef: "shared-group", + expectedGroupVals: []string{"shared-group", "shared-group"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + discoverer := NewUnifiedBackendDiscovererWithStaticBackends( + tt.staticBackends, + nil, // No auth config needed for this test + tt.groupRef, + ) + + backends, err := discoverer.Discover(ctx, tt.groupRef) + require.NoError(t, err) + + // Verify we got the expected number of backends + assert.Len(t, backends, len(tt.expectedGroupVals)) + + // Verify each backend has the correct group metadata + for i, backend := range backends { + assert.NotNil(t, backend.Metadata, "Backend %d should have metadata", i) + assert.Equal(t, tt.expectedGroupVals[i], backend.Metadata["group"], + "Backend %d should have correct group metadata", i) + + // Verify other metadata is preserved + if tt.staticBackends[i].Metadata != nil { + for k, v := range tt.staticBackends[i].Metadata { + if k != "group" { + assert.Equal(t, v, backend.Metadata[k], + "Backend %d should preserve non-group metadata key %s", i, k) + } + } + } + } + }) + } +} diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index d1865ba22f..abc1cfe477 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -17,6 +17,19 @@ import ( authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" ) +// Transport type constants for static backend configuration. +// These define the allowed network transport protocols for vMCP backends in static mode. +const ( + // TransportSSE is the Server-Sent Events transport protocol. + TransportSSE = "sse" + // TransportStreamableHTTP is the streamable HTTP transport protocol. + TransportStreamableHTTP = "streamable-http" +) + +// StaticModeAllowedTransports lists all transport types allowed for static backend configuration. +// This must be kept in sync with the CRD enum validation in StaticBackendConfig.Transport. +var StaticModeAllowedTransports = []string{TransportSSE, TransportStreamableHTTP} + // Duration is a wrapper around time.Duration that marshals/unmarshals as a duration string. // This ensures duration values are serialized as "30s", "1m", etc. instead of nanosecond integers. // +kubebuilder:validation:Type=string @@ -207,6 +220,7 @@ type AuthzConfig struct { // StaticBackendConfig defines a pre-configured backend server for static mode. // This allows vMCP to operate without Kubernetes API access by embedding all backend // information directly in the configuration. +// +gendoc // +kubebuilder:object:generate=true type StaticBackendConfig struct { // Name is the backend identifier. @@ -218,12 +232,16 @@ type StaticBackendConfig struct { // +kubebuilder:validation:Required URL string `json:"url" yaml:"url"` - // Transport is the MCP transport protocol: "stdio", "http", "sse", "streamable-http" - // +kubebuilder:validation:Enum=stdio;http;sse;streamable-http + // Transport is the MCP transport protocol: "sse" or "streamable-http" + // Only network transports supported by vMCP client are allowed. + // +kubebuilder:validation:Enum=sse;streamable-http // +kubebuilder:validation:Required Transport string `json:"transport" yaml:"transport"` - // Metadata stores additional backend information. + // Metadata is a custom key-value map for storing additional backend information + // such as labels, tags, or other arbitrary data (e.g., "env": "prod", "region": "us-east-1"). + // This is NOT Kubernetes ObjectMeta - it's a simple string map for user-defined metadata. + // Reserved keys: "group" is automatically set by vMCP and any user-provided value will be overridden. // +optional Metadata map[string]string `json:"metadata,omitempty" yaml:"metadata,omitempty"` } diff --git a/pkg/vmcp/discovery/middleware_test.go b/pkg/vmcp/discovery/middleware_test.go index 7cbaad0ab1..0f975d860b 100644 --- a/pkg/vmcp/discovery/middleware_test.go +++ b/pkg/vmcp/discovery/middleware_test.go @@ -28,6 +28,50 @@ func createTestSessionManager(t *testing.T) *transportsession.Manager { return sessionMgr } +// unorderedBackendsMatcher is a gomock matcher that compares backend slices without caring about order. +// This is needed because ImmutableRegistry.List() iterates over a map which doesn't guarantee order. +type unorderedBackendsMatcher struct { + expected []vmcp.Backend +} + +func (m unorderedBackendsMatcher) Matches(x any) bool { + actual, ok := x.([]vmcp.Backend) + if !ok { + return false + } + if len(actual) != len(m.expected) { + return false + } + + // Create maps for comparison + expectedMap := make(map[string]vmcp.Backend) + for _, b := range m.expected { + expectedMap[b.ID] = b + } + + actualMap := make(map[string]vmcp.Backend) + for _, b := range actual { + actualMap[b.ID] = b + } + + // Check all expected backends are present + for id, expectedBackend := range expectedMap { + actualBackend, found := actualMap[id] + if !found { + return false + } + if expectedBackend.ID != actualBackend.ID || expectedBackend.Name != actualBackend.Name { + return false + } + } + + return true +} + +func (unorderedBackendsMatcher) String() string { + return "matches backends regardless of order" +} + func TestMiddleware_InitializeRequest(t *testing.T) { t.Parallel() @@ -67,7 +111,7 @@ func TestMiddleware_InitializeRequest(t *testing.T) { // Expect discovery to be called for initialize request (no session ID) mockMgr.EXPECT(). - Discover(gomock.Any(), backends). + Discover(gomock.Any(), unorderedBackendsMatcher{backends}). Return(expectedCaps, nil) // Create a test handler that verifies capabilities are in context @@ -302,7 +346,7 @@ func TestMiddleware_CapabilitiesInContext(t *testing.T) { } mockMgr.EXPECT(). - Discover(gomock.Any(), backends). + Discover(gomock.Any(), unorderedBackendsMatcher{backends}). Return(expectedCaps, nil) // Create handler that inspects context in detail diff --git a/pkg/vmcp/workloads/k8s.go b/pkg/vmcp/workloads/k8s.go index fdcc751bac..24b081da81 100644 --- a/pkg/vmcp/workloads/k8s.go +++ b/pkg/vmcp/workloads/k8s.go @@ -199,6 +199,9 @@ func (d *k8sDiscoverer) mcpServerToBackend(ctx context.Context, mcpServer *mcpv1 // Generate URL from status or reconstruct from spec url := mcpServer.Status.URL if url == "" { + // Use ProxyPort (not McpPort) because it's the externally accessible port + // that the egress proxy listens on. This is what vMCP connects to. + // The McpPort is only for internal container-to-container communication. port := int(mcpServer.Spec.ProxyPort) if port == 0 { port = int(mcpServer.Spec.Port) // Fallback to deprecated Port field diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go index 5c4f0a0939..18b5d9d63b 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go @@ -1623,7 +1623,7 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, timeout, pollingInterval).Should(BeTrue()) }) - It("should clean up RBAC when switching from dynamic to static", func() { + It("should preserve RBAC resources when switching from dynamic to static", func() { By("Creating VirtualMCPServer in dynamic mode") vmcpServer := &mcpv1alpha1.VirtualMCPServer{ ObjectMeta: metav1.ObjectMeta{ @@ -1673,35 +1673,52 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { return k8sClient.Update(ctx, vmcpServer) }, 30*time.Second, 2*time.Second).Should(Succeed()) - By("Waiting for operator to reconcile and clean up RBAC") - // Wait for RBAC resources to be deleted - Eventually(func() bool { + By("Verifying RBAC resources remain after mode switch (left for garbage collection)") + // RBAC resources are NOT deleted during mode switching - they remain until VirtualMCPServer deletion + // This follows Kubernetes controller patterns: rely on owner references and garbage collection + Consistently(func() error { err := k8sClient.Get(ctx, types.NamespacedName{ Name: serviceAccountName, Namespace: testNamespace, }, sa) - return err != nil // Should not exist after mode switch - }, 2*time.Minute, 5*time.Second).Should(BeTrue(), "ServiceAccount should be deleted after switching to static mode") + return err + }, 15*time.Second, 2*time.Second).Should(Succeed(), + "ServiceAccount not actively deleted on mode switch - left for garbage collection via owner references") - By("Verifying Role was also deleted") + By("Verifying Role remains (left for garbage collection)") role := &rbacv1.Role{} - Consistently(func() bool { + Consistently(func() error { err := k8sClient.Get(ctx, types.NamespacedName{ Name: serviceAccountName, Namespace: testNamespace, }, role) - return err != nil - }, 10*time.Second, 2*time.Second).Should(BeTrue(), "Role should not exist after mode switch") + return err + }, 15*time.Second, 2*time.Second).Should(Succeed(), + "Role not actively deleted on mode switch - left for garbage collection via owner references") - By("Verifying RoleBinding was also deleted") + By("Verifying RoleBinding remains (left for garbage collection)") rb := &rbacv1.RoleBinding{} - Consistently(func() bool { + Consistently(func() error { err := k8sClient.Get(ctx, types.NamespacedName{ Name: serviceAccountName, Namespace: testNamespace, }, rb) - return err != nil - }, 10*time.Second, 2*time.Second).Should(BeTrue(), "RoleBinding should not exist after mode switch") + return err + }, 15*time.Second, 2*time.Second).Should(Succeed(), + "RoleBinding not actively deleted on mode switch - left for garbage collection via owner references") + + By("Verifying Deployment uses default ServiceAccount in static mode") + Eventually(func() string { + deployment := &appsv1.Deployment{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, deployment) + if err != nil { + return "error" + } + return deployment.Spec.Template.Spec.ServiceAccountName + }, 30*time.Second, 2*time.Second).Should(BeEmpty(), "Deployment should use default ServiceAccount (empty string) in static mode") By("Verifying VirtualMCPServer is still ready after mode switch") Eventually(func() error { @@ -1720,4 +1737,111 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, 2*time.Minute, 5*time.Second).Should(Succeed(), "VirtualMCPServer should remain ready after mode switch") }) }) + + Context("Garbage Collection", func() { + var vmcpServerName = "test-vmcp-gc" + + It("should garbage collect RBAC resources when VirtualMCPServer is deleted", func() { + By("Creating VirtualMCPServer in dynamic mode") + vmcpServer := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpServerName, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: mcpGroupName}, + IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ + Type: "anonymous", + }, + OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ + Source: "discovered", // Dynamic mode - creates RBAC + }, + ServiceType: "ClusterIP", + }, + } + Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be ready") + WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) + + serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) + + By("Verifying RBAC resources exist") + sa := &corev1.ServiceAccount{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist") + + role := &rbacv1.Role{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, role) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "Role should exist") + + rb := &rbacv1.RoleBinding{} + Eventually(func() error { + return k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, rb) + }, 30*time.Second, 2*time.Second).Should(Succeed(), "RoleBinding should exist") + + By("Verifying RBAC resources have owner references to VirtualMCPServer") + Expect(sa.OwnerReferences).NotTo(BeEmpty(), "ServiceAccount should have owner references") + Expect(sa.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) + Expect(sa.OwnerReferences[0].Name).To(Equal(vmcpServerName)) + + Expect(role.OwnerReferences).NotTo(BeEmpty(), "Role should have owner references") + Expect(role.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) + Expect(role.OwnerReferences[0].Name).To(Equal(vmcpServerName)) + + Expect(rb.OwnerReferences).NotTo(BeEmpty(), "RoleBinding should have owner references") + Expect(rb.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) + Expect(rb.OwnerReferences[0].Name).To(Equal(vmcpServerName)) + + By("Deleting VirtualMCPServer") + Expect(k8sClient.Delete(ctx, vmcpServer)).To(Succeed()) + + By("Waiting for VirtualMCPServer to be fully deleted") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: vmcpServerName, + Namespace: testNamespace, + }, vmcpServer) + return errors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue(), "VirtualMCPServer should be deleted") + + By("Verifying ServiceAccount is garbage collected") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, sa) + return errors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue(), "ServiceAccount should be garbage collected via owner reference") + + By("Verifying Role is garbage collected") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, role) + return errors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue(), "Role should be garbage collected via owner reference") + + By("Verifying RoleBinding is garbage collected") + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: serviceAccountName, + Namespace: testNamespace, + }, rb) + return errors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue(), "RoleBinding should be garbage collected via owner reference") + }) + }) }) diff --git a/test/integration/vmcp/helpers/helpers_test.go b/test/integration/vmcp/helpers/helpers_test.go new file mode 100644 index 0000000000..0438b9deb2 --- /dev/null +++ b/test/integration/vmcp/helpers/helpers_test.go @@ -0,0 +1,76 @@ +package helpers + +import ( + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" +) + +// TestGetToolNames tests the GetToolNames helper function. +func TestGetToolNames(t *testing.T) { + t.Parallel() + tests := []struct { + name string + result *mcp.ListToolsResult + expected []string + }{ + { + name: "empty tools", + result: &mcp.ListToolsResult{ + Tools: []mcp.Tool{}, + }, + expected: []string{}, + }, + { + name: "single tool", + result: &mcp.ListToolsResult{ + Tools: []mcp.Tool{ + {Name: "tool1"}, + }, + }, + expected: []string{"tool1"}, + }, + { + name: "multiple tools", + result: &mcp.ListToolsResult{ + Tools: []mcp.Tool{ + {Name: "tool1"}, + {Name: "tool2"}, + {Name: "tool3"}, + }, + }, + expected: []string{"tool1", "tool2", "tool3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + names := GetToolNames(tt.result) + assert.Equal(t, tt.expected, names) + }) + } +} + +// TestAssertTextContains tests the AssertTextContains helper. +func TestAssertTextContains(t *testing.T) { + t.Parallel() + t.Run("all substrings present", func(t *testing.T) { + t.Parallel() + text := "hello world, this is a test" + // Should not fail + AssertTextContains(t, text, "hello", "world", "test") + }) +} + +// TestAssertTextNotContains tests the AssertTextNotContains helper. +func TestAssertTextNotContains(t *testing.T) { + t.Parallel() + t.Run("no forbidden substrings", func(t *testing.T) { + t.Parallel() + text := "hello world" + // Should not fail + AssertTextNotContains(t, text, "password", "secret") + }) +} From bdd4d63ae2bc0ae18949451d8ad2b25ced17379d Mon Sep 17 00:00:00 2001 From: taskbot Date: Mon, 19 Jan 2026 16:29:14 +0100 Subject: [PATCH 4/5] fixes from review --- .../controllers/mcpremoteproxy_controller.go | 3 +++ .../virtualmcpserver_controller.go | 13 ++++++---- .../virtualmcpserver_vmcpconfig.go | 6 +++++ cmd/thv-operator/pkg/controllerutil/rbac.go | 4 ++++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 2 ++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 2 ++ docs/operator/crd-api.md | 4 ++-- pkg/vmcp/aggregator/discoverer.go | 8 +++++++ pkg/vmcp/aggregator/discoverer_test.go | 24 +++++++++++++++++++ pkg/vmcp/config/config.go | 2 ++ .../virtualmcp/virtualmcp_lifecycle_test.go | 10 ++++---- 11 files changed, 67 insertions(+), 11 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index e7d209d467..ef425303f0 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -465,6 +465,9 @@ func (r *MCPRemoteProxyReconciler) validateGroupRef(ctx context.Context, proxy * } // ensureRBACResources ensures that the RBAC resources are in place for the remote proxy +// TODO: This uses EnsureRBACResource which only creates RBAC but never updates them. +// Consider adopting the MCPRegistry pattern (pkg/registryapi/rbac.go) which uses +// CreateOrUpdate + RetryOnConflict to automatically update RBAC rules during operator upgrades. func (r *MCPRemoteProxyReconciler) ensureRBACResources(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error { proxyRunnerNameForRBAC := proxyRunnerServiceAccountNameForRemoteProxy(proxy.Name) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 6595e520cc..8adb1797f0 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -497,8 +497,13 @@ func (r *VirtualMCPServerReconciler) ensureAllResources( } // ensureRBACResources ensures RBAC resources for VirtualMCPServer in dynamic mode. -// In static mode, RBAC creation is skipped. Existing RBAC resources (if any) remain until -// the VirtualMCPServer is deleted - they will be garbage collected via owner references. +// In static mode, RBAC creation is skipped. When switching dynamic→static, existing RBAC +// resources are NOT deleted - they persist until VirtualMCPServer deletion via owner references. +// This follows standard Kubernetes garbage collection patterns. +// +// TODO: This uses EnsureRBACResource which only creates RBAC but never updates them. +// Consider adopting the MCPRegistry pattern (pkg/registryapi/rbac.go) which uses +// CreateOrUpdate + RetryOnConflict to automatically update RBAC rules during operator upgrades. func (r *VirtualMCPServerReconciler) ensureRBACResources( ctx context.Context, vmcp *mcpv1alpha1.VirtualMCPServer, @@ -506,8 +511,8 @@ func (r *VirtualMCPServerReconciler) ensureRBACResources( // Determine the outgoing auth source mode source := outgoingAuthSource(vmcp) - // Static mode (inline): Skip RBAC creation - // Owner references ensure garbage collection when VirtualMCPServer is deleted + // Static mode (inline): Skip RBAC creation/deletion + // Existing resources from dynamic mode persist until VirtualMCPServer deletion if source == OutgoingAuthSourceInline { return nil } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go index 13567fde5c..9be45407a9 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_vmcpconfig.go @@ -7,6 +7,7 @@ import ( "gopkg.in/yaml.v3" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/log" mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/kubernetes/configmaps" @@ -146,6 +147,11 @@ func (r *VirtualMCPServerReconciler) discoverBackendsWithMetadata( authConfig, err = r.buildOutgoingAuthConfig(ctx, vmcp, typedWorkloads) if err != nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.V(1).Info("Failed to build outgoing auth config, continuing without authentication", + "error", err, + "virtualmcpserver", vmcp.Name, + "namespace", vmcp.Namespace) authConfig = nil // Continue without auth config on error } } diff --git a/cmd/thv-operator/pkg/controllerutil/rbac.go b/cmd/thv-operator/pkg/controllerutil/rbac.go index a7eadd2938..76c0865dc6 100644 --- a/cmd/thv-operator/pkg/controllerutil/rbac.go +++ b/cmd/thv-operator/pkg/controllerutil/rbac.go @@ -13,6 +13,10 @@ import ( ) // EnsureRBACResource is a generic helper function to ensure a Kubernetes RBAC resource exists +// LIMITATION: This only creates resources if they don't exist - it does NOT update them. +// If RBAC rules change in an operator upgrade, existing resources won't be updated. +// For a better pattern that supports updates, see pkg/registryapi/rbac.go which uses +// CreateOrUpdate + RetryOnConflict. func EnsureRBACResource( ctx context.Context, c client.Client, diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 175c00dd06..66d212a897 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -252,6 +252,7 @@ spec: type: string url: description: URL is the backend's MCP server base URL. + pattern: ^https?:// type: string required: - name @@ -561,6 +562,7 @@ spec: type: boolean issuer: description: Issuer is the OIDC issuer URL. + pattern: ^https?:// type: string protectedResourceAllowPrivateIp: description: |- diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index 4410bf494e..fd038ae44b 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -255,6 +255,7 @@ spec: type: string url: description: URL is the backend's MCP server base URL. + pattern: ^https?:// type: string required: - name @@ -564,6 +565,7 @@ spec: type: boolean issuer: description: Issuer is the OIDC issuer URL. + pattern: ^https?:// type: string protectedResourceAllowPrivateIp: description: |- diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index f44aa34856..0f14569796 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -343,7 +343,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `issuer` _string_ | Issuer is the OIDC issuer URL. | | | +| `issuer` _string_ | Issuer is the OIDC issuer URL. | | Pattern: `^https?://`
| | `clientId` _string_ | ClientID is the OAuth client ID. | | | | `clientSecretEnv` _string_ | ClientSecretEnv is the name of the environment variable containing the client secret.
This is the secure way to reference secrets - the actual secret value is never stored
in configuration files, only the environment variable name.
The secret value will be resolved from this environment variable at runtime. | | | | `audience` _string_ | Audience is the required token audience. | | | @@ -457,7 +457,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | | `name` _string_ | Name is the backend identifier.
Must match the backend name from the MCPGroup for auth config resolution. | | Required: \{\}
| -| `url` _string_ | URL is the backend's MCP server base URL. | | Required: \{\}
| +| `url` _string_ | URL is the backend's MCP server base URL. | | Pattern: `^https?://`
Required: \{\}
| | `transport` _string_ | Transport is the MCP transport protocol: "sse" or "streamable-http"
Only network transports supported by vMCP client are allowed. | | Enum: [sse streamable-http]
Required: \{\}
| | `metadata` _object (keys:string, values:string)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 367eb187cc..13bc8f9a63 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -126,6 +126,14 @@ func (d *backendDiscoverer) Discover(ctx context.Context, groupRef string) ([]vm return d.discoverFromStaticConfig() } + // If staticBackends was explicitly set (even if empty), but groupsManager is nil, + // this discoverer was created for static mode with an empty backend list. + // Return empty list instead of falling through to dynamic mode which would panic. + if d.staticBackends != nil && d.groupsManager == nil { + logger.Infof("Static mode with empty backend list, returning no backends") + return []vmcp.Backend{}, nil + } + // Dynamic mode: Discover backends from K8s API at runtime logger.Infof("Dynamic mode: discovering backends from K8s API") diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 2ce3341506..8bab6ec003 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -1169,6 +1169,30 @@ func TestBackendDiscoverer_applyAuthConfigToBackend(t *testing.T) { }) } +// TestStaticBackendDiscoverer_EmptyBackendList verifies that when a static discoverer +// is created with an empty backend list, it gracefully returns an empty list instead of +// panicking due to nil groupsManager (regression test for nil pointer dereference). +func TestStaticBackendDiscoverer_EmptyBackendList(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + // Create a static discoverer with empty backend list (not nil, but zero length) + // This simulates the edge case where staticBackends was set but is empty + discoverer := NewUnifiedBackendDiscovererWithStaticBackends( + []config.StaticBackendConfig{}, // Empty slice, not nil + nil, // No auth config + "test-group", + ) + + // This should return empty list without panicking + // Previously would panic when falling through to dynamic mode with nil groupsManager + backends, err := discoverer.Discover(ctx, "test-group") + + require.NoError(t, err) + assert.Empty(t, backends) +} + // TestStaticBackendDiscoverer_MetadataGroupOverride verifies that the "group" metadata key // is always overridden with the groupRef value, even if user provides a different value. func TestStaticBackendDiscoverer_MetadataGroupOverride(t *testing.T) { diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index abc1cfe477..6765c4cd1a 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -175,6 +175,7 @@ type IncomingAuthConfig struct { // +gendoc type OIDCConfig struct { // Issuer is the OIDC issuer URL. + // +kubebuilder:validation:Pattern=`^https?://` Issuer string `json:"issuer" yaml:"issuer"` // ClientID is the OAuth client ID. @@ -230,6 +231,7 @@ type StaticBackendConfig struct { // URL is the backend's MCP server base URL. // +kubebuilder:validation:Required + // +kubebuilder:validation:Pattern=`^https?://` URL string `json:"url" yaml:"url"` // Transport is the MCP transport protocol: "sse" or "streamable-http" diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go index 18b5d9d63b..d1b745e189 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go @@ -1674,8 +1674,8 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, 30*time.Second, 2*time.Second).Should(Succeed()) By("Verifying RBAC resources remain after mode switch (left for garbage collection)") - // RBAC resources are NOT deleted during mode switching - they remain until VirtualMCPServer deletion - // This follows Kubernetes controller patterns: rely on owner references and garbage collection + // When switching dynamic→static, RBAC resources are NOT actively deleted. + // They persist with owner references and will be garbage collected on VirtualMCPServer deletion. Consistently(func() error { err := k8sClient.Get(ctx, types.NamespacedName{ Name: serviceAccountName, @@ -1683,7 +1683,7 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, sa) return err }, 15*time.Second, 2*time.Second).Should(Succeed(), - "ServiceAccount not actively deleted on mode switch - left for garbage collection via owner references") + "ServiceAccount should persist after dynamic→static mode switch") By("Verifying Role remains (left for garbage collection)") role := &rbacv1.Role{} @@ -1694,7 +1694,7 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, role) return err }, 15*time.Second, 2*time.Second).Should(Succeed(), - "Role not actively deleted on mode switch - left for garbage collection via owner references") + "Role should persist after dynamic→static mode switch") By("Verifying RoleBinding remains (left for garbage collection)") rb := &rbacv1.RoleBinding{} @@ -1705,7 +1705,7 @@ var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { }, rb) return err }, 15*time.Second, 2*time.Second).Should(Succeed(), - "RoleBinding not actively deleted on mode switch - left for garbage collection via owner references") + "RoleBinding should persist after dynamic→static mode switch") By("Verifying Deployment uses default ServiceAccount in static mode") Eventually(func() string { From c0e707f6bd2f050d0be7220fc13e6650cdec3a3c Mon Sep 17 00:00:00 2001 From: taskbot Date: Tue, 20 Jan 2026 11:34:37 +0100 Subject: [PATCH 5/5] fix ci --- .../virtualmcp_discovered_mode_test.go | 379 ++++ .../virtualmcp/virtualmcp_lifecycle_test.go | 1847 ----------------- 2 files changed, 379 insertions(+), 1847 deletions(-) delete mode 100644 test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go index b8e3e89f19..3ab460bce7 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_discovered_mode_test.go @@ -2,7 +2,9 @@ package virtualmcp import ( "context" + "encoding/json" "fmt" + "io" "net/http" "strings" "time" @@ -19,6 +21,13 @@ import ( "github.com/stacklok/toolhive/test/e2e/images" ) +// ReadinessResponse represents the /readyz endpoint response +type ReadinessResponse struct { + Status string `json:"status"` + Mode string `json:"mode"` + Reason string `json:"reason,omitempty"` +} + var _ = Describe("VirtualMCPServer Discovered Mode", Ordered, func() { var ( testNamespace = "default" @@ -402,4 +411,374 @@ var _ = Describe("VirtualMCPServer Discovered Mode", Ordered, func() { Expect(backendNames).To(ContainElements(backend1Name, backend2Name)) }) }) + + Context("when dynamically adding a new backend", func() { + var ( + backend3Name = "backend-dynamic-fetch" + initialToolCount int + ) + + AfterAll(func() { + // Clean up the dynamic backend + backend3 := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backend3Name, + Namespace: testNamespace, + }, + } + _ = k8sClient.Delete(ctx, backend3) + }) + + It("should record initial tool count", func() { + By("Creating MCP client to get initial tool count") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + Eventually(func() error { + err = mcpClient.Start(testCtx) + if err != nil { + return err + } + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-initial-count", + Version: "1.0.0", + } + + _, err = mcpClient.Initialize(testCtx, initRequest) + return err + }, 30*time.Second, 5*time.Second).Should(Succeed()) + + var tools *mcp.ListToolsResult + Eventually(func() error { + var err error + tools, err = mcpClient.ListTools(testCtx, mcp.ListToolsRequest{}) + return err + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + initialToolCount = len(tools.Tools) + GinkgoWriter.Printf("Initial tool count: %d\n", initialToolCount) + }) + + It("should detect new backend and update tool list", func() { + By("Adding third backend MCPServer") + backend3 := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backend3Name, + Namespace: testNamespace, + }, + Spec: mcpv1alpha1.MCPServerSpec{ + GroupRef: mcpGroupName, + Image: images.GofetchServerImage, + Transport: "streamable-http", + ProxyPort: 8080, + McpPort: 8080, + }, + } + Expect(k8sClient.Create(ctx, backend3)).To(Succeed()) + + By("Waiting for new backend to be ready") + Eventually(func() error { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backend3Name, + Namespace: testNamespace, + }, server) + if err != nil { + return err + } + if server.Status.Phase != mcpv1alpha1.MCPServerPhaseRunning { + return fmt.Errorf("backend not ready, phase: %s", server.Status.Phase) + } + return nil + }, timeout, pollingInterval).Should(Succeed()) + + By("Verifying group now has three backends") + Eventually(func() int { + backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) + if err != nil { + return 0 + } + return len(backends) + }, 30*time.Second, 2*time.Second).Should(Equal(3)) + + By("Verifying tool count increased with new session") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + + Eventually(func() error { + mcpClient, err := client.NewStreamableHttpClient(serverURL) + if err != nil { + return err + } + defer mcpClient.Close() + + testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + err = mcpClient.Start(testCtx) + if err != nil { + return err + } + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-after-add", + Version: "1.0.0", + } + + _, err = mcpClient.Initialize(testCtx, initRequest) + if err != nil { + return err + } + + tools, err := mcpClient.ListTools(testCtx, mcp.ListToolsRequest{}) + if err != nil { + return err + } + + if len(tools.Tools) <= initialToolCount { + return fmt.Errorf("expected more tools, got %d (was %d)", len(tools.Tools), initialToolCount) + } + return nil + }, 1*time.Minute, 10*time.Second).Should(Succeed()) + }) + }) + + Context("when dynamically removing a backend", func() { + It("should detect backend removal and update tool list", func() { + By("Getting current tool count") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + mcpClient, err := client.NewStreamableHttpClient(serverURL) + Expect(err).ToNot(HaveOccurred()) + defer mcpClient.Close() + + testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + Eventually(func() error { + err = mcpClient.Start(testCtx) + if err != nil { + return err + } + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-before-remove", + Version: "1.0.0", + } + + _, err = mcpClient.Initialize(testCtx, initRequest) + return err + }, 30*time.Second, 5*time.Second).Should(Succeed()) + + var toolsBeforeRemoval *mcp.ListToolsResult + Eventually(func() error { + var err error + toolsBeforeRemoval, err = mcpClient.ListTools(testCtx, mcp.ListToolsRequest{}) + return err + }, 30*time.Second, 2*time.Second).Should(Succeed()) + + toolCountBefore := len(toolsBeforeRemoval.Tools) + GinkgoWriter.Printf("Before removal: %d tools\n", toolCountBefore) + + By("Removing backend2 (osv)") + backend2 := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: backend2Name, + Namespace: testNamespace, + }, + } + Expect(k8sClient.Delete(ctx, backend2)).To(Succeed()) + + By("Waiting for backend deletion") + Eventually(func() bool { + server := &mcpv1alpha1.MCPServer{} + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: backend2Name, + Namespace: testNamespace, + }, server) + return err != nil + }, timeout, pollingInterval).Should(BeTrue()) + + By("Verifying group now has fewer backends") + Eventually(func() int { + backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) + if err != nil { + return -1 + } + return len(backends) + }, 30*time.Second, 2*time.Second).Should(BeNumerically("<", 3)) + + By("Verifying tool count decreased with new session") + Eventually(func() error { + mcpClient2, err := client.NewStreamableHttpClient(serverURL) + if err != nil { + return err + } + defer mcpClient2.Close() + + testCtx2, cancel2 := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel2() + + err = mcpClient2.Start(testCtx2) + if err != nil { + return err + } + + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "toolhive-e2e-after-remove", + Version: "1.0.0", + } + + _, err = mcpClient2.Initialize(testCtx2, initRequest) + if err != nil { + return err + } + + tools, err := mcpClient2.ListTools(testCtx2, mcp.ListToolsRequest{}) + if err != nil { + return err + } + + if len(tools.Tools) >= toolCountBefore { + return fmt.Errorf("expected fewer tools, got %d (was %d)", len(tools.Tools), toolCountBefore) + } + return nil + }, 1*time.Minute, 10*time.Second).Should(Succeed()) + }) + }) + + Context("when testing health and readiness endpoints", func() { + It("should expose /health endpoint that always returns 200", func() { + vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) + + By("Checking /health endpoint") + resp, err := http.Get(vmcpURL + "/health") + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + + var health map[string]string + err = json.NewDecoder(resp.Body).Decode(&health) + Expect(err).NotTo(HaveOccurred()) + Expect(health["status"]).To(Equal("ok")) + }) + + It("should expose /readyz endpoint", func() { + vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) + + By("Checking /readyz endpoint is accessible") + resp, err := http.Get(vmcpURL + "/readyz") + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + Fail(fmt.Sprintf("unexpected status code: %d, body: %s", resp.StatusCode, string(body))) + } + + By("Parsing readiness response") + var readiness ReadinessResponse + err = json.NewDecoder(resp.Body).Decode(&readiness) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying readiness status") + Expect(readiness.Status).To(Equal("ready"), "Status should be ready") + }) + + It("should distinguish between /health and /readyz", func() { + vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) + + By("Getting /health response") + healthResp, err := http.Get(vmcpURL + "/health") + Expect(err).NotTo(HaveOccurred()) + defer healthResp.Body.Close() + + By("Getting /readyz response") + readyResp, err := http.Get(vmcpURL + "/readyz") + Expect(err).NotTo(HaveOccurred()) + defer readyResp.Body.Close() + + // Both should return 200 when ready + Expect(healthResp.StatusCode).To(Equal(http.StatusOK)) + Expect(readyResp.StatusCode).To(Equal(http.StatusOK)) + + // Parse both responses + var health map[string]string + err = json.NewDecoder(healthResp.Body).Decode(&health) + Expect(err).NotTo(HaveOccurred()) + + var readiness ReadinessResponse + err = json.NewDecoder(readyResp.Body).Decode(&readiness) + Expect(err).NotTo(HaveOccurred()) + + // Health is simple status + Expect(health).To(HaveKey("status")) + Expect(health).NotTo(HaveKey("mode")) + + // Readiness includes status + Expect(readiness.Status).To(Equal("ready")) + }) + }) + + Context("when testing status endpoint", func() { + It("should expose /status endpoint with group reference", func() { + vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) + + By("Checking /status endpoint") + resp, err := http.Get(vmcpURL + "/status") + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + + var status map[string]interface{} + err = json.NewDecoder(resp.Body).Decode(&status) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying group_ref is present") + Expect(status).To(HaveKey("group_ref")) + groupRef, ok := status["group_ref"].(string) + Expect(ok).To(BeTrue()) + Expect(groupRef).To(ContainSubstring(mcpGroupName)) + }) + + It("should list discovered backends in status", func() { + vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) + + By("Getting /status response") + resp, err := http.Get(vmcpURL + "/status") + Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + + var status map[string]interface{} + err = json.NewDecoder(resp.Body).Decode(&status) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying backends are listed") + Expect(status).To(HaveKey("backends")) + backends, ok := status["backends"].([]interface{}) + Expect(ok).To(BeTrue()) + Expect(backends).NotTo(BeEmpty(), "Should have at least one backend") + + // Verify backend structure + backend, ok := backends[0].(map[string]interface{}) + Expect(ok).To(BeTrue(), "backend should be a map") + Expect(backend).To(HaveKey("name")) + Expect(backend).To(HaveKey("health")) + Expect(backend).To(HaveKey("transport")) + }) + }) }) diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go deleted file mode 100644 index d1b745e189..0000000000 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_lifecycle_test.go +++ /dev/null @@ -1,1847 +0,0 @@ -package virtualmcp - -import ( - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "strings" - "time" - - "github.com/mark3labs/mcp-go/client" - "github.com/mark3labs/mcp-go/mcp" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" - - mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" - vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" - "github.com/stacklok/toolhive/test/e2e/images" -) - -// NOTE: These tests verify DynamicRegistry functionality with full operator integration. -// The vMCP server now uses DynamicRegistry in Kubernetes mode and supports dynamic -// backend discovery via BackendWatcher. New sessions will see updated backends when -// they are added/removed from the MCPGroup. Existing sessions retain their original -// capability snapshot. -// -// Implementation status: DynamicRegistry is fully integrated with BackendReconciler that -// watches MCPServer/MCPRemoteProxy resources and updates the registry in real-time. - -// getBackendName safely extracts the backend name from a status response interface. -func getBackendName(b interface{}) string { - if backend, ok := b.(map[string]interface{}); ok { - if name, ok := backend["name"].(string); ok { - return name - } - } - return "" -} - -var _ = Describe("VirtualMCPServer Lifecycle - Dynamic Backend Discovery", Ordered, func() { - var ( - testNamespace = "default" - mcpGroupName = "test-lifecycle-group" - vmcpServerName = "test-vmcp-lifecycle" - backend1Name = "backend-lifecycle-fetch" - backend2Name = "backend-lifecycle-osv" - backend3Name = "backend-lifecycle-dynamic" // Backend added dynamically - timeout = 3 * time.Minute - pollingInterval = 1 * time.Second - vmcpNodePort int32 - ) - - BeforeAll(func() { - By("Creating MCPGroup") - CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, - "Test MCP Group for VirtualMCP lifecycle E2E tests", timeout, pollingInterval) - - By("Creating initial backend MCPServer - fetch (streamable-http)") - backend1 := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend1Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, backend1)).To(Succeed()) - - By("Waiting for initial backend MCPServer to be ready") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backend1Name, - Namespace: testNamespace, - }, server) - if err != nil { - return fmt.Errorf("failed to get server: %w", err) - } - - if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { - return nil - } - return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) - }, timeout, pollingInterval).Should(Succeed(), "Initial backend should be ready") - - By("Creating VirtualMCPServer in discovered mode") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{ - Group: mcpGroupName, - Aggregation: &vmcpconfig.AggregationConfig{ - ConflictResolution: "prefix", - }, - }, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - ServiceType: "NodePort", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - By("Waiting for VirtualMCPServer to discover backends") - WaitForCondition(ctx, k8sClient, vmcpServerName, testNamespace, "BackendsDiscovered", "True", timeout, pollingInterval) - - By("Getting NodePort for VirtualMCPServer") - vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - By(fmt.Sprintf("VirtualMCPServer accessible at http://localhost:%d", vmcpNodePort)) - - By("Waiting for VirtualMCPServer to be accessible") - Eventually(func() error { - httpClient := &http.Client{Timeout: 5 * time.Second} - url := fmt.Sprintf("http://localhost:%d/health", vmcpNodePort) - resp, err := httpClient.Get(url) - if err != nil { - return fmt.Errorf("health check failed: %w", err) - } - defer resp.Body.Close() - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("unexpected status code: %d", resp.StatusCode) - } - return nil - }, 30*time.Second, 2*time.Second).Should(Succeed(), "VirtualMCPServer health endpoint should be accessible") - }) - - AfterAll(func() { - By("Cleaning up VirtualMCPServer") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - } - if err := k8sClient.Delete(ctx, vmcpServer); err != nil { - GinkgoWriter.Printf("Warning: failed to delete VirtualMCPServer: %v\n", err) - } - - By("Cleaning up all backend MCPServers") - for _, backendName := range []string{backend1Name, backend2Name, backend3Name} { - backend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backendName, - Namespace: testNamespace, - }, - } - if err := k8sClient.Delete(ctx, backend); err != nil { - GinkgoWriter.Printf("Warning: failed to delete backend %s: %v\n", backendName, err) - } - } - - By("Cleaning up MCPGroup") - mcpGroup := &mcpv1alpha1.MCPGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: mcpGroupName, - Namespace: testNamespace, - }, - } - if err := k8sClient.Delete(ctx, mcpGroup); err != nil { - GinkgoWriter.Printf("Warning: failed to delete MCPGroup: %v\n", err) - } - }) - - var initialToolCount int - - Context("when testing initial backend discovery", func() { - It("should discover tools from initial backend", func() { - By("Creating MCP client for VirtualMCPServer") - serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - mcpClient, err := client.NewStreamableHttpClient(serverURL) - Expect(err).ToNot(HaveOccurred()) - defer mcpClient.Close() - - By("Starting transport and initializing connection") - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - Eventually(func() error { - initCtx, initCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer initCancel() - - err = mcpClient.Start(initCtx) - if err != nil { - return fmt.Errorf("failed to start transport: %w", err) - } - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "toolhive-e2e-lifecycle-test", - Version: "1.0.0", - } - - _, err = mcpClient.Initialize(initCtx, initRequest) - if err != nil { - return fmt.Errorf("failed to initialize: %w", err) - } - - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "MCP client should initialize successfully") - - By("Listing tools from VirtualMCPServer") - var initialTools *mcp.ListToolsResult - Eventually(func() error { - listRequest := mcp.ListToolsRequest{} - var err error - initialTools, err = mcpClient.ListTools(ctx, listRequest) - if err != nil { - return fmt.Errorf("failed to list tools: %w", err) - } - if len(initialTools.Tools) == 0 { - return fmt.Errorf("no tools returned") - } - return nil - }, 30*time.Second, 2*time.Second).Should(Succeed(), "Should be able to list tools") - - initialToolCount = len(initialTools.Tools) - By(fmt.Sprintf("Initial state: VirtualMCPServer has %d tools", initialToolCount)) - for _, tool := range initialTools.Tools { - GinkgoWriter.Printf(" Initial tool: %s - %s\n", tool.Name, tool.Description) - } - - // Verify we have at least one tool from the initial backend - Expect(initialTools.Tools).ToNot(BeEmpty(), "VirtualMCPServer should have tools from initial backend") - }) - - It("should have exactly one backend in the group", func() { - backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) - Expect(err).ToNot(HaveOccurred()) - Expect(backends).To(HaveLen(1), "Should have exactly one backend initially") - Expect(backends[0].Name).To(Equal(backend1Name)) - }) - }) - - Context("when dynamically adding a new backend", func() { - It("should detect the new backend and update tool list", func() { - By("Adding second backend MCPServer - osv (streamable-http)") - backend2 := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend2Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.OSVMCPServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, backend2)).To(Succeed()) - - By("Waiting for new backend MCPServer to be ready") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backend2Name, - Namespace: testNamespace, - }, server) - if err != nil { - return fmt.Errorf("failed to get server: %w", err) - } - - if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { - return nil - } - return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) - }, timeout, pollingInterval).Should(Succeed(), "New backend should be ready") - - By("Verifying the group now has two backends") - Eventually(func() int { - backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) - if err != nil { - return 0 - } - return len(backends) - }, 30*time.Second, 2*time.Second).Should(Equal(2), "Should have two backends after adding") - - By("Waiting for VirtualMCPServer to reconcile and discover tools from both backends") - // Use Eventually to wait for the VirtualMCPServer to: - // 1. Detect the new backend in the group via operator reconciliation - // 2. Update the DynamicRegistry (which increments version) - // 3. Invalidate cached capabilities - // 4. Rediscover capabilities from both backends - serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - - var updatedTools *mcp.ListToolsResult - Eventually(func() error { - // Create a fresh client for each attempt to ensure we're not hitting stale cache - mcpClient, err := client.NewStreamableHttpClient(serverURL) - if err != nil { - return fmt.Errorf("failed to create client: %w", err) - } - defer mcpClient.Close() - - testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - // Start and initialize - err = mcpClient.Start(testCtx) - if err != nil { - return fmt.Errorf("failed to start transport: %w", err) - } - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "toolhive-e2e-lifecycle-test-add", - Version: "1.0.0", - } - - _, err = mcpClient.Initialize(testCtx, initRequest) - if err != nil { - return fmt.Errorf("failed to initialize: %w", err) - } - - // List tools - listRequest := mcp.ListToolsRequest{} - updatedTools, err = mcpClient.ListTools(testCtx, listRequest) - if err != nil { - return fmt.Errorf("failed to list tools: %w", err) - } - - currentToolCount := len(updatedTools.Tools) - - // Log current state for debugging - if currentToolCount > 0 { - GinkgoWriter.Printf("Attempt: %d tools found (initial: %d)\n", currentToolCount, initialToolCount) - for _, tool := range updatedTools.Tools { - GinkgoWriter.Printf(" - %s\n", tool.Name) - } - } - - // Should have more tools now (from both backends) - // Check if tool count increased from initial state - if currentToolCount <= initialToolCount { - return fmt.Errorf("expected more tools after adding backend, got %d (initial: %d)", currentToolCount, initialToolCount) - } - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "Should see more tools after adding second backend") - - By(fmt.Sprintf("After adding backend: VirtualMCPServer now has %d tools", len(updatedTools.Tools))) - for _, tool := range updatedTools.Tools { - GinkgoWriter.Printf(" Updated tool: %s - %s\n", tool.Name, tool.Description) - } - }) - }) - - Context("when dynamically removing a backend", func() { - It("should detect backend removal and update tool list", func() { - By("Getting current tool count") - serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - mcpClient, err := client.NewStreamableHttpClient(serverURL) - Expect(err).ToNot(HaveOccurred()) - defer mcpClient.Close() - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - Eventually(func() error { - initCtx, initCancel := context.WithTimeout(context.Background(), 10*time.Second) - defer initCancel() - - err = mcpClient.Start(initCtx) - if err != nil { - return fmt.Errorf("failed to start transport: %w", err) - } - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "toolhive-e2e-lifecycle-test-before-remove", - Version: "1.0.0", - } - - _, err = mcpClient.Initialize(initCtx, initRequest) - if err != nil { - return fmt.Errorf("failed to initialize: %w", err) - } - - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed()) - - var toolsBeforeRemoval *mcp.ListToolsResult - Eventually(func() error { - listRequest := mcp.ListToolsRequest{} - var err error - toolsBeforeRemoval, err = mcpClient.ListTools(ctx, listRequest) - if err != nil { - return fmt.Errorf("failed to list tools: %w", err) - } - return nil - }, 30*time.Second, 2*time.Second).Should(Succeed()) - - toolCountBefore := len(toolsBeforeRemoval.Tools) - By(fmt.Sprintf("Before removal: %d tools", toolCountBefore)) - - By("Removing the second backend (osv)") - backend2 := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend2Name, - Namespace: testNamespace, - }, - } - Expect(k8sClient.Delete(ctx, backend2)).To(Succeed()) - - By("Waiting for backend deletion to complete") - Eventually(func() bool { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backend2Name, - Namespace: testNamespace, - }, server) - return err != nil // Should fail to get when deleted - }, timeout, pollingInterval).Should(BeTrue(), "Backend should be deleted") - - By("Verifying the group now has one backend") - Eventually(func() int { - backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) - if err != nil { - return -1 - } - return len(backends) - }, 30*time.Second, 2*time.Second).Should(Equal(1), "Should have one backend after removal") - - By("Waiting for VirtualMCPServer to detect backend removal and update tool list") - var toolsAfterRemoval *mcp.ListToolsResult - Eventually(func() error { - // Create a fresh client for each attempt - mcpClient2, err := client.NewStreamableHttpClient(serverURL) - if err != nil { - return fmt.Errorf("failed to create client: %w", err) - } - defer mcpClient2.Close() - - testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - // Start and initialize - err = mcpClient2.Start(testCtx) - if err != nil { - return fmt.Errorf("failed to start transport: %w", err) - } - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "toolhive-e2e-lifecycle-test-after-remove", - Version: "1.0.0", - } - - _, err = mcpClient2.Initialize(testCtx, initRequest) - if err != nil { - return fmt.Errorf("failed to initialize: %w", err) - } - - // List tools - listRequest := mcp.ListToolsRequest{} - toolsAfterRemoval, err = mcpClient2.ListTools(testCtx, listRequest) - if err != nil { - return fmt.Errorf("failed to list tools: %w", err) - } - - toolCountAfter := len(toolsAfterRemoval.Tools) - - // Verify tool count decreased (tools from removed backend are gone) - if toolCountAfter >= toolCountBefore { - return fmt.Errorf("expected fewer tools after removal, got %d (was %d)", toolCountAfter, toolCountBefore) - } - - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "Should have fewer tools after backend removal") - - By(fmt.Sprintf("After removal: %d tools (was %d)", len(toolsAfterRemoval.Tools), toolCountBefore)) - - By("Verifying tools from removed backend are no longer present") - for _, tool := range toolsAfterRemoval.Tools { - GinkgoWriter.Printf(" Remaining tool: %s - %s\n", tool.Name, tool.Description) - // Tools from osv backend should not be present - Expect(strings.Contains(strings.ToLower(tool.Name), "osv")).To(BeFalse(), - "Tools from removed osv backend should not be present") - } - }) - }) - - Context("when testing cache invalidation", func() { - It("should invalidate cache when backends change", func() { - By("Adding a third backend to trigger cache invalidation") - backend3 := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend3Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, // Use fetch image for simplicity - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, backend3)).To(Succeed()) - - By("Waiting for new backend to be ready") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backend3Name, - Namespace: testNamespace, - }, server) - if err != nil { - return fmt.Errorf("failed to get server: %w", err) - } - - if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { - return nil - } - return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) - }, timeout, pollingInterval).Should(Succeed()) - - By("Verifying tool list is updated (cache was invalidated)") - serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - - var tools *mcp.ListToolsResult - Eventually(func() error { - // Create a fresh client for each attempt - mcpClient, err := client.NewStreamableHttpClient(serverURL) - if err != nil { - return fmt.Errorf("failed to create client: %w", err) - } - defer mcpClient.Close() - - testCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - // Start and initialize - err = mcpClient.Start(testCtx) - if err != nil { - return fmt.Errorf("failed to start transport: %w", err) - } - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "toolhive-e2e-lifecycle-test-cache", - Version: "1.0.0", - } - - _, err = mcpClient.Initialize(testCtx, initRequest) - if err != nil { - return fmt.Errorf("failed to initialize: %w", err) - } - - // List tools - listRequest := mcp.ListToolsRequest{} - tools, err = mcpClient.ListTools(testCtx, listRequest) - if err != nil { - return fmt.Errorf("failed to list tools: %w", err) - } - - // Should now have tools from 2 backends (backend1 and backend3) - if len(tools.Tools) < 1 { - return fmt.Errorf("expected tools from active backends, got %d", len(tools.Tools)) - } - - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "Cache should be invalidated and show updated tools") - - By(fmt.Sprintf("After cache invalidation: VirtualMCPServer has %d tools from active backends", len(tools.Tools))) - - By("Verifying backends in the group") - backends, err := GetMCPGroupBackends(ctx, k8sClient, mcpGroupName, testNamespace) - Expect(err).ToNot(HaveOccurred()) - Expect(backends).To(HaveLen(2), "Should have two backends after adding third backend") - - backendNames := make([]string, len(backends)) - for i, backend := range backends { - backendNames[i] = backend.Name - } - Expect(backendNames).To(ContainElements(backend1Name, backend3Name)) - Expect(backendNames).ToNot(ContainElement(backend2Name), "Removed backend should not be present") - }) - }) -}) - -// ReadinessResponse represents the /readyz endpoint response -type ReadinessResponse struct { - Status string `json:"status"` - Mode string `json:"mode"` - Reason string `json:"reason,omitempty"` -} - -// VirtualMCPServer K8s Manager Infrastructure Tests -// These tests verify the K8s manager integration that was implemented as part of THV-2884. -// This includes BackendWatcher with BackendReconciler for dynamic backend discovery, -// manager creation, readiness probes, cache sync, and endpoint behavior. -var _ = Describe("VirtualMCPServer K8s Manager Infrastructure", Ordered, func() { - var ( - testNamespace = "default" - mcpGroupName = "test-k8s-manager-infra-group" - vmcpServerName = "test-vmcp-k8s-manager-infra" - backendName = "backend-k8s-manager-infra-fetch" - timeout = 3 * time.Minute - pollingInterval = 2 * time.Second - vmcpNodePort int32 - ) - - BeforeAll(func() { - By("Creating MCPGroup for K8s manager infrastructure tests") - CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, - "Test MCP Group for K8s manager infrastructure E2E tests", timeout, pollingInterval) - - By("Creating backend MCPServer") - backend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backendName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, backend)).To(Succeed()) - - By("Waiting for backend MCPServer to be ready") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backendName, - Namespace: testNamespace, - }, server) - if err != nil { - return fmt.Errorf("failed to get server: %w", err) - } - - if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { - return nil - } - return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) - }, timeout, pollingInterval).Should(Succeed(), "Backend should be ready") - - By("Creating VirtualMCPServer with discovered auth source (dynamic mode)") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{ - Group: mcpGroupName, - Aggregation: &vmcpconfig.AggregationConfig{ - ConflictResolution: "prefix", - }, - }, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "discovered", // This triggers K8s manager creation - }, - ServiceType: "NodePort", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - By("Getting NodePort for VirtualMCPServer") - vmcpNodePort = GetVMCPNodePort(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - By(fmt.Sprintf("VirtualMCPServer is ready on NodePort: %d", vmcpNodePort)) - - By("Waiting for VirtualMCPServer to be accessible") - Eventually(func() error { - httpClient := &http.Client{Timeout: 5 * time.Second} - url := fmt.Sprintf("http://localhost:%d/health", vmcpNodePort) - resp, err := httpClient.Get(url) - if err != nil { - return fmt.Errorf("health check failed: %w", err) - } - defer resp.Body.Close() - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return fmt.Errorf("unexpected status code: %d", resp.StatusCode) - } - return nil - }, 30*time.Second, 2*time.Second).Should(Succeed(), "VirtualMCPServer health endpoint should be accessible") - }) - - AfterAll(func() { - By("Cleaning up VirtualMCPServer") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, vmcpServer) - - By("Cleaning up backend MCPServer") - backend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backendName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, backend) - - By("Cleaning up MCPGroup") - group := &mcpv1alpha1.MCPGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: mcpGroupName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, group) - }) - - Context("Readiness Probe Integration", func() { - It("should expose /readyz endpoint", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Checking /readyz endpoint is accessible") - Eventually(func() error { - resp, err := http.Get(vmcpURL + "/readyz") - if err != nil { - return fmt.Errorf("failed to connect to /readyz: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("unexpected status code: %d, body: %s", resp.StatusCode, string(body)) - } - - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "/readyz should return 200 OK") - }) - - It("should return dynamic mode status", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Getting /readyz response") - resp, err := http.Get(vmcpURL + "/readyz") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - Expect(resp.StatusCode).To(Equal(http.StatusOK)) - - By("Parsing readiness response") - var readiness ReadinessResponse - err = json.NewDecoder(resp.Body).Decode(&readiness) - Expect(err).NotTo(HaveOccurred()) - - By("Verifying dynamic mode is enabled") - Expect(readiness.Status).To(Equal("ready"), "Status should be ready") - Expect(readiness.Mode).To(Equal("dynamic"), "Mode should be dynamic since outgoingAuth.source is 'discovered'") - }) - - It("should indicate cache sync in dynamic mode", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Verifying cache is synced") - resp, err := http.Get(vmcpURL + "/readyz") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - var readiness ReadinessResponse - err = json.NewDecoder(resp.Body).Decode(&readiness) - Expect(err).NotTo(HaveOccurred()) - - // In dynamic mode with synced cache, status should be "ready" - Expect(readiness.Status).To(Equal("ready")) - Expect(readiness.Mode).To(Equal("dynamic")) - // Reason should be empty when ready - Expect(readiness.Reason).To(BeEmpty()) - }) - }) - - Context("K8s Manager Lifecycle", func() { - It("should start with K8s manager running", func() { - By("Verifying pod is running") - Eventually(func() error { - pods := &corev1.PodList{} - err := k8sClient.List(ctx, pods, - ctrlclient.InNamespace(testNamespace), - ctrlclient.MatchingLabels{"app.kubernetes.io/instance": vmcpServerName}) - if err != nil { - return fmt.Errorf("failed to list pods: %w", err) - } - - if len(pods.Items) == 0 { - return fmt.Errorf("no pods found") - } - - pod := pods.Items[0] - if pod.Status.Phase != corev1.PodRunning { - return fmt.Errorf("pod not running yet, phase: %s", pod.Status.Phase) - } - - // Check pod is ready - for _, condition := range pod.Status.Conditions { - if condition.Type == corev1.PodReady { - if condition.Status != corev1.ConditionTrue { - return fmt.Errorf("pod not ready: %s", condition.Message) - } - return nil - } - } - - return fmt.Errorf("pod ready condition not found") - }, timeout, pollingInterval).Should(Succeed(), "Pod should be running and ready") - }) - - It("should have healthy container status", func() { - By("Getting pod name") - pods := &corev1.PodList{} - err := k8sClient.List(ctx, pods, - ctrlclient.InNamespace(testNamespace), - ctrlclient.MatchingLabels{"app.kubernetes.io/instance": vmcpServerName}) - Expect(err).NotTo(HaveOccurred()) - Expect(pods.Items).NotTo(BeEmpty(), "Should have at least one pod") - - podName := pods.Items[0].Name - - By("Checking container status") - Eventually(func() error { - pod := &corev1.Pod{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: podName, - Namespace: testNamespace, - }, pod) - if err != nil { - return err - } - - // Check all containers are ready - for _, status := range pod.Status.ContainerStatuses { - if !status.Ready { - return fmt.Errorf("container %s not ready", status.Name) - } - } - - return nil - }, timeout, pollingInterval).Should(Succeed(), "All containers should be ready") - }) - }) - - Context("Dynamic Backend Discovery Lifecycle", func() { - var ( - dynamicBackend1Name = "dynamic-backend-1" - dynamicBackend2Name = "dynamic-backend-2" - ) - - AfterEach(func() { - // Cleanup any dynamic backends created in tests - _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: dynamicBackend1Name, - Namespace: testNamespace, - }, - }) - _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: dynamicBackend2Name, - Namespace: testNamespace, - }, - }) - }) - - It("should discover new backends added to the group", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Getting initial backend count") - resp, err := http.Get(vmcpURL + "/status") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - var initialStatus map[string]interface{} - err = json.NewDecoder(resp.Body).Decode(&initialStatus) - Expect(err).NotTo(HaveOccurred()) - - initialBackends, ok := initialStatus["backends"].([]interface{}) - Expect(ok).To(BeTrue(), "backends field should be an array") - initialCount := len(initialBackends) - - By("Creating a new backend MCPServer in the same group") - newBackend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: dynamicBackend1Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, newBackend)).To(Succeed()) - - By("Waiting for new backend to be running") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: dynamicBackend1Name, - Namespace: testNamespace, - }, server) - if err != nil { - return err - } - if server.Status.Phase != mcpv1alpha1.MCPServerPhaseRunning { - return fmt.Errorf("backend not running yet, phase: %s", server.Status.Phase) - } - return nil - }, timeout, pollingInterval).Should(Succeed()) - - By("Verifying new backend appears in vMCP status") - Eventually(func() bool { - resp, err := http.Get(vmcpURL + "/status") - if err != nil { - return false - } - defer resp.Body.Close() - - var status map[string]interface{} - if err := json.NewDecoder(resp.Body).Decode(&status); err != nil { - return false - } - - backends, ok := status["backends"].([]interface{}) - if !ok { - return false - } - if len(backends) != initialCount+1 { - return false - } - - // Check that the new backend is in the list - for _, b := range backends { - if strings.Contains(getBackendName(b), dynamicBackend1Name) { - return true - } - } - return false - }, timeout, pollingInterval).Should(BeTrue(), "New backend should be discovered") - }) - - It("should remove backends deleted from the group", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Creating a backend to be deleted") - tempBackend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: dynamicBackend2Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, tempBackend)).To(Succeed()) - - By("Waiting for backend to be running and discovered") - Eventually(func() bool { - resp, err := http.Get(vmcpURL + "/status") - if err != nil { - return false - } - defer resp.Body.Close() - - var status map[string]interface{} - if err := json.NewDecoder(resp.Body).Decode(&status); err != nil { - return false - } - - backends, ok := status["backends"].([]interface{}) - if !ok { - return false - } - for _, b := range backends { - if strings.Contains(getBackendName(b), dynamicBackend2Name) { - return true - } - } - return false - }, timeout, pollingInterval).Should(BeTrue(), "Backend should be discovered") - - By("Deleting the backend") - Expect(k8sClient.Delete(ctx, tempBackend)).To(Succeed()) - - By("Waiting for backend to be fully deleted from K8s") - Eventually(func() bool { - err := k8sClient.Get(ctx, ctrlclient.ObjectKey{ - Name: dynamicBackend2Name, - Namespace: testNamespace, - }, &mcpv1alpha1.MCPServer{}) - return errors.IsNotFound(err) - }, timeout, pollingInterval).Should(BeTrue(), "Backend should be deleted from K8s") - - By("Waiting for backend pod to be deleted") - Eventually(func() int { - podList := &corev1.PodList{} - err := k8sClient.List(ctx, podList, ctrlclient.InNamespace(testNamespace), - ctrlclient.MatchingLabels{"app.kubernetes.io/name": dynamicBackend2Name}) - if err != nil { - return -1 - } - return len(podList.Items) - }, timeout, pollingInterval).Should(Equal(0), "Backend pods should be deleted") - - By("Verifying backend is removed from vMCP status") - Eventually(func() bool { - resp, err := http.Get(vmcpURL + "/status") - if err != nil { - return false - } - defer resp.Body.Close() - - var status map[string]interface{} - if err := json.NewDecoder(resp.Body).Decode(&status); err != nil { - return false - } - - backends, ok := status["backends"].([]interface{}) - if !ok { - return false - } - - for _, b := range backends { - if strings.Contains(getBackendName(b), dynamicBackend2Name) { - return false // Backend still present - } - } - return true // Backend not found (removed) - }, timeout*2, pollingInterval).Should(BeTrue(), "Deleted backend should be removed from status") - }) - - It("should not discover backends from different groups", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - differentGroup := "different-group" - - By("Creating a group with a different name") - CreateMCPGroupAndWait(ctx, k8sClient, differentGroup, testNamespace, - "Different group for isolation testing", timeout, pollingInterval) - defer func() { - _ = k8sClient.Delete(ctx, &mcpv1alpha1.MCPGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: differentGroup, - Namespace: testNamespace, - }, - }) - }() - - By("Creating a backend in the different group") - otherGroupBackend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "other-group-backend", - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: differentGroup, // Different group - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, otherGroupBackend)).To(Succeed()) - defer func() { - _ = k8sClient.Delete(ctx, otherGroupBackend) - }() - - By("Waiting for backend to be running") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: "other-group-backend", - Namespace: testNamespace, - }, server) - if err != nil { - return err - } - if server.Status.Phase != mcpv1alpha1.MCPServerPhaseRunning { - return fmt.Errorf("backend not running yet") - } - return nil - }, timeout, pollingInterval).Should(Succeed()) - - By("Verifying backend from different group is NOT discovered") - Consistently(func() bool { - resp, err := http.Get(vmcpURL + "/status") - if err != nil { - return true // Continue checking - } - defer resp.Body.Close() - - var status map[string]interface{} - if err := json.NewDecoder(resp.Body).Decode(&status); err != nil { - return true - } - - backends, ok := status["backends"].([]interface{}) - if !ok { - return true // Continue checking if structure is unexpected - } - for _, b := range backends { - if strings.Contains(getBackendName(b), "other-group-backend") { - return false // Backend found - test should fail - } - } - return true // Backend not found - correct - }, 10*time.Second, pollingInterval).Should(BeTrue(), "Backend from different group should not be discovered") - }) - }) - - Context("Health Endpoints", func() { - It("should expose /health endpoint that always returns 200", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Checking /health endpoint") - resp, err := http.Get(vmcpURL + "/health") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - Expect(resp.StatusCode).To(Equal(http.StatusOK)) - - var health map[string]string - err = json.NewDecoder(resp.Body).Decode(&health) - Expect(err).NotTo(HaveOccurred()) - Expect(health["status"]).To(Equal("ok")) - }) - - It("should distinguish between /health and /readyz", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Getting /health response") - healthResp, err := http.Get(vmcpURL + "/health") - Expect(err).NotTo(HaveOccurred()) - defer healthResp.Body.Close() - - By("Getting /readyz response") - readyResp, err := http.Get(vmcpURL + "/readyz") - Expect(err).NotTo(HaveOccurred()) - defer readyResp.Body.Close() - - // Both should return 200 when ready - Expect(healthResp.StatusCode).To(Equal(http.StatusOK)) - Expect(readyResp.StatusCode).To(Equal(http.StatusOK)) - - // Parse both responses - var health map[string]string - err = json.NewDecoder(healthResp.Body).Decode(&health) - Expect(err).NotTo(HaveOccurred()) - - var readiness ReadinessResponse - err = json.NewDecoder(readyResp.Body).Decode(&readiness) - Expect(err).NotTo(HaveOccurred()) - - // Health is simple status - Expect(health).To(HaveKey("status")) - Expect(health).NotTo(HaveKey("mode")) - - // Readiness includes mode information - Expect(readiness.Status).To(Equal("ready")) - Expect(readiness.Mode).To(Equal("dynamic")) - }) - }) - - Context("Status Endpoint", func() { - It("should expose /status endpoint with group reference", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Checking /status endpoint") - resp, err := http.Get(vmcpURL + "/status") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - Expect(resp.StatusCode).To(Equal(http.StatusOK)) - - var status map[string]interface{} - err = json.NewDecoder(resp.Body).Decode(&status) - Expect(err).NotTo(HaveOccurred()) - - By("Verifying group_ref is present") - Expect(status).To(HaveKey("group_ref")) - groupRef, ok := status["group_ref"].(string) - Expect(ok).To(BeTrue()) - Expect(groupRef).To(ContainSubstring(mcpGroupName)) - }) - - It("should list discovered backends", func() { - vmcpURL := fmt.Sprintf("http://localhost:%d", vmcpNodePort) - - By("Getting /status response") - resp, err := http.Get(vmcpURL + "/status") - Expect(err).NotTo(HaveOccurred()) - defer resp.Body.Close() - - var status map[string]interface{} - err = json.NewDecoder(resp.Body).Decode(&status) - Expect(err).NotTo(HaveOccurred()) - - By("Verifying backends are listed") - Expect(status).To(HaveKey("backends")) - backends, ok := status["backends"].([]interface{}) - Expect(ok).To(BeTrue()) - Expect(backends).NotTo(BeEmpty(), "Should have at least one backend") - - // Verify backend structure - backend, ok := backends[0].(map[string]interface{}) - Expect(ok).To(BeTrue(), "backend should be a map") - Expect(backend).To(HaveKey("name")) - Expect(backend).To(HaveKey("health")) - Expect(backend).To(HaveKey("transport")) - }) - }) -}) - -// VirtualMCPServer Dynamic vs Static Mode Tests -// These tests verify the operator behavior for different outgoingAuth.source modes: -// - discovered (dynamic): vMCP discovers backends at runtime via K8s API, requires RBAC -// - inline (static): All backends are pre-configured in ConfigMap, no RBAC needed -var _ = Describe("VirtualMCPServer Mode Configuration", Ordered, func() { - var ( - testNamespace = "default" - mcpGroupName = "test-mode-config-group" - backend1Name = "backend-mode-fetch" - timeout = 3 * time.Minute - pollingInterval = 2 * time.Second - ) - - BeforeAll(func() { - By("Creating MCPGroup for mode configuration tests") - CreateMCPGroupAndWait(ctx, k8sClient, mcpGroupName, testNamespace, - "Test MCP Group for mode configuration E2E tests", timeout, pollingInterval) - - By("Creating backend MCPServer") - backend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend1Name, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.MCPServerSpec{ - GroupRef: mcpGroupName, - Image: images.GofetchServerImage, - Transport: "streamable-http", - ProxyPort: 8080, - McpPort: 8080, - }, - } - Expect(k8sClient.Create(ctx, backend)).To(Succeed()) - - By("Waiting for backend MCPServer to be ready") - Eventually(func() error { - server := &mcpv1alpha1.MCPServer{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: backend1Name, - Namespace: testNamespace, - }, server) - if err != nil { - return fmt.Errorf("failed to get server: %w", err) - } - - if server.Status.Phase == mcpv1alpha1.MCPServerPhaseRunning { - return nil - } - return fmt.Errorf("backend not ready yet, phase: %s", server.Status.Phase) - }, timeout, pollingInterval).Should(Succeed(), "Backend should be ready") - }) - - AfterAll(func() { - By("Cleaning up backend MCPServer") - backend := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: backend1Name, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, backend) - - By("Cleaning up MCPGroup") - group := &mcpv1alpha1.MCPGroup{ - ObjectMeta: metav1.ObjectMeta{ - Name: mcpGroupName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, group) - }) - - Context("Dynamic Mode (discovered)", func() { - var vmcpServerName = "test-vmcp-dynamic-mode" - - AfterEach(func() { - By("Cleaning up VirtualMCPServer") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, vmcpServer) - - By("Waiting for VirtualMCPServer deletion") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - return err != nil - }, timeout, pollingInterval).Should(BeTrue()) - }) - - It("should create RBAC resources in dynamic mode", func() { - By("Creating VirtualMCPServer with discovered source (dynamic mode)") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{Group: mcpGroupName}, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "discovered", // Dynamic mode - should create RBAC - }, - ServiceType: "ClusterIP", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) - - By("Verifying ServiceAccount was created") - sa := &corev1.ServiceAccount{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist in dynamic mode") - - By("Verifying Role was created with K8s API permissions") - role := &rbacv1.Role{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, role) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "Role should exist in dynamic mode") - - // Verify Role has correct permissions - Expect(role.Rules).NotTo(BeEmpty()) - - // Check for ConfigMap and Secret permissions - hasConfigMapPerms := false - hasToolHivePerms := false - hasStatusPerms := false - - for _, rule := range role.Rules { - // Check for ConfigMap/Secret read permissions - if len(rule.APIGroups) > 0 && rule.APIGroups[0] == "" { - for _, resource := range rule.Resources { - if resource == "configmaps" || resource == "secrets" { - hasConfigMapPerms = true - } - } - } - - // Check for ToolHive resource read permissions - if len(rule.APIGroups) > 0 && rule.APIGroups[0] == "toolhive.stacklok.dev" { - for _, resource := range rule.Resources { - if resource == "mcpgroups" || resource == "mcpservers" { - hasToolHivePerms = true - } - if resource == "virtualmcpservers/status" { - hasStatusPerms = true - } - } - } - } - - Expect(hasConfigMapPerms).To(BeTrue(), "Role should have ConfigMap/Secret read permissions") - Expect(hasToolHivePerms).To(BeTrue(), "Role should have ToolHive resource read permissions") - Expect(hasStatusPerms).To(BeTrue(), "Role should have status update permissions") - - By("Verifying RoleBinding was created") - rb := &rbacv1.RoleBinding{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, rb) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "RoleBinding should exist in dynamic mode") - - // Verify RoleBinding references correct ServiceAccount and Role - Expect(rb.RoleRef.Name).To(Equal(serviceAccountName)) - Expect(rb.RoleRef.Kind).To(Equal("Role")) - Expect(rb.Subjects).To(HaveLen(1)) - Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount")) - Expect(rb.Subjects[0].Name).To(Equal(serviceAccountName)) - - By("Verifying Deployment uses the ServiceAccount") - deployment := &appsv1.Deployment{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, deployment) - }, 30*time.Second, 2*time.Second).Should(Succeed()) - - Expect(deployment.Spec.Template.Spec.ServiceAccountName).To(Equal(serviceAccountName), - "Deployment should use the created ServiceAccount in dynamic mode") - }) - - It("should have minimal ConfigMap in dynamic mode", func() { - By("Creating VirtualMCPServer with discovered source (dynamic mode)") - // Use local variable to avoid modifying Context-level vmcpServerName - localVmcpName := vmcpServerName + "-configmap" - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: localVmcpName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{Group: mcpGroupName}, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "discovered", // Dynamic mode - }, - ServiceType: "ClusterIP", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - // Add explicit cleanup for this test's resource with the modified name - DeferCleanup(func() { - By("Cleaning up VirtualMCPServer with modified name") - _ = k8sClient.Delete(ctx, vmcpServer) - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: localVmcpName, - Namespace: testNamespace, - }, vmcpServer) - return err != nil - }, timeout, pollingInterval).Should(BeTrue()) - }) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, localVmcpName, testNamespace, timeout, pollingInterval) - - By("Verifying ConfigMap contains minimal content") - configMapName := fmt.Sprintf("%s-vmcp-config", localVmcpName) - configMap := &corev1.ConfigMap{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: configMapName, - Namespace: testNamespace, - }, configMap) - }, 30*time.Second, 2*time.Second).Should(Succeed()) - - Expect(configMap.Data).To(HaveKey("config.yaml")) - configYAML := configMap.Data["config.yaml"] - - // In dynamic mode, ConfigMap should NOT contain backend URLs/details - // vMCP discovers these at runtime via K8s API - Expect(configYAML).To(ContainSubstring("source: discovered")) - Expect(configYAML).NotTo(ContainSubstring("url: http://"), - "Dynamic mode ConfigMap should not contain backend URLs") - }) - }) - - Context("Static Mode (inline)", func() { - var vmcpServerName = "test-vmcp-static-mode" - - AfterEach(func() { - By("Cleaning up VirtualMCPServer") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, vmcpServer) - - By("Waiting for VirtualMCPServer deletion") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - return err != nil - }, timeout, pollingInterval).Should(BeTrue()) - }) - - It("should NOT create RBAC resources in static mode", func() { - By("Creating VirtualMCPServer with inline source (static mode)") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{Group: mcpGroupName}, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "inline", // Static mode - should NOT create RBAC - }, - ServiceType: "ClusterIP", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) - - By("Verifying ServiceAccount was NOT created") - sa := &corev1.ServiceAccount{} - Consistently(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - return err != nil // Should not exist - }, 10*time.Second, 2*time.Second).Should(BeTrue(), "ServiceAccount should not exist in static mode") - - By("Verifying Role was NOT created") - role := &rbacv1.Role{} - Consistently(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, role) - return err != nil // Should not exist - }, 10*time.Second, 2*time.Second).Should(BeTrue(), "Role should not exist in static mode") - - By("Verifying RoleBinding was NOT created") - rb := &rbacv1.RoleBinding{} - Consistently(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, rb) - return err != nil // Should not exist - }, 10*time.Second, 2*time.Second).Should(BeTrue(), "RoleBinding should not exist in static mode") - - By("Verifying Deployment uses default ServiceAccount") - deployment := &appsv1.Deployment{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, deployment) - }, 30*time.Second, 2*time.Second).Should(Succeed()) - - // In static mode, ServiceAccountName should be empty (uses default) - Expect(deployment.Spec.Template.Spec.ServiceAccountName).To(BeEmpty(), - "Deployment should use default ServiceAccount in static mode") - }) - }) - - Context("Mode Switching", func() { - var vmcpServerName = "test-vmcp-mode-switch" - - AfterEach(func() { - By("Cleaning up VirtualMCPServer") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - } - _ = k8sClient.Delete(ctx, vmcpServer) - - By("Waiting for all resources to be cleaned up") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - return err != nil - }, timeout, pollingInterval).Should(BeTrue()) - }) - - It("should preserve RBAC resources when switching from dynamic to static", func() { - By("Creating VirtualMCPServer in dynamic mode") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{Group: mcpGroupName}, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "discovered", // Start in dynamic mode - }, - ServiceType: "ClusterIP", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready with RBAC") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) - - By("Verifying RBAC resources exist in dynamic mode") - sa := &corev1.ServiceAccount{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist before mode switch") - - By("Switching to static mode") - Eventually(func() error { - // Get latest version - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - if err != nil { - return err - } - - // Update to static mode - vmcpServer.Spec.OutgoingAuth.Source = "inline" - return k8sClient.Update(ctx, vmcpServer) - }, 30*time.Second, 2*time.Second).Should(Succeed()) - - By("Verifying RBAC resources remain after mode switch (left for garbage collection)") - // When switching dynamic→static, RBAC resources are NOT actively deleted. - // They persist with owner references and will be garbage collected on VirtualMCPServer deletion. - Consistently(func() error { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - return err - }, 15*time.Second, 2*time.Second).Should(Succeed(), - "ServiceAccount should persist after dynamic→static mode switch") - - By("Verifying Role remains (left for garbage collection)") - role := &rbacv1.Role{} - Consistently(func() error { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, role) - return err - }, 15*time.Second, 2*time.Second).Should(Succeed(), - "Role should persist after dynamic→static mode switch") - - By("Verifying RoleBinding remains (left for garbage collection)") - rb := &rbacv1.RoleBinding{} - Consistently(func() error { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, rb) - return err - }, 15*time.Second, 2*time.Second).Should(Succeed(), - "RoleBinding should persist after dynamic→static mode switch") - - By("Verifying Deployment uses default ServiceAccount in static mode") - Eventually(func() string { - deployment := &appsv1.Deployment{} - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, deployment) - if err != nil { - return "error" - } - return deployment.Spec.Template.Spec.ServiceAccountName - }, 30*time.Second, 2*time.Second).Should(BeEmpty(), "Deployment should use default ServiceAccount (empty string) in static mode") - - By("Verifying VirtualMCPServer is still ready after mode switch") - Eventually(func() error { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - if err != nil { - return err - } - - if vmcpServer.Status.Phase != mcpv1alpha1.VirtualMCPServerPhaseReady { - return fmt.Errorf("VirtualMCPServer not ready, phase: %s", vmcpServer.Status.Phase) - } - return nil - }, 2*time.Minute, 5*time.Second).Should(Succeed(), "VirtualMCPServer should remain ready after mode switch") - }) - }) - - Context("Garbage Collection", func() { - var vmcpServerName = "test-vmcp-gc" - - It("should garbage collect RBAC resources when VirtualMCPServer is deleted", func() { - By("Creating VirtualMCPServer in dynamic mode") - vmcpServer := &mcpv1alpha1.VirtualMCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: vmcpServerName, - Namespace: testNamespace, - }, - Spec: mcpv1alpha1.VirtualMCPServerSpec{ - Config: vmcpconfig.Config{Group: mcpGroupName}, - IncomingAuth: &mcpv1alpha1.IncomingAuthConfig{ - Type: "anonymous", - }, - OutgoingAuth: &mcpv1alpha1.OutgoingAuthConfig{ - Source: "discovered", // Dynamic mode - creates RBAC - }, - ServiceType: "ClusterIP", - }, - } - Expect(k8sClient.Create(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be ready") - WaitForVirtualMCPServerReady(ctx, k8sClient, vmcpServerName, testNamespace, timeout, pollingInterval) - - serviceAccountName := fmt.Sprintf("%s-vmcp", vmcpServerName) - - By("Verifying RBAC resources exist") - sa := &corev1.ServiceAccount{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "ServiceAccount should exist") - - role := &rbacv1.Role{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, role) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "Role should exist") - - rb := &rbacv1.RoleBinding{} - Eventually(func() error { - return k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, rb) - }, 30*time.Second, 2*time.Second).Should(Succeed(), "RoleBinding should exist") - - By("Verifying RBAC resources have owner references to VirtualMCPServer") - Expect(sa.OwnerReferences).NotTo(BeEmpty(), "ServiceAccount should have owner references") - Expect(sa.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) - Expect(sa.OwnerReferences[0].Name).To(Equal(vmcpServerName)) - - Expect(role.OwnerReferences).NotTo(BeEmpty(), "Role should have owner references") - Expect(role.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) - Expect(role.OwnerReferences[0].Name).To(Equal(vmcpServerName)) - - Expect(rb.OwnerReferences).NotTo(BeEmpty(), "RoleBinding should have owner references") - Expect(rb.OwnerReferences[0].Kind).To(Equal("VirtualMCPServer")) - Expect(rb.OwnerReferences[0].Name).To(Equal(vmcpServerName)) - - By("Deleting VirtualMCPServer") - Expect(k8sClient.Delete(ctx, vmcpServer)).To(Succeed()) - - By("Waiting for VirtualMCPServer to be fully deleted") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: vmcpServerName, - Namespace: testNamespace, - }, vmcpServer) - return errors.IsNotFound(err) - }, timeout, pollingInterval).Should(BeTrue(), "VirtualMCPServer should be deleted") - - By("Verifying ServiceAccount is garbage collected") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, sa) - return errors.IsNotFound(err) - }, timeout, pollingInterval).Should(BeTrue(), "ServiceAccount should be garbage collected via owner reference") - - By("Verifying Role is garbage collected") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, role) - return errors.IsNotFound(err) - }, timeout, pollingInterval).Should(BeTrue(), "Role should be garbage collected via owner reference") - - By("Verifying RoleBinding is garbage collected") - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{ - Name: serviceAccountName, - Namespace: testNamespace, - }, rb) - return errors.IsNotFound(err) - }, timeout, pollingInterval).Should(BeTrue(), "RoleBinding should be garbage collected via owner reference") - }) - }) -})