diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index c73e902f41..84b503678f 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -1274,6 +1274,18 @@ func (r *MCPRemoteProxyReconciler) containerNeedsUpdate( // Check if environment variables have changed expectedEnv := r.buildEnvVarsForProxy(ctx, proxy) + configName := ctrlutil.EmbeddedAuthServerConfigName( + proxy.Spec.ExternalAuthConfigRef, proxy.Spec.AuthServerRef, + ) + if configName != "" { + _, _, authServerEnvVars, err := ctrlutil.GenerateAuthServerConfigByName( + ctx, r.Client, proxy.Namespace, configName, + ) + if err != nil { + return true + } + expectedEnv = append(expectedEnv, authServerEnvVars...) + } if !reflect.DeepEqual(container.Env, expectedEnv) { return true } diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go index fdbf86583b..6686f30d76 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment_test.go @@ -700,6 +700,186 @@ func TestEnsureService(t *testing.T) { } } +func TestMCPRemoteProxyDeploymentNeedsUpdate_EmbeddedAuthLegacyEnvStable(t *testing.T) { + t.Parallel() + + scheme := createRunConfigTestScheme() + + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "embedded-auth", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeEmbeddedAuthServer, + EmbeddedAuthServer: &mcpv1alpha1.EmbeddedAuthServerConfig{ + UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{ + { + Name: "google", + Type: mcpv1alpha1.UpstreamProviderTypeOIDC, + OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{ + IssuerURL: "https://accounts.google.com", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "upstream-secret", + Key: "client-secret", + }, + }, + }, + }, + }, + }, + } + + proxy := &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ProxyPort: 8080, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: authConfig.Name, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(authConfig). + Build() + + reconciler := &MCPRemoteProxyReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + + assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) +} + +func TestMCPRemoteProxyDeploymentNeedsUpdate_EmbeddedAuthAuthServerRefEnvStable(t *testing.T) { + t.Parallel() + + scheme := createRunConfigTestScheme() + + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "embedded-auth", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeEmbeddedAuthServer, + EmbeddedAuthServer: &mcpv1alpha1.EmbeddedAuthServerConfig{ + UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{ + { + Name: "google", + Type: mcpv1alpha1.UpstreamProviderTypeOIDC, + OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{ + IssuerURL: "https://accounts.google.com", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "upstream-secret", + Key: "client-secret", + }, + }, + }, + }, + }, + }, + } + + proxy := &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ProxyPort: 8080, + AuthServerRef: &mcpv1alpha1.AuthServerRef{ + Kind: "MCPExternalAuthConfig", + Name: authConfig.Name, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(authConfig). + Build() + + reconciler := &MCPRemoteProxyReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + + assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) +} + +func TestMCPRemoteProxyDeploymentNeedsUpdate_TokenExchangeDoesNotDrift(t *testing.T) { + t.Parallel() + + scheme := createRunConfigTestScheme() + + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "exchange-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "token-secret", + Key: "client-secret", + }, + Audience: "api", + }, + }, + } + + proxy := &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + ProxyPort: 8080, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: authConfig.Name, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithRuntimeObjects(authConfig). + Build() + + reconciler := &MCPRemoteProxyReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + deployment := reconciler.deploymentForMCPRemoteProxy(t.Context(), proxy, "test-checksum") + require.NotNil(t, deployment) + + assert.False(t, reconciler.deploymentNeedsUpdate(t.Context(), deployment, proxy, "test-checksum")) +} + // TestBuildEnvVarsForProxy tests environment variable building func TestBuildEnvVarsForProxy(t *testing.T) { t.Parallel() diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 44068ccf43..a3f955ada8 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -1654,6 +1654,20 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate( }) } } + + // Add embedded auth server environment variables. AuthServerRef takes precedence; + // externalAuthConfigRef is used as a fallback (legacy path). + if configName := ctrlutil.EmbeddedAuthServerConfigName( + mcpServer.Spec.ExternalAuthConfigRef, mcpServer.Spec.AuthServerRef, + ); configName != "" { + _, _, authServerEnvVars, err := ctrlutil.GenerateAuthServerConfigByName( + ctx, r.Client, mcpServer.Namespace, configName, + ) + if err != nil { + return true + } + expectedProxyEnv = append(expectedProxyEnv, authServerEnvVars...) + } // Add default environment variables that are always injected expectedProxyEnv = ctrlutil.EnsureRequiredEnvVars(ctx, expectedProxyEnv) if !equality.Semantic.DeepEqual(container.Env, expectedProxyEnv) { diff --git a/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go b/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go index ba6e516752..9adfedfb9f 100644 --- a/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go +++ b/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go @@ -15,7 +15,6 @@ package controllers import ( - "context" "testing" "github.com/stretchr/testify/assert" @@ -30,6 +29,189 @@ import ( "github.com/stacklok/toolhive/pkg/container/kubernetes" ) +func TestMCPServerDeploymentNeedsUpdate_EmbeddedAuthLegacyEnvStable(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + + externalAuthConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "embedded-auth", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeEmbeddedAuthServer, + EmbeddedAuthServer: &mcpv1alpha1.EmbeddedAuthServerConfig{ + UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{ + { + Name: "google", + Type: mcpv1alpha1.UpstreamProviderTypeOIDC, + OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{ + IssuerURL: "https://accounts.google.com", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "upstream-secret", + Key: "client-secret", + }, + }, + }, + }, + }, + }, + } + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Image: "test-image", + ProxyPort: 8080, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: externalAuthConfig.Name, + }, + }, + } + + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(externalAuthConfig). + Build() + r := newTestMCPServerReconciler(client, scheme, kubernetes.PlatformKubernetes) + + deployment := r.deploymentForMCPServer(t.Context(), mcpServer, "test-checksum") + require.NotNil(t, deployment) + require.Len(t, deployment.Spec.Template.Spec.Containers, 1) + require.Contains(t, deployment.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{ + Name: "TOOLHIVE_UPSTREAM_CLIENT_SECRET_GOOGLE", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "upstream-secret"}, + Key: "client-secret", + }, + }, + }) + + assert.False(t, r.deploymentNeedsUpdate(t.Context(), deployment, mcpServer, "test-checksum")) +} + +func TestMCPServerDeploymentNeedsUpdate_EmbeddedAuthAuthServerRefEnvStable(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "embedded-auth", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeEmbeddedAuthServer, + EmbeddedAuthServer: &mcpv1alpha1.EmbeddedAuthServerConfig{ + UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{ + { + Name: "google", + Type: mcpv1alpha1.UpstreamProviderTypeOIDC, + OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{ + IssuerURL: "https://accounts.google.com", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "upstream-secret", + Key: "client-secret", + }, + }, + }, + }, + }, + }, + } + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Image: "test-image", + ProxyPort: 8080, + AuthServerRef: &mcpv1alpha1.AuthServerRef{ + Kind: "MCPExternalAuthConfig", + Name: authConfig.Name, + }, + }, + } + + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(authConfig). + Build() + r := newTestMCPServerReconciler(client, scheme, kubernetes.PlatformKubernetes) + + deployment := r.deploymentForMCPServer(t.Context(), mcpServer, "test-checksum") + require.NotNil(t, deployment) + require.Len(t, deployment.Spec.Template.Spec.Containers, 1) + + assert.False(t, r.deploymentNeedsUpdate(t.Context(), deployment, mcpServer, "test-checksum")) +} + +func TestMCPServerDeploymentNeedsUpdate_TokenExchangeDoesNotDrift(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + require.NoError(t, mcpv1alpha1.AddToScheme(scheme)) + + authConfig := &mcpv1alpha1.MCPExternalAuthConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "exchange-config", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPExternalAuthConfigSpec{ + Type: mcpv1alpha1.ExternalAuthTypeTokenExchange, + TokenExchange: &mcpv1alpha1.TokenExchangeConfig{ + TokenURL: "https://oauth.example.com/token", + ClientID: "client-id", + ClientSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "token-secret", + Key: "client-secret", + }, + Audience: "api", + }, + }, + } + + mcpServer := &mcpv1alpha1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-server", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPServerSpec{ + Image: "test-image", + ProxyPort: 8080, + ExternalAuthConfigRef: &mcpv1alpha1.ExternalAuthConfigRef{ + Name: authConfig.Name, + }, + }, + } + + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(authConfig). + Build() + r := newTestMCPServerReconciler(client, scheme, kubernetes.PlatformKubernetes) + + deployment := r.deploymentForMCPServer(t.Context(), mcpServer, "test-checksum") + require.NotNil(t, deployment) + require.Len(t, deployment.Spec.Template.Spec.Containers, 1) + + assert.False(t, r.deploymentNeedsUpdate(t.Context(), deployment, mcpServer, "test-checksum")) +} + func TestResourceOverrides(t *testing.T) { t.Parallel() @@ -341,7 +523,7 @@ func TestResourceOverrides(t *testing.T) { r := newTestMCPServerReconciler(client, scheme, kubernetes.PlatformKubernetes) // Test deployment creation - ctx := context.Background() + ctx := t.Context() deployment := r.deploymentForMCPServer(ctx, tt.mcpServer, "test-checksum") require.NotNil(t, deployment) @@ -349,7 +531,7 @@ func TestResourceOverrides(t *testing.T) { assert.Equal(t, tt.expectedDeploymentAnns, deployment.Annotations) // Test service creation - service := r.serviceForMCPServer(context.Background(), tt.mcpServer) + service := r.serviceForMCPServer(t.Context(), tt.mcpServer) require.NotNil(t, service) assert.Equal(t, tt.expectedServiceLabels, service.Labels) @@ -615,7 +797,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) { t.Parallel() // Create a deployment and manually set up its state to isolate proxy env testing - ctx := context.Background() + ctx := t.Context() deployment := r.deploymentForMCPServer(ctx, tt.mcpServer, "test-checksum") require.NotNil(t, deployment) require.Len(t, deployment.Spec.Template.Spec.Containers, 1) @@ -627,7 +809,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) { deployment.Spec.Template.Spec.Containers[0].Image = getToolhiveRunnerImage() // Test if deployment needs update - should correlate with env change expectation - needsUpdate := r.deploymentNeedsUpdate(context.Background(), deployment, tt.mcpServer, "test-checksum") + needsUpdate := r.deploymentNeedsUpdate(t.Context(), deployment, tt.mcpServer, "test-checksum") if tt.expectEnvChange { assert.True(t, needsUpdate, "Expected deployment update due to proxy env change") @@ -663,7 +845,7 @@ func TestMCPServerSessionAffinityNone(t *testing.T) { client := fake.NewClientBuilder().WithScheme(scheme).Build() r := newTestMCPServerReconciler(client, scheme, kubernetes.PlatformKubernetes) - service := r.serviceForMCPServer(context.Background(), mcpServer) + service := r.serviceForMCPServer(t.Context(), mcpServer) require.NotNil(t, service) assert.Equal(t, corev1.ServiceAffinityNone, service.Spec.SessionAffinity) } diff --git a/pkg/container/kubernetes/client_test.go b/pkg/container/kubernetes/client_test.go index e68a65812f..6d12be60ee 100644 --- a/pkg/container/kubernetes/client_test.go +++ b/pkg/container/kubernetes/client_test.go @@ -193,7 +193,7 @@ func TestCreateContainerWithPodTemplatePatch(t *testing.T) { // Deploy the workload _, err := client.DeployWorkload( - context.Background(), + t.Context(), "test-image", "test-container", []string{"test-command"}, @@ -215,7 +215,7 @@ func TestCreateContainerWithPodTemplatePatch(t *testing.T) { // Get the created StatefulSet statefulSet, err := clientset.AppsV1().StatefulSets("default").Get( - context.Background(), + t.Context(), "test-container", metav1.GetOptions{}, ) @@ -729,7 +729,7 @@ func TestCreateContainerWithMCP(t *testing.T) { // Deploy the workload _, err := client.DeployWorkload( - context.Background(), + t.Context(), tc.image, "test-container", tc.command, @@ -751,7 +751,7 @@ func TestCreateContainerWithMCP(t *testing.T) { // Get the created StatefulSet statefulSet, err := clientset.AppsV1().StatefulSets("default").Get( - context.Background(), + t.Context(), "test-container", metav1.GetOptions{}, ) @@ -959,7 +959,7 @@ func TestAttachToWorkloadNoPodFound(t *testing.T) { client.namespaceFunc = func() string { return defaultNamespace } // Call AttachToWorkload with a workload that has no pods - _, _, err := client.AttachToWorkload(context.Background(), "nonexistent-workload") + _, _, err := client.AttachToWorkload(t.Context(), "nonexistent-workload") // Should return error immediately (no pods found) require.Error(t, err) @@ -1302,7 +1302,7 @@ func TestDeployWorkloadBackendReplicas(t *testing.T) { client.namespaceFunc = func() string { return defaultNamespace } _, err := client.DeployWorkload( - context.Background(), + t.Context(), "test-image", "test-container", nil, map[string]string{}, map[string]string{}, nil, "streamable-http", options, false, @@ -1310,7 +1310,7 @@ func TestDeployWorkloadBackendReplicas(t *testing.T) { require.NoError(t, err) sts, err := clientset.AppsV1().StatefulSets(defaultNamespace).Get( - context.Background(), "test-container", metav1.GetOptions{}, + t.Context(), "test-container", metav1.GetOptions{}, ) require.NoError(t, err) return sts