diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index b5fcce7f9..34a29547e 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -31,7 +31,6 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/pkg/container/kubernetes" - "github.com/stacklok/toolhive/pkg/logger" ) // MCPServerReconciler reconciles a MCPServer object @@ -290,7 +289,7 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( err = r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: mcpServer.Namespace}, service) if err != nil && errors.IsNotFound(err) { // Define a new service - svc := r.serviceForMCPServer(mcpServer) + svc := r.serviceForMCPServer(ctx, mcpServer) if svc == nil { ctxLogger.Error(nil, "Failed to create Service object") return ctrl.Result{}, fmt.Errorf("failed to create Service object") @@ -319,7 +318,7 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } // Check if the deployment spec changed - if r.deploymentNeedsUpdate(deployment, mcpServer) { + if r.deploymentNeedsUpdate(ctx, deployment, mcpServer) { // Update the deployment newDeployment := r.deploymentForMCPServer(ctx, mcpServer) deployment.Spec = newDeployment.Spec @@ -337,7 +336,7 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Check if the service spec changed if serviceNeedsUpdate(service, mcpServer) { // Update the service - newService := r.serviceForMCPServer(mcpServer) + newService := r.serviceForMCPServer(ctx, mcpServer) service.Spec.Ports = newService.Spec.Ports err = r.Update(ctx, service) if err != nil { @@ -523,7 +522,7 @@ func (r *MCPServerReconciler) createRBACResource( ctxLogger := log.FromContext(ctx) desired := createResource() if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil { - logger.Errorf("Failed to set controller reference for %s: %v", resourceType, err) + ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) return nil } @@ -550,7 +549,7 @@ func (r *MCPServerReconciler) updateRBACResourceIfNeeded( ctxLogger := log.FromContext(ctx) desired := createResource() if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil { - logger.Errorf("Failed to set controller reference for %s: %v", resourceType, err) + ctxLogger.Error(err, "Failed to set controller reference", "resourceType", resourceType) return nil } @@ -708,7 +707,8 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp if finalPodTemplateSpec != nil { podTemplatePatch, err := json.Marshal(finalPodTemplateSpec) if err != nil { - logger.Errorf("Failed to marshal pod template spec: %v", err) + 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))) } @@ -746,7 +746,8 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp if finalPodTemplateSpec != nil { podTemplatePatch, err := json.Marshal(finalPodTemplateSpec) if err != nil { - logger.Errorf("Failed to marshal pod template spec: %v", err) + 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))) } @@ -966,7 +967,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp proxyRunnerPodSecurityContext := securityBuilder.BuildPodSecurityContext() proxyRunnerContainerSecurityContext := securityBuilder.BuildContainerSecurityContext() - env = ensureRequiredEnvVars(env) + env = ensureRequiredEnvVars(ctx, env) dep := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -1034,15 +1035,17 @@ func (r *MCPServerReconciler) deploymentForMCPServer(ctx context.Context, m *mcp // Set MCPServer instance as the owner and controller if err := controllerutil.SetControllerReference(m, dep, r.Scheme); err != nil { - logger.Errorf("Failed to set controller reference for Deployment: %v", err) + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to set controller reference for Deployment") return nil } return dep } -func ensureRequiredEnvVars(env []corev1.EnvVar) []corev1.EnvVar { +func ensureRequiredEnvVars(ctx context.Context, env []corev1.EnvVar) []corev1.EnvVar { // Check for the existence of the XDG_CONFIG_HOME, HOME, TOOLHIVE_RUNTIME, and UNSTRUCTURED_LOGS environment variables // and set them to defaults if they don't exist + ctxLogger := log.FromContext(ctx) xdgConfigHomeFound := false homeFound := false toolhiveRuntimeFound := false @@ -1062,21 +1065,21 @@ func ensureRequiredEnvVars(env []corev1.EnvVar) []corev1.EnvVar { } } if !xdgConfigHomeFound { - logger.Debugf("XDG_CONFIG_HOME not found, setting to /tmp") + ctxLogger.V(1).Info("XDG_CONFIG_HOME not found, setting to /tmp") env = append(env, corev1.EnvVar{ Name: "XDG_CONFIG_HOME", Value: "/tmp", }) } if !homeFound { - logger.Debugf("HOME not found, setting to /tmp") + ctxLogger.V(1).Info("HOME not found, setting to /tmp") env = append(env, corev1.EnvVar{ Name: "HOME", Value: "/tmp", }) } if !toolhiveRuntimeFound { - logger.Debugf("TOOLHIVE_RUNTIME not found, setting to kubernetes") + ctxLogger.V(1).Info("TOOLHIVE_RUNTIME not found, setting to kubernetes") env = append(env, corev1.EnvVar{ Name: "TOOLHIVE_RUNTIME", Value: "kubernetes", @@ -1084,7 +1087,7 @@ func ensureRequiredEnvVars(env []corev1.EnvVar) []corev1.EnvVar { } // Always use structured JSON logs in Kubernetes (not configurable) if !unstructuredLogsFound { - logger.Debugf("UNSTRUCTURED_LOGS not found, setting to false for structured JSON logging") + ctxLogger.V(1).Info("UNSTRUCTURED_LOGS not found, setting to false for structured JSON logging") env = append(env, corev1.EnvVar{ Name: "UNSTRUCTURED_LOGS", Value: "false", @@ -1104,7 +1107,7 @@ func createServiceURL(mcpServerName, namespace string, port int32) string { } // serviceForMCPServer returns a MCPServer Service object -func (r *MCPServerReconciler) serviceForMCPServer(m *mcpv1alpha1.MCPServer) *corev1.Service { +func (r *MCPServerReconciler) serviceForMCPServer(ctx context.Context, m *mcpv1alpha1.MCPServer) *corev1.Service { ls := labelsForMCPServer(m.Name) // we want to generate a service name that is unique for the proxy service @@ -1144,7 +1147,8 @@ func (r *MCPServerReconciler) serviceForMCPServer(m *mcpv1alpha1.MCPServer) *cor // Set MCPServer instance as the owner and controller if err := controllerutil.SetControllerReference(m, svc, r.Scheme); err != nil { - logger.Errorf("Failed to set controller reference for Service: %v", err) + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to set controller reference for Service") return nil } return svc @@ -1256,7 +1260,11 @@ func (r *MCPServerReconciler) finalizeMCPServer(ctx context.Context, m *mcpv1alp // deploymentNeedsUpdate checks if the deployment needs to be updated // //nolint:gocyclo -func (r *MCPServerReconciler) deploymentNeedsUpdate(deployment *appsv1.Deployment, mcpServer *mcpv1alpha1.MCPServer) bool { +func (r *MCPServerReconciler) deploymentNeedsUpdate( + ctx context.Context, + deployment *appsv1.Deployment, + mcpServer *mcpv1alpha1.MCPServer, +) bool { // Check if the container args have changed if len(deployment.Spec.Template.Spec.Containers) > 0 { container := deployment.Spec.Template.Spec.Containers[0] @@ -1371,7 +1379,7 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate(deployment *appsv1.Deploymen } } // Add default environment variables that are always injected - expectedProxyEnv = ensureRequiredEnvVars(expectedProxyEnv) + expectedProxyEnv = ensureRequiredEnvVars(ctx, expectedProxyEnv) if !reflect.DeepEqual(container.Env, expectedProxyEnv) { return true } @@ -1401,7 +1409,8 @@ func (r *MCPServerReconciler) deploymentNeedsUpdate(deployment *appsv1.Deploymen if expectedPodTemplateSpec != nil { expectedPatch, err := json.Marshal(expectedPodTemplateSpec) if err != nil { - logger.Errorf("Failed to marshal expected pod template spec: %v", err) + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to marshal expected pod template spec") return true // Assume change if we can't marshal } expectedPatchString := string(expectedPatch) @@ -1705,7 +1714,7 @@ func (r *MCPServerReconciler) generateOIDCArgs(ctx context.Context, m *mcpv1alph switch m.Spec.OIDCConfig.Type { case mcpv1alpha1.OIDCConfigTypeKubernetes: - args = append(args, r.generateKubernetesOIDCArgs(m)...) + args = append(args, r.generateKubernetesOIDCArgs(ctx, m)...) case mcpv1alpha1.OIDCConfigTypeConfigMap: args = append(args, r.generateConfigMapOIDCArgs(ctx, m)...) case mcpv1alpha1.OIDCConfigTypeInline: @@ -1716,13 +1725,14 @@ func (r *MCPServerReconciler) generateOIDCArgs(ctx context.Context, m *mcpv1alph } // generateKubernetesOIDCArgs generates OIDC args for Kubernetes service account token validation -func (*MCPServerReconciler) generateKubernetesOIDCArgs(m *mcpv1alpha1.MCPServer) []string { +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 { - logger.Infof("Kubernetes OIDCConfig is nil for MCPServer %s, using default configuration", m.Name) + 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 @@ -1804,7 +1814,8 @@ func (r *MCPServerReconciler) generateConfigMapOIDCArgs( // nolint:gocyclo Namespace: m.Namespace, }, configMap) if err != nil { - logger.Errorf("Failed to get ConfigMap %s: %v", config.Name, err) + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to get ConfigMap", "configMapName", config.Name) return args } diff --git a/cmd/thv-operator/controllers/mcpserver_oidc_test.go b/cmd/thv-operator/controllers/mcpserver_oidc_test.go index 2e5e65e25..216125c6c 100644 --- a/cmd/thv-operator/controllers/mcpserver_oidc_test.go +++ b/cmd/thv-operator/controllers/mcpserver_oidc_test.go @@ -359,7 +359,7 @@ func TestGenerateKubernetesOIDCArgs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - args := reconciler.generateKubernetesOIDCArgs(tt.mcpServer) + args := reconciler.generateKubernetesOIDCArgs(context.Background(), tt.mcpServer) assert.Equal(t, tt.expectedArgs, args) }) } diff --git a/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go b/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go index 68cf8e88b..e00563ca6 100644 --- a/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go +++ b/cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go @@ -314,7 +314,7 @@ func TestResourceOverrides(t *testing.T) { assert.Equal(t, tt.expectedDeploymentAnns, deployment.Annotations) // Test service creation - service := r.serviceForMCPServer(tt.mcpServer) + service := r.serviceForMCPServer(context.Background(), tt.mcpServer) require.NotNil(t, service) assert.Equal(t, tt.expectedServiceLabels, service.Labels) @@ -457,7 +457,7 @@ func TestDeploymentNeedsUpdateServiceAccount(t *testing.T) { require.NotNil(t, deployment) // Test with the current deployment - this should NOT need update - needsUpdate := r.deploymentNeedsUpdate(deployment, mcpServer) + needsUpdate := r.deploymentNeedsUpdate(context.Background(), deployment, mcpServer) // With the service account bug fixed, this should now return false assert.False(t, needsUpdate, "deploymentNeedsUpdate should return false when deployment matches MCPServer spec") @@ -637,7 +637,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(deployment, tt.mcpServer) + needsUpdate := r.deploymentNeedsUpdate(context.Background(), deployment, tt.mcpServer) if tt.expectEnvChange { assert.True(t, needsUpdate, "Expected deployment update due to proxy env change") @@ -718,7 +718,7 @@ func TestDeploymentNeedsUpdateToolsFilter(t *testing.T) { mcpServer.Spec.ToolsFilter = tt.newToolsFilter - needsUpdate := r.deploymentNeedsUpdate(deployment, mcpServer) + needsUpdate := r.deploymentNeedsUpdate(context.Background(), deployment, mcpServer) assert.Equal(t, tt.expectEnvChange, needsUpdate) }) } diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig.go b/cmd/thv-operator/controllers/mcpserver_runconfig.go index 3f6a04a8e..b46c442df 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig.go @@ -22,7 +22,6 @@ import ( mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1" "github.com/stacklok/toolhive/pkg/authz" - "github.com/stacklok/toolhive/pkg/logger" "github.com/stacklok/toolhive/pkg/operator/accessors" "github.com/stacklok/toolhive/pkg/runner" transporttypes "github.com/stacklok/toolhive/pkg/transport/types" @@ -92,7 +91,7 @@ func (r *MCPServerReconciler) ensureRunConfigConfigMap(ctx context.Context, m *m } // Validate the RunConfig before creating the ConfigMap - if err := r.validateRunConfig(runConfig); err != nil { + if err := r.validateRunConfig(ctx, runConfig); err != nil { return fmt.Errorf("invalid RunConfig: %w", err) } @@ -354,7 +353,7 @@ func labelsForRunConfig(mcpServerName string) map[string]string { } // validateRunConfig validates a RunConfig for operator-managed deployments -func (r *MCPServerReconciler) validateRunConfig(config *runner.RunConfig) error { +func (r *MCPServerReconciler) validateRunConfig(ctx context.Context, config *runner.RunConfig) error { if config == nil { return fmt.Errorf("RunConfig cannot be nil") } @@ -387,7 +386,8 @@ func (r *MCPServerReconciler) validateRunConfig(config *runner.RunConfig) error return err } - logger.Debugf("RunConfig validation passed for %s", config.Name) + ctxLogger := log.FromContext(ctx) + ctxLogger.V(1).Info("RunConfig validation passed", "name", config.Name) return nil } diff --git a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go b/cmd/thv-operator/controllers/mcpserver_runconfig_test.go index 93704419c..1b08f401b 100644 --- a/cmd/thv-operator/controllers/mcpserver_runconfig_test.go +++ b/cmd/thv-operator/controllers/mcpserver_runconfig_test.go @@ -1441,7 +1441,7 @@ func TestValidateRunConfig(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() r := &MCPServerReconciler{} - err := r.validateRunConfig(tt.config) + err := r.validateRunConfig(t.Context(), tt.config) if tt.expectErr { assert.Error(t, err)