Skip to content

Replace internal logger with uber-go/zap #1362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/regup/app/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ package app

import (
"github.com/spf13/cobra"

log "github.com/stacklok/toolhive/pkg/logger"
)

var logger = log.NewLogger()

var rootCmd = &cobra.Command{
Use: "regup",
DisableAutoGenTag: true,
Expand Down
3 changes: 1 addition & 2 deletions cmd/regup/app/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/spf13/cobra"

"github.com/stacklok/toolhive/pkg/container/verifier"
"github.com/stacklok/toolhive/pkg/logger"
"github.com/stacklok/toolhive/pkg/registry"
)

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

// Create verifier
v, err := verifier.New(server)
v, err := verifier.New(server, logger)
if err != nil {
return fmt.Errorf("failed to create verifier: %w", err)
}
Expand Down
3 changes: 0 additions & 3 deletions cmd/regup/app/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,11 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/stacklok/toolhive/pkg/logger"
"github.com/stacklok/toolhive/pkg/registry"
)

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

tests := []struct {
name string
Expand Down
5 changes: 2 additions & 3 deletions cmd/regup/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ import (
"os"

"github.com/stacklok/toolhive/cmd/regup/app"
"github.com/stacklok/toolhive/pkg/logger"
log "github.com/stacklok/toolhive/pkg/logger"
)

func main() {
// Initialize the logger system
logger.Initialize()
logger := log.NewLogger()

if err := app.NewRootCmd().Execute(); err != nil {
logger.Errorf("%v", err)
Expand Down
23 changes: 12 additions & 11 deletions cmd/thv-operator/controllers/mcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"strings"
"time"

"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
Expand All @@ -28,13 +29,13 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log"

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

// MCPServerReconciler reconciles a MCPServer object
type MCPServerReconciler struct {
client.Client
Scheme *runtime.Scheme
logger *zap.SugaredLogger
}

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

// Check if the deployment spec changed
if deploymentNeedsUpdate(deployment, mcpServer) {
if deploymentNeedsUpdate(deployment, mcpServer, r.logger) {
// Update the deployment
newDeployment := r.deploymentForMCPServer(mcpServer)
deployment.Spec = newDeployment.Spec
Expand Down Expand Up @@ -283,7 +284,7 @@ func (r *MCPServerReconciler) createRBACResource(
) error {
desired := createResource()
if err := controllerutil.SetControllerReference(mcpServer, desired, r.Scheme); err != nil {
logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
r.logger.Error(fmt.Sprintf("Failed to set controller reference for %s", resourceType), err)
return nil
}

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

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

// Set MCPServer instance as the owner and controller
if err := controllerutil.SetControllerReference(m, dep, r.Scheme); err != nil {
logger.Error("Failed to set controller reference for Deployment", err)
r.logger.Error("Failed to set controller reference for Deployment", err)
return nil
}
return dep
Expand Down Expand Up @@ -676,7 +677,7 @@ 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.Error("Failed to set controller reference for Service", err)
r.logger.Error("Failed to set controller reference for Service", err)
return nil
}
return svc
Expand Down Expand Up @@ -773,7 +774,7 @@ func (r *MCPServerReconciler) finalizeMCPServer(ctx context.Context, m *mcpv1alp
// deploymentNeedsUpdate checks if the deployment needs to be updated
//
//nolint:gocyclo
func deploymentNeedsUpdate(deployment *appsv1.Deployment, mcpServer *mcpv1alpha1.MCPServer) bool {
func deploymentNeedsUpdate(deployment *appsv1.Deployment, mcpServer *mcpv1alpha1.MCPServer, logger *zap.SugaredLogger) bool {
// Check if the container args have changed
if len(deployment.Spec.Template.Spec.Containers) > 0 {
container := deployment.Spec.Template.Spec.Containers[0]
Expand Down Expand Up @@ -1198,13 +1199,13 @@ 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 (r *MCPServerReconciler) generateKubernetesOIDCArgs(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)
r.logger.Infof("Kubernetes OIDCConfig is nil for MCPServer %s, using default configuration", m.Name)
defaultUseClusterAuth := true
config = &mcpv1alpha1.KubernetesOIDCConfig{
UseClusterAuth: &defaultUseClusterAuth, // Default to true
Expand Down Expand Up @@ -1272,7 +1273,7 @@ func (r *MCPServerReconciler) generateConfigMapOIDCArgs( // nolint:gocyclo
Namespace: m.Namespace,
}, configMap)
if err != nil {
logger.Errorf("Failed to get ConfigMap %s: %v", config.Name, err)
r.logger.Errorf("Failed to get ConfigMap %s: %v", config.Name, err)
return args
}

Expand Down
16 changes: 8 additions & 8 deletions cmd/thv-operator/controllers/mcpserver_oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,9 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"

mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
"github.com/stacklok/toolhive/pkg/logger"
log "github.com/stacklok/toolhive/pkg/logger"
)

func init() {
// Initialize logger for tests
logger.Initialize()
}

func TestGenerateOIDCArgs(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -230,6 +225,7 @@ func TestGenerateOIDCArgs(t *testing.T) {
reconciler := &MCPServerReconciler{
Client: fakeClient,
Scheme: scheme,
logger: log.NewLogger(),
}

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

reconciler := &MCPServerReconciler{}
reconciler := &MCPServerReconciler{
logger: log.NewLogger(),
}

tests := []struct {
name string
Expand Down Expand Up @@ -357,7 +355,9 @@ func TestGenerateKubernetesOIDCArgs(t *testing.T) {
func TestGenerateInlineOIDCArgs(t *testing.T) {
t.Parallel()

reconciler := &MCPServerReconciler{}
reconciler := &MCPServerReconciler{
logger: log.NewLogger(),
}

tests := []struct {
name string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client/fake"

mcpv1alpha1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1alpha1"
log "github.com/stacklok/toolhive/pkg/logger"
)

func TestResourceOverrides(t *testing.T) {
Expand Down Expand Up @@ -263,6 +264,7 @@ func TestResourceOverrides(t *testing.T) {
r := &MCPServerReconciler{
Client: client,
Scheme: scheme,
logger: log.NewLogger(),
}

// Test deployment creation
Expand Down Expand Up @@ -366,6 +368,7 @@ func TestDeploymentNeedsUpdateServiceAccount(t *testing.T) {
r := &MCPServerReconciler{
Client: client,
Scheme: scheme,
logger: log.NewLogger(),
}

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

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

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

tests := []struct {
Expand Down Expand Up @@ -563,7 +567,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 := deploymentNeedsUpdate(deployment, tt.mcpServer)
needsUpdate := deploymentNeedsUpdate(deployment, tt.mcpServer, r.logger)

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

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

mcpServer.Spec.ToolsFilter = tt.newToolsFilter

needsUpdate := deploymentNeedsUpdate(deployment, mcpServer)
needsUpdate := deploymentNeedsUpdate(deployment, mcpServer, r.logger)
assert.Equal(t, tt.expectEnvChange, needsUpdate)
})
}
Expand Down
3 changes: 0 additions & 3 deletions cmd/thv-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ func main() {
"Enabling this will ensure there is only one active controller manager.")
flag.Parse()

// Initialize the structured logger
logger.Initialize()

// Set the controller-runtime logger to use our structured logger
ctrl.SetLogger(logger.NewLogr())

Expand Down
8 changes: 4 additions & 4 deletions cmd/thv-proxyrunner/app/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ package app
import (
"github.com/spf13/cobra"
"github.com/spf13/viper"
"go.uber.org/zap"

"github.com/stacklok/toolhive/pkg/logger"
log "github.com/stacklok/toolhive/pkg/logger"
)

var logger *zap.SugaredLogger = log.NewLogger()

var rootCmd = &cobra.Command{
Use: "thv-proxyrunner",
DisableAutoGenTag: true,
Expand All @@ -20,9 +23,6 @@ It is written in Go and has extensive test coverage—including input validation
logger.Errorf("Error displaying help: %v", err)
}
},
PersistentPreRun: func(_ *cobra.Command, _ []string) {
logger.Initialize()
},
}

// NewRootCmd creates a new root command for the ToolHive CLI.
Expand Down
9 changes: 4 additions & 5 deletions cmd/thv-proxyrunner/app/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"github.com/spf13/cobra"

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

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

var imageMetadata *registry.ImageMetadata

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

workloadManager, err := workloads.NewManagerFromRuntime(rt)
workloadManager, err := workloads.NewManagerFromRuntime(rt, logger)
if err != nil {
return fmt.Errorf("failed to create workload manager: %v", err)
}
Expand Down
4 changes: 0 additions & 4 deletions cmd/thv-proxyrunner/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,9 @@ import (
"os"

"github.com/stacklok/toolhive/cmd/thv-proxyrunner/app"
"github.com/stacklok/toolhive/pkg/logger"
)

func main() {
// Initialize the logger
logger.Initialize()

// Skip update check for completion command or if we are running in kubernetes
if err := app.NewRootCmd().Execute(); err != nil {
os.Exit(1)
Expand Down
Loading