From 72ae99ca7f56cf6d848d5cb22e1d8977b7c60bf8 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 2 Feb 2022 13:43:10 +0100 Subject: [PATCH] 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 | 7 +++ hack/tools/logcheck/main_test.go | 7 +++ hack/tools/logcheck/pkg/logcheck.go | 28 +++++++++++- .../src/helpers/doNotAllowDirectCalls.go | 43 +++++++++++++++++++ 4 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 hack/tools/logcheck/testdata/src/helpers/doNotAllowDirectCalls.go diff --git a/hack/tools/logcheck/README.md b/hack/tools/logcheck/README.md index 4c86d9349..b454237dc 100644 --- a/hack/tools/logcheck/README.md +++ b/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. diff --git a/hack/tools/logcheck/main_test.go b/hack/tools/logcheck/main_test.go index f591d68e9..296c7653d 100644 --- a/hack/tools/logcheck/main_test.go +++ b/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) { diff --git a/hack/tools/logcheck/pkg/logcheck.go b/hack/tools/logcheck/pkg/logcheck.go index 123c1c9c8..665e697bf 100644 --- a/hack/tools/logcheck/pkg/logcheck.go +++ b/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 } } diff --git a/hack/tools/logcheck/testdata/src/helpers/doNotAllowDirectCalls.go b/hack/tools/logcheck/testdata/src/helpers/doNotAllowDirectCalls.go new file mode 100644 index 000000000..22d8bb84d --- /dev/null +++ b/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) +}