Replace global logger with context-aware logging in operator (#2007)

Resolves #1830

This change replaces all instances of global logger usage in the
ToolHive operator with context-aware logging to preserve trace
context for better observability.

Changes:
- Updated validateRunConfig() to accept context and use log.FromContext()
- Updated helper functions in mcpserver_controller.go to use context-aware logging:
  - ensureRequiredEnvVars()
  - serviceForMCPServer()
  - deploymentNeedsUpdate()
  - generateKubernetesOIDCArgs()
  - generateConfigMapOIDCArgs()
- Removed unused global logger imports
- Updated function signatures to accept context.Context parameters
- Fixed all calling sites to pass context
- Updated test files to accommodate new function signatures
- Fixed linting issues (line length violations)

All tests pass and no regressions introduced.

Author: JAORMX

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,29 +1065,29 @@ 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",
 		})
 	}
 	// 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
 	}
 

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)
 		})
 	}

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)
 		})
 	}

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
 }
 

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)