From 32f29cad9302ea5382a39e8c1c5ead85abb8cef6 Mon Sep 17 00:00:00 2001 From: Harsha Narayana Date: Fri, 1 Jul 2022 10:12:01 +0530 Subject: [PATCH] structured-logging: enable KobjSlice usage and warn Kobjs usage --- logcheck/pkg/logcheck.go | 27 +++++++++++++++++++ logcheck/testdata/src/k8s.io/klog/v2/klog.go | 5 ++++ .../onlyAllowContextual.go | 3 +++ .../onlyAllowContextual/whitelistedKlog.go | 2 +- 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/logcheck/pkg/logcheck.go b/logcheck/pkg/logcheck.go index 92901e1..a1cce99 100644 --- a/logcheck/pkg/logcheck.go +++ b/logcheck/pkg/logcheck.go @@ -37,6 +37,7 @@ const ( contextualCheck = "contextual" withHelpersCheck = "with-helpers" verbosityZeroCheck = "verbosity-zero" + deprecationsCheck = "deprecations" ) type checks map[string]*bool @@ -59,6 +60,7 @@ func Analyser() *analysis.Analyzer { contextualCheck: new(bool), withHelpersCheck: new(bool), verbosityZeroCheck: new(bool), + deprecationsCheck: new(bool), }, } c.fileOverrides.validChecks = map[string]bool{} @@ -73,6 +75,7 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`) logcheckFlags.BoolVar(c.enabled[contextualCheck], prefix+contextualCheck, false, `When true, logcheck will only allow log calls for contextual logging (retrieving a Logger from klog or the context and logging through that) and warn about all others.`) logcheckFlags.BoolVar(c.enabled[withHelpersCheck], prefix+withHelpersCheck, false, `When true, logcheck will warn about direct calls to WithName, WithValues and NewContext.`) logcheckFlags.BoolVar(c.enabled[verbosityZeroCheck], prefix+verbosityZeroCheck, true, `When true, logcheck will check whether the parameter for V() is 0.`) + logcheckFlags.BoolVar(c.enabled[deprecationsCheck], prefix+deprecationsCheck, true, `When true, logcheck will analyze the usage of deprecated Klog function 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 @@ -149,6 +152,17 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { return } + // Check for Deprecated function usage + if c.isEnabled(deprecationsCheck, filename) { + message, deprecatedUse := isDeprecatedContextualCall(fName) + if deprecatedUse { + pass.Report(analysis.Diagnostic{ + Pos: fun.Pos(), + Message: message, + }) + } + } + // Matching if any unstructured logging function is used. if c.isEnabled(structuredCheck, filename) && isUnstructured(fName) { pass.Report(analysis.Diagnostic{ @@ -301,6 +315,18 @@ func isUnstructured(fName string) bool { return false } +func isDeprecatedContextualCall(fName string) (message string, deprecatedUse bool) { + deprecatedContextualLogHelper := map[string]string{ + "KObjs": "KObjSlice", + } + var replacementFunction string + if replacementFunction, deprecatedUse = deprecatedContextualLogHelper[fName]; deprecatedUse { + message = fmt.Sprintf(`Detected usage of deprecated helper "%s". Please switch to "%s" instead.`, fName, replacementFunction) + return + } + return +} + func isContextualCall(fName string) bool { // List of klog functions we still want to use after migration to // contextual logging. This is an allow list, so any new acceptable @@ -315,6 +341,7 @@ func isContextualCall(fName string) bool { "FromContext", "KObj", "KObjs", + "KObjSlice", "KRef", "LoggerWithName", "LoggerWithValues", diff --git a/logcheck/testdata/src/k8s.io/klog/v2/klog.go b/logcheck/testdata/src/k8s.io/klog/v2/klog.go index d62a5d4..f05f566 100644 --- a/logcheck/testdata/src/k8s.io/klog/v2/klog.go +++ b/logcheck/testdata/src/k8s.io/klog/v2/klog.go @@ -221,6 +221,11 @@ func KObjs(obj interface{}) interface{} { return nil } +// KObjSlice emulates klog.KObjSlice +func KObjSlice(obj interface{}) interface{} { + return nil +} + func KRef(namespace, name string) interface{} { return nil } diff --git a/logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go b/logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go index 4cc5e12..e6a71b2 100644 --- a/logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go +++ b/logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go @@ -29,4 +29,7 @@ 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` + + klog.KObjs(nil) // want `Detected usage of deprecated helper "KObjs". Please switch to "KObjSlice" instead.` + klog.InfoS("test log", "key", klog.KObjs(nil)) // want `function "InfoS" should not be used, convert to contextual logging` `Detected usage of deprecated helper "KObjs". Please switch to "KObjSlice" instead.` } diff --git a/logcheck/testdata/src/onlyAllowContextual/whitelistedKlog.go b/logcheck/testdata/src/onlyAllowContextual/whitelistedKlog.go index 07bb2a2..6acce7c 100644 --- a/logcheck/testdata/src/onlyAllowContextual/whitelistedKlog.go +++ b/logcheck/testdata/src/onlyAllowContextual/whitelistedKlog.go @@ -27,7 +27,7 @@ import ( func allowKlog() { klog.KObj(nil) - klog.KObjs(nil) + klog.KObjSlice(nil) klog.KRef("", "") klog.FromContext(nil) klog.TODO()