diff --git a/hack/tools/logcheck/README.md b/hack/tools/logcheck/README.md index 00f423eaa..4c86d9349 100644 --- a/hack/tools/logcheck/README.md +++ b/hack/tools/logcheck/README.md @@ -46,6 +46,12 @@ the next section. Unstructured klog logging calls are flagged as error. +## klog + +None of the klog logging methods may be used. This is even stricter than +`unstructured`. Instead, code should retrieve a logr.Logger from klog and log +through that. + ## parameters Key/value parameters for structured logging calls are checked: diff --git a/hack/tools/logcheck/main_test.go b/hack/tools/logcheck/main_test.go index 20a8a047c..f591d68e9 100644 --- a/hack/tools/logcheck/main_test.go +++ b/hack/tools/logcheck/main_test.go @@ -57,6 +57,23 @@ func TestAnalyzer(t *testing.T) { }, testPackage: "parameters", }, + { + name: "Do not allow klog", + enabled: map[string]string{ + "unstructured": "true", + "klog": "true", + }, + testPackage: "doNotAllowKlog", + }, + { + name: "Filter klog", + enabled: map[string]string{ + "unstructured": "false", + "klog": "false", + }, + override: "testdata/src/doNotAllowKlog/klog_logging", + testPackage: "doNotAllowKlog", + }, { name: "importrename", enabled: map[string]string{ @@ -72,11 +89,17 @@ func TestAnalyzer(t *testing.T) { testPackage: "verbose", }, { - name: "gologr", + name: "gologr", + enabled: map[string]string{ + "klog": "true", + }, testPackage: "gologr", }, { - name: "contextual", + name: "contextual", + enabled: map[string]string{ + "klog": "true", + }, testPackage: "contextual", }, } diff --git a/hack/tools/logcheck/pkg/logcheck.go b/hack/tools/logcheck/pkg/logcheck.go index 8883c300b..123c1c9c8 100644 --- a/hack/tools/logcheck/pkg/logcheck.go +++ b/hack/tools/logcheck/pkg/logcheck.go @@ -34,6 +34,7 @@ import ( const ( unstructuredCheck = "unstructured" parametersCheck = "parameters" + klogCheck = "klog" ) type checks map[string]*bool @@ -53,6 +54,7 @@ func Analyser() *analysis.Analyzer { enabled: checks{ unstructuredCheck: new(bool), parametersCheck: new(bool), + klogCheck: new(bool), }, } c.fileOverrides.validChecks = map[string]bool{} @@ -64,6 +66,7 @@ func Analyser() *analysis.Analyzer { logcheckFlags.BoolVar(c.enabled[unstructuredCheck], prefix+unstructuredCheck, true, `When true, logcheck will warn about calls to unstructured klog methods (Info, Infof, Error, Errorf, Warningf, etc).`) logcheckFlags.BoolVar(c.enabled[parametersCheck], prefix+parametersCheck, true, `When true, logcheck will check parameters of structured logging calls.`) + logcheckFlags.BoolVar(c.enabled[klogCheck], prefix+klogCheck, false, `When true, logcheck will warn about any legacy klog calls.`) logcheckFlags.Var(&c.fileOverrides, "config", `A file which overrides the global settings for checks on a per-file basis via regular expressions.`) // Use env variables as defaults. This is necessary when used as plugin @@ -132,27 +135,33 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { // Now we need to determine whether is is coming from klog. if isKlog(selExpr.X, pass) { + if c.isEnabled(klogCheck, filename) && isLegacyKlogCall(fName) { + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: fmt.Sprintf("function %q should not be used, convert to contextual logging", fName), + }) + return + } + // Matching if any unstructured logging function is used. - if !isUnstructured((fName)) { - if c.isEnabled(parametersCheck, filename) { - // if format specifier is used, check for arg length will most probably fail - // so check for format specifier first and skip if found - if checkForFormatSpecifier(fexpr, pass) { - return - } - if fName == "InfoS" { - isKeysValid(args[1:], fun, pass, fName) - } else if fName == "ErrorS" { - isKeysValid(args[2:], fun, pass, fName) - } + if c.isEnabled(unstructuredCheck, filename) && isUnstructured(fName) { + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: fmt.Sprintf("unstructured logging function %q should not be used", fName), + }) + return + } + + if c.isEnabled(parametersCheck, filename) { + // if format specifier is used, check for arg length will most probably fail + // so check for format specifier first and skip if found + if checkForFormatSpecifier(fexpr, pass) { + return } - } else { - if c.isEnabled(unstructuredCheck, filename) { - msg := fmt.Sprintf("unstructured logging function %q should not be used", fName) - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: msg, - }) + if fName == "InfoS" { + isKeysValid(args[1:], fun, pass, fName) + } else if fName == "ErrorS" { + isKeysValid(args[2:], fun, pass, fName) } // Also check structured calls. @@ -254,6 +263,30 @@ func isUnstructured(fName string) bool { return false } +func isLegacyKlogCall(fName string) bool { + // List of klog functions we still want to use after migration to structured logging. + contextual := []string{ + "Background", + "ClearLogger", + "FromContext", + "KObj", + "KObjs", + "KRef", + "LoggerWithName", + "LoggerWithValues", + "NewContext", + "SetLogger", + "TODO", + } + for _, name := range contextual { + if fName == name { + return false + } + } + + return true +} + // isKeysValid check if all keys in keyAndValues is string type func isKeysValid(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string) { if len(keyValues)%2 != 0 { diff --git a/hack/tools/logcheck/testdata/src/doNotAllowKlog/doNotAllowKlog.go b/hack/tools/logcheck/testdata/src/doNotAllowKlog/doNotAllowKlog.go new file mode 100644 index 000000000..a43fe7fb0 --- /dev/null +++ b/hack/tools/logcheck/testdata/src/doNotAllowKlog/doNotAllowKlog.go @@ -0,0 +1,32 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This fake package is created as golang.org/x/tools/go/analysis/analysistest +// expects it to be here for loading. This package is used to test allow-unstructured +// flag which suppresses errors when unstructured logging is used. +// This is a test file for unstructured logging static check tool unit tests. + +package allowUnstructuredLogs + +import ( + klog "k8s.io/klog/v2" +) + +func doNotAlllowKlog() { + klog.InfoS("test log") // want `function "InfoS" should not be used, convert to contextual logging` + klog.ErrorS(nil, "test log") // want `function "ErrorS" should not be used, convert to contextual logging` + klog.V(1).Infof("test log") // want `function "V" should not be used, convert to contextual logging` `function "Infof" should not be used, convert to contextual logging` +} diff --git a/hack/tools/logcheck/testdata/src/doNotAllowKlog/whitelistedKlog.go b/hack/tools/logcheck/testdata/src/doNotAllowKlog/whitelistedKlog.go new file mode 100644 index 000000000..c590b9f49 --- /dev/null +++ b/hack/tools/logcheck/testdata/src/doNotAllowKlog/whitelistedKlog.go @@ -0,0 +1,37 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This fake package is created as golang.org/x/tools/go/analysis/analysistest +// expects it to be here for loading. This package is used to test allow-unstructured +// flag which suppresses errors when unstructured logging is used. +// This is a test file for unstructured logging static check tool unit tests. + +package allowUnstructuredLogs + +import ( + klog "k8s.io/klog/v2" +) + +func allowKlog() { + klog.KObj(nil) + klog.KObjs(nil) + klog.KRef("", "") + klog.FromContext(nil) + klog.TODO() + klog.Background() + klog.LoggerWithName(klog.Logger{}, "foo") + klog.LoggerWithValues(klog.Logger{}, "a", "b") +} diff --git a/hack/tools/logcheck/testdata/src/github.com/go-logr/logr/logr.go b/hack/tools/logcheck/testdata/src/github.com/go-logr/logr/logr.go index aa54eb112..fa2da1e9d 100644 --- a/hack/tools/logcheck/testdata/src/github.com/go-logr/logr/logr.go +++ b/hack/tools/logcheck/testdata/src/github.com/go-logr/logr/logr.go @@ -18,6 +18,10 @@ limitations under the License. // with golang.org/x/tools/go/analysis/analysistest. package logr +import ( + "context" +) + type Logger struct{} func (l Logger) Enabled() bool { return false } @@ -26,3 +30,7 @@ func (l Logger) WithValues(kv ...interface{}) Logger { return l } func (l Logger) V(level int) Logger { return l } func (l Logger) Info(msg string, kv ...interface{}) {} func (l Logger) Error(err error, msg string, kv ...interface{}) {} + +func NewContext(ctx context.Context, logger Logger) context.Context { + return nil +} diff --git a/hack/tools/logcheck/testdata/src/k8s.io/klog/v2/klog.go b/hack/tools/logcheck/testdata/src/k8s.io/klog/v2/klog.go index 20cf8949e..d62a5d433 100644 --- a/hack/tools/logcheck/testdata/src/k8s.io/klog/v2/klog.go +++ b/hack/tools/logcheck/testdata/src/k8s.io/klog/v2/klog.go @@ -19,6 +19,12 @@ limitations under the License. package v2 +import ( + "context" + + "github.com/go-logr/logr" +) + // Verbose is a boolean type that implements Infof (like Printf) etc. // See the documentation of V for more information. type Verbose struct { @@ -27,6 +33,8 @@ type Verbose struct { type Level int32 +type Logger = logr.Logger + func V(level Level) Verbose { return Verbose{enabled: false} @@ -202,3 +210,41 @@ func Exitln(args ...interface{}) { // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing. func Exitf(format string, args ...interface{}) { } + +// KObj emulates klog.KObj +func KObj(obj interface{}) interface{} { + return nil +} + +// KObjs emulates klog.KObjs +func KObjs(obj interface{}) interface{} { + return nil +} + +func KRef(namespace, name string) interface{} { + return nil +} + +func FromContext(ctx context.Context) Logger { + return Logger{} +} + +func NewContext(ctx context.Context, logger Logger) context.Context { + return ctx +} + +func LoggerWithName(logger Logger, name string) Logger { + return Logger{} +} + +func LoggerWithValues(logger Logger, kvs ...interface{}) Logger { + return Logger{} +} + +func TODO() Logger { + return Logger{} +} + +func Background() Logger { + return Logger{} +}