From 3d4960691205705bad81d117535457f669fa25ff Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:42:01 +1200 Subject: [PATCH 1/9] Import zapr and zap packages --- v2/go.mod | 3 +++ v2/go.sum | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/v2/go.mod b/v2/go.mod index 6ee5536c989..bc2f4846aed 100644 --- a/v2/go.mod +++ b/v2/go.mod @@ -27,6 +27,7 @@ require ( github.com/benbjohnson/clock v1.3.5 github.com/dnaeon/go-vcr v1.2.0 github.com/go-logr/logr v1.4.2 + github.com/go-logr/zapr v1.3.0 github.com/go-sql-driver/mysql v1.8.1 github.com/google/go-cmp v0.6.0 github.com/google/uuid v1.6.0 @@ -40,6 +41,7 @@ require ( github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.20.2 github.com/spf13/cobra v1.8.1 + go.uber.org/zap v1.27.0 golang.org/x/crypto v0.26.0 golang.org/x/exp v0.0.0-20240823005443-9b4947da3948 golang.org/x/sync v0.8.0 @@ -115,6 +117,7 @@ require ( go.opentelemetry.io/otel/sdk v1.29.0 // indirect go.opentelemetry.io/otel/trace v1.29.0 // indirect go.opentelemetry.io/proto/otlp v1.3.1 // indirect + go.uber.org/multierr v1.11.0 // indirect golang.org/x/net v0.28.0 // indirect golang.org/x/oauth2 v0.22.0 // indirect golang.org/x/sys v0.24.0 // indirect diff --git a/v2/go.sum b/v2/go.sum index 88b8d7586ca..900c98b6688 100644 --- a/v2/go.sum +++ b/v2/go.sum @@ -469,8 +469,8 @@ go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9i go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= go.uber.org/zap v1.17.0/go.mod h1:MXVU+bhUf/A7Xi2HNOnopQOrmycQ5Ih87HtOu4q5SSo= -go.uber.org/zap v1.26.0 h1:sI7k6L95XOKS281NhVKOFCUNIvv9e0w4BF8N3u+tCRo= -go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so= +go.uber.org/zap v1.27.0 h1:aJMhYGrd5QSmlpLMr2MftRKl7t8J8PTZPA732ud/XR8= +go.uber.org/zap v1.27.0/go.mod h1:GB2qFLM7cTU87MWRP2mPIjqfIDnGu+VIO4V/SdhGo2E= golang.org/x/crypto v0.0.0-20181029021203-45a5f77698d3/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= From f0549c4ec82da45a1bdd9b96d4414e0b70d35672 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:42:21 +1200 Subject: [PATCH 2/9] Create logging package to create loggers --- v2/cmd/controller/logging/logging.go | 87 ++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 v2/cmd/controller/logging/logging.go diff --git a/v2/cmd/controller/logging/logging.go b/v2/cmd/controller/logging/logging.go new file mode 100644 index 00000000000..27eccc18be7 --- /dev/null +++ b/v2/cmd/controller/logging/logging.go @@ -0,0 +1,87 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package logging + +import ( + "flag" + + "github.com/go-logr/logr" + "github.com/go-logr/zapr" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "k8s.io/klog/v2/textlogger" +) + +var ( + // verbose indicates whether we should use verbose logging. + // Default is no + verbose *bool + + // useJson indicates whether we should output logs in JSON format. + // Default is no + useJson *bool +) + +// Create returns a new logger, ready for use. +// This can be called multiple times if required. +func Create() logr.Logger { + if useJson != nil && *useJson { + log, err := createJSONLogger() + if err != nil { + log = createTextLogger() + log.Error(err, "failed to create JSON logger, falling back to text") + } + + return log + } + + return createTextLogger() +} + +func createTextLogger() logr.Logger { + opts := []textlogger.ConfigOption{} + if verbose != nil && *verbose { + opts = append(opts, textlogger.Verbosity(3)) + } + + cfg := textlogger.NewConfig(opts...) + return textlogger.NewLogger(cfg) +} + +func createJSONLogger() (logr.Logger, error) { + + level := zap.NewAtomicLevelAt(zap.InfoLevel) + if verbose != nil && *verbose { + level = zap.NewAtomicLevelAt(zap.DebugLevel) + } + + encoder := zap.NewProductionEncoderConfig() + encoder.EncodeTime = zapcore.ISO8601TimeEncoder + + cfg := zap.Config{ + Level: level, + Development: false, + Encoding: "json", + EncoderConfig: encoder, + OutputPaths: []string{"stderr"}, + ErrorOutputPaths: []string{"stderr"}, + } + + logger, err := cfg.Build() + if err != nil { + return logr.Logger{}, err + } + + return zapr.NewLogger(logger), nil +} + +// InitFlags initializes the flags for the logging package +func InitFlags(fs *flag.FlagSet) { + verbose = fs.Bool("verbose", false, "Enable verbose logging") + fs.BoolVar(verbose, "v", false, "Enable verbose logging") + + useJson = fs.Bool("json-logging", false, "Enable JSON logging") +} From dde8ce29c3d6d7830fc572e85602813eb7de601f Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:43:01 +1200 Subject: [PATCH 3/9] Change ParseFlags() to InitFlags() --- v2/cmd/controller/app/flags.go | 86 ++++++++++++++-------------------- v2/cmd/controller/app/setup.go | 22 ++++----- 2 files changed, 46 insertions(+), 62 deletions(-) diff --git a/v2/cmd/controller/app/flags.go b/v2/cmd/controller/app/flags.go index 6383ee9effa..ea36206a0e0 100644 --- a/v2/cmd/controller/app/flags.go +++ b/v2/cmd/controller/app/flags.go @@ -8,77 +8,61 @@ package app import ( "flag" "fmt" - - "k8s.io/klog/v2" - - "github.com/Azure/azure-service-operator/v2/internal/version" ) type Flags struct { - MetricsAddr string - ProfilingMetrics bool - SecureMetrics bool - HealthAddr string - WebhookPort int - WebhookCertDir string - EnableLeaderElection bool - CRDManagementMode string - CRDPatterns string // This is a ';' delimited string containing a collection of patterns + MetricsAddr *string + ProfilingMetrics *bool + SecureMetrics *bool + HealthAddr *string + WebhookPort *int + WebhookCertDir *string + EnableLeaderElection *bool + CRDManagementMode *string + CRDPatterns *string // This is a ';' delimited string containing a collection of patterns } func (f Flags) String() string { return fmt.Sprintf( "MetricsAddr: %s, SecureMetrics: %t, ProfilingMetrics: %t, HealthAddr: %s, WebhookPort: %d, WebhookCertDir: %s, EnableLeaderElection: %t, CRDManagementMode: %s, CRDPatterns: %s", - f.MetricsAddr, - f.SecureMetrics, - f.ProfilingMetrics, - f.HealthAddr, - f.WebhookPort, - f.WebhookCertDir, - f.EnableLeaderElection, - f.CRDManagementMode, - f.CRDPatterns) + *f.MetricsAddr, + *f.SecureMetrics, + *f.ProfilingMetrics, + *f.HealthAddr, + *f.WebhookPort, + *f.WebhookCertDir, + *f.EnableLeaderElection, + *f.CRDManagementMode, + *f.CRDPatterns) } -func ParseFlags(args []string) (Flags, error) { - exeName := args[0] + " " + version.BuildVersion - flagSet := flag.NewFlagSet(exeName, flag.ExitOnError) - klog.InitFlags(flagSet) - - var metricsAddr string - var profilingMetrics bool - var secureMetrics bool - var healthAddr string - var webhookPort int - var webhookCertDir string - var enableLeaderElection bool - var crdManagementMode string - var crdPatterns string +func InitFlags(flagSet *flag.FlagSet) Flags { // default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted. - flagSet.StringVar(&metricsAddr, "metrics-addr", "0", "The address the metric endpoint binds to.") - flagSet.BoolVar(&secureMetrics, "secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS") - flagSet.BoolVar(&profilingMetrics, "profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints") + metricsAddr := flagSet.String("metrics-addr", "0", "The address the metric endpoint binds to.") + secureMetrics := flagSet.Bool("secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS") + profilingMetrics := flagSet.Bool("profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints") - flagSet.StringVar(&healthAddr, "health-addr", "", "The address the healthz endpoint binds to.") - flagSet.IntVar(&webhookPort, "webhook-port", 9443, "The port the webhook endpoint binds to.") - flagSet.StringVar(&webhookCertDir, "webhook-cert-dir", "", "The directory the webhook server's certs are stored.") - flagSet.BoolVar(&enableLeaderElection, "enable-leader-election", false, - "Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.") - flagSet.StringVar(&crdManagementMode, "crd-management", "auto", - "Instructs the operator on how it should manage the Custom Resource Definitions. One of 'auto', 'none'") - flagSet.StringVar(&crdPatterns, "crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.") + healthAddr := flagSet.String("health-addr", "", "The address the healthz endpoint binds to.") + webhookPort := flagSet.Int("webhook-port", 9443, "The port the webhook endpoint binds to.") + webhookCertDir := flagSet.String("webhook-cert-dir", "", "The directory the webhook server's certs are stored.") - flagSet.Parse(args[1:]) //nolint:errcheck + enableLeaderElection := flagSet.Bool("enable-leader-election", false, "Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.") + + crdManagementMode := flagSet.String("crd-management", "auto", + "Instructs the operator on how it should manage the Custom Resource Definitions. One of 'auto', 'none'") + crdPatterns := flagSet.String("crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.") return Flags{ - MetricsAddr: metricsAddr, - SecureMetrics: secureMetrics, + MetricsAddr: metricsAddr, + SecureMetrics: secureMetrics, + ProfilingMetrics: profilingMetrics, + HealthAddr: healthAddr, WebhookPort: webhookPort, WebhookCertDir: webhookCertDir, EnableLeaderElection: enableLeaderElection, CRDManagementMode: crdManagementMode, CRDPatterns: crdPatterns, - }, nil + } } diff --git a/v2/cmd/controller/app/setup.go b/v2/cmd/controller/app/setup.go index 49dd4415f85..09ef3d86064 100644 --- a/v2/cmd/controller/app/setup.go +++ b/v2/cmd/controller/app/setup.go @@ -78,13 +78,13 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag mgr, err := ctrl.NewManager(k8sConfig, ctrl.Options{ Scheme: scheme, NewCache: cacheFunc, - LeaderElection: flgs.EnableLeaderElection, + LeaderElection: *flgs.EnableLeaderElection, LeaderElectionID: "controllers-leader-election-azinfra-generated", - HealthProbeBindAddress: flgs.HealthAddr, + HealthProbeBindAddress: *flgs.HealthAddr, Metrics: getMetricsOpts(flgs), WebhookServer: webhook.NewServer(webhook.Options{ - Port: flgs.WebhookPort, - CertDir: flgs.WebhookCertDir, + Port: *flgs.WebhookPort, + CertDir: *flgs.WebhookCertDir, }), }) if err != nil { @@ -110,7 +110,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag os.Exit(1) } - switch flgs.CRDManagementMode { + switch *flgs.CRDManagementMode { case "auto": var goalCRDs []apiextensions.CustomResourceDefinition goalCRDs, err = crdManager.LoadOperatorCRDs(crdmanagement.CRDLocation, cfg.PodNamespace) @@ -122,7 +122,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag // We only apply CRDs if we're in webhooks mode. No other mode will have CRD CRUD permissions if cfg.OperatorMode.IncludesWebhooks() { var installationInstructions []*crdmanagement.CRDInstallationInstruction - installationInstructions, err = crdManager.DetermineCRDsToInstallOrUpgrade(goalCRDs, existingCRDs, flgs.CRDPatterns) + installationInstructions, err = crdManager.DetermineCRDsToInstallOrUpgrade(goalCRDs, existingCRDs, *flgs.CRDPatterns) if err != nil { setupLog.Error(err, "failed to determine CRDs to apply") os.Exit(1) @@ -145,7 +145,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag case "none": setupLog.Info("CRD management mode was set to 'none', the operator will not manage CRDs and assumes they are already installed and matching the operator version") default: - setupLog.Error(fmt.Errorf("invalid CRD management mode: %s", flgs.CRDManagementMode), "failed to initialize CRD client") + setupLog.Error(fmt.Errorf("invalid CRD management mode: %s", *flgs.CRDManagementMode), "failed to initialize CRD client") os.Exit(1) } @@ -214,14 +214,14 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag func getMetricsOpts(flags Flags) server.Options { var metricsOptions server.Options - if flags.SecureMetrics { + if *flags.SecureMetrics { metricsOptions = server.Options{ - BindAddress: flags.MetricsAddr, + BindAddress: *flags.MetricsAddr, SecureServing: true, FilterProvider: filters.WithAuthenticationAndAuthorization, } // Note that pprof endpoints are meant to be sensitive and shouldn't be exposed publicly. - if flags.ProfilingMetrics { + if *flags.ProfilingMetrics { metricsOptions.ExtraHandlers = map[string]http.Handler{ "/debug/pprof/": http.HandlerFunc(pprof.Index), "/debug/pprof/cmdline": http.HandlerFunc(pprof.Cmdline), @@ -232,7 +232,7 @@ func getMetricsOpts(flags Flags) server.Options { } } else { metricsOptions = server.Options{ - BindAddress: flags.MetricsAddr, + BindAddress: *flags.MetricsAddr, } } From 8d84cc96d756756330a3ac071e249bc9f174fe39 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:43:23 +1200 Subject: [PATCH 4/9] Refactor main --- v2/cmd/controller/main.go | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/v2/cmd/controller/main.go b/v2/cmd/controller/main.go index 744e28a9632..1d267fdbaf6 100644 --- a/v2/cmd/controller/main.go +++ b/v2/cmd/controller/main.go @@ -6,31 +6,45 @@ Licensed under the MIT license. package main import ( + "flag" "os" _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" - "k8s.io/klog/v2/klogr" ctrl "sigs.k8s.io/controller-runtime" "github.com/Azure/azure-service-operator/v2/cmd/controller/app" + "github.com/Azure/azure-service-operator/v2/cmd/controller/logging" + "github.com/Azure/azure-service-operator/v2/internal/version" ) func main() { - setupLog := ctrl.Log.WithName("setup") - ctrl.SetLogger(klogr.New()) //nolint: staticcheck + // Set up to parse command line flags + exeName := os.Args[0] + " " + version.BuildVersion + flagSet := flag.NewFlagSet(exeName, flag.ExitOnError) + + // Create a temporary logger for while we get set up + log := logging.Create() + ctx := ctrl.SetupSignalHandler() - flgs, err := app.ParseFlags(os.Args) + // Add application and logging flags + flgs := app.InitFlags(flagSet) + logging.InitFlags(flagSet) + err := flagSet.Parse(os.Args[1:]) if err != nil { - setupLog.Error(err, "failed to parse cmdline flags") + log.Error(err, "failed to parse cmdline flags") os.Exit(1) } - setupLog.Info("Launching with flags", "flags", flgs.String()) - mgr := app.SetupControllerManager(ctx, setupLog, flgs) - setupLog.Info("starting manager") + // Replace the logger with a configured one + log = logging.Create() + ctrl.SetLogger(log) + log.Info("Launching with flags", "flags", flgs.String()) + + mgr := app.SetupControllerManager(ctx, log, flgs) + log.Info("starting manager") if err = mgr.Start(ctx); err != nil { - setupLog.Error(err, "failed to start manager") + log.Error(err, "failed to start manager") os.Exit(1) } } From 8c5a013c63a9ddd84ac7def5a66a3c2ad5e33fd2 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:47:18 +1200 Subject: [PATCH 5/9] Switch other use to textlogger --- v2/internal/genericarmclient/suite_test.go | 7 +++++-- v2/test/multitenant/suite_test.go | 7 +++++-- v2/test/pre-release/suite_test.go | 7 +++++-- v2/test/suite_test.go | 7 +++++-- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/v2/internal/genericarmclient/suite_test.go b/v2/internal/genericarmclient/suite_test.go index 9594cefc782..d4f9cf7fa54 100644 --- a/v2/internal/genericarmclient/suite_test.go +++ b/v2/internal/genericarmclient/suite_test.go @@ -13,7 +13,8 @@ import ( "github.com/onsi/gomega" "github.com/onsi/gomega/format" - "k8s.io/klog/v2/klogr" + "k8s.io/klog/v2/textlogger" + ctrl "sigs.k8s.io/controller-runtime" "github.com/Azure/azure-service-operator/v2/internal/testcommon" @@ -33,7 +34,9 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - ctrl.SetLogger(klogr.New()) //nolint: staticcheck + cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + log := textlogger.NewLogger(cfg) + ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.ResourcePrefix, diff --git a/v2/test/multitenant/suite_test.go b/v2/test/multitenant/suite_test.go index b3e4f6ee6d1..b9996bb4d9e 100644 --- a/v2/test/multitenant/suite_test.go +++ b/v2/test/multitenant/suite_test.go @@ -13,7 +13,8 @@ import ( "github.com/onsi/gomega" "github.com/onsi/gomega/format" - "k8s.io/klog/v2/klogr" + "k8s.io/klog/v2/textlogger" + ctrl "sigs.k8s.io/controller-runtime" "github.com/Azure/azure-service-operator/v2/internal/testcommon" @@ -37,7 +38,9 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - ctrl.SetLogger(klogr.New()) //nolint: staticcheck + cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + log := textlogger.NewLogger(cfg) + ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, diff --git a/v2/test/pre-release/suite_test.go b/v2/test/pre-release/suite_test.go index b3e4f6ee6d1..b9996bb4d9e 100644 --- a/v2/test/pre-release/suite_test.go +++ b/v2/test/pre-release/suite_test.go @@ -13,7 +13,8 @@ import ( "github.com/onsi/gomega" "github.com/onsi/gomega/format" - "k8s.io/klog/v2/klogr" + "k8s.io/klog/v2/textlogger" + ctrl "sigs.k8s.io/controller-runtime" "github.com/Azure/azure-service-operator/v2/internal/testcommon" @@ -37,7 +38,9 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - ctrl.SetLogger(klogr.New()) //nolint: staticcheck + cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + log := textlogger.NewLogger(cfg) + ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, diff --git a/v2/test/suite_test.go b/v2/test/suite_test.go index 88807e89a38..348dbfe9805 100644 --- a/v2/test/suite_test.go +++ b/v2/test/suite_test.go @@ -13,7 +13,8 @@ import ( "github.com/onsi/gomega" "github.com/onsi/gomega/format" - "k8s.io/klog/v2/klogr" + "k8s.io/klog/v2/textlogger" + ctrl "sigs.k8s.io/controller-runtime" "github.com/Azure/azure-service-operator/v2/internal/testcommon" @@ -37,7 +38,9 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - ctrl.SetLogger(klogr.New()) //nolint: staticcheck + cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + log := textlogger.NewLogger(cfg) + ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, From 473d62b7db08c2a246740edf81171164d68fa67f Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 14:51:18 +1200 Subject: [PATCH 6/9] Switch other uses --- v2/internal/controllers/suite_test.go | 5 ++++- .../testcommon/kube_test_context_envtest.go | 18 ++++++++++++------ v2/pkg/genruntime/test/suite_test.go | 5 ++++- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/v2/internal/controllers/suite_test.go b/v2/internal/controllers/suite_test.go index d56749760e8..261f054117c 100644 --- a/v2/internal/controllers/suite_test.go +++ b/v2/internal/controllers/suite_test.go @@ -42,8 +42,11 @@ func setup() error { // If you need to debug envtest setup/teardown, // set a global logger for controller-runtime: + // // import (ctrl "sigs.k8s.io/controller-runtime") - // ctrl.SetLogger(klogr.New()) + // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // log := textlogger.NewLogger(cfg) + // ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.ResourcePrefix, diff --git a/v2/internal/testcommon/kube_test_context_envtest.go b/v2/internal/testcommon/kube_test_context_envtest.go index 87c68571401..ac5dbf09574 100644 --- a/v2/internal/testcommon/kube_test_context_envtest.go +++ b/v2/internal/testcommon/kube_test_context_envtest.go @@ -30,7 +30,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/envtest" - ctrllog "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -89,10 +88,13 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources) Scheme: scheme, } - // TODO: Switch to klogr.New() below the below if we want controller-runtime logs in the tests. + // Switch logger below if we want controller-runtime logs in the tests. // By default we've disabled controller runtime logs because they're very verbose and usually not useful. - // ctrl.SetLogger(klogr.New()) - ctrl.SetLogger(logr.New(ctrllog.NullLogSink{})) + // import (ctrl "sigs.k8s.io/controller-runtime") + // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // log := textlogger.NewLogger(cfg) + // ctrl.SetLogger(log) + ctrl.SetLogger(logr.Discard()) log.Println("Starting envtest") kubeConfig, err := environment.Start() @@ -160,8 +162,12 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources) // TODO: Uncomment the below if we want controller-runtime logs in the tests. // By default we've disabled controller runtime logs because they're very verbose and usually not useful. - // ctrl.SetLogger(klogr.New()) - ctrl.SetLogger(logr.New(ctrllog.NullLogSink{})) + // + // import (ctrl "sigs.k8s.io/controller-runtime") + // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // log := textlogger.NewLogger(cfg) + // ctrl.SetLogger(log) + ctrl.SetLogger(logr.Discard()) loggerFactory := func(obj metav1.Object) logr.Logger { result := namespaceResources.Lookup(obj.GetNamespace()) diff --git a/v2/pkg/genruntime/test/suite_test.go b/v2/pkg/genruntime/test/suite_test.go index 787684c0246..ec1180ccc1c 100644 --- a/v2/pkg/genruntime/test/suite_test.go +++ b/v2/pkg/genruntime/test/suite_test.go @@ -41,8 +41,11 @@ func setup() error { // If you need to debug envtest setup/teardown, // set a global logger for controller-runtime: + // // import (ctrl "sigs.k8s.io/controller-runtime") - // ctrl.SetLogger(klogr.New()) + // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // log := textlogger.NewLogger(cfg) + // ctrl.SetLogger(log) nameConfig := testcommon.NewResourceNameConfig( testcommon.ResourcePrefix, From e118fc1a06008d168b3f951cbbe4a402d6595c31 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Aug 2024 21:15:33 +0000 Subject: [PATCH 7/9] Fix lint issues --- v2/cmd/controller/app/flags.go | 1 - v2/cmd/controller/logging/logging.go | 1 - v2/internal/genericarmclient/suite_test.go | 6 +++--- v2/test/multitenant/suite_test.go | 7 +++---- v2/test/pre-release/suite_test.go | 7 +++---- v2/test/suite_test.go | 7 +++---- 6 files changed, 12 insertions(+), 17 deletions(-) diff --git a/v2/cmd/controller/app/flags.go b/v2/cmd/controller/app/flags.go index ea36206a0e0..2c7463cc037 100644 --- a/v2/cmd/controller/app/flags.go +++ b/v2/cmd/controller/app/flags.go @@ -37,7 +37,6 @@ func (f Flags) String() string { } func InitFlags(flagSet *flag.FlagSet) Flags { - // default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted. metricsAddr := flagSet.String("metrics-addr", "0", "The address the metric endpoint binds to.") secureMetrics := flagSet.Bool("secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS") diff --git a/v2/cmd/controller/logging/logging.go b/v2/cmd/controller/logging/logging.go index 27eccc18be7..e58bd51e8fa 100644 --- a/v2/cmd/controller/logging/logging.go +++ b/v2/cmd/controller/logging/logging.go @@ -52,7 +52,6 @@ func createTextLogger() logr.Logger { } func createJSONLogger() (logr.Logger, error) { - level := zap.NewAtomicLevelAt(zap.InfoLevel) if verbose != nil && *verbose { level = zap.NewAtomicLevelAt(zap.DebugLevel) diff --git a/v2/internal/genericarmclient/suite_test.go b/v2/internal/genericarmclient/suite_test.go index d4f9cf7fa54..2b2e7ec28a1 100644 --- a/v2/internal/genericarmclient/suite_test.go +++ b/v2/internal/genericarmclient/suite_test.go @@ -27,8 +27,6 @@ var testContext testcommon.TestContext func setup() error { recordReplay := os.Getenv("RECORD_REPLAY") != "0" - log.Println("Running test setup") - gomega.SetDefaultEventuallyTimeout(DefaultEventuallyTimeout) gomega.SetDefaultEventuallyPollingInterval(5 * time.Second) format.TruncateThreshold = 4000 // Force a longer truncate threshold @@ -38,6 +36,8 @@ func setup() error { log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) + log.Info("Running test setup") + nameConfig := testcommon.NewResourceNameConfig( testcommon.ResourcePrefix, "-", @@ -47,7 +47,7 @@ func setup() error { // set global test context testContext = testcommon.NewTestContext(testcommon.DefaultTestRegion, recordReplay, nameConfig) - log.Println("Done with test setup") + log.Info("Done with test setup") return nil } diff --git a/v2/test/multitenant/suite_test.go b/v2/test/multitenant/suite_test.go index b9996bb4d9e..924a6d9ff96 100644 --- a/v2/test/multitenant/suite_test.go +++ b/v2/test/multitenant/suite_test.go @@ -6,7 +6,6 @@ Licensed under the MIT license. package multitenant_test import ( - "log" "os" "testing" "time" @@ -27,8 +26,6 @@ const ( var globalTestContext testcommon.KubeGlobalContext func setup() error { - log.Println("Running test setup") - // Note: These are set just so we have somewhat reasonable defaults. Almost all // usage of Eventually is done through the testContext wrapper which manages its // own timeouts. @@ -42,6 +39,8 @@ func setup() error { log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) + log.Info("Running test setup") + nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, "-", @@ -58,7 +57,7 @@ func setup() error { return err } - log.Print("Done with test setup") + log.Info("Done with test setup") globalTestContext = newGlobalTestContext return nil } diff --git a/v2/test/pre-release/suite_test.go b/v2/test/pre-release/suite_test.go index b9996bb4d9e..924a6d9ff96 100644 --- a/v2/test/pre-release/suite_test.go +++ b/v2/test/pre-release/suite_test.go @@ -6,7 +6,6 @@ Licensed under the MIT license. package multitenant_test import ( - "log" "os" "testing" "time" @@ -27,8 +26,6 @@ const ( var globalTestContext testcommon.KubeGlobalContext func setup() error { - log.Println("Running test setup") - // Note: These are set just so we have somewhat reasonable defaults. Almost all // usage of Eventually is done through the testContext wrapper which manages its // own timeouts. @@ -42,6 +39,8 @@ func setup() error { log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) + log.Info("Running test setup") + nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, "-", @@ -58,7 +57,7 @@ func setup() error { return err } - log.Print("Done with test setup") + log.Info("Done with test setup") globalTestContext = newGlobalTestContext return nil } diff --git a/v2/test/suite_test.go b/v2/test/suite_test.go index 348dbfe9805..9e35f075c13 100644 --- a/v2/test/suite_test.go +++ b/v2/test/suite_test.go @@ -6,7 +6,6 @@ Licensed under the MIT license. package test import ( - "log" "os" "testing" "time" @@ -27,8 +26,6 @@ const ( var globalTestContext testcommon.KubeGlobalContext func setup() error { - log.Println("Running test setup") - // Note: These are set just so we have somewhat reasonable defaults. Almost all // usage of Eventually is done through the testContext wrapper which manages its // own timeouts. @@ -42,6 +39,8 @@ func setup() error { log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) + log.Info("Running test setup") + nameConfig := testcommon.NewResourceNameConfig( testcommon.LiveResourcePrefix, "-", @@ -58,7 +57,7 @@ func setup() error { return err } - log.Print("Done with test setup") + log.Info("Done with test setup") globalTestContext = newGlobalTestContext return nil } From 4202fc5b30a4299f8a565c5195ab3424f1797d0e Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Fri, 30 Aug 2024 00:41:19 +0000 Subject: [PATCH 8/9] Refactor based on PR feedback --- v2/cmd/controller/app/flags.go | 75 ++++++++----------- v2/cmd/controller/app/setup.go | 26 +++---- v2/cmd/controller/logging/logging.go | 69 ++++++++++------- v2/cmd/controller/main.go | 12 +-- v2/internal/controllers/suite_test.go | 2 +- v2/internal/genericarmclient/suite_test.go | 3 +- .../testcommon/kube_test_context_envtest.go | 4 +- v2/pkg/genruntime/test/suite_test.go | 2 +- v2/test/multitenant/suite_test.go | 3 +- v2/test/pre-release/suite_test.go | 3 +- v2/test/suite_test.go | 3 +- 11 files changed, 104 insertions(+), 98 deletions(-) diff --git a/v2/cmd/controller/app/flags.go b/v2/cmd/controller/app/flags.go index 2c7463cc037..29cb9a52875 100644 --- a/v2/cmd/controller/app/flags.go +++ b/v2/cmd/controller/app/flags.go @@ -11,57 +11,46 @@ import ( ) type Flags struct { - MetricsAddr *string - ProfilingMetrics *bool - SecureMetrics *bool - HealthAddr *string - WebhookPort *int - WebhookCertDir *string - EnableLeaderElection *bool - CRDManagementMode *string - CRDPatterns *string // This is a ';' delimited string containing a collection of patterns + MetricsAddr string + ProfilingMetrics bool + SecureMetrics bool + HealthAddr string + WebhookPort int + WebhookCertDir string + EnableLeaderElection bool + CRDManagementMode string + CRDPatterns string // This is a ';' delimited string containing a collection of patterns } func (f Flags) String() string { return fmt.Sprintf( "MetricsAddr: %s, SecureMetrics: %t, ProfilingMetrics: %t, HealthAddr: %s, WebhookPort: %d, WebhookCertDir: %s, EnableLeaderElection: %t, CRDManagementMode: %s, CRDPatterns: %s", - *f.MetricsAddr, - *f.SecureMetrics, - *f.ProfilingMetrics, - *f.HealthAddr, - *f.WebhookPort, - *f.WebhookCertDir, - *f.EnableLeaderElection, - *f.CRDManagementMode, - *f.CRDPatterns) + f.MetricsAddr, + f.SecureMetrics, + f.ProfilingMetrics, + f.HealthAddr, + f.WebhookPort, + f.WebhookCertDir, + f.EnableLeaderElection, + f.CRDManagementMode, + f.CRDPatterns) } -func InitFlags(flagSet *flag.FlagSet) Flags { - // default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted. - metricsAddr := flagSet.String("metrics-addr", "0", "The address the metric endpoint binds to.") - secureMetrics := flagSet.Bool("secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS") - profilingMetrics := flagSet.Bool("profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints") - - healthAddr := flagSet.String("health-addr", "", "The address the healthz endpoint binds to.") - webhookPort := flagSet.Int("webhook-port", 9443, "The port the webhook endpoint binds to.") - webhookCertDir := flagSet.String("webhook-cert-dir", "", "The directory the webhook server's certs are stored.") - - enableLeaderElection := flagSet.Bool("enable-leader-election", false, "Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.") +func InitFlags(flagSet *flag.FlagSet) *Flags { + result := &Flags{} - crdManagementMode := flagSet.String("crd-management", "auto", + // default here for 'MetricsAddr' is set to "0", which sets metrics to be disabled if 'metrics-addr' flag is omitted. + flagSet.StringVar(&result.MetricsAddr, "metrics-addr", "0", "The address the metric endpoint binds to.") + flagSet.BoolVar(&result.SecureMetrics, "secure-metrics", true, "Enable secure metrics. This secures the pprof and metrics endpoints via Kubernetes RBAC and HTTPS") + flagSet.BoolVar(&result.ProfilingMetrics, "profiling-metrics", false, "Enable pprof metrics, only enabled in conjunction with secure-metrics. This will enable serving pprof metrics endpoints") + flagSet.StringVar(&result.HealthAddr, "health-addr", "", "The address the healthz endpoint binds to.") + flagSet.IntVar(&result.WebhookPort, "webhook-port", 9443, "The port the webhook endpoint binds to.") + flagSet.StringVar(&result.WebhookCertDir, "webhook-cert-dir", "", "The directory the webhook server's certs are stored.") + flagSet.BoolVar(&result.EnableLeaderElection, "enable-leader-election", false, "Enable leader election for controllers manager. Enabling this will ensure there is only one active controllers manager.") + + flagSet.StringVar(&result.CRDManagementMode, "crd-management", "auto", "Instructs the operator on how it should manage the Custom Resource Definitions. One of 'auto', 'none'") - crdPatterns := flagSet.String("crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.") - - return Flags{ - MetricsAddr: metricsAddr, - SecureMetrics: secureMetrics, - ProfilingMetrics: profilingMetrics, + flagSet.StringVar(&result.CRDPatterns, "crd-pattern", "", "Install these CRDs. CRDs already in the cluster will also always be upgraded.") - HealthAddr: healthAddr, - WebhookPort: webhookPort, - WebhookCertDir: webhookCertDir, - EnableLeaderElection: enableLeaderElection, - CRDManagementMode: crdManagementMode, - CRDPatterns: crdPatterns, - } + return result } diff --git a/v2/cmd/controller/app/setup.go b/v2/cmd/controller/app/setup.go index 09ef3d86064..d28129f9e8d 100644 --- a/v2/cmd/controller/app/setup.go +++ b/v2/cmd/controller/app/setup.go @@ -52,7 +52,7 @@ import ( "github.com/Azure/azure-service-operator/v2/pkg/genruntime/conditions" ) -func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flags) manager.Manager { +func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs *Flags) manager.Manager { scheme := controllers.CreateScheme() _ = apiextensions.AddToScheme(scheme) // Used for managing CRDs @@ -78,13 +78,13 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag mgr, err := ctrl.NewManager(k8sConfig, ctrl.Options{ Scheme: scheme, NewCache: cacheFunc, - LeaderElection: *flgs.EnableLeaderElection, + LeaderElection: flgs.EnableLeaderElection, LeaderElectionID: "controllers-leader-election-azinfra-generated", - HealthProbeBindAddress: *flgs.HealthAddr, + HealthProbeBindAddress: flgs.HealthAddr, Metrics: getMetricsOpts(flgs), WebhookServer: webhook.NewServer(webhook.Options{ - Port: *flgs.WebhookPort, - CertDir: *flgs.WebhookCertDir, + Port: flgs.WebhookPort, + CertDir: flgs.WebhookCertDir, }), }) if err != nil { @@ -110,7 +110,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag os.Exit(1) } - switch *flgs.CRDManagementMode { + switch flgs.CRDManagementMode { case "auto": var goalCRDs []apiextensions.CustomResourceDefinition goalCRDs, err = crdManager.LoadOperatorCRDs(crdmanagement.CRDLocation, cfg.PodNamespace) @@ -122,7 +122,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag // We only apply CRDs if we're in webhooks mode. No other mode will have CRD CRUD permissions if cfg.OperatorMode.IncludesWebhooks() { var installationInstructions []*crdmanagement.CRDInstallationInstruction - installationInstructions, err = crdManager.DetermineCRDsToInstallOrUpgrade(goalCRDs, existingCRDs, *flgs.CRDPatterns) + installationInstructions, err = crdManager.DetermineCRDsToInstallOrUpgrade(goalCRDs, existingCRDs, flgs.CRDPatterns) if err != nil { setupLog.Error(err, "failed to determine CRDs to apply") os.Exit(1) @@ -145,7 +145,7 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag case "none": setupLog.Info("CRD management mode was set to 'none', the operator will not manage CRDs and assumes they are already installed and matching the operator version") default: - setupLog.Error(fmt.Errorf("invalid CRD management mode: %s", *flgs.CRDManagementMode), "failed to initialize CRD client") + setupLog.Error(fmt.Errorf("invalid CRD management mode: %s", flgs.CRDManagementMode), "failed to initialize CRD client") os.Exit(1) } @@ -211,17 +211,17 @@ func SetupControllerManager(ctx context.Context, setupLog logr.Logger, flgs Flag return mgr } -func getMetricsOpts(flags Flags) server.Options { +func getMetricsOpts(flags *Flags) server.Options { var metricsOptions server.Options - if *flags.SecureMetrics { + if flags.SecureMetrics { metricsOptions = server.Options{ - BindAddress: *flags.MetricsAddr, + BindAddress: flags.MetricsAddr, SecureServing: true, FilterProvider: filters.WithAuthenticationAndAuthorization, } // Note that pprof endpoints are meant to be sensitive and shouldn't be exposed publicly. - if *flags.ProfilingMetrics { + if flags.ProfilingMetrics { metricsOptions.ExtraHandlers = map[string]http.Handler{ "/debug/pprof/": http.HandlerFunc(pprof.Index), "/debug/pprof/cmdline": http.HandlerFunc(pprof.Cmdline), @@ -232,7 +232,7 @@ func getMetricsOpts(flags Flags) server.Options { } } else { metricsOptions = server.Options{ - BindAddress: *flags.MetricsAddr, + BindAddress: flags.MetricsAddr, } } diff --git a/v2/cmd/controller/logging/logging.go b/v2/cmd/controller/logging/logging.go index e58bd51e8fa..0d059ef0f6b 100644 --- a/v2/cmd/controller/logging/logging.go +++ b/v2/cmd/controller/logging/logging.go @@ -15,53 +15,62 @@ import ( "k8s.io/klog/v2/textlogger" ) -var ( - // verbose indicates whether we should use verbose logging. - // Default is no - verbose *bool +type Config struct { + // verbose indicates the level of logging. + // Higher values indicate more logging. + verbose int - // useJson indicates whether we should output logs in JSON format. + // useJSON indicates whether we should output logs in JSON format. // Default is no - useJson *bool -) + useJSON bool +} // Create returns a new logger, ready for use. // This can be called multiple times if required. -func Create() logr.Logger { - if useJson != nil && *useJson { - log, err := createJSONLogger() +func Create(cfg *Config) logr.Logger { + if cfg != nil && cfg.useJSON { + log, err := createJSONLogger(cfg) if err != nil { - log = createTextLogger() + log = createTextLogger(cfg) log.Error(err, "failed to create JSON logger, falling back to text") } return log } - return createTextLogger() + return createTextLogger(cfg) } -func createTextLogger() logr.Logger { +func createTextLogger(cfg *Config) logr.Logger { opts := []textlogger.ConfigOption{} - if verbose != nil && *verbose { - opts = append(opts, textlogger.Verbosity(3)) + if cfg != nil { + opts = append(opts, textlogger.Verbosity(cfg.verbose)) } - cfg := textlogger.NewConfig(opts...) - return textlogger.NewLogger(cfg) + c := textlogger.NewConfig(opts...) + return textlogger.NewLogger(c) } -func createJSONLogger() (logr.Logger, error) { - level := zap.NewAtomicLevelAt(zap.InfoLevel) - if verbose != nil && *verbose { - level = zap.NewAtomicLevelAt(zap.DebugLevel) +func createJSONLogger(cfg *Config) (logr.Logger, error) { + level := zap.InfoLevel + if cfg != nil { + switch cfg.verbose { + case 0: + level = zap.ErrorLevel + case 1: + level = zap.WarnLevel + case 2: + level = zap.InfoLevel + case 3: + level = zap.DebugLevel + } } encoder := zap.NewProductionEncoderConfig() encoder.EncodeTime = zapcore.ISO8601TimeEncoder - cfg := zap.Config{ - Level: level, + c := zap.Config{ + Level: zap.NewAtomicLevelAt(level), Development: false, Encoding: "json", EncoderConfig: encoder, @@ -69,7 +78,7 @@ func createJSONLogger() (logr.Logger, error) { ErrorOutputPaths: []string{"stderr"}, } - logger, err := cfg.Build() + logger, err := c.Build() if err != nil { return logr.Logger{}, err } @@ -78,9 +87,13 @@ func createJSONLogger() (logr.Logger, error) { } // InitFlags initializes the flags for the logging package -func InitFlags(fs *flag.FlagSet) { - verbose = fs.Bool("verbose", false, "Enable verbose logging") - fs.BoolVar(verbose, "v", false, "Enable verbose logging") +func InitFlags(fs *flag.FlagSet) *Config { + result := &Config{} + + fs.IntVar(&result.verbose, "verbose", 2, "Enable verbose logging") + fs.IntVar(&result.verbose, "v", 2, "Enable verbose logging") + + fs.BoolVar(&result.useJSON, "json-logging", false, "Enable JSON logging") - useJson = fs.Bool("json-logging", false, "Enable JSON logging") + return result } diff --git a/v2/cmd/controller/main.go b/v2/cmd/controller/main.go index 1d267fdbaf6..7cb6b042e1e 100644 --- a/v2/cmd/controller/main.go +++ b/v2/cmd/controller/main.go @@ -23,13 +23,13 @@ func main() { flagSet := flag.NewFlagSet(exeName, flag.ExitOnError) // Create a temporary logger for while we get set up - log := logging.Create() + log := logging.Create(&logging.Config{}) ctx := ctrl.SetupSignalHandler() // Add application and logging flags - flgs := app.InitFlags(flagSet) - logging.InitFlags(flagSet) + appFlags := app.InitFlags(flagSet) + logFlags := logging.InitFlags(flagSet) err := flagSet.Parse(os.Args[1:]) if err != nil { log.Error(err, "failed to parse cmdline flags") @@ -37,11 +37,11 @@ func main() { } // Replace the logger with a configured one - log = logging.Create() + log = logging.Create(logFlags) ctrl.SetLogger(log) - log.Info("Launching with flags", "flags", flgs.String()) + log.Info("Launching with flags", "flags", appFlags.String()) - mgr := app.SetupControllerManager(ctx, log, flgs) + mgr := app.SetupControllerManager(ctx, log, appFlags) log.Info("starting manager") if err = mgr.Start(ctx); err != nil { log.Error(err, "failed to start manager") diff --git a/v2/internal/controllers/suite_test.go b/v2/internal/controllers/suite_test.go index 261f054117c..6d20e5f0f0c 100644 --- a/v2/internal/controllers/suite_test.go +++ b/v2/internal/controllers/suite_test.go @@ -44,7 +44,7 @@ func setup() error { // set a global logger for controller-runtime: // // import (ctrl "sigs.k8s.io/controller-runtime") - // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests // log := textlogger.NewLogger(cfg) // ctrl.SetLogger(log) diff --git a/v2/internal/genericarmclient/suite_test.go b/v2/internal/genericarmclient/suite_test.go index 2b2e7ec28a1..56c864f5db0 100644 --- a/v2/internal/genericarmclient/suite_test.go +++ b/v2/internal/genericarmclient/suite_test.go @@ -17,6 +17,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" + . "github.com/Azure/azure-service-operator/v2/internal/logging" "github.com/Azure/azure-service-operator/v2/internal/testcommon" ) @@ -32,7 +33,7 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) diff --git a/v2/internal/testcommon/kube_test_context_envtest.go b/v2/internal/testcommon/kube_test_context_envtest.go index ac5dbf09574..2449b73d078 100644 --- a/v2/internal/testcommon/kube_test_context_envtest.go +++ b/v2/internal/testcommon/kube_test_context_envtest.go @@ -91,7 +91,7 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources) // Switch logger below if we want controller-runtime logs in the tests. // By default we've disabled controller runtime logs because they're very verbose and usually not useful. // import (ctrl "sigs.k8s.io/controller-runtime") - // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests // log := textlogger.NewLogger(cfg) // ctrl.SetLogger(log) ctrl.SetLogger(logr.Discard()) @@ -164,7 +164,7 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources) // By default we've disabled controller runtime logs because they're very verbose and usually not useful. // // import (ctrl "sigs.k8s.io/controller-runtime") - // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests // log := textlogger.NewLogger(cfg) // ctrl.SetLogger(log) ctrl.SetLogger(logr.Discard()) diff --git a/v2/pkg/genruntime/test/suite_test.go b/v2/pkg/genruntime/test/suite_test.go index ec1180ccc1c..3d36ec9bf6d 100644 --- a/v2/pkg/genruntime/test/suite_test.go +++ b/v2/pkg/genruntime/test/suite_test.go @@ -43,7 +43,7 @@ func setup() error { // set a global logger for controller-runtime: // // import (ctrl "sigs.k8s.io/controller-runtime") - // cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + // cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests // log := textlogger.NewLogger(cfg) // ctrl.SetLogger(log) diff --git a/v2/test/multitenant/suite_test.go b/v2/test/multitenant/suite_test.go index 924a6d9ff96..ad1456afbfb 100644 --- a/v2/test/multitenant/suite_test.go +++ b/v2/test/multitenant/suite_test.go @@ -16,6 +16,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" + . "github.com/Azure/azure-service-operator/v2/internal/logging" "github.com/Azure/azure-service-operator/v2/internal/testcommon" ) @@ -35,7 +36,7 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) diff --git a/v2/test/pre-release/suite_test.go b/v2/test/pre-release/suite_test.go index 924a6d9ff96..ad1456afbfb 100644 --- a/v2/test/pre-release/suite_test.go +++ b/v2/test/pre-release/suite_test.go @@ -16,6 +16,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" + . "github.com/Azure/azure-service-operator/v2/internal/logging" "github.com/Azure/azure-service-operator/v2/internal/testcommon" ) @@ -35,7 +36,7 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) diff --git a/v2/test/suite_test.go b/v2/test/suite_test.go index 9e35f075c13..40666718461 100644 --- a/v2/test/suite_test.go +++ b/v2/test/suite_test.go @@ -16,6 +16,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" + . "github.com/Azure/azure-service-operator/v2/internal/logging" "github.com/Azure/azure-service-operator/v2/internal/testcommon" ) @@ -35,7 +36,7 @@ func setup() error { format.TruncateThreshold = 4000 // Force a longer truncate threshold // setup global logger for controller-runtime: - cfg := textlogger.NewConfig(textlogger.Verbosity(3)) // Use verbose logging in tests + cfg := textlogger.NewConfig(textlogger.Verbosity(Debug)) // Use verbose logging in tests log := textlogger.NewLogger(cfg) ctrl.SetLogger(log) From b0aae16b9ac7a2c3c6d66eeb3339b9d13e0272ee Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 2 Sep 2024 11:03:49 +1200 Subject: [PATCH 9/9] Address PR feedback --- v2/cmd/controller/logging/logging.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/v2/cmd/controller/logging/logging.go b/v2/cmd/controller/logging/logging.go index 0d059ef0f6b..4d77168960b 100644 --- a/v2/cmd/controller/logging/logging.go +++ b/v2/cmd/controller/logging/logging.go @@ -13,12 +13,14 @@ import ( "go.uber.org/zap" "go.uber.org/zap/zapcore" "k8s.io/klog/v2/textlogger" + + "github.com/Azure/azure-service-operator/v2/internal/logging" ) type Config struct { - // verbose indicates the level of logging. + // verbosity indicates the level of logging. // Higher values indicate more logging. - verbose int + verbosity int // useJSON indicates whether we should output logs in JSON format. // Default is no @@ -44,7 +46,7 @@ func Create(cfg *Config) logr.Logger { func createTextLogger(cfg *Config) logr.Logger { opts := []textlogger.ConfigOption{} if cfg != nil { - opts = append(opts, textlogger.Verbosity(cfg.verbose)) + opts = append(opts, textlogger.Verbosity(cfg.verbosity)) } c := textlogger.NewConfig(opts...) @@ -54,14 +56,14 @@ func createTextLogger(cfg *Config) logr.Logger { func createJSONLogger(cfg *Config) (logr.Logger, error) { level := zap.InfoLevel if cfg != nil { - switch cfg.verbose { + switch cfg.verbosity { case 0: level = zap.ErrorLevel case 1: level = zap.WarnLevel case 2: level = zap.InfoLevel - case 3: + default: // 3 or above level = zap.DebugLevel } } @@ -90,8 +92,8 @@ func createJSONLogger(cfg *Config) (logr.Logger, error) { func InitFlags(fs *flag.FlagSet) *Config { result := &Config{} - fs.IntVar(&result.verbose, "verbose", 2, "Enable verbose logging") - fs.IntVar(&result.verbose, "v", 2, "Enable verbose logging") + fs.IntVar(&result.verbosity, "verbose", logging.Verbose, "Enable verbose logging") + fs.IntVar(&result.verbosity, "v", logging.Verbose, "Enable verbose logging") fs.BoolVar(&result.useJSON, "json-logging", false, "Enable JSON logging")