Add Static linter for structured logging

[adisky]

Signed-off-by: Aditi Sharma adi.sky17@gmail.com

What this PR does / why we need it:
Adding Static linter to avoid regressions after migration to structured logging.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Ref Issue: kubernetes/kubernetes#98975
See Discussion: kubernetes/kubernetes#99090
Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

None.

[nikhita]

Can we also plug this new tool into the CI? Currently, the CI is not running the tests in this PR

[nikhita]

fmt is not used here, so it can be removed, right?

[nikhita]

nit: $KUBE_ROOT no longer makes sense in the klog repo 😄

[BenTheElder]

maybe just ./pkg/kubelet/lifecycle ?

GOPATH is going away in go 1.17 anyhow, so users will have to start calling tools like this from within a module, where it will either be k8s.io/kubernetes/pkg/kubelet/lifecycle or ./pkg/kubelet/lifecycle.

[BenTheElder]

This PR can add testing this module here:

klog/.github/workflows/test.yml

Line 17 in 06bfcd3

- name: Test

Add something like (cd hack/tools && go test -v -race ./...) below line 20 I think

[adisky]

@nikhita @BenTheElder Thanks for the quick review :). Addressed all except the code comments that i am working on.

[BenTheElder]

I really like this, thanks!
Some nits/suggestions, nothing serious 😅

[BenTheElder]

/assign @dims

klog/OWNERS

Line 12 in 06bfcd3

- dims

[dims]

/approve
/lgtm

thanks for adding this @adisky !

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adisky, dims

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nikhita]

There is no FatalS so what would be recommend to use as an alternative?

Edit: never mind, I guess the recommend approach is to use ErrorS :)

[adisky]

Right it will be replaced by ErrorS, here is the guideline
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#replacing-fatal-calls

[neolit123]

Fatal* must be replaced with ErrorS + os.Exit() or this would be a change in behavior.
the migrate guide is inaccurate in this regard.

EDIT: actually NVM it does mention it:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#replacing-fatal-calls

[pohly]

This is taking a shortcut. Was that intentional because the alternative is too complex?

What we want to compare against is k8s.io/klog/v2, not just the name that the package was imported under. As it stands now, the following code will not be recognized as using klog.Info:

import glog "k8s.io/klog/v2"

func someFunc() {
  glog.Info("hello world"
}

It must be possible to lookup the selector, but after staring at the analysis.Pass for a while I still haven't figured out how. Any ideas?

[pohly]

I found a solution. This here works and also covers other cases that weren't handled before, like if klogV := klog.V(1); klogV.Enabled() { klogV.Info("should not be used") }:

// isKlog checks whether an expression is klog.Verbose or the klog package itself.
func isKlog(expr ast.Expr, pass *analysis.Pass) bool {
	// For klog.V(1) and klogV := klog.V(1) we can decide based on the type.
	if typeAndValue, ok := pass.TypesInfo.Types[expr]; ok {
		switch t := typeAndValue.Type.(type) {
		case *types.Named:
			typeName := t.Obj()
			name := typeName.Name()
			pkg := typeName.Pkg()
			pkgPath := pkg.Path()
			if name == "Verbose" && pkgPath == "k8s.io/klog/v2" {
				return true
			}
		}
	}

	// In "klog.Info", "klog" is a package identifier. It doesn't need to
	// be "klog" because here we look up the actual package.
	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" {
					return true
				}
			}
		}
	}

	return false
}

I'm currently collecting some improvements of logcheck for contextual logging and will include this in that patch set.

IMHO it's not important enough to be fixed right away. Import renaming was done in some projects that migrated from glog, but seems less common nowadays.