From c0d5dad674591a8ebaa97efbe74f75861638a250 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 24 Oct 2024 14:30:42 +0300 Subject: [PATCH 1/2] Move getting a logger from config to the config package Signed-off-by: Radoslav Dimitrov --- cmd/dev/app/rule_type/rttst.go | 3 +- cmd/dev/app/testserver/testserver.go | 3 +- cmd/reminder/app/start.go | 3 +- cmd/server/app/encryption_purge_sessions.go | 3 +- cmd/server/app/encryption_rotate.go | 3 +- cmd/server/app/history_purge.go | 3 +- cmd/server/app/migrate_down.go | 3 +- cmd/server/app/migrate_up.go | 3 +- cmd/server/app/migrate_version.go | 3 +- cmd/server/app/serve.go | 2 +- cmd/server/app/webhook_update.go | 3 +- internal/logger/logger_setup.go | 60 --------------------- internal/reminder/logger/logger_setup.go | 20 ------- pkg/config/reminder/logging.go | 12 +++++ pkg/config/server/logging.go | 58 +++++++++++++++++++- 15 files changed, 80 insertions(+), 102 deletions(-) delete mode 100644 internal/logger/logger_setup.go delete mode 100644 internal/reminder/logger/logger_setup.go diff --git a/cmd/dev/app/rule_type/rttst.go b/cmd/dev/app/rule_type/rttst.go index 44ddf1e946..51cf4cf815 100644 --- a/cmd/dev/app/rule_type/rttst.go +++ b/cmd/dev/app/rule_type/rttst.go @@ -25,7 +25,6 @@ import ( engif "github.com/mindersec/minder/internal/engine/interfaces" entModels "github.com/mindersec/minder/internal/entities/models" entProps "github.com/mindersec/minder/internal/entities/properties" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/internal/profiles" "github.com/mindersec/minder/internal/profiles/models" "github.com/mindersec/minder/internal/providers/credentials" @@ -251,7 +250,7 @@ func runEvaluationForRules( // Enable logging for the engine ctx := context.Background() logConfig := serverconfig.LoggingConfig{Level: cmd.Flag("log-level").Value.String()} - ctx = logger.FromFlags(logConfig).WithContext(ctx) + ctx = serverconfig.LoggerFromConfigFlags(logConfig).WithContext(ctx) // convert to EntityInfoWrapper as that's what the engine operates on inf, err := entityWithPropertiesToEntityInfoWrapper(ewp, prov) diff --git a/cmd/dev/app/testserver/testserver.go b/cmd/dev/app/testserver/testserver.go index 368087aae6..3ce8d903be 100644 --- a/cmd/dev/app/testserver/testserver.go +++ b/cmd/dev/app/testserver/testserver.go @@ -23,7 +23,6 @@ import ( mockauthz "github.com/mindersec/minder/internal/authz/mock" "github.com/mindersec/minder/internal/controlplane/metrics" "github.com/mindersec/minder/internal/db/embedded" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/internal/metrics/meters" "github.com/mindersec/minder/internal/providers/ratecache" provtelemetry "github.com/mindersec/minder/internal/providers/telemetry" @@ -54,7 +53,7 @@ func runTestServer(cmd *cobra.Command, _ []string) error { ctx, cancel := signal.NotifyContext(cmd.Context(), os.Interrupt) defer cancel() - ctx = logger.FromFlags(cfg.LoggingConfig).WithContext(ctx) + ctx = serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(ctx) l := zerolog.Ctx(ctx) l.Info().Msgf("Initializing logger in level: %s", cfg.LoggingConfig.Level) diff --git a/cmd/reminder/app/start.go b/cmd/reminder/app/start.go index 37da7b91d8..3ba5ba6250 100644 --- a/cmd/reminder/app/start.go +++ b/cmd/reminder/app/start.go @@ -16,7 +16,6 @@ import ( "github.com/mindersec/minder/internal/db" "github.com/mindersec/minder/internal/reminder" - "github.com/mindersec/minder/internal/reminder/logger" "github.com/mindersec/minder/pkg/config" reminderconfig "github.com/mindersec/minder/pkg/config/reminder" ) @@ -42,7 +41,7 @@ func start(cmd *cobra.Command, _ []string) error { return fmt.Errorf("error validating config: %w", err) } - ctx = logger.FromFlags(cfg.LoggingConfig).WithContext(ctx) + ctx = reminderconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(ctx) dbConn, _, err := cfg.Database.GetDBConnection(ctx) if err != nil { diff --git a/cmd/server/app/encryption_purge_sessions.go b/cmd/server/app/encryption_purge_sessions.go index 659b49b7b9..e0083d0b2b 100644 --- a/cmd/server/app/encryption_purge_sessions.go +++ b/cmd/server/app/encryption_purge_sessions.go @@ -15,7 +15,6 @@ import ( "github.com/spf13/viper" "github.com/mindersec/minder/internal/db" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -31,7 +30,7 @@ var purgeCmd = &cobra.Command{ cliErrorf(cmd, "unable to read config: %s", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) // instantiate `db.Store` so we can run queries store, closer, err := wireUpDB(ctx, cfg) diff --git a/cmd/server/app/encryption_rotate.go b/cmd/server/app/encryption_rotate.go index 65a737d4c1..7323979c0e 100644 --- a/cmd/server/app/encryption_rotate.go +++ b/cmd/server/app/encryption_rotate.go @@ -17,7 +17,6 @@ import ( "github.com/mindersec/minder/internal/crypto" "github.com/mindersec/minder/internal/db" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -44,7 +43,7 @@ var rotateCmd = &cobra.Command{ cliErrorf(cmd, "default key ID not defined in crypto config - exiting") } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) zerolog.Ctx(ctx).Debug(). Str("default_key_id", cfg.Crypto.Default.KeyID). diff --git a/cmd/server/app/history_purge.go b/cmd/server/app/history_purge.go index e18312818a..8e13d10091 100644 --- a/cmd/server/app/history_purge.go +++ b/cmd/server/app/history_purge.go @@ -15,7 +15,6 @@ import ( "github.com/spf13/viper" "github.com/mindersec/minder/internal/db" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -37,7 +36,7 @@ func historyPurgeCommand(cmd *cobra.Command, _ []string) error { cliErrorf(cmd, "unable to read config: %s", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) // instantiate `db.Store` so we can run queries store, closer, err := wireUpDB(ctx, cfg) diff --git a/cmd/server/app/migrate_down.go b/cmd/server/app/migrate_down.go index 0ee26592fa..a130ea72a3 100644 --- a/cmd/server/app/migrate_down.go +++ b/cmd/server/app/migrate_down.go @@ -13,7 +13,6 @@ import ( "github.com/spf13/viper" "github.com/mindersec/minder/database" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -28,7 +27,7 @@ var downCmd = &cobra.Command{ return fmt.Errorf("unable to read config: %w", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) // Database configuration dbConn, connString, err := cfg.Database.GetDBConnection(ctx) diff --git a/cmd/server/app/migrate_up.go b/cmd/server/app/migrate_up.go index a056e80e3a..ef813cd9ed 100644 --- a/cmd/server/app/migrate_up.go +++ b/cmd/server/app/migrate_up.go @@ -18,7 +18,6 @@ import ( "github.com/mindersec/minder/database" "github.com/mindersec/minder/internal/authz" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -34,7 +33,7 @@ var upCmd = &cobra.Command{ return fmt.Errorf("unable to read config: %w", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) // Database configuration dbConn, connString, err := cfg.Database.GetDBConnection(ctx) diff --git a/cmd/server/app/migrate_version.go b/cmd/server/app/migrate_version.go index bd4cd1e610..87e5e33688 100644 --- a/cmd/server/app/migrate_version.go +++ b/cmd/server/app/migrate_version.go @@ -15,7 +15,6 @@ import ( "github.com/spf13/viper" "github.com/mindersec/minder/database" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/pkg/config" serverconfig "github.com/mindersec/minder/pkg/config/server" ) @@ -31,7 +30,7 @@ var versionCmd = &cobra.Command{ return fmt.Errorf("unable to read config: %w", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) // Database configuration dbConn, connString, err := cfg.Database.GetDBConnection(ctx) diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index c7f62596f6..03245d25e7 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -47,7 +47,7 @@ var serveCmd = &cobra.Command{ os.Exit(0) } - ctx = logger.FromFlags(cfg.LoggingConfig).WithContext(ctx) + ctx = serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(ctx) l := zerolog.Ctx(ctx) l.Info().Msgf("Initializing logger in level: %s", cfg.LoggingConfig.Level) diff --git a/cmd/server/app/webhook_update.go b/cmd/server/app/webhook_update.go index f549e532d9..27cbf1f07c 100644 --- a/cmd/server/app/webhook_update.go +++ b/cmd/server/app/webhook_update.go @@ -20,7 +20,6 @@ import ( "github.com/mindersec/minder/internal/crypto" "github.com/mindersec/minder/internal/db" propssvc "github.com/mindersec/minder/internal/entities/properties/service" - "github.com/mindersec/minder/internal/logger" "github.com/mindersec/minder/internal/providers" ghprovider "github.com/mindersec/minder/internal/providers/github" "github.com/mindersec/minder/internal/providers/github/clients" @@ -60,7 +59,7 @@ func runCmdWebhookUpdate(cmd *cobra.Command, _ []string) error { return fmt.Errorf("unable to read config: %w", err) } - ctx := logger.FromFlags(cfg.LoggingConfig).WithContext(context.Background()) + ctx := serverconfig.LoggerFromConfigFlags(cfg.LoggingConfig).WithContext(context.Background()) providerName := cmd.Flag("provider").Value.String() diff --git a/internal/logger/logger_setup.go b/internal/logger/logger_setup.go deleted file mode 100644 index 3678de6f23..0000000000 --- a/internal/logger/logger_setup.go +++ /dev/null @@ -1,60 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors -// SPDX-License-Identifier: Apache-2.0 - -package logger - -import ( - "io" - "os" - "path/filepath" - - "github.com/rs/zerolog" - "github.com/rs/zerolog/log" - - "github.com/mindersec/minder/internal/util" - config "github.com/mindersec/minder/pkg/config/server" -) - -// FromFlags configures logging and returns a logger with settings matching -// the supplied cfg. It also performs some global initialization, because -// that's how zerolog works. -func FromFlags(cfg config.LoggingConfig) zerolog.Logger { - zlevel := util.ViperLogLevelToZerologLevel(cfg.Level) - zerolog.SetGlobalLevel(zlevel) - - loggers := []io.Writer{} - - // Conform to https://github.com/open-telemetry/oteps/blob/main/text/logs/0097-log-data-model.md#example-log-records - // Unfortunately, these can't be set on a per-logger basis except by ConsoleWriter - zerolog.ErrorFieldName = "exception.message" - zerolog.TimestampFieldName = "Timestamp" - zerolog.TimeFieldFormat = zerolog.TimeFormatUnixNano - - if cfg.LogFile != "" { - cfg.LogFile = filepath.Clean(cfg.LogFile) - file, err := os.OpenFile(cfg.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) - // NOTE: we are leaking the open file here - if err != nil { - log.Err(err).Msg("Failed to open log file, defaulting to stdout") - } else { - loggers = append(loggers, file) - } - } - - if cfg.Format == Text { - loggers = append(loggers, zerolog.NewConsoleWriter()) - } else { - loggers = append(loggers, os.Stdout) - } - - logger := zerolog.New(zerolog.MultiLevelWriter(loggers...)).With(). - Caller(). - Timestamp(). - Logger() - - // Use this logger when calling zerolog.Ctx(nil), etc - zerolog.DefaultContextLogger = &logger - log.Logger = logger - - return logger -} diff --git a/internal/reminder/logger/logger_setup.go b/internal/reminder/logger/logger_setup.go deleted file mode 100644 index 95fd34d9c0..0000000000 --- a/internal/reminder/logger/logger_setup.go +++ /dev/null @@ -1,20 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors -// SPDX-License-Identifier: Apache-2.0 - -// Package logger provides the configuration for the reminder logger -package logger - -import ( - "os" - - "github.com/rs/zerolog" - - "github.com/mindersec/minder/internal/util" - config "github.com/mindersec/minder/pkg/config/reminder" -) - -// FromFlags creates a new logger from the provided configuration -func FromFlags(cfg config.LoggingConfig) zerolog.Logger { - level := util.ViperLogLevelToZerologLevel(cfg.Level) - return zerolog.New(os.Stdout).Level(level).With().Timestamp().Logger() -} diff --git a/pkg/config/reminder/logging.go b/pkg/config/reminder/logging.go index 4b36503145..47e9d0c3b4 100644 --- a/pkg/config/reminder/logging.go +++ b/pkg/config/reminder/logging.go @@ -3,7 +3,19 @@ package reminder +import ( + "github.com/mindersec/minder/internal/util" + "github.com/rs/zerolog" + "os" +) + // LoggingConfig is the configuration for the logger type LoggingConfig struct { Level string `mapstructure:"level" default:"info"` } + +// LoggerFromConfigFlags creates a new logger from the provided configuration +func LoggerFromConfigFlags(cfg LoggingConfig) zerolog.Logger { + level := util.ViperLogLevelToZerologLevel(cfg.Level) + return zerolog.New(os.Stdout).Level(level).With().Timestamp().Logger() +} diff --git a/pkg/config/server/logging.go b/pkg/config/server/logging.go index 4949e93264..4509a262d2 100644 --- a/pkg/config/server/logging.go +++ b/pkg/config/server/logging.go @@ -3,14 +3,70 @@ package server +import ( + "github.com/mindersec/minder/internal/util" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" + "io" + "os" + "path/filepath" +) + +// Text is the constant for the text format +const Text = "text" + // LoggingConfig is the configuration for the logging package type LoggingConfig struct { Level string `mapstructure:"level" default:"debug"` Format string `mapstructure:"format" default:"json"` LogFile string `mapstructure:"logFile" default:""` - // LogPayloads controls whether or not message payloads are ever logged. + // LogPayloads controls whether message payloads are ever logged. // For debugging purposes, it may be useful to log the payloads that result // in error conditions, but could also leak PII. LogPayloads bool `mapstructure:"logPayloads" default:"false"` } + +// LoggerFromConfigFlags configures logging and returns a logger with settings matching +// the supplied cfg. It also performs some global initialization, because +// that's how zerolog works. +func LoggerFromConfigFlags(cfg LoggingConfig) zerolog.Logger { + zlevel := util.ViperLogLevelToZerologLevel(cfg.Level) + zerolog.SetGlobalLevel(zlevel) + + loggers := []io.Writer{} + + // Conform to https://github.com/open-telemetry/oteps/blob/main/text/logs/0097-log-data-model.md#example-log-records + // Unfortunately, these can't be set on a per-logger basis except by ConsoleWriter + zerolog.ErrorFieldName = "exception.message" + zerolog.TimestampFieldName = "Timestamp" + zerolog.TimeFieldFormat = zerolog.TimeFormatUnixNano + + if cfg.LogFile != "" { + cfg.LogFile = filepath.Clean(cfg.LogFile) + file, err := os.OpenFile(cfg.LogFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) + // NOTE: we are leaking the open file here + if err != nil { + log.Err(err).Msg("Failed to open log file, defaulting to stdout") + } else { + loggers = append(loggers, file) + } + } + + if cfg.Format == Text { + loggers = append(loggers, zerolog.NewConsoleWriter()) + } else { + loggers = append(loggers, os.Stdout) + } + + logger := zerolog.New(zerolog.MultiLevelWriter(loggers...)).With(). + Caller(). + Timestamp(). + Logger() + + // Use this logger when calling zerolog.Ctx(nil), etc + zerolog.DefaultContextLogger = &logger + log.Logger = logger + + return logger +} From 029bdbf22abc0bcd9cdade905228abe98d054fa2 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 24 Oct 2024 14:45:51 +0300 Subject: [PATCH 2/2] Fix linting errors Signed-off-by: Radoslav Dimitrov --- pkg/config/reminder/logging.go | 6 ++++-- pkg/config/server/logging.go | 8 +++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/config/reminder/logging.go b/pkg/config/reminder/logging.go index 47e9d0c3b4..2ffc4e5c28 100644 --- a/pkg/config/reminder/logging.go +++ b/pkg/config/reminder/logging.go @@ -4,9 +4,11 @@ package reminder import ( - "github.com/mindersec/minder/internal/util" - "github.com/rs/zerolog" "os" + + "github.com/rs/zerolog" + + "github.com/mindersec/minder/internal/util" ) // LoggingConfig is the configuration for the logger diff --git a/pkg/config/server/logging.go b/pkg/config/server/logging.go index 4509a262d2..2a26796d99 100644 --- a/pkg/config/server/logging.go +++ b/pkg/config/server/logging.go @@ -4,12 +4,14 @@ package server import ( - "github.com/mindersec/minder/internal/util" - "github.com/rs/zerolog" - "github.com/rs/zerolog/log" "io" "os" "path/filepath" + + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" + + "github.com/mindersec/minder/internal/util" ) // Text is the constant for the text format