From ed1ebdb0dc14c76d53fd1bd7cf8e8f3c3acc48d6 Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Tue, 14 Oct 2025 13:57:18 +0100 Subject: [PATCH 1/3] removes flag driven config for operator and proxyrunner Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- .../controllers/mcpserver_controller.go | 209 ++++-------------- .../controllers/mcpserver_runconfig.go | 175 +++++++++++++-- 2 files changed, 195 insertions(+), 189 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 3f80de163..c3dffcfa9 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -84,9 +84,6 @@ var defaultRBACRules = []rbacv1.PolicyRule{ // mcpContainerName is the name of the mcp container used in pod templates const mcpContainerName = "mcp" -// trueValue is the string value "true" used for environment variable comparisons -const trueValue = "true" - // Restart annotation keys for triggering pod restart const ( RestartedAtAnnotationKey = "mcpserver.toolhive.stacklok.dev/restarted-at" @@ -751,121 +748,46 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp volumeMounts := []corev1.VolumeMount{} volumes := []corev1.Volume{} - // Check if global ConfigMap mode is enabled via environment variable - useConfigMap := os.Getenv("TOOLHIVE_USE_CONFIGMAP") == trueValue - if useConfigMap { - // Also add pod template patch for secrets and service account (same as regular flags approach) - // If service account is not specified, use the default MCP server service account - serviceAccount := m.Spec.ServiceAccount - if serviceAccount == nil { - defaultSA := mcpServerServiceAccountName(m.Name) - serviceAccount = &defaultSA - } - finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec). - WithServiceAccount(serviceAccount). - WithSecrets(m.Spec.Secrets). - Build() - // Add pod template patch if we have one - if finalPodTemplateSpec != nil { - podTemplatePatch, err := json.Marshal(finalPodTemplateSpec) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to marshal pod template spec") - } else { - args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch))) - } + // Also add pod template patch for secrets and service account (same as regular flags approach) + // If service account is not specified, use the default MCP server service account + serviceAccount := m.Spec.ServiceAccount + if serviceAccount == nil { + defaultSA := mcpServerServiceAccountName(m.Name) + serviceAccount = &defaultSA + } + finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec). + WithServiceAccount(serviceAccount). + WithSecrets(m.Spec.Secrets). + Build() + // Add pod template patch if we have one + if finalPodTemplateSpec != nil { + podTemplatePatch, err := json.Marshal(finalPodTemplateSpec) + if err != nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to marshal pod template spec") + } else { + args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch))) } + } - // Add volume mount for ConfigMap - configMapName := fmt.Sprintf("%s-runconfig", m.Name) - volumeMounts = append(volumeMounts, corev1.VolumeMount{ - Name: "runconfig", - MountPath: "/etc/runconfig", - ReadOnly: true, - }) + // Add volume mount for ConfigMap + configMapName := fmt.Sprintf("%s-runconfig", m.Name) + volumeMounts = append(volumeMounts, corev1.VolumeMount{ + Name: "runconfig", + MountPath: "/etc/runconfig", + ReadOnly: true, + }) - volumes = append(volumes, corev1.Volume{ - Name: "runconfig", - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: configMapName, - }, + volumes = append(volumes, corev1.Volume{ + Name: "runconfig", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: configMapName, }, }, - }) - } else { - // Use individual configuration flags (existing behavior) - args = append(args, fmt.Sprintf("--proxy-port=%d", m.Spec.Port)) - args = append(args, fmt.Sprintf("--name=%s", m.Name)) - args = append(args, fmt.Sprintf("--transport=%s", m.Spec.Transport)) - args = append(args, fmt.Sprintf("--host=%s", getProxyHost())) - if m.Spec.TargetPort != 0 { - args = append(args, fmt.Sprintf("--target-port=%d", m.Spec.TargetPort)) - } - // Add proxy mode for stdio transport - if m.Spec.ProxyMode != "" { - args = append(args, fmt.Sprintf("--proxy-mode=%s", m.Spec.ProxyMode)) - } - // Add trust proxy headers flag if enabled - if m.Spec.TrustProxyHeaders { - args = append(args, "--trust-proxy-headers") - } - } - - // Add pod template patch and permission profile only if not using ConfigMap - // When using ConfigMap, these are included in the runconfig.json - if !useConfigMap { - // Generate pod template patch for secrets and merge with user-provided patch - // If service account is not specified, use the default MCP server service account - serviceAccount := m.Spec.ServiceAccount - if serviceAccount == nil { - defaultSA := mcpServerServiceAccountName(m.Name) - serviceAccount = &defaultSA - } - finalPodTemplateSpec := NewMCPServerPodTemplateSpecBuilder(m.Spec.PodTemplateSpec). - WithServiceAccount(serviceAccount). - WithSecrets(m.Spec.Secrets). - Build() - // Add pod template patch if we have one - if finalPodTemplateSpec != nil { - podTemplatePatch, err := json.Marshal(finalPodTemplateSpec) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to marshal pod template spec") - } else { - args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch))) - } - } - - // Add permission profile args - if m.Spec.PermissionProfile != nil { - switch m.Spec.PermissionProfile.Type { - case mcpv1alpha1.PermissionProfileTypeBuiltin: - args = append(args, fmt.Sprintf("--permission-profile=%s", m.Spec.PermissionProfile.Name)) - case mcpv1alpha1.PermissionProfileTypeConfigMap: - args = append(args, fmt.Sprintf("--permission-profile-path=/etc/toolhive/profiles/%s", m.Spec.PermissionProfile.Key)) - } - } - } - - // Add OIDC configuration args only if not using ConfigMap - // When using ConfigMap, OIDC configuration is included in the runconfig.json - if !useConfigMap && m.Spec.OIDCConfig != nil { - // Create a context with timeout for OIDC configuration operations - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - oidcArgs := r.generateOIDCArgs(ctx, m) - args = append(args, oidcArgs...) - - // Add OAuth discovery resource URL for RFC 9728 compliance - resourceURL := m.Spec.OIDCConfig.ResourceURL - if resourceURL == "" { - resourceURL = createServiceURL(m.Name, m.Namespace, m.Spec.Port) - } - args = append(args, fmt.Sprintf("--resource-url=%s", resourceURL)) - } + }, + }) // Add authorization configuration args only if not using ConfigMap // When using ConfigMap, authorization configuration is included in the runconfig.json @@ -873,61 +795,9 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp authzArgs := r.generateAuthzArgs(m) args = append(args, authzArgs...) } - - // Add audit configuration args only if not using ConfigMap - // When using ConfigMap, audit configuration is included in the runconfig.json - if !useConfigMap && m.Spec.Audit != nil && m.Spec.Audit.Enabled { - args = append(args, "--enable-audit") - } - - // Add environment variables and tools filter only if not using ConfigMap - if !useConfigMap { - // Add environment variables as --env flags for the MCP server - for _, e := range m.Spec.Env { - args = append(args, fmt.Sprintf("--env=%s=%s", e.Name, e.Value)) - } - - // Add tools filter args - if len(m.Spec.ToolsFilter) > 0 { - slices.Sort(m.Spec.ToolsFilter) - args = append(args, fmt.Sprintf("--tools=%s", strings.Join(m.Spec.ToolsFilter, ","))) - } - } - - // Add OpenTelemetry configuration args only if not using ConfigMap - // When using ConfigMap, telemetry configuration is included in the runconfig.json - if !useConfigMap && m.Spec.Telemetry != nil { - if m.Spec.Telemetry.OpenTelemetry != nil { - otelArgs := r.generateOpenTelemetryArgs(m) - args = append(args, otelArgs...) - } - - if m.Spec.Telemetry.Prometheus != nil { - prometheusArgs := r.generatePrometheusArgs(m) - args = append(args, prometheusArgs...) - } - } - - // Always add the image as it's required by proxy runner command signature - // When using ConfigMap, the image from ConfigMap takes precedence, but we still need - // to provide this as a positional argument to satisfy the command requirements - args = append(args, m.Spec.Image) - - // Add additional args only if not using ConfigMap - if !useConfigMap && len(m.Spec.Args) > 0 { - args = append(args, "--") - args = append(args, m.Spec.Args...) - } - // Prepare container env vars for the proxy container env := []corev1.EnvVar{} - // Add OpenTelemetry environment variables - if m.Spec.Telemetry != nil && m.Spec.Telemetry.OpenTelemetry != nil { - otelEnvVars := r.generateOpenTelemetryEnvVars(m) - env = append(env, otelEnvVars...) - } - // Add user-specified proxy environment variables from ResourceOverrides if m.Spec.ResourceOverrides != nil && m.Spec.ResourceOverrides.ProxyDeployment != nil { for _, envVar := range m.Spec.ResourceOverrides.ProxyDeployment.Env { @@ -957,6 +827,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp } // Add volume mount for permission profile if using configmap + // TODO: Do we use permission profiles? if m.Spec.PermissionProfile != nil && m.Spec.PermissionProfile.Type == mcpv1alpha1.PermissionProfileTypeConfigMap { volumeMounts = append(volumeMounts, corev1.VolumeMount{ Name: "permission-profile", @@ -977,6 +848,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp } // Add volume mounts for authorization configuration + //TODO: Do we use auth mounts? authzVolumeMount, authzVolume := r.generateAuthzVolumeConfig(m) if authzVolumeMount != nil { volumeMounts = append(volumeMounts, *authzVolumeMount) @@ -984,6 +856,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp } // Prepare container resources + //TODO: It seems that this is for the proxyrunner but not the underlying MCP server resources := corev1.ResourceRequirements{} if m.Spec.Resources.Limits.CPU != "" || m.Spec.Resources.Limits.Memory != "" { resources.Limits = corev1.ResourceList{} @@ -1030,12 +903,6 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp } } - // Check for Vault Agent Injection and add env-file-dir argument if needed - // Only add the flag when not using ConfigMap mode (when using ConfigMap, this is handled via the runconfig.json) - if !useConfigMap && hasVaultAgentInjection(deploymentTemplateAnnotations) { - args = append(args, "--env-file-dir=/vault/secrets") - } - // Detect platform and prepare ProxyRunner's pod and container security context _, err := r.detectPlatform(ctx) if err != nil { diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index b33ac325f..7cfe7d8bc 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -296,7 +296,7 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPSer } // Add telemetry configuration if specified - addTelemetryConfigOptions(&options, m.Spec.Telemetry, m.Name) + addTelemetryConfigOptions(&options, m.Spec.Telemetry, m.Name, m.Namespace) // Add authorization configuration if specified ctx, cancel := context.WithTimeout(context.Background(), defaultAPITimeout) @@ -570,6 +570,7 @@ func addTelemetryConfigOptions( options *[]runner.RunConfigBuilderOption, telemetryConfig *mcpv1alpha1.TelemetryConfig, mcpServerName string, + mcpServerNamespace string, ) { if telemetryConfig == nil { return @@ -624,6 +625,11 @@ func addTelemetryConfigOptions( otelEnablePrometheusMetricsPath = telemetryConfig.Prometheus.Enabled } + otelEnvironmentVariables = append( + otelEnvironmentVariables, + fmt.Sprintf("OTEL_RESOURCE_ATTRIBUTES=service.name=%s,service.namespace=%s", otelServiceName, mcpServerNamespace), + ) + // Add telemetry config to options *options = append(*options, runner.WithTelemetryConfig( otelEndpoint, @@ -732,34 +738,167 @@ func (r *MCPServerReconciler) addAuthzConfigOptions( } // addOIDCConfigOptions adds OIDC authentication configuration options to the builder options -func addOIDCConfigOptions( +func (r *MCPServerReconciler) addOIDCConfigOptions( + ctx context.Context, options *[]runner.RunConfigBuilderOption, oidcConfig *mcpv1alpha1.OIDCConfigRef, + m *mcpv1alpha1.MCPServer, ) { if oidcConfig == nil { return } - // Handle inline OIDC configuration - if oidcConfig.Type == mcpv1alpha1.OIDCConfigTypeInline && oidcConfig.Inline != nil { - inline := oidcConfig.Inline + // Add OAuth discovery resource URL for RFC 9728 compliance + resourceURL := oidcConfig.ResourceURL + if resourceURL == "" { + resourceURL = createServiceURL(m.Name, m.Namespace, m.Spec.Port) + } + + switch oidcConfig.Type { + case mcpv1alpha1.OIDCConfigTypeKubernetes: + config := oidcConfig.Kubernetes + + // Set defaults if config is nil + if config == nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.Info("Kubernetes OIDCConfig is nil, using default configuration", "mcpServer", m.Name) + defaultUseClusterAuth := true + config = &mcpv1alpha1.KubernetesOIDCConfig{ + UseClusterAuth: &defaultUseClusterAuth, // Default to true + } + } + + // Handle UseClusterAuth with default of true if nil + useClusterAuth := true // default value + if config.UseClusterAuth != nil { + useClusterAuth = *config.UseClusterAuth + } + + if oidcConfig.Kubernetes != nil { + config := oidcConfig.Kubernetes + issuer := config.Issuer + if issuer == "" { + issuer = "https://kubernetes.default.svc" + } + + audience := config.Audience + if audience == "" { + audience = "toolhive" + } + + thvCABundlePath := "" + jwksAuthTokenPath := "" + jwksAllowPrivateIP := false + + if useClusterAuth { + thvCABundlePath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + jwksAuthTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" + jwksAllowPrivateIP = true + } + + // Add OIDC config to options + *options = append(*options, runner.WithOIDCConfig( + issuer, + audience, + config.JWKSURL, + config.IntrospectionURL, + "", + "", + thvCABundlePath, + jwksAuthTokenPath, + resourceURL, + jwksAllowPrivateIP, + )) + } + case mcpv1alpha1.OIDCConfigTypeConfigMap: + + if oidcConfig.ConfigMap == nil { + return + } + + config := oidcConfig.ConfigMap + + // Read the ConfigMap and extract OIDC configuration from documented keys + configMap := &corev1.ConfigMap{} + err := r.Get(ctx, types.NamespacedName{ + Name: config.Name, + Namespace: m.Namespace, + }, configMap) + if err != nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to get ConfigMap", "configMapName", config.Name) + return + } + + // Extract OIDC configuration from well-known keys + issuer := "" + if i, exists := configMap.Data["issuer"]; exists && i != "" { + issuer = i + } + audience := "" + if a, exists := configMap.Data["audience"]; exists && a != "" { + audience = a + } + jwksURL := "" + if j, exists := configMap.Data["jwksUrl"]; exists && j != "" { + jwksURL = j + } + introspectionURL := "" + if i, exists := configMap.Data["introspectionUrl"]; exists && i != "" { + introspectionURL = i + } + clientID := "" + if c, exists := configMap.Data["clientId"]; exists && c != "" { + clientID = c + } + clientSecret := "" + if c, exists := configMap.Data["clientSecret"]; exists && c != "" { + clientSecret = c + } + thvCABundlePath := "" + if thvCABundlePath, exists := configMap.Data["thvCABundlePath"]; exists && thvCABundlePath != "" { + thvCABundlePath = thvCABundlePath + } + jwksAuthTokenPath := "" + if j, exists := configMap.Data["jwksAuthTokenPath"]; exists && j != "" { + jwksAuthTokenPath = j + } + jwksAllowPrivateIP := false + if jwksAllowPrivateIP, exists := configMap.Data["jwksAllowPrivateIP"]; exists && jwksAllowPrivateIP == "true" { + jwksAllowPrivateIP = "true" + } - // Add OIDC config to options *options = append(*options, runner.WithOIDCConfig( - inline.Issuer, - inline.Audience, - inline.JWKSURL, - inline.IntrospectionURL, - inline.ClientID, - inline.ClientSecret, - inline.ThvCABundlePath, - inline.JWKSAuthTokenPath, - "", // resourceURL - not available in InlineOIDCConfig - inline.JWKSAllowPrivateIP, + issuer, + audience, + jwksURL, + introspectionURL, + clientID, + clientSecret, + thvCABundlePath, + jwksAuthTokenPath, + resourceURL, + jwksAllowPrivateIP, )) + case mcpv1alpha1.OIDCConfigTypeInline: + if oidcConfig.Inline != nil { + inline := oidcConfig.Inline + + // Add OIDC config to options + *options = append(*options, runner.WithOIDCConfig( + inline.Issuer, + inline.Audience, + inline.JWKSURL, + inline.IntrospectionURL, + inline.ClientID, + inline.ClientSecret, + inline.ThvCABundlePath, + inline.JWKSAuthTokenPath, + resourceURL, + inline.JWKSAllowPrivateIP, + )) + } } - - // ConfigMap and Kubernetes types are not currently supported for OIDC configuration } // addAuditConfigOptions adds audit configuration options to the builder options From 62e3b95837713a51471c1f786aac69f6abc7cb53 Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Tue, 14 Oct 2025 14:38:25 +0100 Subject: [PATCH 2/3] removes more code Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- .../controllers/mcpserver_authz_test.go | 152 ------ .../controllers/mcpserver_controller.go | 302 ------------ .../controllers/mcpserver_oidc_test.go | 450 ------------------ .../mcpserver_opentelemetry_test.go | 64 +-- .../controllers/mcpserver_runconfig.go | 2 +- 5 files changed, 33 insertions(+), 937 deletions(-) delete mode 100644 cmd/thv-operator/controllers/mcpserver_oidc_test.go diff --git a/cmd/thv-operator/controllers/mcpserver_authz_test.go b/cmd/thv-operator/controllers/mcpserver_authz_test.go index e4dded1ac..399f6a095 100644 --- a/cmd/thv-operator/controllers/mcpserver_authz_test.go +++ b/cmd/thv-operator/controllers/mcpserver_authz_test.go @@ -16,158 +16,6 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" ) -func TestGenerateAuthzArgs(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - - tests := []struct { - name string - mcpServer *mcpv1alpha1.MCPServer - configMaps []corev1.ConfigMap - expectedArgs []string - }{ - { - name: "no authz config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - }, - }, - expectedArgs: nil, - }, - { - name: "configmap authz config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - AuthzConfig: &mcpv1alpha1.AuthzConfigRef{ - Type: mcpv1alpha1.AuthzConfigTypeConfigMap, - ConfigMap: &mcpv1alpha1.ConfigMapAuthzRef{ - Name: "test-authz-config", - Key: "authz.json", - }, - }, - }, - }, - configMaps: []corev1.ConfigMap{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-authz-config", - Namespace: "test-namespace", - }, - Data: map[string]string{ - "authz.json": `{ - "version": "1.0", - "type": "cedarv1", - "cedar": { - "policies": ["permit(principal, action == Action::\"call_tool\", resource == Tool::\"weather\");"], - "entities_json": "[]" - } - }`, - }, - }, - }, - expectedArgs: []string{"--authz-config=/etc/toolhive/authz/authz.json"}, - }, - { - name: "inline authz config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - AuthzConfig: &mcpv1alpha1.AuthzConfigRef{ - Type: mcpv1alpha1.AuthzConfigTypeInline, - Inline: &mcpv1alpha1.InlineAuthzConfig{ - Policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"weather");`, - `permit(principal, action == Action::"get_prompt", resource == Prompt::"greeting");`, - }, - EntitiesJSON: "[]", - }, - }, - }, - }, - expectedArgs: []string{"--authz-config=/etc/toolhive/authz/authz.json"}, - }, - { - name: "configmap authz config with default key", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - AuthzConfig: &mcpv1alpha1.AuthzConfigRef{ - Type: mcpv1alpha1.AuthzConfigTypeConfigMap, - ConfigMap: &mcpv1alpha1.ConfigMapAuthzRef{ - Name: "test-authz-config", - // Key not specified, should default to "authz.json" - }, - }, - }, - }, - configMaps: []corev1.ConfigMap{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-authz-config", - Namespace: "test-namespace", - }, - Data: map[string]string{ - "authz.json": `{ - "version": "1.0", - "type": "cedarv1", - "cedar": { - "policies": ["permit(principal, action, resource);"], - "entities_json": "[]" - } - }`, - }, - }, - }, - expectedArgs: []string{"--authz-config=/etc/toolhive/authz/authz.json"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - // Create fake client with ConfigMaps - objects := []runtime.Object{tt.mcpServer} - for i := range tt.configMaps { - objects = append(objects, &tt.configMaps[i]) - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(objects...). - Build() - - reconciler := &MCPServerReconciler{ - Client: fakeClient, - Scheme: scheme, - } - - args := reconciler.generateAuthzArgs(tt.mcpServer) - assert.Equal(t, tt.expectedArgs, args) - }) - } -} - func TestEnsureAuthzConfigMap(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index c3dffcfa9..66cac8be8 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -789,12 +789,6 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp }, }) - // Add authorization configuration args only if not using ConfigMap - // When using ConfigMap, authorization configuration is included in the runconfig.json - if !useConfigMap && m.Spec.AuthzConfig != nil { - authzArgs := r.generateAuthzArgs(m) - args = append(args, authzArgs...) - } // Prepare container env vars for the proxy container env := []corev1.EnvVar{} @@ -1652,302 +1646,6 @@ func getToolhiveRunnerImage() string { return image } -// generateOIDCArgs generates OIDC command-line arguments based on the OIDC configuration -func (r *MCPServerReconciler) generateOIDCArgs(ctx context.Context, m *mcpv1alpha1.MCPServer) []string { - var args []string - - if m.Spec.OIDCConfig == nil { - return args - } - - switch m.Spec.OIDCConfig.Type { - case mcpv1alpha1.OIDCConfigTypeKubernetes: - args = append(args, r.generateKubernetesOIDCArgs(ctx, m)...) - case mcpv1alpha1.OIDCConfigTypeConfigMap: - args = append(args, r.generateConfigMapOIDCArgs(ctx, m)...) - case mcpv1alpha1.OIDCConfigTypeInline: - args = append(args, r.generateInlineOIDCArgs(m)...) - } - - return args -} - -// generateKubernetesOIDCArgs generates OIDC args for Kubernetes service account token validation -func (*MCPServerReconciler) generateKubernetesOIDCArgs(ctx context.Context, m *mcpv1alpha1.MCPServer) []string { - var args []string - config := m.Spec.OIDCConfig.Kubernetes - - // Set defaults if config is nil - if config == nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Info("Kubernetes OIDCConfig is nil, using default configuration", "mcpServer", m.Name) - defaultUseClusterAuth := true - config = &mcpv1alpha1.KubernetesOIDCConfig{ - UseClusterAuth: &defaultUseClusterAuth, // Default to true - } - } - - // Handle UseClusterAuth with default of true if nil - useClusterAuth := true // default value - if config.UseClusterAuth != nil { - useClusterAuth = *config.UseClusterAuth - } - - // Issuer (default: https://kubernetes.default.svc) - issuer := config.Issuer - if issuer == "" { - issuer = "https://kubernetes.default.svc" - } - args = append(args, fmt.Sprintf("--oidc-issuer=%s", issuer)) - - // Audience (default: toolhive) - audience := config.Audience - if audience == "" { - audience = "toolhive" - } - args = append(args, fmt.Sprintf("--oidc-audience=%s", audience)) - - // JWKS URL (optional - if empty, thv will use OIDC discovery) - jwksURL := config.JWKSURL - if jwksURL != "" { - args = append(args, fmt.Sprintf("--oidc-jwks-url=%s", jwksURL)) - } - - // Introspection URL (optional - if empty, thv will use OIDC discovery) - introspectionURL := config.IntrospectionURL - if introspectionURL != "" { - args = append(args, fmt.Sprintf("--oidc-introspection-url=%s", introspectionURL)) - } - - // Add cluster auth flags if enabled (default is true) - if useClusterAuth { - args = append(args, "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt") - args = append(args, "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token") - args = append(args, "--jwks-allow-private-ip") - } - - // Client ID (format: {serviceAccount}.{namespace}.svc.cluster.local) - serviceAccount := config.ServiceAccount - if serviceAccount == "" { - serviceAccount = "default" // Use default service account if not specified - } - - namespace := config.Namespace - if namespace == "" { - namespace = m.Namespace // Use MCPServer's namespace if not specified - } - - clientID := fmt.Sprintf("%s.%s.svc.cluster.local", serviceAccount, namespace) - args = append(args, fmt.Sprintf("--oidc-client-id=%s", clientID)) - - return args -} - -// generateConfigMapOIDCArgs generates OIDC args for ConfigMap-based configuration -func (r *MCPServerReconciler) generateConfigMapOIDCArgs( // nolint:gocyclo - ctx context.Context, - m *mcpv1alpha1.MCPServer, -) []string { - var args []string - config := m.Spec.OIDCConfig.ConfigMap - - if config == nil { - return args - } - - // Read the ConfigMap and extract OIDC configuration from documented keys - configMap := &corev1.ConfigMap{} - err := r.Get(ctx, types.NamespacedName{ - Name: config.Name, - Namespace: m.Namespace, - }, configMap) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to get ConfigMap", "configMapName", config.Name) - return args - } - - // Extract OIDC configuration from well-known keys - if issuer, exists := configMap.Data["issuer"]; exists && issuer != "" { - args = append(args, fmt.Sprintf("--oidc-issuer=%s", issuer)) - } - if audience, exists := configMap.Data["audience"]; exists && audience != "" { - args = append(args, fmt.Sprintf("--oidc-audience=%s", audience)) - } - if jwksURL, exists := configMap.Data["jwksUrl"]; exists && jwksURL != "" { - args = append(args, fmt.Sprintf("--oidc-jwks-url=%s", jwksURL)) - } - if introspectionURL, exists := configMap.Data["introspectionUrl"]; exists && introspectionURL != "" { - args = append(args, fmt.Sprintf("--oidc-introspection-url=%s", introspectionURL)) - } - if clientID, exists := configMap.Data["clientId"]; exists && clientID != "" { - args = append(args, fmt.Sprintf("--oidc-client-id=%s", clientID)) - } - if clientSecret, exists := configMap.Data["clientSecret"]; exists && clientSecret != "" { - args = append(args, fmt.Sprintf("--oidc-client-secret=%s", clientSecret)) - } - if thvCABundlePath, exists := configMap.Data["thvCABundlePath"]; exists && thvCABundlePath != "" { - args = append(args, fmt.Sprintf("--thv-ca-bundle=%s", thvCABundlePath)) - } - if jwksAuthTokenPath, exists := configMap.Data["jwksAuthTokenPath"]; exists && jwksAuthTokenPath != "" { - args = append(args, fmt.Sprintf("--jwks-auth-token-file=%s", jwksAuthTokenPath)) - } - if jwksAllowPrivateIP, exists := configMap.Data["jwksAllowPrivateIP"]; exists && jwksAllowPrivateIP == "true" { - args = append(args, "--jwks-allow-private-ip") - } - - return args -} - -// generateInlineOIDCArgs generates OIDC args for inline configuration -func (*MCPServerReconciler) generateInlineOIDCArgs(m *mcpv1alpha1.MCPServer) []string { - var args []string - config := m.Spec.OIDCConfig.Inline - - if config == nil { - return args - } - - // Issuer (required) - if config.Issuer != "" { - args = append(args, fmt.Sprintf("--oidc-issuer=%s", config.Issuer)) - } - - // Audience (optional) - if config.Audience != "" { - args = append(args, fmt.Sprintf("--oidc-audience=%s", config.Audience)) - } - - // JWKS URL (optional) - if config.JWKSURL != "" { - args = append(args, fmt.Sprintf("--oidc-jwks-url=%s", config.JWKSURL)) - } - - // Introspection URL (optional) - if config.IntrospectionURL != "" { - args = append(args, fmt.Sprintf("--oidc-introspection-url=%s", config.IntrospectionURL)) - } - - // CA Bundle path (optional) - if config.ThvCABundlePath != "" { - args = append(args, fmt.Sprintf("--thv-ca-bundle=%s", config.ThvCABundlePath)) - } - - // Auth token path (optional) - if config.JWKSAuthTokenPath != "" { - args = append(args, fmt.Sprintf("--jwks-auth-token-file=%s", config.JWKSAuthTokenPath)) - } - - // Allow private IP access (optional) - if config.JWKSAllowPrivateIP { - args = append(args, "--jwks-allow-private-ip") - } - - // Client ID (optional) - if config.ClientID != "" { - args = append(args, fmt.Sprintf("--oidc-client-id=%s", config.ClientID)) - } - - return args -} - -// generateAuthzArgs generates authorization command-line arguments based on the configuration type -func (*MCPServerReconciler) generateAuthzArgs(m *mcpv1alpha1.MCPServer) []string { - var args []string - - if m.Spec.AuthzConfig == nil { - return args - } - - // Validate that the configuration is properly set based on type - switch m.Spec.AuthzConfig.Type { - case mcpv1alpha1.AuthzConfigTypeConfigMap: - if m.Spec.AuthzConfig.ConfigMap == nil { - return args - } - case mcpv1alpha1.AuthzConfigTypeInline: - if m.Spec.AuthzConfig.Inline == nil { - return args - } - default: - return args - } - - // Both ConfigMap and inline configurations use the same mounted path - authzConfigPath := fmt.Sprintf("/etc/toolhive/authz/%s", defaultAuthzKey) - args = append(args, fmt.Sprintf("--authz-config=%s", authzConfigPath)) - - return args -} - -// generatePrometheusArgs generates Prometheus command-line arguments based on the configuration -func (*MCPServerReconciler) generatePrometheusArgs(m *mcpv1alpha1.MCPServer) []string { - var args []string - - if m.Spec.Telemetry == nil || m.Spec.Telemetry.Prometheus == nil { - return args - } - - // Add Prometheus metrics path flag if Prometheus is enabled in telemetry config - if m.Spec.Telemetry.Prometheus.Enabled { - args = append(args, "--enable-prometheus-metrics-path") - } - - return args -} - -// generateOpenTelemetryArgs generates OpenTelemetry command-line arguments based on the configuration -func (*MCPServerReconciler) generateOpenTelemetryArgs(m *mcpv1alpha1.MCPServer) []string { - var args []string - - if m.Spec.Telemetry == nil || m.Spec.Telemetry.OpenTelemetry == nil { - return args - } - - otel := m.Spec.Telemetry.OpenTelemetry - - // Add endpoint - if otel.Endpoint != "" { - args = append(args, fmt.Sprintf("--otel-endpoint=%s", otel.Endpoint)) - } - - // Add service name - if otel.ServiceName != "" { - args = append(args, fmt.Sprintf("--otel-service-name=%s", otel.ServiceName)) - } - - // Add headers (multiple --otel-headers flags) - for _, header := range otel.Headers { - args = append(args, fmt.Sprintf("--otel-headers=%s", header)) - } - - // Add insecure flag - if otel.Insecure { - args = append(args, "--otel-insecure") - } - - // Handle tracing configuration - if otel.Tracing != nil { - if otel.Tracing.Enabled { - args = append(args, "--otel-tracing-enabled=true") - args = append(args, fmt.Sprintf("--otel-tracing-sampling-rate=%s", otel.Tracing.SamplingRate)) - } else { - args = append(args, "--otel-tracing-enabled=false") - } - } - - // Handle metrics configuration - if otel.Metrics != nil { - if otel.Metrics.Enabled { - args = append(args, "--otel-metrics-enabled=true") - } else { - args = append(args, "--otel-metrics-enabled=false") - } - } - - return args -} - // generateOpenTelemetryEnvVars generates OpenTelemetry environment variables for the proxy container func (*MCPServerReconciler) generateOpenTelemetryEnvVars(m *mcpv1alpha1.MCPServer) []corev1.EnvVar { var envVars []corev1.EnvVar diff --git a/cmd/thv-operator/controllers/mcpserver_oidc_test.go b/cmd/thv-operator/controllers/mcpserver_oidc_test.go deleted file mode 100644 index 216125c6c..000000000 --- a/cmd/thv-operator/controllers/mcpserver_oidc_test.go +++ /dev/null @@ -1,450 +0,0 @@ -// Copyright 2025 Stacklok, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package controllers - -import ( - "context" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - - mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" - "github.com/stacklok/toolhive/pkg/logger" -) - -func init() { - // Initialize logger for tests - logger.Initialize() -} - -func TestGenerateOIDCArgs(t *testing.T) { - t.Parallel() - - scheme := runtime.NewScheme() - require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) - require.NoError(t, corev1.AddToScheme(scheme)) - - tests := []struct { - name string - mcpServer *mcpv1alpha1.MCPServer - configMaps []corev1.ConfigMap - expectedArgs []string - }{ - { - name: "no OIDC config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - }, - }, - expectedArgs: nil, - }, - { - name: "kubernetes OIDC config with defaults", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{}, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://kubernetes.default.svc", - "--oidc-audience=toolhive", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=default.test-namespace.svc.cluster.local", - }, - }, - { - name: "kubernetes OIDC config with custom values", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{ - ServiceAccount: "custom-sa", - Namespace: "custom-namespace", - Audience: "custom-audience", - Issuer: "https://custom.issuer.com", - JWKSURL: "https://custom.issuer.com/jwks", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://custom.issuer.com", - "--oidc-audience=custom-audience", - "--oidc-jwks-url=https://custom.issuer.com/jwks", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=custom-sa.custom-namespace.svc.cluster.local", - }, - }, - { - name: "inline OIDC config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeInline, - Inline: &mcpv1alpha1.InlineOIDCConfig{ - Issuer: "https://accounts.google.com", - Audience: "my-google-client", - JWKSURL: "https://www.googleapis.com/oauth2/v3/certs", - ClientID: "my-client-id", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://accounts.google.com", - "--oidc-audience=my-google-client", - "--oidc-jwks-url=https://www.googleapis.com/oauth2/v3/certs", - "--oidc-client-id=my-client-id", - }, - }, - { - name: "configmap OIDC config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeConfigMap, - ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ - Name: "oidc-config", - }, - }, - }, - }, - configMaps: []corev1.ConfigMap{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "oidc-config", - Namespace: "test-namespace", - }, - Data: map[string]string{ - "issuer": "https://accounts.google.com", - "audience": "my-google-client", - "jwksUrl": "https://www.googleapis.com/oauth2/v3/certs", - "clientId": "my-client-id", - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://accounts.google.com", - "--oidc-audience=my-google-client", - "--oidc-jwks-url=https://www.googleapis.com/oauth2/v3/certs", - "--oidc-client-id=my-client-id", - }, - }, - { - name: "configmap OIDC config with partial data", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image", - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeConfigMap, - ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ - Name: "oidc-config", - }, - }, - }, - }, - configMaps: []corev1.ConfigMap{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "oidc-config", - Namespace: "test-namespace", - }, - Data: map[string]string{ - "issuer": "https://accounts.google.com", - "audience": "my-google-client", - // jwksUrl and clientId are missing - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://accounts.google.com", - "--oidc-audience=my-google-client", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - // Create fake client with configmaps - objs := make([]runtime.Object, len(tt.configMaps)) - for i, cm := range tt.configMaps { - objs[i] = &cm - } - fakeClient := fake.NewClientBuilder(). - WithScheme(scheme). - WithRuntimeObjects(objs...). - Build() - - reconciler := &MCPServerReconciler{ - Client: fakeClient, - Scheme: scheme, - } - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - - args := reconciler.generateOIDCArgs(ctx, tt.mcpServer) - - assert.Equal(t, tt.expectedArgs, args) - }) - } -} - -func TestGenerateKubernetesOIDCArgs(t *testing.T) { - t.Parallel() - - reconciler := &MCPServerReconciler{} - - tests := []struct { - name string - mcpServer *mcpv1alpha1.MCPServer - expectedArgs []string - }{ - { - name: "nil kubernetes config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: nil, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://kubernetes.default.svc", - "--oidc-audience=toolhive", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=default.test-namespace.svc.cluster.local", - }, - }, - { - name: "empty kubernetes config", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{}, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://kubernetes.default.svc", - "--oidc-audience=toolhive", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=default.test-namespace.svc.cluster.local", - }, - }, - { - name: "custom service account only", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{ - ServiceAccount: "my-service-account", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://kubernetes.default.svc", - "--oidc-audience=toolhive", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=my-service-account.test-namespace.svc.cluster.local", - }, - }, - { - name: "custom namespace only", - mcpServer: &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "test-namespace", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeKubernetes, - Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{ - Namespace: "my-namespace", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://kubernetes.default.svc", - "--oidc-audience=toolhive", - "--thv-ca-bundle=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", - "--jwks-auth-token-file=/var/run/secrets/kubernetes.io/serviceaccount/token", - "--jwks-allow-private-ip", - "--oidc-client-id=default.my-namespace.svc.cluster.local", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - args := reconciler.generateKubernetesOIDCArgs(context.Background(), tt.mcpServer) - assert.Equal(t, tt.expectedArgs, args) - }) - } -} - -func TestGenerateInlineOIDCArgs(t *testing.T) { - t.Parallel() - - reconciler := &MCPServerReconciler{} - - tests := []struct { - name string - mcpServer *mcpv1alpha1.MCPServer - expectedArgs []string - }{ - { - name: "nil inline config", - mcpServer: &mcpv1alpha1.MCPServer{ - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeInline, - Inline: nil, - }, - }, - }, - expectedArgs: nil, - }, - { - name: "empty inline config", - mcpServer: &mcpv1alpha1.MCPServer{ - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeInline, - Inline: &mcpv1alpha1.InlineOIDCConfig{}, - }, - }, - }, - expectedArgs: nil, - }, - { - name: "issuer only", - mcpServer: &mcpv1alpha1.MCPServer{ - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeInline, - Inline: &mcpv1alpha1.InlineOIDCConfig{ - Issuer: "https://accounts.google.com", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://accounts.google.com", - }, - }, - { - name: "all fields", - mcpServer: &mcpv1alpha1.MCPServer{ - Spec: mcpv1alpha1.MCPServerSpec{ - OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ - Type: mcpv1alpha1.OIDCConfigTypeInline, - Inline: &mcpv1alpha1.InlineOIDCConfig{ - Issuer: "https://accounts.google.com", - Audience: "my-audience", - JWKSURL: "https://www.googleapis.com/oauth2/v3/certs", - ClientID: "my-client-id", - }, - }, - }, - }, - expectedArgs: []string{ - "--oidc-issuer=https://accounts.google.com", - "--oidc-audience=my-audience", - "--oidc-jwks-url=https://www.googleapis.com/oauth2/v3/certs", - "--oidc-client-id=my-client-id", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - args := reconciler.generateInlineOIDCArgs(tt.mcpServer) - assert.Equal(t, tt.expectedArgs, args) - }) - } -} diff --git a/cmd/thv-operator/controllers/mcpserver_opentelemetry_test.go b/cmd/thv-operator/controllers/mcpserver_opentelemetry_test.go index 468c5652b..bebb6b679 100644 --- a/cmd/thv-operator/controllers/mcpserver_opentelemetry_test.go +++ b/cmd/thv-operator/controllers/mcpserver_opentelemetry_test.go @@ -140,41 +140,41 @@ func TestTelemetryArgs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - client := fake.NewClientBuilder().WithScheme(scheme).Build() - r := &MCPServerReconciler{ - Client: client, - Scheme: scheme, - } + // client := fake.NewClientBuilder().WithScheme(scheme).Build() + // r := &MCPServerReconciler{ + // Client: client, + // Scheme: scheme, + // } - mcpServer := &mcpv1alpha1.MCPServer{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-server", - Namespace: "default", - }, - Spec: mcpv1alpha1.MCPServerSpec{ - Image: "test-image:latest", - Telemetry: func() *mcpv1alpha1.TelemetryConfig { - if tt.telemetryConfig == nil && !tt.prometheusEnabled { - return nil - } - telemetryConfig := &mcpv1alpha1.TelemetryConfig{ - OpenTelemetry: tt.telemetryConfig.OpenTelemetry, - } - if tt.prometheusEnabled { - telemetryConfig.Prometheus = &mcpv1alpha1.PrometheusConfig{ - Enabled: true, - } - } - return telemetryConfig - }(), - }, - } + // mcpServer := &mcpv1alpha1.MCPServer{ + // ObjectMeta: metav1.ObjectMeta{ + // Name: "test-server", + // Namespace: "default", + // }, + // Spec: mcpv1alpha1.MCPServerSpec{ + // Image: "test-image:latest", + // Telemetry: func() *mcpv1alpha1.TelemetryConfig { + // if tt.telemetryConfig == nil && !tt.prometheusEnabled { + // return nil + // } + // telemetryConfig := &mcpv1alpha1.TelemetryConfig{ + // OpenTelemetry: tt.telemetryConfig.OpenTelemetry, + // } + // if tt.prometheusEnabled { + // telemetryConfig.Prometheus = &mcpv1alpha1.PrometheusConfig{ + // Enabled: true, + // } + // } + // return telemetryConfig + // }(), + // }, + // } - args := r.generateOpenTelemetryArgs(mcpServer) - args = append(args, r.generatePrometheusArgs(mcpServer)...) + // args := r.generateOpenTelemetryArgs(mcpServer) + // args = append(args, r.generatePrometheusArgs(mcpServer)...) - // Check that all expected arguments are present, regardless of order - assert.ElementsMatch(t, tt.expectedArgs, args) + // // Check that all expected arguments are present, regardless of order + // assert.ElementsMatch(t, tt.expectedArgs, args) }) } } diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index 7cfe7d8bc..14e2518da 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -307,7 +307,7 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPSer } // Add OIDC authentication configuration if specified - addOIDCConfigOptions(&options, m.Spec.OIDCConfig) + r.addOIDCConfigOptions(ctx, &options, m.Spec.OIDCConfig, m) // Add audit configuration if specified addAuditConfigOptions(&options, m.Spec.Audit) From f2ec7e01e3df3706d5c8667da00fa7a84a6a0fdc Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Wed, 15 Oct 2025 10:17:26 +0000 Subject: [PATCH 3/3] Resolve merge conflicts with main branch Resolves conflicts between the flag-driven config removal and main's OIDC refactoring (commit 61d04b4). Changes: - Adopt main's oidc.Resolver pattern for cleaner OIDC config handling - Add cmd/thv-operator/pkg/oidc package with resolver implementation - Update addOIDCConfigOptions to use the resolver - Maintain this PR's telemetry namespace parameter addition The resolution keeps this PR's goal of removing flag-driven config while incorporating main's improved OIDC architecture. Co-authored-by: Chris Burns --- .../controllers/mcpserver_runconfig.go | 181 ++----- cmd/thv-operator/pkg/oidc/resolver.go | 223 +++++++++ cmd/thv-operator/pkg/oidc/resolver_test.go | 442 ++++++++++++++++++ 3 files changed, 692 insertions(+), 154 deletions(-) create mode 100644 cmd/thv-operator/pkg/oidc/resolver.go create mode 100644 cmd/thv-operator/pkg/oidc/resolver_test.go diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index 14e2518da..eee0888ee 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -21,6 +21,7 @@ import ( "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/oidc" "github.com/stacklok/toolhive/pkg/authz" "github.com/stacklok/toolhive/pkg/operator/accessors" "github.com/stacklok/toolhive/pkg/runner" @@ -306,8 +307,9 @@ func (r *MCPServerReconciler) createRunConfigFromMCPServer(m *mcpv1alpha1.MCPSer return nil, fmt.Errorf("failed to process AuthzConfig: %w", err) } - // Add OIDC authentication configuration if specified - r.addOIDCConfigOptions(ctx, &options, m.Spec.OIDCConfig, m) + if err := r.addOIDCConfigOptions(ctx, &options, m); err != nil { + return nil, fmt.Errorf("failed to process OIDCConfig: %w", err) + } // Add audit configuration if specified addAuditConfigOptions(&options, m.Spec.Audit) @@ -741,164 +743,35 @@ func (r *MCPServerReconciler) addAuthzConfigOptions( func (r *MCPServerReconciler) addOIDCConfigOptions( ctx context.Context, options *[]runner.RunConfigBuilderOption, - oidcConfig *mcpv1alpha1.OIDCConfigRef, m *mcpv1alpha1.MCPServer, -) { - if oidcConfig == nil { - return - } +) error { - // Add OAuth discovery resource URL for RFC 9728 compliance - resourceURL := oidcConfig.ResourceURL - if resourceURL == "" { - resourceURL = createServiceURL(m.Name, m.Namespace, m.Spec.Port) + // Use the OIDC resolver to get configuration + resolver := oidc.NewResolver(r.Client) + oidcConfig, err := resolver.Resolve(ctx, m) + if err != nil { + return fmt.Errorf("failed to resolve OIDC configuration: %w", err) } - switch oidcConfig.Type { - case mcpv1alpha1.OIDCConfigTypeKubernetes: - config := oidcConfig.Kubernetes - - // Set defaults if config is nil - if config == nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Info("Kubernetes OIDCConfig is nil, using default configuration", "mcpServer", m.Name) - defaultUseClusterAuth := true - config = &mcpv1alpha1.KubernetesOIDCConfig{ - UseClusterAuth: &defaultUseClusterAuth, // Default to true - } - } - - // Handle UseClusterAuth with default of true if nil - useClusterAuth := true // default value - if config.UseClusterAuth != nil { - useClusterAuth = *config.UseClusterAuth - } - - if oidcConfig.Kubernetes != nil { - config := oidcConfig.Kubernetes - issuer := config.Issuer - if issuer == "" { - issuer = "https://kubernetes.default.svc" - } - - audience := config.Audience - if audience == "" { - audience = "toolhive" - } - - thvCABundlePath := "" - jwksAuthTokenPath := "" - jwksAllowPrivateIP := false - - if useClusterAuth { - thvCABundlePath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" - jwksAuthTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" - jwksAllowPrivateIP = true - } - - // Add OIDC config to options - *options = append(*options, runner.WithOIDCConfig( - issuer, - audience, - config.JWKSURL, - config.IntrospectionURL, - "", - "", - thvCABundlePath, - jwksAuthTokenPath, - resourceURL, - jwksAllowPrivateIP, - )) - } - case mcpv1alpha1.OIDCConfigTypeConfigMap: - - if oidcConfig.ConfigMap == nil { - return - } + if oidcConfig == nil { + return nil + } - config := oidcConfig.ConfigMap + // Add OIDC config to options + *options = append(*options, runner.WithOIDCConfig( + oidcConfig.Issuer, + oidcConfig.Audience, + oidcConfig.JWKSURL, + oidcConfig.IntrospectionURL, + oidcConfig.ClientID, + oidcConfig.ClientSecret, + oidcConfig.ThvCABundlePath, + oidcConfig.JWKSAuthTokenPath, + oidcConfig.ResourceURL, + oidcConfig.JWKSAllowPrivateIP, + )) - // Read the ConfigMap and extract OIDC configuration from documented keys - configMap := &corev1.ConfigMap{} - err := r.Get(ctx, types.NamespacedName{ - Name: config.Name, - Namespace: m.Namespace, - }, configMap) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to get ConfigMap", "configMapName", config.Name) - return - } - - // Extract OIDC configuration from well-known keys - issuer := "" - if i, exists := configMap.Data["issuer"]; exists && i != "" { - issuer = i - } - audience := "" - if a, exists := configMap.Data["audience"]; exists && a != "" { - audience = a - } - jwksURL := "" - if j, exists := configMap.Data["jwksUrl"]; exists && j != "" { - jwksURL = j - } - introspectionURL := "" - if i, exists := configMap.Data["introspectionUrl"]; exists && i != "" { - introspectionURL = i - } - clientID := "" - if c, exists := configMap.Data["clientId"]; exists && c != "" { - clientID = c - } - clientSecret := "" - if c, exists := configMap.Data["clientSecret"]; exists && c != "" { - clientSecret = c - } - thvCABundlePath := "" - if thvCABundlePath, exists := configMap.Data["thvCABundlePath"]; exists && thvCABundlePath != "" { - thvCABundlePath = thvCABundlePath - } - jwksAuthTokenPath := "" - if j, exists := configMap.Data["jwksAuthTokenPath"]; exists && j != "" { - jwksAuthTokenPath = j - } - jwksAllowPrivateIP := false - if jwksAllowPrivateIP, exists := configMap.Data["jwksAllowPrivateIP"]; exists && jwksAllowPrivateIP == "true" { - jwksAllowPrivateIP = "true" - } - - *options = append(*options, runner.WithOIDCConfig( - issuer, - audience, - jwksURL, - introspectionURL, - clientID, - clientSecret, - thvCABundlePath, - jwksAuthTokenPath, - resourceURL, - jwksAllowPrivateIP, - )) - case mcpv1alpha1.OIDCConfigTypeInline: - if oidcConfig.Inline != nil { - inline := oidcConfig.Inline - - // Add OIDC config to options - *options = append(*options, runner.WithOIDCConfig( - inline.Issuer, - inline.Audience, - inline.JWKSURL, - inline.IntrospectionURL, - inline.ClientID, - inline.ClientSecret, - inline.ThvCABundlePath, - inline.JWKSAuthTokenPath, - resourceURL, - inline.JWKSAllowPrivateIP, - )) - } - } + return nil } // addAuditConfigOptions adds audit configuration options to the builder options diff --git a/cmd/thv-operator/pkg/oidc/resolver.go b/cmd/thv-operator/pkg/oidc/resolver.go new file mode 100644 index 000000000..91ff7d4d1 --- /dev/null +++ b/cmd/thv-operator/pkg/oidc/resolver.go @@ -0,0 +1,223 @@ +// Package oidc provides utilities for resolving OIDC configuration from various sources +// including Kubernetes service accounts, ConfigMaps, and inline configurations. +package oidc + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +const ( + // K8s service account paths + defaultK8sCABundlePath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" + defaultK8sTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token" //nolint:gosec + defaultK8sIssuer = "https://kubernetes.default.svc" + defaultK8sAudience = "toolhive" +) + +// OIDCConfig represents the resolved OIDC configuration values +type OIDCConfig struct { //nolint:revive // Keeping OIDCConfig name for backward compatibility + Issuer string + Audience string + JWKSURL string + IntrospectionURL string + ClientID string + ClientSecret string + ThvCABundlePath string + JWKSAuthTokenPath string + ResourceURL string + JWKSAllowPrivateIP bool +} + +// Resolver is the interface for resolving OIDC configuration from various sources +type Resolver interface { + // Resolve takes an MCPServer and its OIDC configuration reference and returns the resolved OIDC config + Resolve(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) (*OIDCConfig, error) +} + +// NewResolver creates a new OIDC configuration resolver +// It accepts an optional Kubernetes client for ConfigMap resolution +func NewResolver(k8sClient client.Client) Resolver { + return &resolver{ + client: k8sClient, + } +} + +// resolver is the concrete implementation of the Resolver interface +type resolver struct { + client client.Client +} + +// Resolve resolves the OIDC configuration based on the type specified in OIDCConfigRef +func (r *resolver) Resolve(ctx context.Context, mcpServer *mcpv1alpha1.MCPServer) (*OIDCConfig, error) { + if mcpServer.Spec.OIDCConfig == nil { + return nil, nil + } + + oidcConfig := mcpServer.Spec.OIDCConfig + + // Calculate resource URL for RFC 9728 compliance + resourceURL := oidcConfig.ResourceURL + if resourceURL == "" { + resourceURL = createServiceURL(mcpServer.Name, mcpServer.Namespace, mcpServer.Spec.Port) + } + + switch oidcConfig.Type { + case mcpv1alpha1.OIDCConfigTypeKubernetes: + return r.resolveKubernetesConfig(ctx, oidcConfig.Kubernetes, resourceURL, mcpServer) + case mcpv1alpha1.OIDCConfigTypeConfigMap: + return r.resolveConfigMapConfig(ctx, oidcConfig.ConfigMap, resourceURL, mcpServer) + case mcpv1alpha1.OIDCConfigTypeInline: + return r.resolveInlineConfig(oidcConfig.Inline, resourceURL) + default: + return nil, fmt.Errorf("unknown OIDC config type: %s", oidcConfig.Type) + } +} + +// resolveKubernetesConfig resolves OIDC configuration for Kubernetes type +func (*resolver) resolveKubernetesConfig( + ctx context.Context, + config *mcpv1alpha1.KubernetesOIDCConfig, + resourceURL string, + mcpServer *mcpv1alpha1.MCPServer, +) (*OIDCConfig, error) { + // Set defaults if config is nil + if config == nil { + ctxLogger := log.FromContext(ctx) + ctxLogger.Info("Kubernetes OIDCConfig is nil, using default configuration", "mcpServer", mcpServer.Name) + defaultUseClusterAuth := true + config = &mcpv1alpha1.KubernetesOIDCConfig{ + UseClusterAuth: &defaultUseClusterAuth, + } + } + + // Handle UseClusterAuth with default of true if nil + useClusterAuth := true // default value + if config.UseClusterAuth != nil { + useClusterAuth = *config.UseClusterAuth + } + + result := &OIDCConfig{ + ResourceURL: resourceURL, + } + + // Set issuer with default + result.Issuer = config.Issuer + if result.Issuer == "" { + result.Issuer = defaultK8sIssuer + } + + // Set audience with default + result.Audience = config.Audience + if result.Audience == "" { + result.Audience = defaultK8sAudience + } + + // Set JWKS and introspection URLs + result.JWKSURL = config.JWKSURL + result.IntrospectionURL = config.IntrospectionURL + + // Apply cluster auth settings if enabled + if useClusterAuth { + result.ThvCABundlePath = defaultK8sCABundlePath + result.JWKSAuthTokenPath = defaultK8sTokenPath + result.JWKSAllowPrivateIP = true + } + + return result, nil +} + +// resolveConfigMapConfig resolves OIDC configuration from a ConfigMap +func (r *resolver) resolveConfigMapConfig( + ctx context.Context, + configRef *mcpv1alpha1.ConfigMapOIDCRef, + resourceURL string, + mcpServer *mcpv1alpha1.MCPServer, +) (*OIDCConfig, error) { + if configRef == nil { + return nil, nil + } + + if r.client == nil { + return nil, fmt.Errorf("kubernetes client is required for ConfigMap OIDC resolution") + } + + // Read the ConfigMap + configMap := &corev1.ConfigMap{} + err := r.client.Get(ctx, types.NamespacedName{ + Name: configRef.Name, + Namespace: mcpServer.Namespace, + }, configMap) + if err != nil { + return nil, fmt.Errorf("failed to get OIDC ConfigMap %s/%s: %w", + mcpServer.Namespace, configRef.Name, err) + } + + config := &OIDCConfig{ + ResourceURL: resourceURL, + } + + // Extract string values + config.Issuer = getMapValue(configMap.Data, "issuer") + config.Audience = getMapValue(configMap.Data, "audience") + config.JWKSURL = getMapValue(configMap.Data, "jwksUrl") + config.IntrospectionURL = getMapValue(configMap.Data, "introspectionUrl") + config.ClientID = getMapValue(configMap.Data, "clientId") + config.ClientSecret = getMapValue(configMap.Data, "clientSecret") + config.ThvCABundlePath = getMapValue(configMap.Data, "thvCABundlePath") + //nolint:gosec // This is just a config key name, not a credential + config.JWKSAuthTokenPath = getMapValue(configMap.Data, "jwksAuthTokenPath") + + // Handle boolean value + if v, exists := configMap.Data["jwksAllowPrivateIP"]; exists && v == "true" { + config.JWKSAllowPrivateIP = true + } + + return config, nil +} + +// resolveInlineConfig resolves inline OIDC configuration +func (*resolver) resolveInlineConfig( + config *mcpv1alpha1.InlineOIDCConfig, + resourceURL string, +) (*OIDCConfig, error) { + if config == nil { + return nil, nil + } + + return &OIDCConfig{ + Issuer: config.Issuer, + Audience: config.Audience, + JWKSURL: config.JWKSURL, + IntrospectionURL: config.IntrospectionURL, + ClientID: config.ClientID, + ClientSecret: config.ClientSecret, + ThvCABundlePath: config.ThvCABundlePath, + JWKSAuthTokenPath: config.JWKSAuthTokenPath, + ResourceURL: resourceURL, + JWKSAllowPrivateIP: config.JWKSAllowPrivateIP, + }, nil +} + +// getMapValue is a helper to extract string values from a map +func getMapValue(data map[string]string, key string) string { + if v, exists := data[key]; exists && v != "" { + return v + } + return "" +} + +// createServiceURL creates a service URL from MCPServer details +func createServiceURL(name, namespace string, port int32) string { + if port == 0 { + port = 8080 + } + return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", name, namespace, port) +} diff --git a/cmd/thv-operator/pkg/oidc/resolver_test.go b/cmd/thv-operator/pkg/oidc/resolver_test.go new file mode 100644 index 000000000..7f7196de3 --- /dev/null +++ b/cmd/thv-operator/pkg/oidc/resolver_test.go @@ -0,0 +1,442 @@ +package oidc + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" +) + +func TestResolve_KubernetesType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mcpServer *mcpv1alpha1.MCPServer + expected *OIDCConfig + }{ + { + name: "kubernetes with defaults", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 8080, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeKubernetes, + Kubernetes: nil, // nil config should use defaults + }, + }, + }, + expected: &OIDCConfig{ + Issuer: defaultK8sIssuer, + Audience: defaultK8sAudience, + ThvCABundlePath: defaultK8sCABundlePath, + JWKSAuthTokenPath: defaultK8sTokenPath, + ResourceURL: "http://test-server.test-ns.svc.cluster.local:8080", + JWKSAllowPrivateIP: true, + }, + }, + { + name: "kubernetes with custom values", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom-server", + Namespace: "custom-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 9090, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeKubernetes, + ResourceURL: "https://custom-resource.example.com", + Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{ + Issuer: "https://custom-issuer.example.com", + Audience: "custom-audience", + JWKSURL: "https://custom-issuer.example.com/.well-known/jwks.json", + IntrospectionURL: "https://custom-issuer.example.com/introspect", + UseClusterAuth: func(b bool) *bool { return &b }(true), + }, + }, + }, + }, + expected: &OIDCConfig{ + Issuer: "https://custom-issuer.example.com", + Audience: "custom-audience", + JWKSURL: "https://custom-issuer.example.com/.well-known/jwks.json", + IntrospectionURL: "https://custom-issuer.example.com/introspect", + ThvCABundlePath: defaultK8sCABundlePath, + JWKSAuthTokenPath: defaultK8sTokenPath, + ResourceURL: "https://custom-resource.example.com", + JWKSAllowPrivateIP: true, + }, + }, + { + name: "kubernetes with UseClusterAuth false", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-cluster-auth-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 8080, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeKubernetes, + Kubernetes: &mcpv1alpha1.KubernetesOIDCConfig{ + UseClusterAuth: func(b bool) *bool { return &b }(false), + }, + }, + }, + }, + expected: &OIDCConfig{ + Issuer: defaultK8sIssuer, + Audience: defaultK8sAudience, + ResourceURL: "http://no-cluster-auth-server.test-ns.svc.cluster.local:8080", + JWKSAllowPrivateIP: false, // Should be false when UseClusterAuth is false + // CA bundle and token paths should be empty + ThvCABundlePath: "", + JWKSAuthTokenPath: "", + }, + }, + { + name: "nil oidc config returns nil", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: nil, + }, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + resolver := NewResolver(nil) + config, err := resolver.Resolve(context.Background(), tt.mcpServer) + require.NoError(t, err) + assert.Equal(t, tt.expected, config) + }) + } +} + +func TestResolve_ConfigMapType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mcpServer *mcpv1alpha1.MCPServer + configMap *corev1.ConfigMap + expected *OIDCConfig + expectErr bool + }{ + { + name: "configmap with all values", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 8080, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ + Name: "oidc-config", + }, + }, + }, + }, + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oidc-config", + Namespace: "test-ns", + }, + Data: map[string]string{ + "issuer": "https://auth.example.com", + "audience": "test-audience", + "jwksUrl": "https://auth.example.com/.well-known/jwks.json", + "introspectionUrl": "https://auth.example.com/introspect", + "clientId": "test-client", + "clientSecret": "test-secret", + "thvCABundlePath": "/etc/ssl/ca.pem", + "jwksAuthTokenPath": "/etc/auth/token", + "jwksAllowPrivateIP": "true", + }, + }, + expected: &OIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test-audience", + JWKSURL: "https://auth.example.com/.well-known/jwks.json", + IntrospectionURL: "https://auth.example.com/introspect", + ClientID: "test-client", + ClientSecret: "test-secret", + ThvCABundlePath: "/etc/ssl/ca.pem", + JWKSAuthTokenPath: "/etc/auth/token", + ResourceURL: "http://test-server.test-ns.svc.cluster.local:8080", + JWKSAllowPrivateIP: true, + }, + }, + { + name: "configmap with partial values", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "partial-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeConfigMap, + ResourceURL: "https://custom-resource.example.com", + ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ + Name: "partial-config", + }, + }, + }, + }, + configMap: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "partial-config", + Namespace: "test-ns", + }, + Data: map[string]string{ + "issuer": "https://partial.example.com", + "audience": "partial-audience", + "jwksAllowPrivateIP": "false", // explicitly false + }, + }, + expected: &OIDCConfig{ + Issuer: "https://partial.example.com", + Audience: "partial-audience", + ResourceURL: "https://custom-resource.example.com", + JWKSAllowPrivateIP: false, + }, + }, + { + name: "configmap not found", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ + Name: "missing-config", + }, + }, + }, + }, + expectErr: true, + }, + { + name: "nil configmap ref returns nil", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeConfigMap, + ConfigMap: nil, + }, + }, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Create fake client with ConfigMap if provided + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = mcpv1alpha1.AddToScheme(scheme) + + var objects []runtime.Object + if tt.configMap != nil { + objects = append(objects, tt.configMap) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(objects...). + Build() + + resolver := NewResolver(fakeClient) + config, err := resolver.Resolve(context.Background(), tt.mcpServer) + + if tt.expectErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.expected, config) + } + }) + } +} + +func TestResolve_InlineType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mcpServer *mcpv1alpha1.MCPServer + expected *OIDCConfig + }{ + { + name: "inline with all values", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 8080, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://inline.example.com", + Audience: "inline-audience", + JWKSURL: "https://inline.example.com/.well-known/jwks.json", + IntrospectionURL: "https://inline.example.com/introspect", + ClientID: "inline-client", + ClientSecret: "inline-secret", + ThvCABundlePath: "/etc/ssl/inline-ca.pem", + JWKSAuthTokenPath: "/etc/auth/inline-token", + JWKSAllowPrivateIP: true, + }, + }, + }, + }, + expected: &OIDCConfig{ + Issuer: "https://inline.example.com", + Audience: "inline-audience", + JWKSURL: "https://inline.example.com/.well-known/jwks.json", + IntrospectionURL: "https://inline.example.com/introspect", + ClientID: "inline-client", + ClientSecret: "inline-secret", + ThvCABundlePath: "/etc/ssl/inline-ca.pem", + JWKSAuthTokenPath: "/etc/auth/inline-token", + ResourceURL: "http://test-server.test-ns.svc.cluster.local:8080", + JWKSAllowPrivateIP: true, + }, + }, + { + name: "inline with custom resource URL", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom-server", + Namespace: "custom-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Port: 9090, + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + ResourceURL: "https://custom-resource.example.com", + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://inline.example.com", + Audience: "inline-audience", + }, + }, + }, + }, + expected: &OIDCConfig{ + Issuer: "https://inline.example.com", + Audience: "inline-audience", + ResourceURL: "https://custom-resource.example.com", + }, + }, + { + name: "nil inline config returns nil", + mcpServer: &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: nil, + }, + }, + }, + expected: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + resolver := NewResolver(nil) + config, err := resolver.Resolve(context.Background(), tt.mcpServer) + require.NoError(t, err) + assert.Equal(t, tt.expected, config) + }) + } +} + +func TestResolve_UnknownType(t *testing.T) { + t.Parallel() + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: "unknown-type", + }, + }, + } + + resolver := NewResolver(nil) + config, err := resolver.Resolve(context.Background(), mcpServer) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "unknown OIDC config type") + assert.Nil(t, config) +} + +func TestResolve_NoClientForConfigMap(t *testing.T) { + t.Parallel() + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "test-ns", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + OIDCConfig: &mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapOIDCRef{ + Name: "test-config", + }, + }, + }, + } + + resolver := NewResolver(nil) // No client provided + config, err := resolver.Resolve(context.Background(), mcpServer) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "kubernetes client is required") + assert.Nil(t, config) +}