Skip to content

Commit a7681d9

Browse files
committed
Replace internal logger with uber-go/zap
Eliminates singleton logger pattern in favor of per-component zap instances. Replaces slog-based logging interface with direct zap calls throughout the codebase. * Remove global logger singleton and wrapper functions * Replace slog with uber-go/zap SugaredLogger * Pass logger instances to all components and functions * Maintain structured/unstructured log switching via UNSTRUCTURED_LOGS * Preserve debug flag integration for log level control * Use go-logr/zapr for NewLogr() Kubernetes logr.Logger compatibility Signed-off-by: Varun Rajput <[email protected]>
1 parent 133d31f commit a7681d9

File tree

140 files changed

+1742
-1535
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

140 files changed

+1742
-1535
lines changed

cmd/regup/app/root.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,12 @@ package app
33

44
import (
55
"github.com/spf13/cobra"
6+
7+
log "github.com/stacklok/toolhive/pkg/logger"
68
)
79

10+
var logger = log.NewLogger()
11+
812
var rootCmd = &cobra.Command{
913
Use: "regup",
1014
DisableAutoGenTag: true,

cmd/regup/app/update.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"github.com/spf13/cobra"
1616

1717
"github.com/stacklok/toolhive/pkg/container/verifier"
18-
"github.com/stacklok/toolhive/pkg/logger"
1918
"github.com/stacklok/toolhive/pkg/registry"
2019
)
2120

@@ -304,7 +303,7 @@ func verifyServerProvenance(name string, server *registry.ImageMetadata) error {
304303
logger.Infof("Verifying provenance for server %s with image %s", name, server.Image)
305304

306305
// Create verifier
307-
v, err := verifier.New(server)
306+
v, err := verifier.New(server, logger)
308307
if err != nil {
309308
return fmt.Errorf("failed to create verifier: %w", err)
310309
}

cmd/regup/app/update_test.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,11 @@ import (
99
"github.com/stretchr/testify/assert"
1010
"github.com/stretchr/testify/require"
1111

12-
"github.com/stacklok/toolhive/pkg/logger"
1312
"github.com/stacklok/toolhive/pkg/registry"
1413
)
1514

1615
//nolint:paralleltest // This test manages temporary directories and cannot run in parallel
1716
func TestUpdateCmdFunc(t *testing.T) {
18-
// Initialize logger for tests
19-
logger.Initialize()
2017

2118
tests := []struct {
2219
name string

cmd/regup/main.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,11 @@ import (
55
"os"
66

77
"github.com/stacklok/toolhive/cmd/regup/app"
8-
"github.com/stacklok/toolhive/pkg/logger"
8+
log "github.com/stacklok/toolhive/pkg/logger"
99
)
1010

1111
func main() {
12-
// Initialize the logger system
13-
logger.Initialize()
12+
logger := log.NewLogger()
1413

1514
if err := app.NewRootCmd().Execute(); err != nil {
1615
logger.Errorf("%v", err)

cmd/thv-operator/controllers/mcpserver_controller.go

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"strings"
1414
"time"
1515

16+
"go.uber.org/zap"
1617
appsv1 "k8s.io/api/apps/v1"
1718
corev1 "k8s.io/api/core/v1"
1819
rbacv1 "k8s.io/api/rbac/v1"
@@ -28,13 +29,13 @@ import (
2829
"sigs.k8s.io/controller-runtime/pkg/log"
2930

3031
mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
31-
"github.com/stacklok/toolhive/pkg/logger"
3232
)
3333

3434
// MCPServerReconciler reconciles a MCPServer object
3535
type MCPServerReconciler struct {
3636
client.Client
3737
Scheme *runtime.Scheme
38+
logger *zap.SugaredLogger
3839
}
3940

4041
// defaultRBACRules are the default RBAC rules that the
@@ -222,7 +223,7 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
222223
}
223224

224225
// Check if the deployment spec changed
225-
if deploymentNeedsUpdate(deployment, mcpServer) {
226+
if deploymentNeedsUpdate(deployment, mcpServer, r.logger) {
226227
// Update the deployment
227228
newDeployment := r.deploymentForMCPServer(mcpServer)
228229
deployment.Spec = newDeployment.Spec
@@ -283,7 +284,7 @@ func (r *MCPServerReconciler) createRBACResource(
283284
) error {
284285
desired := createResource()
285286
if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil {
286-
logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
287+
r.logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
287288
return nil
288289
}
289290

@@ -309,7 +310,7 @@ func (r *MCPServerReconciler) updateRBACResourceIfNeeded(
309310
) error {
310311
desired := createResource()
311312
if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil {
312-
logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
313+
r.logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
313314
return nil
314315
}
315316

@@ -404,7 +405,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(m *mcpv1alpha1.MCPServer) *
404405
if finalPodTemplateSpec != nil {
405406
podTemplatePatch, err := json.Marshal(finalPodTemplateSpec)
406407
if err != nil {
407-
logger.Errorf("Failed to marshal pod template spec: %v", err)
408+
r.logger.Errorf("Failed to marshal pod template spec: %v", err)
408409
} else {
409410
args = append(args, fmt.Sprintf("--k8s-pod-patch=%s", string(podTemplatePatch)))
410411
}
@@ -619,7 +620,7 @@ func (r *MCPServerReconciler) deploymentForMCPServer(m *mcpv1alpha1.MCPServer) *
619620

620621
// Set MCPServer instance as the owner and controller
621622
if err := controllerutil.SetControllerReference(m, dep, r.Scheme); err != nil {
622-
logger.Error("Failed to set controller reference for Deployment", err)
623+
r.logger.Error("Failed to set controller reference for Deployment", err)
623624
return nil
624625
}
625626
return dep
@@ -676,7 +677,7 @@ func (r *MCPServerReconciler) serviceForMCPServer(m *mcpv1alpha1.MCPServer) *cor
676677

677678
// Set MCPServer instance as the owner and controller
678679
if err := controllerutil.SetControllerReference(m, svc, r.Scheme); err != nil {
679-
logger.Error("Failed to set controller reference for Service", err)
680+
r.logger.Error("Failed to set controller reference for Service", err)
680681
return nil
681682
}
682683
return svc
@@ -773,7 +774,7 @@ func (r *MCPServerReconciler) finalizeMCPServer(ctx context.Context, m *mcpv1alp
773774
// deploymentNeedsUpdate checks if the deployment needs to be updated
774775
//
775776
//nolint:gocyclo
776-
func deploymentNeedsUpdate(deployment *appsv1.Deployment, mcpServer *mcpv1alpha1.MCPServer) bool {
777+
func deploymentNeedsUpdate(deployment *appsv1.Deployment, mcpServer *mcpv1alpha1.MCPServer, logger *zap.SugaredLogger) bool {
777778
// Check if the container args have changed
778779
if len(deployment.Spec.Template.Spec.Containers) > 0 {
779780
container := deployment.Spec.Template.Spec.Containers[0]
@@ -1198,13 +1199,13 @@ func (r *MCPServerReconciler) generateOIDCArgs(ctx context.Context, m *mcpv1alph
11981199
}
11991200

12001201
// generateKubernetesOIDCArgs generates OIDC args for Kubernetes service account token validation
1201-
func (*MCPServerReconciler) generateKubernetesOIDCArgs(m *mcpv1alpha1.MCPServer) []string {
1202+
func (r *MCPServerReconciler) generateKubernetesOIDCArgs(m *mcpv1alpha1.MCPServer) []string {
12021203
var args []string
12031204
config := m.Spec.OIDCConfig.Kubernetes
12041205

12051206
// Set defaults if config is nil
12061207
if config == nil {
1207-
logger.Infof("Kubernetes OIDCConfig is nil for MCPServer %s, using default configuration", m.Name)
1208+
r.logger.Infof("Kubernetes OIDCConfig is nil for MCPServer %s, using default configuration", m.Name)
12081209
defaultUseClusterAuth := true
12091210
config = &mcpv1alpha1.KubernetesOIDCConfig{
12101211
UseClusterAuth: &defaultUseClusterAuth, // Default to true
@@ -1272,7 +1273,7 @@ func (r *MCPServerReconciler) generateConfigMapOIDCArgs( // nolint:gocyclo
12721273
Namespace: m.Namespace,
12731274
}, configMap)
12741275
if err != nil {
1275-
logger.Errorf("Failed to get ConfigMap %s: %v", config.Name, err)
1276+
r.logger.Errorf("Failed to get ConfigMap %s: %v", config.Name, err)
12761277
return args
12771278
}
12781279

cmd/thv-operator/controllers/mcpserver_oidc_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,9 @@ import (
2727
"sigs.k8s.io/controller-runtime/pkg/client/fake"
2828

2929
mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
30-
"github.com/stacklok/toolhive/pkg/logger"
30+
log "github.com/stacklok/toolhive/pkg/logger"
3131
)
3232

33-
func init() {
34-
// Initialize logger for tests
35-
logger.Initialize()
36-
}
37-
3833
func TestGenerateOIDCArgs(t *testing.T) {
3934
t.Parallel()
4035

@@ -230,6 +225,7 @@ func TestGenerateOIDCArgs(t *testing.T) {
230225
reconciler := &MCPServerReconciler{
231226
Client: fakeClient,
232227
Scheme: scheme,
228+
logger: log.NewLogger(),
233229
}
234230

235231
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
@@ -245,7 +241,9 @@ func TestGenerateOIDCArgs(t *testing.T) {
245241
func TestGenerateKubernetesOIDCArgs(t *testing.T) {
246242
t.Parallel()
247243

248-
reconciler := &MCPServerReconciler{}
244+
reconciler := &MCPServerReconciler{
245+
logger: log.NewLogger(),
246+
}
249247

250248
tests := []struct {
251249
name string
@@ -357,7 +355,9 @@ func TestGenerateKubernetesOIDCArgs(t *testing.T) {
357355
func TestGenerateInlineOIDCArgs(t *testing.T) {
358356
t.Parallel()
359357

360-
reconciler := &MCPServerReconciler{}
358+
reconciler := &MCPServerReconciler{
359+
logger: log.NewLogger(),
360+
}
361361

362362
tests := []struct {
363363
name string

cmd/thv-operator/controllers/mcpserver_resource_overrides_test.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"sigs.k8s.io/controller-runtime/pkg/client/fake"
2626

2727
mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
28+
log "github.com/stacklok/toolhive/pkg/logger"
2829
)
2930

3031
func TestResourceOverrides(t *testing.T) {
@@ -263,6 +264,7 @@ func TestResourceOverrides(t *testing.T) {
263264
r := &MCPServerReconciler{
264265
Client: client,
265266
Scheme: scheme,
267+
logger: log.NewLogger(),
266268
}
267269

268270
// Test deployment creation
@@ -366,6 +368,7 @@ func TestDeploymentNeedsUpdateServiceAccount(t *testing.T) {
366368
r := &MCPServerReconciler{
367369
Client: client,
368370
Scheme: scheme,
371+
logger: log.NewLogger(),
369372
}
370373

371374
mcpServer := &mcpv1alpha1.MCPServer{
@@ -384,7 +387,7 @@ func TestDeploymentNeedsUpdateServiceAccount(t *testing.T) {
384387
require.NotNil(t, deployment)
385388

386389
// Test with the current deployment - this should NOT need update
387-
needsUpdate := deploymentNeedsUpdate(deployment, mcpServer)
390+
needsUpdate := deploymentNeedsUpdate(deployment, mcpServer, r.logger)
388391

389392
// With the service account bug fixed, this should now return false
390393
assert.False(t, needsUpdate, "deploymentNeedsUpdate should return false when deployment matches MCPServer spec")
@@ -400,6 +403,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
400403
r := &MCPServerReconciler{
401404
Client: client,
402405
Scheme: scheme,
406+
logger: log.NewLogger(),
403407
}
404408

405409
tests := []struct {
@@ -563,7 +567,7 @@ func TestDeploymentNeedsUpdateProxyEnv(t *testing.T) {
563567
deployment.Spec.Template.Spec.Containers[0].Image = getToolhiveRunnerImage()
564568

565569
// Test if deployment needs update - should correlate with env change expectation
566-
needsUpdate := deploymentNeedsUpdate(deployment, tt.mcpServer)
570+
needsUpdate := deploymentNeedsUpdate(deployment, tt.mcpServer, r.logger)
567571

568572
if tt.expectEnvChange {
569573
assert.True(t, needsUpdate, "Expected deployment update due to proxy env change")
@@ -588,6 +592,7 @@ func TestDeploymentNeedsUpdateToolsFilter(t *testing.T) {
588592
r := &MCPServerReconciler{
589593
Client: client,
590594
Scheme: scheme,
595+
logger: log.NewLogger(),
591596
}
592597

593598
tests := []struct {
@@ -643,7 +648,7 @@ func TestDeploymentNeedsUpdateToolsFilter(t *testing.T) {
643648

644649
mcpServer.Spec.ToolsFilter = tt.newToolsFilter
645650

646-
needsUpdate := deploymentNeedsUpdate(deployment, mcpServer)
651+
needsUpdate := deploymentNeedsUpdate(deployment, mcpServer, r.logger)
647652
assert.Equal(t, tt.expectEnvChange, needsUpdate)
648653
})
649654
}

cmd/thv-operator/main.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,6 @@ func main() {
4848
"Enabling this will ensure there is only one active controller manager.")
4949
flag.Parse()
5050

51-
// Initialize the structured logger
52-
logger.Initialize()
53-
5451
// Set the controller-runtime logger to use our structured logger
5552
ctrl.SetLogger(logger.NewLogr())
5653

cmd/thv-proxyrunner/app/commands.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@ package app
44
import (
55
"github.com/spf13/cobra"
66
"github.com/spf13/viper"
7+
"go.uber.org/zap"
78

8-
"github.com/stacklok/toolhive/pkg/logger"
9+
log "github.com/stacklok/toolhive/pkg/logger"
910
)
1011

12+
var logger *zap.SugaredLogger = log.NewLogger()
13+
1114
var rootCmd = &cobra.Command{
1215
Use: "thv-proxyrunner",
1316
DisableAutoGenTag: true,
@@ -20,9 +23,6 @@ It is written in Go and has extensive test coverage—including input validation
2023
logger.Errorf("Error displaying help: %v", err)
2124
}
2225
},
23-
PersistentPreRun: func(_ *cobra.Command, _ []string) {
24-
logger.Initialize()
25-
},
2626
}
2727

2828
// NewRootCmd creates a new root command for the ToolHive CLI.

cmd/thv-proxyrunner/app/run.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"github.com/spf13/cobra"
99

1010
"github.com/stacklok/toolhive/pkg/container"
11-
"github.com/stacklok/toolhive/pkg/logger"
1211
"github.com/stacklok/toolhive/pkg/registry"
1312
"github.com/stacklok/toolhive/pkg/runner"
1413
"github.com/stacklok/toolhive/pkg/transport"
@@ -224,7 +223,7 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
224223
finalOtelEndpoint, finalOtelSamplingRate, finalOtelEnvironmentVariables := "", 0.0, []string{}
225224

226225
// Create container runtime
227-
rt, err := container.NewFactory().Create(ctx)
226+
rt, err := container.NewFactory(logger).Create(ctx)
228227
if err != nil {
229228
return fmt.Errorf("failed to create container runtime: %v", err)
230229
}
@@ -233,12 +232,12 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
233232
// If we have called the CLI directly, we use the CLIEnvVarValidator.
234233
// If we are running in detached mode, or the CLI is wrapped by the K8s operator,
235234
// we use the DetachedEnvVarValidator.
236-
envVarValidator := &runner.DetachedEnvVarValidator{}
235+
envVarValidator := runner.NewDetachedEnvVarValidator(logger)
237236

238237
var imageMetadata *registry.ImageMetadata
239238

240239
// Initialize a new RunConfig with values from command-line flags
241-
runConfig, err := runner.NewRunConfigBuilder().
240+
runConfig, err := runner.NewRunConfigBuilder(logger).
242241
WithRuntime(rt).
243242
WithCmdArgs(cmdArgs).
244243
WithName(runName).
@@ -266,7 +265,7 @@ func runCmdFunc(cmd *cobra.Command, args []string) error {
266265
return fmt.Errorf("failed to create RunConfig: %v", err)
267266
}
268267

269-
workloadManager := workloads.NewManagerFromRuntime(rt)
268+
workloadManager := workloads.NewManagerFromRuntime(rt, logger)
270269
return workloadManager.RunWorkload(ctx, runConfig)
271270
}
272271

0 commit comments

Comments
 (0)