check for interface inheritance #15

pohly · 2023-03-28T16:28:41Z

This is a static analysis check for the problem reported in kubernetes/kubernetes#115950: converting unstructured logging of the form "%v", someObj to "obj", someObj produces undesirable output in the klog text format when someObj is of a type which has an incomplete fmt.Stringer implementation because it inherits that from an embedded field.

In Kubernetes, this currently finds:

cmd/kubelet/app/server.go:259:61: The type *k8s.io/kubernetes/pkg/kubelet/apis/config.KubeletConfiguration 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. (logcheck)
			klog.V(5).InfoS("KubeletConfiguration", "configuration", config)
			                                                         ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:206:61: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
				logger.V(5).Info("Volume detached--skipping", "volume", attachedVolume)
				                                                        ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:235:84: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
				logger.V(5).Info("Cannot detach volume because it is still mounted", "volume", attachedVolume)
				                                                                               ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:255:113: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
				logger.Error(err, "UpdateNodeStatusForNode failed while attempting to report volume as attached", "volume", attachedVolume)
				                                                                                                            ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:265:73: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
			logger.V(5).Info("Starting attacherDetacher.DetachVolume", "volume", attachedVolume)
			                                                                     ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:273:69: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
					logger.Info("attacherDetacher.DetachVolume started", "volume", attachedVolume)
					                                                               ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:276:202: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
					logger.Info("attacherDetacher.DetachVolume started: this volume is not safe to detach, but maxWaitForUnmountDuration expired, force detaching", "duration", rc.maxWaitForUnmountDuration, "volume", attachedVolume)
					                                                                                                                                                                                                    ^
pkg/controller/volume/attachdetach/reconciler/reconciler.go:288:83: The type k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache.AttachedVolume inherits (*k8s.io/kubernetes/pkg/volume/util/operationexecutor.AttachedVolume).String as implementation of fmt.Stringer, which covers only a subset of the value. Implement String() for the type or wrap it with TODO. (logcheck)
					logger.Error(err, "attacherDetacher.DetachVolume failed to start", "volume", attachedVolume)
					                                                                             ^

k8s-ci-robot · 2023-03-28T16:28:49Z

This issue is currently awaiting triage.

If logtools contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pohly · 2023-03-28T18:47:26Z

The same check also makes sense for logr.Marshaler.

pohly · 2023-04-12T08:43:34Z

I added logr.Marshaler checking and proposed klog.Format.

That PR needs to get merged first.

/hold

pohly · 2023-04-25T13:14:12Z

/hold cancel

klog.Format got merged.

pohly · 2023-05-09T17:00:37Z

/assign @serathius

logcheck/pkg/logcheck.go

serathius · 2023-05-11T07:58:07Z

logcheck/pkg/logcheck.go

@@ -561,6 +565,14 @@ func kvCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName st

 	for index, arg := range keyValues {
 		if index%2 != 0 {
+			// Check values?


Oh, this makes this loop even harder to read. Could we separate key and value validation by using a dedicated functions? For example

func kvCheck(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, keyCheckEnabled, parametersCheckEnabled, valueCheckEnabled bool) { ... for index, arg := range keyValues { if index%2 == 0 { checkKey(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, keyCheckEnabled) checkParameters(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, parametersCheckEnabled) } checkValue(keyValues []ast.Expr, fun ast.Expr, pass *analysis.Pass, funName string, valueCheckEnabled) } }

Agreed, let's split it up.

I think a switch statement with two cases works best here because it puts both check calls at the same indention. Indenting one but not the other as in an if+continue looks weird.

Yes, switch is great idea. I hate golang aversion for else.

serathius · 2023-05-11T12:32:51Z

/lgtm
/approve

k8s-ci-robot · 2023-05-11T12:32:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, serathius

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~~ [pohly,serathius]

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

pohly · 2023-05-11T12:41:17Z

/hold

golangci-lint is unhappy.

The two flavors of key checking (plain ASCII and the stricter Kubernetes guidelines) are now done in the same function. This is a first step towards also checking values in that same function. While at it, a small bug for format specifier checking of klog calls gets fixed: it was done twice, once without honoring the enable flag for it.

If a type inherits String from some embedded field, then the klog text output will only print the result of that function call, which then includes only information about the embedded field (example: kubernetes/kubernetes#115950). The right solution for this problem is TBD.

serathius · 2023-05-11T12:53:30Z

/lgtm

pohly · 2023-05-11T13:48:00Z

/hold cancel

pohly · 2023-05-11T14:32:24Z

/hold

pohly · 2023-05-11T14:32:45Z

/hold cancel

Tide seems to have missed the label change... let's do it again.

k8s-ci-robot requested review from dims and thockin March 28, 2023 16:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 28, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2023

pohly changed the title ~~check stringer~~ WIP: check stringer Mar 28, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2023

pohly force-pushed the check-stringer branch from 262d9f5 to b23bdb5 Compare April 12, 2023 08:41

pohly changed the title ~~WIP: check stringer~~ check for interface inheritance Apr 12, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023

pohly force-pushed the check-stringer branch from b23bdb5 to 1399d95 Compare April 12, 2023 08:46

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2023

k8s-ci-robot assigned serathius May 9, 2023

pohly mentioned this pull request May 11, 2023

logcheck: allow klog.Format for contextual logging #17

Merged

serathius reviewed May 11, 2023

View reviewed changes

logcheck/pkg/logcheck.go Outdated Show resolved Hide resolved

serathius reviewed May 11, 2023

View reviewed changes

pohly force-pushed the check-stringer branch from 1399d95 to 19e4048 Compare May 11, 2023 12:22

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2023

pohly force-pushed the check-stringer branch from 19e4048 to 137dd42 Compare May 11, 2023 12:43

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 11, 2023

k8s-ci-robot merged commit ff0f1e9 into kubernetes-sigs:main May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check for interface inheritance #15

check for interface inheritance #15

pohly commented Mar 28, 2023

k8s-ci-robot commented Mar 28, 2023

pohly commented Mar 28, 2023

pohly commented Apr 12, 2023

pohly commented Apr 25, 2023

pohly commented May 9, 2023

serathius May 11, 2023

pohly May 11, 2023

serathius May 11, 2023

serathius commented May 11, 2023

k8s-ci-robot commented May 11, 2023

pohly commented May 11, 2023

serathius commented May 11, 2023

pohly commented May 11, 2023

pohly commented May 11, 2023

pohly commented May 11, 2023

check for interface inheritance #15

check for interface inheritance #15

Conversation

pohly commented Mar 28, 2023

k8s-ci-robot commented Mar 28, 2023

pohly commented Mar 28, 2023

pohly commented Apr 12, 2023

pohly commented Apr 25, 2023

pohly commented May 9, 2023

serathius May 11, 2023

Choose a reason for hiding this comment

pohly May 11, 2023

Choose a reason for hiding this comment

serathius May 11, 2023

Choose a reason for hiding this comment

serathius commented May 11, 2023

k8s-ci-robot commented May 11, 2023

pohly commented May 11, 2023

serathius commented May 11, 2023

pohly commented May 11, 2023

pohly commented May 11, 2023

pohly commented May 11, 2023