diff --git a/go.mod b/go.mod index fabf9b8f66..3ebb0392c5 100644 --- a/go.mod +++ b/go.mod @@ -5,8 +5,8 @@ go 1.16 require ( github.com/evanphx/json-patch v4.11.0+incompatible github.com/fsnotify/fsnotify v1.4.9 - github.com/go-logr/logr v0.4.0 - github.com/go-logr/zapr v0.4.0 + github.com/go-logr/logr v1.2.0 + github.com/go-logr/zapr v1.2.0 github.com/imdario/mergo v0.3.12 // indirect github.com/onsi/ginkgo v1.16.4 github.com/onsi/gomega v1.15.0 @@ -23,6 +23,7 @@ require ( k8s.io/apimachinery v0.22.2 k8s.io/client-go v0.22.2 k8s.io/component-base v0.22.2 + k8s.io/klog/v2 v2.20.0 // indirect k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a sigs.k8s.io/yaml v1.2.0 ) diff --git a/go.sum b/go.sum index 293f15d330..3666ce269c 100644 --- a/go.sum +++ b/go.sum @@ -124,10 +124,12 @@ github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG1KdI/P7A= github.com/go-logr/logr v0.1.0/go.mod h1:ixOQHD9gLJUVQQ2ZOR7zLEifBX6tGkNJF4QyIY7sIas= github.com/go-logr/logr v0.2.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU= -github.com/go-logr/logr v0.4.0 h1:K7/B1jt6fIBQVd4Owv2MqGQClcgf0R266+7C/QjRcLc= github.com/go-logr/logr v0.4.0/go.mod h1:z6/tIYblkpsD+a4lm/fGIIU9mZ+XfAiaFtq7xTgseGU= -github.com/go-logr/zapr v0.4.0 h1:uc1uML3hRYL9/ZZPdgHS/n8Nzo+eaYL/Efxkkamf7OM= -github.com/go-logr/zapr v0.4.0/go.mod h1:tabnROwaDl0UNxkVeFRbY8bwB37GwRv0P8lg6aAiEnk= +github.com/go-logr/logr v1.0.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +github.com/go-logr/logr v1.2.0 h1:QK40JKJyMdUDz+h+xvCsru/bJhvG0UxvePV0ufL/AcE= +github.com/go-logr/logr v1.2.0/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +github.com/go-logr/zapr v1.2.0 h1:n4JnPI1T3Qq1SFEi/F8rwLrZERp2bso19PJZDB9dayk= +github.com/go-logr/zapr v1.2.0/go.mod h1:Qa4Bsj2Vb+FAVeAKsLD8RLQ+YRJB8YDmOAKxaBQf7Ro= github.com/go-openapi/jsonpointer v0.19.3/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonpointer v0.19.5/go.mod h1:Pl9vOtqEWErmShwVjC8pYs9cog34VGT37dQOVbmoatg= github.com/go-openapi/jsonreference v0.19.3/go.mod h1:rjx6GuL8TTa9VaixXglHmQmIL98+wF9xc8zWvFonSJ8= @@ -752,8 +754,9 @@ k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8 k8s.io/gengo v0.0.0-20201214224949-b6c5ce23f027/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E= k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y= -k8s.io/klog/v2 v2.9.0 h1:D7HV+n1V57XeZ0m6tdRkfknthUaM06VFbWldOFh8kzM= k8s.io/klog/v2 v2.9.0/go.mod h1:hy9LJ/NvuK+iVyP4Ehqva4HxZG/oXyIS3n3Jmire4Ec= +k8s.io/klog/v2 v2.20.0 h1:tlyxlSvd63k7axjhuchckaRJm+a92z5GSOrTOQY5sHw= +k8s.io/klog/v2 v2.20.0/go.mod h1:Gm8eSIfQN6457haJuPaMxZw4wyP5k+ykPFlrhQDvhvw= k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e h1:KLHHjkdQFomZy8+06csTWZ0m1343QqxZhR2LJ1OxCYM= k8s.io/kube-openapi v0.0.0-20210421082810-95288971da7e/go.mod h1:vHXdDvt9+2spS2Rx9ql3I8tycm3H9FDfdUoIuKCefvw= k8s.io/utils v0.0.0-20210819203725-bdf08cb9a70a h1:8dYfu/Fc9Gz2rNJKB9IQRGgQOh2clmRzNIPPY1xLY5g= diff --git a/pkg/builder/controller.go b/pkg/builder/controller.go index 2cd4ce9de1..9a74d6ec9a 100644 --- a/pkg/builder/controller.go +++ b/pkg/builder/controller.go @@ -305,7 +305,7 @@ func (blder *Builder) doController(r reconcile.Reconciler) error { } // Setup the logger. - if ctrlOptions.Log == nil { + if ctrlOptions.Log.GetSink() == nil { ctrlOptions.Log = blder.mgr.GetLogger() } ctrlOptions.Log = ctrlOptions.Log.WithValues("reconciler group", gvk.Group, "reconciler kind", gvk.Kind) diff --git a/pkg/builder/controller_test.go b/pkg/builder/controller_test.go index 62c5044e99..a0c0a4288b 100644 --- a/pkg/builder/controller_test.go +++ b/pkg/builder/controller_test.go @@ -53,14 +53,26 @@ func (typedNoop) Reconcile(context.Context, reconcile.Request) (reconcile.Result return reconcile.Result{}, nil } -type testLogger struct { +type testLogSink struct { logr.Logger } -func (l *testLogger) WithName(_ string) logr.Logger { +func (l *testLogSink) Init(info logr.RuntimeInfo) { +} + +func (l *testLogSink) Enabled(level int) bool { + return true +} + +func (l *testLogSink) Info(level int, msg string, keysAndValues ...interface{}) { + +} + +func (l *testLogSink) WithValues(keysAndValues ...interface{}) logr.LogSink { return l } -func (l *testLogger) WithValues(_ ...interface{}) logr.Logger { + +func (l *testLogSink) WithName(name string) logr.LogSink { return l } @@ -225,12 +237,12 @@ var _ = Describe("application", func() { It("should override logger during creation of controller", func() { - logger := &testLogger{} + logSink := &testLogSink{} newController = func(name string, mgr manager.Manager, options controller.Options) (controller.Controller, error) { - if options.Log == logger { + if options.Log.GetSink() == logSink { return controller.New(name, mgr, options) } - return nil, fmt.Errorf("logger expected %T but found %T", logger, options.Log) + return nil, fmt.Errorf("logSink expected %T but found %T", logSink, options.Log) } By("creating a controller manager") @@ -240,7 +252,7 @@ var _ = Describe("application", func() { instance, err := ControllerManagedBy(m). For(&appsv1.ReplicaSet{}). Owns(&appsv1.ReplicaSet{}). - WithLogger(logger). + WithLogger(logr.New(logSink)). Build(noop) Expect(err).NotTo(HaveOccurred()) Expect(instance).NotTo(BeNil()) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index dfd0fa9dd8..4b8ee8e7c5 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -245,7 +245,7 @@ func setOptionsDefaults(options Options) Options { } } - if options.Logger == nil { + if options.Logger.GetSink() == nil { options.Logger = logf.RuntimeLog.WithName("cluster") } diff --git a/pkg/config/v1alpha1/zz_generated.deepcopy.go b/pkg/config/v1alpha1/zz_generated.deepcopy.go index 752fa9754c..5329bef667 100644 --- a/pkg/config/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/config/v1alpha1/zz_generated.deepcopy.go @@ -1,3 +1,4 @@ +//go:build !ignore_autogenerated // +build !ignore_autogenerated // Code generated by controller-gen. DO NOT EDIT. diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 88ba786717..a5c850274e 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -104,7 +104,7 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller return nil, fmt.Errorf("must specify Name for Controller") } - if options.Log == nil { + if options.Log.GetSink() == nil { options.Log = mgr.GetLogger() } diff --git a/pkg/internal/recorder/recorder_test.go b/pkg/internal/recorder/recorder_test.go index 08d60f9deb..86bcdd36f6 100644 --- a/pkg/internal/recorder/recorder_test.go +++ b/pkg/internal/recorder/recorder_test.go @@ -29,7 +29,7 @@ var _ = Describe("recorder.Provider", func() { makeBroadcaster := func() (record.EventBroadcaster, bool) { return record.NewBroadcaster(), true } Describe("NewProvider", func() { It("should return a provider instance and a nil error.", func() { - provider, err := recorder.NewProvider(cfg, scheme.Scheme, logr.DiscardLogger{}, makeBroadcaster) + provider, err := recorder.NewProvider(cfg, scheme.Scheme, logr.Discard(), makeBroadcaster) Expect(provider).NotTo(BeNil()) Expect(err).NotTo(HaveOccurred()) }) @@ -38,14 +38,14 @@ var _ = Describe("recorder.Provider", func() { // Invalid the config cfg1 := *cfg cfg1.Host = "invalid host" - _, err := recorder.NewProvider(&cfg1, scheme.Scheme, logr.DiscardLogger{}, makeBroadcaster) + _, err := recorder.NewProvider(&cfg1, scheme.Scheme, logr.Discard(), makeBroadcaster) Expect(err).NotTo(BeNil()) Expect(err.Error()).To(ContainSubstring("failed to init client")) }) }) Describe("GetEventRecorder", func() { It("should return a recorder instance.", func() { - provider, err := recorder.NewProvider(cfg, scheme.Scheme, logr.DiscardLogger{}, makeBroadcaster) + provider, err := recorder.NewProvider(cfg, scheme.Scheme, logr.Discard(), makeBroadcaster) Expect(err).NotTo(HaveOccurred()) recorder := provider.GetEventRecorderFor("test") diff --git a/pkg/log/deleg.go b/pkg/log/deleg.go index 9d73947dac..8cd5510d60 100644 --- a/pkg/log/deleg.go +++ b/pkg/log/deleg.go @@ -25,16 +25,15 @@ import ( // loggerPromise knows how to populate a concrete logr.Logger // with options, given an actual base logger later on down the line. type loggerPromise struct { - logger *DelegatingLogger + logger *delegatingLogSink childPromises []*loggerPromise promisesLock sync.Mutex - name *string - tags []interface{} - level int + name *string + tags []interface{} } -func (p *loggerPromise) WithName(l *DelegatingLogger, name string) *loggerPromise { +func (p *loggerPromise) WithName(l *delegatingLogSink, name string) *loggerPromise { res := &loggerPromise{ logger: l, name: &name, @@ -48,7 +47,7 @@ func (p *loggerPromise) WithName(l *DelegatingLogger, name string) *loggerPromis } // WithValues provides a new Logger with the tags appended. -func (p *loggerPromise) WithValues(l *DelegatingLogger, tags ...interface{}) *loggerPromise { +func (p *loggerPromise) WithValues(l *delegatingLogSink, tags ...interface{}) *loggerPromise { res := &loggerPromise{ logger: l, tags: tags, @@ -61,61 +60,54 @@ func (p *loggerPromise) WithValues(l *DelegatingLogger, tags ...interface{}) *lo return res } -func (p *loggerPromise) V(l *DelegatingLogger, level int) *loggerPromise { - res := &loggerPromise{ - logger: l, - level: level, - promisesLock: sync.Mutex{}, - } +// Fulfill instantiates the Logger with the provided logSink. +func (p *loggerPromise) Fulfill(parentLogSink logr.LogSink) { + sink := parentLogSink - p.promisesLock.Lock() - defer p.promisesLock.Unlock() - p.childPromises = append(p.childPromises, res) - return res -} - -// Fulfill instantiates the Logger with the provided logger. -func (p *loggerPromise) Fulfill(parentLogger logr.Logger) { - logger := logr.WithCallDepth(parentLogger, 1) if p.name != nil { - logger = logger.WithName(*p.name) + sink = sink.WithName(*p.name) } if p.tags != nil { - logger = logger.WithValues(p.tags...) - } - if p.level != 0 { - logger = logger.V(p.level) + sink = sink.WithValues(p.tags...) } p.logger.lock.Lock() - p.logger.logger = logger + p.logger.logSink = sink p.logger.promise = nil p.logger.lock.Unlock() for _, childPromise := range p.childPromises { - childPromise.Fulfill(logger) + childPromise.Fulfill(sink) } } -// DelegatingLogger is a logr.Logger that delegates to another logr.Logger. +// delegatingLogSink is a logr.LogSink that delegates to another logr.LogSink. // If the underlying promise is not nil, it registers calls to sub-loggers with // the logging factory to be populated later, and returns a new delegating -// logger. It expects to have *some* logr.Logger set at all times (generally -// a no-op logger before the promises are fulfilled). -type DelegatingLogger struct { +// logSink. It expects to have *some* logr.Logger set at all times (generally +// a no-op logSink before the promises are fulfilled). +type delegatingLogSink struct { lock sync.RWMutex - logger logr.Logger + logSink logr.LogSink promise *loggerPromise + info logr.RuntimeInfo +} + +// Init implements logr.LogSink. +func (l *delegatingLogSink) Init(info logr.RuntimeInfo) { + l.lock.Lock() + defer l.lock.Unlock() + l.info = info } // Enabled tests whether this Logger is enabled. For example, commandline // flags might be used to set the logging verbosity and disable some info // logs. -func (l *DelegatingLogger) Enabled() bool { +func (l *delegatingLogSink) Enabled(v int) bool { l.lock.RLock() defer l.lock.RUnlock() - return l.logger.Enabled() + return l.logSink.Enabled(v) } // Info logs a non-error message with the given key/value pairs as context. @@ -124,10 +116,10 @@ func (l *DelegatingLogger) Enabled() bool { // the log line. The key/value pairs can then be used to add additional // variable information. The key/value pairs should alternate string // keys and arbitrary values. -func (l *DelegatingLogger) Info(msg string, keysAndValues ...interface{}) { +func (l *delegatingLogSink) Info(level int, msg string, keysAndValues ...interface{}) { l.lock.RLock() defer l.lock.RUnlock() - l.logger.Info(msg, keysAndValues...) + l.logSink.Info(level, msg, keysAndValues...) } // Error logs an error, with the given message and key/value pairs as context. @@ -138,41 +130,22 @@ func (l *DelegatingLogger) Info(msg string, keysAndValues ...interface{}) { // The msg field should be used to add context to any underlying error, // while the err field should be used to attach the actual error that // triggered this log line, if present. -func (l *DelegatingLogger) Error(err error, msg string, keysAndValues ...interface{}) { +func (l *delegatingLogSink) Error(err error, msg string, keysAndValues ...interface{}) { l.lock.RLock() defer l.lock.RUnlock() - l.logger.Error(err, msg, keysAndValues...) -} - -// V returns an Logger value for a specific verbosity level, relative to -// this Logger. In other words, V values are additive. V higher verbosity -// level means a log message is less important. It's illegal to pass a log -// level less than zero. -func (l *DelegatingLogger) V(level int) logr.Logger { - l.lock.RLock() - defer l.lock.RUnlock() - - if l.promise == nil { - return l.logger.V(level) - } - - res := &DelegatingLogger{logger: l.logger} - promise := l.promise.V(res, level) - res.promise = promise - - return res + l.logSink.Error(err, msg, keysAndValues...) } // WithName provides a new Logger with the name appended. -func (l *DelegatingLogger) WithName(name string) logr.Logger { +func (l *delegatingLogSink) WithName(name string) logr.LogSink { l.lock.RLock() defer l.lock.RUnlock() if l.promise == nil { - return l.logger.WithName(name) + return l.logSink.WithName(name) } - res := &DelegatingLogger{logger: l.logger} + res := &delegatingLogSink{logSink: l.logSink} promise := l.promise.WithName(res, name) res.promise = promise @@ -180,37 +153,41 @@ func (l *DelegatingLogger) WithName(name string) logr.Logger { } // WithValues provides a new Logger with the tags appended. -func (l *DelegatingLogger) WithValues(tags ...interface{}) logr.Logger { +func (l *delegatingLogSink) WithValues(tags ...interface{}) logr.LogSink { l.lock.RLock() defer l.lock.RUnlock() if l.promise == nil { - return l.logger.WithValues(tags...) + return l.logSink.WithValues(tags...) } - res := &DelegatingLogger{logger: l.logger} + res := &delegatingLogSink{logSink: l.logSink} promise := l.promise.WithValues(res, tags...) res.promise = promise return res } -// Fulfill switches the logger over to use the actual logger +// Fulfill switches the logSink over to use the actual logSink // provided, instead of the temporary initial one, if this method // has not been previously called. -func (l *DelegatingLogger) Fulfill(actual logr.Logger) { +func (l *delegatingLogSink) Fulfill(actual logr.LogSink) { if l.promise != nil { l.promise.Fulfill(actual) } } -// NewDelegatingLogger constructs a new DelegatingLogger which uses -// the given logger before it's promise is fulfilled. -func NewDelegatingLogger(initial logr.Logger) *DelegatingLogger { - l := &DelegatingLogger{ - logger: initial, +// NewDelegatingLogger constructs a new delegatingLogSink which uses +// the given logSink before it's promise is fulfilled. +func NewDelegatingLogger(logSink logr.LogSink) logr.Logger { + l := &delegatingLogSink{ + logSink: logSink, promise: &loggerPromise{promisesLock: sync.Mutex{}}, } l.promise.logger = l - return l + return logr.New(l) +} + +type CanFulfill interface { + Fulfill(actual logr.LogSink) } diff --git a/pkg/log/log.go b/pkg/log/log.go index 229ac7ec35..9591c5367a 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -// Package log contains utilities for fetching a new logger +// Package log contains utilities for fetching a new logSink // when one is not already available. // // The Log Handle @@ -47,16 +47,16 @@ func SetLogger(l logr.Logger) { defer loggerWasSetLock.Unlock() loggerWasSet = true - Log.Fulfill(l) + Log.GetSink().(CanFulfill).Fulfill(l.GetSink()) } // It is safe to assume that if this wasn't set within the first 30 seconds of a binaries -// lifetime, it will never get set. The DelegatingLogger causes a high number of memory -// allocations when not given an actual Logger, so we set a NullLogger to avoid that. +// lifetime, it will never get set. The delegatingLogSink causes a high number of memory +// allocations when not given an actual Logger, so we set a NullLogSink to avoid that. // -// We need to keep the DelegatingLogger because we have various inits() that get a logger from +// We need to keep the delegatingLogSink because we have various inits() that get a logSink from // here. They will always get executed before any code that imports controller-runtime -// has a chance to run and hence to set an actual logger. +// has a chance to run and hence to set an actual logSink. func init() { // Init is blocking, so start a new goroutine go func() { @@ -64,7 +64,7 @@ func init() { loggerWasSetLock.Lock() defer loggerWasSetLock.Unlock() if !loggerWasSet { - Log.Fulfill(NullLogger{}) + Log.GetSink().(CanFulfill).Fulfill(NullLogSink{}) } }() } @@ -74,26 +74,28 @@ var ( loggerWasSet bool ) -// Log is the base logger used by kubebuilder. It delegates +// Log is the base logSink used by kubebuilder. It delegates // to another logr.Logger. You *must* call SetLogger to // get any actual logging. If SetLogger is not called within // the first 30 seconds of a binaries lifetime, it will get -// set to a NullLogger. -var Log = NewDelegatingLogger(NullLogger{}) +// set to a NullLogSink. +var ( + Log = NewDelegatingLogger(NullLogSink{}) +) -// FromContext returns a logger with predefined values from a context.Context. +// FromContext returns a logSink with predefined values from a context.Context. func FromContext(ctx context.Context, keysAndValues ...interface{}) logr.Logger { - var log logr.Logger = Log + log := Log if ctx != nil { - if logger := logr.FromContext(ctx); logger != nil { + if logger, err := logr.FromContext(ctx); err == nil { log = logger } } return log.WithValues(keysAndValues...) } -// IntoContext takes a context and sets the logger as one of its values. -// Use FromContext function to retrieve the logger. +// IntoContext takes a context and sets the logSink as one of its values. +// Use FromContext function to retrieve the logSink. func IntoContext(ctx context.Context, log logr.Logger) context.Context { return logr.NewContext(ctx, log) } diff --git a/pkg/log/log_test.go b/pkg/log/log_test.go index be3ad87997..2c63c9ef16 100644 --- a/pkg/log/log_test.go +++ b/pkg/log/log_test.go @@ -25,7 +25,7 @@ import ( . "github.com/onsi/gomega" ) -var _ logr.Logger = &DelegatingLogger{} +var _ logr.LogSink = &delegatingLogSink{} // logInfo is the information for a particular fakeLogger message. type logInfo struct { @@ -49,7 +49,10 @@ type fakeLogger struct { root *fakeLoggerRoot } -func (f *fakeLogger) WithName(name string) logr.Logger { +func (f *fakeLogger) Init(info logr.RuntimeInfo) { +} + +func (f *fakeLogger) WithName(name string) logr.LogSink { names := append([]string(nil), f.name...) names = append(names, name) return &fakeLogger{ @@ -59,7 +62,7 @@ func (f *fakeLogger) WithName(name string) logr.Logger { } } -func (f *fakeLogger) WithValues(vals ...interface{}) logr.Logger { +func (f *fakeLogger) WithValues(vals ...interface{}) logr.LogSink { tags := append([]interface{}(nil), f.tags...) tags = append(tags, vals...) return &fakeLogger{ @@ -80,7 +83,7 @@ func (f *fakeLogger) Error(err error, msg string, vals ...interface{}) { }) } -func (f *fakeLogger) Info(msg string, vals ...interface{}) { +func (f *fakeLogger) Info(l int, msg string, vals ...interface{}) { tags := append([]interface{}(nil), f.tags...) tags = append(tags, vals...) f.root.messages = append(f.root.messages, logInfo{ @@ -90,27 +93,28 @@ func (f *fakeLogger) Info(msg string, vals ...interface{}) { }) } -func (f *fakeLogger) Enabled() bool { return true } -func (f *fakeLogger) V(lvl int) logr.Logger { return f } +func (f *fakeLogger) Enabled(l int) bool { + return true +} var _ = Describe("logging", func() { - Describe("top-level logger", func() { - It("hold newly created loggers until a logger is set", func() { - By("grabbing a new sub-logger and logging to it") + Describe("top-level logSink", func() { + It("hold newly created loggers until a logSink is set", func() { + By("grabbing a new sub-logSink and logging to it") l1 := Log.WithName("runtimeLog").WithValues("newtag", "newvalue1") l1.Info("before msg") - By("actually setting the logger") + By("actually setting the logSink") logger := &fakeLogger{root: &fakeLoggerRoot{}} - SetLogger(logger) + SetLogger(logr.New(logger)) - By("grabbing another sub-logger and logging to both loggers") + By("grabbing another sub-logSink and logging to both loggers") l2 := Log.WithName("runtimeLog").WithValues("newtag", "newvalue2") l1.Info("after msg 1") l2.Info("after msg 2") - By("ensuring that messages after the logger was set were logged") + By("ensuring that messages after the logSink was set were logged") Expect(logger.root.messages).To(ConsistOf( logInfo{name: []string{"runtimeLog"}, tags: []interface{}{"newtag", "newvalue1"}, msg: "after msg 1"}, logInfo{name: []string{"runtimeLog"}, tags: []interface{}{"newtag", "newvalue2"}, msg: "after msg 2"}, @@ -118,39 +122,39 @@ var _ = Describe("logging", func() { }) }) - Describe("lazy logger initialization", func() { + Describe("lazy logSink initialization", func() { var ( root *fakeLoggerRoot - baseLog logr.Logger - delegLog *DelegatingLogger + baseLog logr.LogSink + delegLog logr.Logger ) BeforeEach(func() { root = &fakeLoggerRoot{} baseLog = &fakeLogger{root: root} - delegLog = NewDelegatingLogger(NullLogger{}) + delegLog = NewDelegatingLogger(NullLogSink{}) }) It("should delegate with name", func() { - By("asking for a logger with a name before fulfill, and logging") + By("asking for a logSink with a name before fulfill, and logging") befFulfill1 := delegLog.WithName("before-fulfill") befFulfill2 := befFulfill1.WithName("two") befFulfill1.Info("before fulfill") - By("logging on the base logger before fulfill") + By("logging on the base logSink before fulfill") delegLog.Info("before fulfill base") By("ensuring that no messages were actually recorded") Expect(root.messages).To(BeEmpty()) By("fulfilling the promise") - delegLog.Fulfill(baseLog) + delegLog.GetSink().(CanFulfill).Fulfill(baseLog) By("logging with the existing loggers after fulfilling") befFulfill1.Info("after 1") befFulfill2.Info("after 2") - By("grabbing a new sub-logger of a previously constructed logger and logging to it") + By("grabbing a new sub-logSink of a previously constructed logSink and logging to it") befFulfill1.WithName("after-from-before").Info("after 3") By("logging with new loggers") @@ -182,7 +186,7 @@ var _ = Describe("logging", func() { child := delegLog.WithName("child") go func() { defer GinkgoRecover() - delegLog.Fulfill(NullLogger{}) + delegLog.GetSink().(CanFulfill).Fulfill(NullLogSink{}) close(fulfillDone) }() go func() { @@ -232,25 +236,25 @@ var _ = Describe("logging", func() { }) It("should delegate with tags", func() { - By("asking for a logger with a name before fulfill, and logging") + By("asking for a logSink with a name before fulfill, and logging") befFulfill1 := delegLog.WithValues("tag1", "val1") befFulfill2 := befFulfill1.WithValues("tag2", "val2") befFulfill1.Info("before fulfill") - By("logging on the base logger before fulfill") + By("logging on the base logSink before fulfill") delegLog.Info("before fulfill base") By("ensuring that no messages were actually recorded") Expect(root.messages).To(BeEmpty()) By("fulfilling the promise") - delegLog.Fulfill(baseLog) + delegLog.GetSink().(CanFulfill).Fulfill(baseLog) By("logging with the existing loggers after fulfilling") befFulfill1.Info("after 1") befFulfill2.Info("after 2") - By("grabbing a new sub-logger of a previously constructed logger and logging to it") + By("grabbing a new sub-logSink of a previously constructed logSink and logging to it") befFulfill1.WithValues("tag3", "val3").Info("after 3") By("logging with new loggers") @@ -267,13 +271,13 @@ var _ = Describe("logging", func() { It("shouldn't fulfill twice", func() { By("fulfilling once") - delegLog.Fulfill(baseLog) + delegLog.GetSink().(CanFulfill).Fulfill(baseLog) By("logging a bit") delegLog.Info("msg 1") - By("fulfilling with a new logger") - delegLog.Fulfill(&fakeLogger{}) + By("fulfilling with a new logSink") + delegLog.GetSink().(CanFulfill).Fulfill(&fakeLogger{}) By("logging some more") delegLog.Info("msg 2") @@ -286,17 +290,17 @@ var _ = Describe("logging", func() { }) }) - Describe("logger from context", func() { - It("should return default logger when context is empty", func() { + Describe("logSink from context", func() { + It("should return default logSink when context is empty", func() { gotLog := FromContext(context.Background()) Expect(gotLog).To(Not(BeNil())) }) - It("should return existing logger", func() { + It("should return existing logSink", func() { root := &fakeLoggerRoot{} baseLog := &fakeLogger{root: root} - wantLog := baseLog.WithName("my-logger") + wantLog := logr.New(baseLog).WithName("my-logSink") ctx := IntoContext(context.Background(), wantLog) gotLog := FromContext(ctx) @@ -304,7 +308,7 @@ var _ = Describe("logging", func() { gotLog.Info("test message") Expect(root.messages).To(ConsistOf( - logInfo{name: []string{"my-logger"}, msg: "test message"}, + logInfo{name: []string{"my-logSink"}, msg: "test message"}, )) }) @@ -312,7 +316,7 @@ var _ = Describe("logging", func() { root := &fakeLoggerRoot{} baseLog := &fakeLogger{root: root} - wantLog := baseLog.WithName("my-logger") + wantLog := logr.New(baseLog).WithName("my-logSink") ctx := IntoContext(context.Background(), wantLog) gotLog := FromContext(ctx, "tag1", "value1") @@ -320,7 +324,7 @@ var _ = Describe("logging", func() { gotLog.Info("test message") Expect(root.messages).To(ConsistOf( - logInfo{name: []string{"my-logger"}, tags: []interface{}{"tag1", "value1"}, msg: "test message"}, + logInfo{name: []string{"my-logSink"}, tags: []interface{}{"tag1", "value1"}, msg: "test message"}, )) }) }) diff --git a/pkg/log/null.go b/pkg/log/null.go index 09a5a02eb6..b8d66511b0 100644 --- a/pkg/log/null.go +++ b/pkg/log/null.go @@ -20,41 +20,40 @@ import ( "github.com/go-logr/logr" ) -// NB: this is the same as the null logger logr/testing, +// NB: this is the same as the null logSink logr/testing, // but avoids accidentally adding the testing flags to // all binaries. -// NullLogger is a logr.Logger that does nothing. -type NullLogger struct{} +// NullLogSink is a logr.Logger that does nothing. +type NullLogSink struct{} -var _ logr.Logger = NullLogger{} +var _ logr.LogSink = NullLogSink{} -// Info implements logr.InfoLogger. -func (NullLogger) Info(_ string, _ ...interface{}) { +// Init implements logr.LogSink. +func (log NullLogSink) Init(_ logr.RuntimeInfo) { +} + +// Info implements logr.LogSink. +func (NullLogSink) Info(_ int, _ string, _ ...interface{}) { // Do nothing. } -// Enabled implements logr.InfoLogger. -func (NullLogger) Enabled() bool { +// Enabled implements logr.LogSink. +func (NullLogSink) Enabled(_ int) bool { return false } -// Error implements logr.Logger. -func (NullLogger) Error(_ error, _ string, _ ...interface{}) { +// Error implements logr.LogSink. +func (NullLogSink) Error(_ error, _ string, _ ...interface{}) { // Do nothing. } -// V implements logr.Logger. -func (log NullLogger) V(_ int) logr.Logger { - return log -} - -// WithName implements logr.Logger. -func (log NullLogger) WithName(_ string) logr.Logger { +// WithName implements logr.LogSink. +func (log NullLogSink) WithName(_ string) logr.LogSink { return log } -// WithValues implements logr.Logger. -func (log NullLogger) WithValues(_ ...interface{}) logr.Logger { +// WithValues implements logr.LogSink. +func (log NullLogSink) WithValues(_ ...interface{}) logr.LogSink { return log } diff --git a/pkg/log/zap/zap_test.go b/pkg/log/zap/zap_test.go index b714284da6..c146a73090 100644 --- a/pkg/log/zap/zap_test.go +++ b/pkg/log/zap/zap_test.go @@ -62,7 +62,7 @@ type fakeLoggerRoot struct { messages []logInfo } -var _ logr.Logger = &fakeLogger{} +var _ logr.LogSink = &fakeLogger{} // fakeLogger is a fake implementation of logr.Logger that records // messages, tags, and names, @@ -74,7 +74,14 @@ type fakeLogger struct { root *fakeLoggerRoot } -func (f *fakeLogger) WithName(name string) logr.Logger { +func (f *fakeLogger) Init(info logr.RuntimeInfo) { +} + +func (f *fakeLogger) Enabled(level int) bool { + return true +} + +func (f *fakeLogger) WithName(name string) logr.LogSink { names := append([]string(nil), f.name...) names = append(names, name) return &fakeLogger{ @@ -84,7 +91,7 @@ func (f *fakeLogger) WithName(name string) logr.Logger { } } -func (f *fakeLogger) WithValues(vals ...interface{}) logr.Logger { +func (f *fakeLogger) WithValues(vals ...interface{}) logr.LogSink { tags := append([]interface{}(nil), f.tags...) tags = append(tags, vals...) return &fakeLogger{ @@ -105,7 +112,7 @@ func (f *fakeLogger) Error(err error, msg string, vals ...interface{}) { }) } -func (f *fakeLogger) Info(msg string, vals ...interface{}) { +func (f *fakeLogger) Info(level int, msg string, vals ...interface{}) { tags := append([]interface{}(nil), f.tags...) tags = append(tags, vals...) f.root.messages = append(f.root.messages, logInfo{ @@ -115,9 +122,6 @@ func (f *fakeLogger) Info(msg string, vals ...interface{}) { }) } -func (f *fakeLogger) Enabled() bool { return true } -func (f *fakeLogger) V(lvl int) logr.Logger { return f } - var _ = Describe("Zap options setup", func() { var opts *Options @@ -575,9 +579,10 @@ var _ = Describe("Zap log level flag options setup", func() { logOut.Truncate(0) logger.V(4).Info("test 4") // Should not be logged Expect(logOut.String()).To(BeEmpty()) - logger.V(-3).Info("test -3") // Log a panic, since V(-1*N) for all N > 0 is not permitted. - Expect(logOut.String()).To(ContainSubstring(`"level":"dpanic"`)) + logger.V(-3).Info("test -3") // Since logr v1.0.0, negative logr level will be force to 0 https://github.com/go-logr/logr/blob/master/logr.go#L269-L279、. + Expect(logOut.String()).To(ContainSubstring("test -3")) }) + It("does not log with positive logr level", func() { By("setting up the logger") logger := New(WriteTo(logOut), Level(zapcore.Level(1))) diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 2d2733f0a6..d8c8e01c51 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -571,7 +571,7 @@ func setOptionsDefaults(options Options) Options { options.GracefulShutdownTimeout = &gracefulShutdownTimeout } - if options.Logger == nil { + if options.Logger.GetSink() == nil { options.Logger = log.Log } diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index cf7dbcf68d..3dcff5fadd 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -243,7 +243,7 @@ func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, err return nil, err } - if opts.Logger == nil { + if opts.Logger.GetSink() == nil { opts.Logger = logf.RuntimeLog.WithName("webhook") } hook.log = opts.Logger