diff --git a/logcheck/main_test.go b/logcheck/main_test.go index 4394332..1f8a0fe 100644 --- a/logcheck/main_test.go +++ b/logcheck/main_test.go @@ -143,6 +143,13 @@ func TestAnalyzer(t *testing.T) { }, testPackage: "allowBadkeysLogs", }, + { + name: "Detect incomplete fmt.Stringer", + enabled: map[string]string{ + "value": "true", + }, + testPackage: "stringer", + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/logcheck/pkg/logcheck.go b/logcheck/pkg/logcheck.go index 52b47a7..a2d45f4 100644 --- a/logcheck/pkg/logcheck.go +++ b/logcheck/pkg/logcheck.go @@ -39,6 +39,7 @@ const ( withHelpersCheck = "with-helpers" verbosityZeroCheck = "verbosity-zero" keyCheck = "key" + valueCheck = "value" deprecationsCheck = "deprecations" ) @@ -63,6 +64,7 @@ func Analyser() *analysis.Analyzer { withHelpersCheck: new(bool), verbosityZeroCheck: new(bool), keyCheck: new(bool), + valueCheck: new(bool), deprecationsCheck: new(bool), }, } @@ -79,6 +81,7 @@ klog methods (Info, Infof, Error, Errorf, Warningf, etc).`) 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[keyCheck], prefix+keyCheck, true, `When true, logcheck will check whether name arguments are valid keys according to the guidelines in (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments).`) + logcheckFlags.BoolVar(c.enabled[valueCheck], prefix+valueCheck, false, `When true, logcheck will check for problematic values (for example, types that have an incomplete fmt.Stringer implementation).`) 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.`) @@ -145,6 +148,9 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { fName := selExpr.Sel.Name filename := pass.Pkg.Path() + "/" + path.Base(pass.Fset.Position(fexpr.Pos()).Filename) + valueCheckEnabled := c.isEnabled(valueCheck, filename) + keyCheckEnabled := c.isEnabled(keyCheck, filename) + parametersCheckEnabled := c.isEnabled(parametersCheck, filename) // Now we need to determine whether it is coming from klog. if isKlog(selExpr.X, pass) { @@ -176,50 +182,37 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { return } - if c.isEnabled(parametersCheck, filename) { + if keyCheckEnabled || parametersCheckEnabled || valueCheckEnabled { // if format specifier is used, check for arg length will most probably fail // so check for format specifier first and skip if found - if checkForFormatSpecifier(fexpr, pass) { + if parametersCheckEnabled && checkForFormatSpecifier(fexpr, pass) { return } - if fName == "InfoS" { - isKeysValid(args[1:], fun, pass, fName) - } else if fName == "ErrorS" { - isKeysValid(args[2:], fun, pass, fName) - } - - // Also check structured calls. - if c.isEnabled(parametersCheck, filename) { - checkForFormatSpecifier(fexpr, pass) + switch fName { + case "InfoS": + kvCheck(args[1:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled) + case "ErrorS": + kvCheck(args[2:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled) } } // verbosity Zero Check if c.isEnabled(verbosityZeroCheck, filename) { checkForVerbosityZero(fexpr, pass) } - // key Check - if c.isEnabled(keyCheck, filename) { + } else if isGoLogger(selExpr.X, pass) { + if keyCheckEnabled || parametersCheckEnabled || valueCheckEnabled { // if format specifier is used, check for arg length will most probably fail // so check for format specifier first and skip if found - if checkFormatSpecifier(fexpr, pass) { + if parametersCheckEnabled && checkForFormatSpecifier(fexpr, pass) { return } - if fName == "InfoS" { - keysCheck(args[1:], fun, pass, fName) - } else if fName == "ErrorS" { - keysCheck(args[2:], fun, pass, fName) - } - } - } else if isGoLogger(selExpr.X, pass) { - if c.isEnabled(parametersCheck, filename) { - checkForFormatSpecifier(fexpr, pass) switch fName { case "WithValues": - isKeysValid(args, fun, pass, fName) + kvCheck(args, fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled) case "Info": - isKeysValid(args[1:], fun, pass, fName) + kvCheck(args[1:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled) case "Error": - isKeysValid(args[2:], fun, pass, fName) + kvCheck(args[2:], fun, pass, fName, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled) } } if c.isEnabled(withHelpersCheck, filename) { @@ -235,22 +228,6 @@ func checkForFunctionExpr(fexpr *ast.CallExpr, pass *analysis.Pass, c *config) { if c.isEnabled(verbosityZeroCheck, filename) { checkForVerbosityZero(fexpr, pass) } - // key Check - if c.isEnabled(keyCheck, filename) { - // if format specifier is used, check for arg length will most probably fail - // so check for format specifier first and skip if found - if checkFormatSpecifier(fexpr, pass) { - return - } - switch fName { - case "WithValues": - keysCheck(args, fun, pass, fName) - case "Info": - keysCheck(args[1:], fun, pass, fName) - case "Error": - keysCheck(args[2:], fun, pass, fName) - } - } } else if fName == "NewContext" && isPackage(selExpr.X, "github.com/go-logr/logr", pass) && c.isEnabled(withHelpersCheck, filename) { @@ -395,45 +372,6 @@ func isContextualCall(fName string) bool { return false } -// isKeysValid check if all keys in keyAndValues is string type -func isKeysValid(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string) { - if len(keyValues)%2 != 0 { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Additional arguments to %s should always be Key Value pairs. Please check if there is any key or value missing.", funName), - }) - return - } - - for index, arg := range keyValues { - if index%2 != 0 { - continue - } - lit, ok := arg.(*ast.BasicLit) - if !ok { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg), - }) - continue - } - if lit.Kind != token.STRING { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value), - }) - continue - } - isASCII := utf8string.NewString(lit.Value).IsASCII() - if !isASCII { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Key positional arguments %s are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.", lit.Value), - }) - } - } -} - func checkForFormatSpecifier(expr *ast.CallExpr, pass *analysis.Pass) bool { if selExpr, ok := expr.Fun.(*ast.SelectorExpr); ok { // extracting function Name like Infof @@ -615,17 +553,9 @@ func isVerbosityZero(expr ast.Expr) bool { return false } -func checkFormatSpecifier(expr *ast.CallExpr, pass *analysis.Pass) bool { - if _, ok := expr.Fun.(*ast.SelectorExpr); ok { - if _, found := hasFormatSpecifier(expr.Args); found { - return true - } - } - return false -} - -// keysCheck check if all keys in keyAndValues are valid keys according to the guidelines. -func keysCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string) { +// kvCheck check if all keys in keyAndValues are valid keys according to the guidelines +// and that the values can be formatted. +func kvCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled bool) { if len(keyValues)%2 != 0 { pass.Report(analysis.Diagnostic{ Pos: fun.Pos(), @@ -635,31 +565,120 @@ func keysCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName } for index, arg := range keyValues { - if index%2 != 0 { - continue - } - lit, ok := arg.(*ast.BasicLit) - if !ok { - pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg), - }) - continue + switch index % 2 { + case 0: + // Key in key/value pair. + checkKey(arg, pass, keyCheckEnabled, parametersCheckEnabled) + case 1: + // Value in key/value pair. + checkValue(arg, pass, valueCheckEnabled) } - if lit.Kind != token.STRING { + } +} + +// checkKey checks the key in a key/value pair. +func checkKey(arg ast.Expr, pass *analysis.Pass, keyCheckEnabled, parametersCheckEnabled bool) { + if !keyCheckEnabled && !parametersCheckEnabled { + return + } + + lit, ok := arg.(*ast.BasicLit) + if !ok { + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", arg), + }) + return + } + + if lit.Kind != token.STRING { + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value), + }) + return + } + + switch { + case parametersCheckEnabled: + // This is the less strict check. + isASCII := utf8string.NewString(lit.Value).IsASCII() + if !isASCII { pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), - Message: fmt.Sprintf("Key positional arguments are expected to be inlined constant strings. Please replace %v provided with string value.", lit.Value), + Pos: arg.Pos(), + Message: fmt.Sprintf("Key positional arguments %s are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.", lit.Value), }) - continue } + case keyCheckEnabled: + // This is the stricter check. keyMatchRe := regexp.MustCompile(`(^[A-Z]{2,}|^[a-z])[[:alnum:]]*$`) match := keyMatchRe.Match([]byte(strings.Trim(lit.Value, "\""))) if !match { pass.Report(analysis.Diagnostic{ - Pos: fun.Pos(), + Pos: arg.Pos(), Message: fmt.Sprintf("Key positional arguments %s are expected to be alphanumeric and start with either one lowercase or two uppercase letters. Please refer to https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#name-arguments.", lit.Value), }) } } } + +// checkValue checks the value in a key/value pair. +func checkValue(arg ast.Expr, pass *analysis.Pass, valueCheckEnabled bool) { + if !valueCheckEnabled { + return + } + + // Check the type. + if typeAndValue, ok := pass.TypesInfo.Types[arg]; ok { + if obj, index, _ := types.LookupFieldOrMethod(typeAndValue.Type, typeAndValue.Addressable(), nil /* package */, "String"); obj != nil { + if function, ok := obj.(*types.Func); ok && isFmtString(function) && len(index) > 1 && !isWrapperStruct(typeAndValue.Type) { + pass.Report(analysis.Diagnostic{ + Pos: arg.Pos(), + Message: fmt.Sprintf("The type %s inherits %s as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO.", typeAndValue.Type.String(), function.FullName()), // TODO: https://github.com/kubernetes/kubernetes/pull/116952 + }) + } + } + } +} + +// isFmtString checks whether the function has the "func() string" signature. +func isFmtString(function *types.Func) bool { + signature, ok := function.Type().(*types.Signature) + if !ok { + return false + } + params := signature.Params() + if params != nil && params.Len() != 0 { + return false + } + results := signature.Results() + if results == nil || results.Len() != 1 { + return false + } + result := results.At(0) + basic, ok := result.Type().(*types.Basic) + if !ok { + return false + } + if basic.Kind() != types.String { + return false + } + return true +} + +// isWrapperStruct returns true for types that are a struct with a single field +// or a pointer to one, like for example +// https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Time. +func isWrapperStruct(t types.Type) bool { + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + if named, ok := t.(*types.Named); ok { + t = named.Underlying() + } + if strct, ok := t.(*types.Struct); ok { + return strct.NumFields() == 1 + } + + return false +} diff --git a/logcheck/testdata/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go b/logcheck/testdata/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go new file mode 100644 index 0000000..597f4f2 --- /dev/null +++ b/logcheck/testdata/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go @@ -0,0 +1,40 @@ +/* +Copyright 2023 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 v1 + +import ( + "fmt" + "strings" +) + +type TypeMeta struct { + Kind string `json:"kind,omitempty" protobuf:"bytes,1,opt,name=kind"` + APIVersion string `json:"apiVersion,omitempty" protobuf:"bytes,2,opt,name=apiVersion"` +} + +// String is generated (https://github.com/kubernetes/apimachinery/blob/v0.26.3/pkg/apis/meta/v1/generated.pb.go#L4757). +func (this *TypeMeta) String() string { + if this == nil { + return "nil" + } + s := strings.Join([]string{`&TypeMeta{`, + `Kind:` + fmt.Sprintf("%v", this.Kind) + `,`, + `APIVersion:` + fmt.Sprintf("%v", this.APIVersion) + `,`, + `}`, + }, "") + return s +} diff --git a/logcheck/testdata/src/stringer/stringer.go b/logcheck/testdata/src/stringer/stringer.go new file mode 100644 index 0000000..6e9fca3 --- /dev/null +++ b/logcheck/testdata/src/stringer/stringer.go @@ -0,0 +1,56 @@ +/* +Copyright 2023 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 stringer + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + klog "k8s.io/klog/v2" +) + +func foo() { + klog.Background().Info("Starting", "config", config{}) + klog.Background().Info("Starting", "config", configWithStringer{}) + klog.Background().Info("Starting", "config", &config{}) // want `The type \*stringer.config inherits \(\*k8s.io/apimachinery/pkg/apis/meta/v1.TypeMeta\).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String\(\) for the type or wrap it with TODO.` + klog.Background().Info("Starting", "config", &configWithStringer{}) + klog.Background().Info("Starting", "config", &simpleConfig{}) +} + +// config mimicks KubeletConfig (see +// https://github.com/kubernetes/kubernetes/pull/115950). As far as logging is +// concerned, the type is broken: it implements fmt.Stringer because it +// embeds TypeMeta, but the result of String() is incomplete. +type config struct { + metav1.TypeMeta // implements fmt.Stringer (but only for addressable values) + + RealField int +} + +type configWithStringer config + +func (c configWithStringer) Size() int { + return 1 +} + +func (c configWithStringer) String() string { + return "foo" +} + +// simpleConfig only has a single field. In this case inheriting the String implementation +// is fine. This occurs for https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#Time. +type simpleConfig struct { + metav1.TypeMeta +}