diff --git a/hack/tools/logcheck/main.go b/hack/tools/logcheck/main.go index 27028dcd..f82f6ea0 100644 --- a/hack/tools/logcheck/main.go +++ b/hack/tools/logcheck/main.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "flag" "fmt" "go/ast" "go/token" @@ -27,21 +28,32 @@ import ( "golang.org/x/tools/go/analysis/singlechecker" ) -// Doc explaining the tool. -const Doc = "Tool to check use of unstructured logging patterns." - -// Analyzer runs static analysis. -var Analyzer = &analysis.Analyzer{ - Name: "logcheck", - Doc: Doc, - Run: run, +type config struct { + // When enabled, logcheck will ignore calls to unstructured klog methods (Info, Infof, Error, Errorf, Warningf, etc) + allowUnstructured bool } func main() { - singlechecker.Main(Analyzer) + singlechecker.Main(analyser()) +} + +func analyser() *analysis.Analyzer { + c := config{} + logcheckFlags := flag.NewFlagSet("", flag.ExitOnError) + logcheckFlags.BoolVar(&c.allowUnstructured, "allow-unstructured", c.allowUnstructured, `when enabled, logcheck will ignore calls to unstructured +klog methods (Info, Infof, Error, Errorf, Warningf, etc)`) + + return &analysis.Analyzer{ + Name: "logcheck", + Doc: "Tool to check use of unstructured logging patterns.", + Run: func(pass *analysis.Pass) (interface{}, error) { + return run(pass, &c) + }, + Flags: *logcheckFlags, + } } -func run(pass *analysis.Pass) (interface{}, error) { +func run(pass *analysis.Pass, c *config) (interface{}, error) { for _, file := range pass.Files { @@ -50,7 +62,7 @@ func run(pass *analysis.Pass) (interface{}, error) { // We are intrested in function calls, as we want to detect klog.* calls // passing all function calls to checkForFunctionExpr if fexpr, ok := n.(*ast.CallExpr); ok { - checkForFunctionExpr(fexpr, pass) + checkForFunctionExpr(fexpr, pass, c) } return true @@ -60,7 +72,7 @@ func run(pass *analysis.Pass) (interface{}, error) { } // checkForFunctionExpr checks for unstructured logging function, prints error if found any. -func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass) { +func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { fun := fexpr.Fun args := fexpr.Args @@ -98,7 +110,7 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass) { } else if fName == "ErrorS" { isKeysValid(args[2:], fun, pass, fName) } - } else { + } else if !c.allowUnstructured { msg := fmt.Sprintf("unstructured logging function %q should not be used", fName) pass.Report(analysis.Diagnostic{ Pos: fun.Pos(), diff --git a/hack/tools/logcheck/main_test.go b/hack/tools/logcheck/main_test.go index b41f541b..3e0c08ae 100644 --- a/hack/tools/logcheck/main_test.go +++ b/hack/tools/logcheck/main_test.go @@ -23,5 +23,27 @@ import ( ) func TestAnalyzer(t *testing.T) { - analysistest.Run(t, analysistest.TestData(), Analyzer) + tests := []struct { + name string + allowUnstructured string + testPackage string + }{ + { + name: "Allow unstructured logs", + allowUnstructured: "true", + testPackage: "allowUnstructuredLogs", + }, + { + name: "Do not allow unstructured logs", + allowUnstructured: "false", + testPackage: "doNotAllowUnstructuredLogs", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + analyzer := analyser() + analyzer.Flags.Set("allow-unstructured", tt.allowUnstructured) + analysistest.Run(t, analysistest.TestData(), analyzer, tt.testPackage) + }) + } } diff --git a/hack/tools/logcheck/testdata/src/allowUnstructuredLogs/allowUnstructuredLogs.go b/hack/tools/logcheck/testdata/src/allowUnstructuredLogs/allowUnstructuredLogs.go new file mode 100644 index 00000000..346410d5 --- /dev/null +++ b/hack/tools/logcheck/testdata/src/allowUnstructuredLogs/allowUnstructuredLogs.go @@ -0,0 +1,63 @@ +/* +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 allowUnstructuredLogs() { + // Structured logs + // Error is expected if structured logging pattern is not used correctly + klog.InfoS("test log") + klog.ErrorS(nil, "test log") + klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.InfoS("Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.InfoS("Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.ErrorS(nil, "Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` + testKey := "a" + klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` + klog.InfoS("test: %s", "testname") // want `structured logging function "InfoS" should not use format specifier "%s"` + klog.ErrorS(nil, "test no.: %d", 1) // want `structured logging function "ErrorS" should not use format specifier "%d"` + + // Unstructured logs + // Error is not expected as this package allows unstructured logging + klog.V(1).Infof("test log") + klog.Infof("test log") + klog.Info("test log") + klog.Infoln("test log") + klog.InfoDepth(1, "test log") + klog.Warning("test log") + klog.Warningf("test log") + klog.WarningDepth(1, "test log") + klog.Error("test log") + klog.Errorf("test log") + klog.Errorln("test log") + klog.ErrorDepth(1, "test log") + klog.Fatal("test log") + klog.Fatalf("test log") + klog.Fatalln("test log") + klog.FatalDepth(1, "test log") +} diff --git a/hack/tools/logcheck/testdata/data.go b/hack/tools/logcheck/testdata/src/doNotAllowUnstructuredLogs/doNotAllowUnstructuredLogs.go similarity index 88% rename from hack/tools/logcheck/testdata/data.go rename to hack/tools/logcheck/testdata/src/doNotAllowUnstructuredLogs/doNotAllowUnstructuredLogs.go index dcdd0d1f..19c254ab 100644 --- a/hack/tools/logcheck/testdata/data.go +++ b/hack/tools/logcheck/testdata/src/doNotAllowUnstructuredLogs/doNotAllowUnstructuredLogs.go @@ -14,15 +14,36 @@ See the License for the specific language governing permissions and limitations under the License. */ -// test file for unstructured logging static check tool unit tests. +// 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 default +// behavior which is to report errors when unstructured logging is used. +// This is a test file for unstructured logging static check tool unit tests. -package testdata +package doNotAllowUnstructuredLogs import ( klog "k8s.io/klog/v2" ) -func printUnstructuredLog() { +func doNotAllowUnstructuredLogs() { + // Structured logs + // Error is expected if structured logging pattern is not used correctly + klog.InfoS("test log") + klog.ErrorS(nil, "test log") + klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.` + klog.InfoS("Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` + klog.InfoS("Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.ErrorS(nil, "Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` + klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` + testKey := "a" + klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` + klog.InfoS("test: %s", "testname") // want `structured logging function "InfoS" should not use format specifier "%s"` + klog.ErrorS(nil, "test no.: %d", 1) // want `structured logging function "ErrorS" should not use format specifier "%d"` + + // Unstructured logs + // Error is expected as this package does not allow unstructured logging klog.V(1).Infof("test log") // want `unstructured logging function "Infof" should not be used` klog.Infof("test log") // want `unstructured logging function "Infof" should not be used` klog.Info("test log") // want `unstructured logging function "Info" should not be used` @@ -41,19 +62,3 @@ func printUnstructuredLog() { klog.FatalDepth(1, "test log") // want `unstructured logging function "FatalDepth" should not be used` } - -func printStructuredLog() { - klog.InfoS("test log") - klog.ErrorS(nil, "test log") - klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.` - klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Additional arguments to ErrorS should always be Key Value pairs. Please check if there is any key or value missing.` - klog.InfoS("Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` - klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID") // want `Key positional arguments "测试" are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.` - klog.InfoS("Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` - klog.ErrorS(nil, "Starting container in a pod", 7, "containerID") // want `Key positional arguments are expected to be inlined constant strings. Please replace 7 provided with string value` - klog.InfoS("Starting container in a pod", map[string]string{"test1": "value"}, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` - testKey := "a" - klog.ErrorS(nil, "Starting container in a pod", testKey, "containerID") // want `Key positional arguments are expected to be inlined constant strings. ` - klog.InfoS("test: %s", "testname") // want `structured logging function "InfoS" should not use format specifier "%s"` - klog.ErrorS(nil, "test no.: %d", 1) // want `structured logging function "ErrorS" should not use format specifier "%d"` -}