Merge pull request #213 from adisky/add-logcheck

Add Static linter for structured logging

.github/workflows/test.yml

@@ -18,6 +18,7 @@ jobs:
       run: |
         go get -t -v ./...
         go test -v -race ./...
+        cd hack/tools && go test -v -race ./...
   lint:
     runs-on: ubuntu-latest
     steps:

hack/tools/go.mod

@@ -0,0 +1,5 @@
+module k8s.io/klog/hack/tools
+
+go 1.15
+
+require golang.org/x/tools v0.1.0

hack/tools/go.sum

@@ -0,0 +1,26 @@
+github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
+golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
+golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
+golang.org/x/mod v0.3.0 h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
+golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA=
+golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
+golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
+golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU=
+golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
+golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
+golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4 h1:myAQVi0cGEoqQVR5POX+8RR2mrocKqNN1hmeMqhX27k=
+golang.org/x/sys v0.0.0-20210119212857-b64e53b001e4/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
+golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
+golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
+golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
+golang.org/x/tools v0.1.0 h1:po9/4sTYwZU9lPhi1tOrb4hCv3qrhiQ77LZfGa2OjwY=
+golang.org/x/tools v0.1.0/go.mod h1:xkSsbof2nBLbhDlRMhhhyNLN/zl3eTqcnHD5viDpcZ0=
+golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
+golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=

hack/tools/logcheck/README.md

@@ -0,0 +1,5 @@
+This directory contains tool for checking use of unstructured logs in a package. It is created to prevent regression after packages have been migrated to use structured logs.
+
+**Installation:**`go install k8s.io/klog/hack/tools/logcheck`
+**Usage:** `$logcheck.go <package_name>`
+`e.g $logcheck ./pkg/kubelet/lifecycle/`

hack/tools/logcheck/main.go

@@ -0,0 +1,112 @@
+/*
+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.
+*/
+
+package main
+
+import (
+	"fmt"
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"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,
+}
+
+func main() {
+	singlechecker.Main(Analyzer)
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+
+	for _, file := range pass.Files {
+
+		ast.Inspect(file, func(n ast.Node) bool {
+
+			// 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.Fun, pass)
+			}
+
+			return true
+		})
+	}
+	return nil, nil
+}
+
+// checkForFunctionExpr checks for unstructured logging function, prints error if found any.
+func checkForFunctionExpr(fun ast.Expr, pass *analysis.Pass) {
+
+	/* we are extracting external package function calls e.g. klog.Infof fmt.Printf
+	   and eliminating calls like setLocalHost()
+	   basically function calls that has selector expression like .
+	*/
+	if selExpr, ok := fun.(*ast.SelectorExpr); ok {
+		// extracting function Name like Infof
+		fName := selExpr.Sel.Name
+
+		// for nested function cases klog.V(1).Infof scenerios
+		// if selExpr.X contains one more caller expression which is selector expression
+		// we are extracting klog and discarding V(1)
+		if n, ok := selExpr.X.(*ast.CallExpr); ok {
+			if _, ok = n.Fun.(*ast.SelectorExpr); ok {
+				selExpr = n.Fun.(*ast.SelectorExpr)
+			}
+		}
+
+		// extracting package name
+		pName, ok := selExpr.X.(*ast.Ident)
+
+		// Matching if package name is klog and any unstructured logging function is used.
+		if ok && pName.Name == "klog" && isUnstructured((fName)) {
+
+			msg := fmt.Sprintf("unstructured logging function %q should not be used", fName)
+			pass.Report(analysis.Diagnostic{
+				Pos:     fun.Pos(),
+				Message: msg,
+			})
+		}
+	}
+}
+
+func isUnstructured(fName string) bool {
+
+	// List of klog functions we do not want to use after migration to structured logging.
+	unstrucured := []string{
+		"Infof", "Info", "Infoln", "InfoDepth",
+		"Warning", "Warningf", "Warningln", "WarningDepth",
+		"Error", "Errorf", "Errorln", "ErrorDepth",
+		"Fatal", "Fatalf", "Fatalln", "FatalDepth",
+	}
+
+	for _, name := range unstrucured {
+		if fName == name {
+			return true
+		}
+	}
+
+	return false
+}

hack/tools/logcheck/main_test.go

@@ -0,0 +1,27 @@
+/*
+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.
+*/
+
+package main
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), Analyzer)
+}

hack/tools/logcheck/testdata/data.go

@@ -0,0 +1,47 @@
+/*
+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.
+*/
+
+// test file for unstructured logging static check tool unit tests.
+
+package testdata
+
+import (
+	klog "k8s.io/klog/v2"
+)
+
+func printUnstructuredLog() {
+	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`
+	klog.Infoln("test log")          // want `unstructured logging function "Infoln" should not be used`
+	klog.InfoDepth(1, "test log")    // want `unstructured logging function "InfoDepth" should not be used`
+	klog.Warning("test log")         // want `unstructured logging function "Warning" should not be used`
+	klog.Warningf("test log")        // want `unstructured logging function "Warningf" should not be used`
+	klog.WarningDepth(1, "test log") // want `unstructured logging function "WarningDepth" should not be used`
+	klog.Error("test log")           // want `unstructured logging function "Error" should not be used`
+	klog.Errorf("test log")          // want `unstructured logging function "Errorf" should not be used`
+	klog.Errorln("test log")         // want `unstructured logging function "Errorln" should not be used`
+	klog.ErrorDepth(1, "test log")   // want `unstructured logging function "ErrorDepth" should not be used`
+	klog.Fatal("test log")           // want `unstructured logging function "Fatal" should not be used`
+	klog.Fatalf("test log")          // want `unstructured logging function "Fatalf" should not be used`
+	klog.Fatalln("test log")         // want `unstructured logging function "Fatalln" should not be used`
+	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")
+}