logcheck: optionally warn about using legacy klog logging

This is even stricter than the check for unstructured klog functions: *all*
legacy logging functions can be banned, leaving only those that are still
needed for contextual logging.

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:

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",
 		},
 	}

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 {

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`
+}

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")
+}

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
+}

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{}
+}