From ce35b7dbc3fac7bd5b7191c6fdf571e7bc20e76a Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Thu, 7 Oct 2021 15:12:45 +0200 Subject: [PATCH] component-base/logs: refactor registry Moving the registry into its own package will avoid dependency cycles in future PRs. Creating loggers through a factory instead of storing pre-created instances will make it possible to create the loggers differently depending on configuration parameters. The factory can also be used to provide additional meta data before creating instances. --- .../src/k8s.io/component-base/logs/config.go | 10 +++---- .../k8s.io/component-base/logs/json/json.go | 18 ++++++++----- .../logs/json/register/register.go | 3 ++- .../src/k8s.io/component-base/logs/options.go | 7 ++--- .../logs/{ => registry}/registry.go | 26 +++++++++++++------ .../k8s.io/component-base/logs/validate.go | 3 ++- vendor/modules.txt | 1 + 7 files changed, 42 insertions(+), 26 deletions(-) rename staging/src/k8s.io/component-base/logs/{ => registry}/registry.go (75%) diff --git a/staging/src/k8s.io/component-base/logs/config.go b/staging/src/k8s.io/component-base/logs/config.go index 3bec829a238..c24bf19db59 100644 --- a/staging/src/k8s.io/component-base/logs/config.go +++ b/staging/src/k8s.io/component-base/logs/config.go @@ -26,6 +26,7 @@ import ( cliflag "k8s.io/component-base/cli/flag" "k8s.io/component-base/config" + "k8s.io/component-base/logs/registry" "k8s.io/klog/v2" ) @@ -35,16 +36,13 @@ const ( JSONLogFormat = "json" ) -// LogRegistry is new init LogFormatRegistry struct -var LogRegistry = NewLogFormatRegistry() - // loggingFlags captures the state of the logging flags, in particular their default value // before flag parsing. It is used by UnsupportedLoggingFlags. var loggingFlags pflag.FlagSet func init() { // Text format is default klog format - LogRegistry.Register(DefaultLogFormat, nil) + registry.LogRegistry.Register(DefaultLogFormat, nil) var fs flag.FlagSet klog.InitFlags(&fs) @@ -63,10 +61,10 @@ func BindLoggingFlags(c *config.LoggingConfiguration, fs *pflag.FlagSet) { // hyphens, even if currently no normalization function is set for the // flag set yet. unsupportedFlags := strings.Join(unsupportedLoggingFlagNames(cliflag.WordSepNormalizeFunc), ", ") - formats := fmt.Sprintf(`"%s"`, strings.Join(LogRegistry.List(), `", "`)) + formats := fmt.Sprintf(`"%s"`, strings.Join(registry.LogRegistry.List(), `", "`)) fs.StringVar(&c.Format, "logging-format", c.Format, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags)) // No new log formats should be added after generation is of flag options - LogRegistry.Freeze() + registry.LogRegistry.Freeze() fs.BoolVar(&c.Sanitization, "experimental-logging-sanitization", c.Sanitization, `[Experimental] When enabled prevents logging of fields tagged as sensitive (passwords, keys, tokens). Runtime log sanitization may introduce significant computation overhead and therefore should not be enabled in production.`) } diff --git a/staging/src/k8s.io/component-base/logs/json/json.go b/staging/src/k8s.io/component-base/logs/json/json.go index 7351c30c32c..03e29f66111 100644 --- a/staging/src/k8s.io/component-base/logs/json/json.go +++ b/staging/src/k8s.io/component-base/logs/json/json.go @@ -24,20 +24,15 @@ import ( "github.com/go-logr/zapr" "go.uber.org/zap" "go.uber.org/zap/zapcore" + + "k8s.io/component-base/logs/registry" ) var ( - // JSONLogger is global json log format logr - JSONLogger logr.Logger - // timeNow stubbed out for testing timeNow = time.Now ) -func init() { - JSONLogger = NewJSONLogger(zapcore.Lock(os.Stdout)) -} - // NewJSONLogger creates a new json logr.Logger using the given Zap Logger to log. func NewJSONLogger(w zapcore.WriteSyncer) logr.Logger { encoder := zapcore.NewJSONEncoder(encoderConfig) @@ -63,3 +58,12 @@ func epochMillisTimeEncoder(_ time.Time, enc zapcore.PrimitiveArrayEncoder) { millis := float64(nanos) / float64(time.Millisecond) enc.AppendFloat64(millis) } + +// Factory produces JSON logger instances. +type Factory struct{} + +func (f Factory) Create() logr.Logger { + return NewJSONLogger(zapcore.Lock(os.Stdout)) +} + +var _ registry.LogFormatFactory = Factory{} diff --git a/staging/src/k8s.io/component-base/logs/json/register/register.go b/staging/src/k8s.io/component-base/logs/json/register/register.go index 4411829b1bf..dea55ec1e25 100644 --- a/staging/src/k8s.io/component-base/logs/json/register/register.go +++ b/staging/src/k8s.io/component-base/logs/json/register/register.go @@ -19,9 +19,10 @@ package register import ( "k8s.io/component-base/logs" json "k8s.io/component-base/logs/json" + "k8s.io/component-base/logs/registry" ) func init() { // JSON format is optional klog format - logs.LogRegistry.Register(logs.JSONLogFormat, &json.JSONLogger) + registry.LogRegistry.Register(logs.JSONLogFormat, json.Factory{}) } diff --git a/staging/src/k8s.io/component-base/logs/options.go b/staging/src/k8s.io/component-base/logs/options.go index af3a4e17874..7f23207fd10 100644 --- a/staging/src/k8s.io/component-base/logs/options.go +++ b/staging/src/k8s.io/component-base/logs/options.go @@ -23,6 +23,7 @@ import ( "k8s.io/component-base/config" "k8s.io/component-base/config/v1alpha1" + "k8s.io/component-base/logs/registry" "k8s.io/component-base/logs/sanitization" ) @@ -58,11 +59,11 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) { // Apply set klog logger from LogFormat type func (o *Options) Apply() { // if log format not exists, use nil loggr - loggr, _ := LogRegistry.Get(o.Config.Format) - if loggr == nil { + factory, _ := registry.LogRegistry.Get(o.Config.Format) + if factory == nil { klog.ClearLogger() } else { - klog.SetLogger(*loggr) + klog.SetLogger(factory.Create()) } if o.Config.Sanitization { klog.SetLogFilter(&sanitization.SanitizingFilter{}) diff --git a/staging/src/k8s.io/component-base/logs/registry.go b/staging/src/k8s.io/component-base/logs/registry/registry.go similarity index 75% rename from staging/src/k8s.io/component-base/logs/registry.go rename to staging/src/k8s.io/component-base/logs/registry/registry.go index 223674ec77f..09213736efc 100644 --- a/staging/src/k8s.io/component-base/logs/registry.go +++ b/staging/src/k8s.io/component-base/logs/registry/registry.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package logs +package registry import ( "fmt" @@ -23,35 +23,45 @@ import ( "github.com/go-logr/logr" ) +// LogRegistry is new init LogFormatRegistry struct +var LogRegistry = NewLogFormatRegistry() + // LogFormatRegistry store klog format registry type LogFormatRegistry struct { - registry map[string]*logr.Logger + registry map[string]LogFormatFactory frozen bool } +// LogFormatFactory provides support for a certain additional, +// non-default log format. +type LogFormatFactory interface { + // Create returns a logger. + Create() logr.Logger +} + // NewLogFormatRegistry return new init LogFormatRegistry struct func NewLogFormatRegistry() *LogFormatRegistry { return &LogFormatRegistry{ - registry: make(map[string]*logr.Logger), + registry: make(map[string]LogFormatFactory), frozen: false, } } // Register new log format registry to global logRegistry. // nil is valid and selects the default klog output. -func (lfr *LogFormatRegistry) Register(name string, logger *logr.Logger) error { +func (lfr *LogFormatRegistry) Register(name string, factory LogFormatFactory) error { if lfr.frozen { return fmt.Errorf("log format is frozen, unable to register log format") } if _, ok := lfr.registry[name]; ok { return fmt.Errorf("log format: %s already exists", name) } - lfr.registry[name] = logger + lfr.registry[name] = factory return nil } // Get specified log format logger -func (lfr *LogFormatRegistry) Get(name string) (*logr.Logger, error) { +func (lfr *LogFormatRegistry) Get(name string) (LogFormatFactory, error) { re, ok := lfr.registry[name] if !ok { return nil, fmt.Errorf("log format: %s does not exists", name) @@ -60,12 +70,12 @@ func (lfr *LogFormatRegistry) Get(name string) (*logr.Logger, error) { } // Set specified log format logger -func (lfr *LogFormatRegistry) Set(name string, logger *logr.Logger) error { +func (lfr *LogFormatRegistry) Set(name string, factory LogFormatFactory) error { if lfr.frozen { return fmt.Errorf("log format is frozen, unable to set log format") } - lfr.registry[name] = logger + lfr.registry[name] = factory return nil } diff --git a/staging/src/k8s.io/component-base/logs/validate.go b/staging/src/k8s.io/component-base/logs/validate.go index f0ed8cde1ce..f123c819b7c 100644 --- a/staging/src/k8s.io/component-base/logs/validate.go +++ b/staging/src/k8s.io/component-base/logs/validate.go @@ -22,6 +22,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" cliflag "k8s.io/component-base/cli/flag" "k8s.io/component-base/config" + "k8s.io/component-base/logs/registry" ) func ValidateLoggingConfiguration(c *config.LoggingConfiguration, fldPath *field.Path) field.ErrorList { @@ -36,7 +37,7 @@ func ValidateLoggingConfiguration(c *config.LoggingConfiguration, fldPath *field } } } - if _, err := LogRegistry.Get(c.Format); err != nil { + if _, err := registry.LogRegistry.Get(c.Format); err != nil { errs = append(errs, field.Invalid(fldPath.Child("format"), c.Format, "Unsupported log format")) } return errs diff --git a/vendor/modules.txt b/vendor/modules.txt index a527c330f66..48619a31763 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1952,6 +1952,7 @@ k8s.io/component-base/logs/datapol k8s.io/component-base/logs/json k8s.io/component-base/logs/json/register k8s.io/component-base/logs/logreduction +k8s.io/component-base/logs/registry k8s.io/component-base/logs/sanitization k8s.io/component-base/logs/testinit k8s.io/component-base/metrics