logcheck: add check for With* and NewContext

The corresponding helper functions in klog should be used instead. This is
needed for the feature gate in Kubernetes.

hack/tools/logcheck/README.md

@@ -65,3 +65,10 @@ invoking logcheck through golangci-lint) or the check can be disabled for the
 file.
 
 In addition, format strings are not allowed as string parameters.
+
+## with-helpers
+
+`logr.Logger.WithName`, `logr.Logger.WithValues` and `logr.NewContext` must not
+be used.  The corresponding helper calls from `k8s.io/klogr` should be used
+instead. This is relevant when support contextual logging is disabled at
+runtime in klog.

hack/tools/logcheck/main_test.go

@@ -102,6 +102,13 @@ func TestAnalyzer(t *testing.T) {
 			},
 			testPackage: "contextual",
 		},
+		{
+			name: "helpers",
+			enabled: map[string]string{
+				"with-helpers": "true",
+			},
+			testPackage: "helpers",
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

hack/tools/logcheck/pkg/logcheck.go

@@ -35,6 +35,7 @@ const (
 	unstructuredCheck = "unstructured"
 	parametersCheck   = "parameters"
 	klogCheck         = "klog"
+	withHelpersCheck  = "with-helpers"
 )
 
 type checks map[string]*bool
@@ -55,6 +56,7 @@ func Analyser() *analysis.Analyzer {
 			unstructuredCheck: new(bool),
 			parametersCheck:   new(bool),
 			klogCheck:         new(bool),
+			withHelpersCheck:  new(bool),
 		},
 	}
 	c.fileOverrides.validChecks = map[string]bool{}
@@ -67,6 +69,7 @@ func Analyser() *analysis.Analyzer {
 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.BoolVar(c.enabled[withHelpersCheck], prefix+withHelpersCheck, false, `When true, logcheck will warn about direct calls to WithName, WithValues and NewContext.`)
 	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
@@ -181,7 +184,24 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) {
 					isKeysValid(args[2:], fun, pass, fName)
 				}
 			}
+			if c.isEnabled(withHelpersCheck, filename) {
+				switch fName {
+				case "WithValues", "WithName":
+					pass.Report(analysis.Diagnostic{
+						Pos:     fun.Pos(),
+						Message: fmt.Sprintf("function %q should be called through klogr.Logger%s", fName, fName),
+					})
+				}
+			}
+		} else if fName == "NewContext" &&
+			isPackage(selExpr.X, "github.com/go-logr/logr", pass) &&
+			c.isEnabled(withHelpersCheck, filename) {
+			pass.Report(analysis.Diagnostic{
+				Pos:     fun.Pos(),
+				Message: fmt.Sprintf("function %q should be called through klogr.NewContext", fName),
+			})
 		}
+
 	}
 }
 
@@ -212,12 +232,18 @@ func isKlog(expr ast.Expr, pass *analysis.Pass) bool {
 
 	// In "klog.Info", "klog" is a package identifier. It doesn't need to
 	// be "klog" because here we look up the actual package.
+	return isPackage(expr, "k8s.io/klog/v2", pass)
+}
+
+// isPackage checks whether an expression is an identifier that refers
+// to a specific package like k8s.io/klog/v2.
+func isPackage(expr ast.Expr, packagePath string, pass *analysis.Pass) bool {
 	if ident, ok := expr.(*ast.Ident); ok {
 		if object, ok := pass.TypesInfo.Uses[ident]; ok {
 			switch object := object.(type) {
 			case *types.PkgName:
 				pkg := object.Imported()
-				if pkg.Path() == "k8s.io/klog/v2" {
+				if pkg.Path() == packagePath {
 					return true
 				}
 			}

hack/tools/logcheck/testdata/src/helpers/doNotAllowDirectCalls.go

@@ -0,0 +1,43 @@
+/*
+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 helpers
+
+import (
+	"context"
+
+	"github.com/go-logr/logr"
+	klog "k8s.io/klog/v2"
+)
+
+var logger klog.Logger
+
+func doNotAlllowDirectCalls() {
+	logger.WithName("foo")                        // want `function "WithName" should be called through klogr.LoggerWithName`
+	logger.WithValues("a", "b")                   // want `function "WithValues" should be called through klogr.LoggerWithValues`
+	logr.NewContext(context.Background(), logger) // want `function "NewContext" should be called through klogr.NewContext`
+}
+
+func allowHelpers() {
+	klog.LoggerWithName(logger, "foo")
+	klog.LoggerWithValues(logger, "a", "b")
+	klog.NewContext(context.Background(), logger)
+}