Add script for restricting import of test only libraries

[vlasebian]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Presubmit check to restrict test only libraries from linking into prod binaries.

Which issue(s) this PR fixes:

Fixes #115175

Special notes for your reviewer:

I implemented a script following the advice in the issue, but I have a few questions to make sure I understood the problem:

• Should the script check all the packages in the project or only the ones under ./cmd? At the moment, the script checks the presence of the testing import in the packages under ./cmd with only one level of recursion.
• Should the script be called from a build rule? If so, which would be the most appropriate?
• In case testing imports are found and the script is called from a build rule, is it okay to print the packages where they were found (so it's easier to remove them) or printing to stdout isn't allowed during that specific build rule?

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @vlasebian. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[pacoxu]

/ok-to-test

Probably, we can add it to

kubernetes/hack/make-rules/verify.sh

Lines 70 to 74 in 24e6b03

	QUICK_PATTERNS+=(
	"verify-api-groups.sh"
	"verify-boilerplate.sh"
	"verify-external-dependencies-version.sh"
	"verify-vendor-licenses.sh"

[pacoxu]

/cc @mikedanese

[liggitt]

test failed with

+++ Running case: verify.testing-import 
+++ working dir: /home/prow/go/src/k8s.io/kubernetes
+++ command: bash "hack/make-rules/../../hack/verify-testing-import.sh"
Testing package imported in:
	/home/prow/go/src/k8s.io/kubernetes/cmd/kube-controller-manager
	/home/prow/go/src/k8s.io/kubernetes/cmd/genkubedocs
	/home/prow/go/src/k8s.io/kubernetes/cmd/kubelet
	/home/prow/go/src/k8s.io/kubernetes/cmd/kubemark
	/home/prow/go/src/k8s.io/kubernetes/cmd/kube-scheduler
	/home/prow/go/src/k8s.io/kubernetes/cmd/kube-proxy
	/home/prow/go/src/k8s.io/kubernetes/cmd/kube-apiserver
	/home/prow/go/src/k8s.io/kubernetes/cmd/cloud-controller-manager
	/home/prow/go/src/k8s.io/kubernetes/cmd/genman
+++ exit code: 1
+++ error: 1
�[0;31mFAILED   verify-testing-import.sh	22s

not sure if those are legitimate and need fixing or spurious and the verify script needs fixing

[vlasebian]

Hi! Thank you for taking your time to check my PR. With the current version of the script I get the following results:

• without testing imports:

  ~/go/src/k8s.io/kubernetes/cmd/dependencycheck
  ~/go/src/k8s.io/kubernetes/cmd/genyaml
  ~/go/src/k8s.io/kubernetes/cmd/preferredimports
  ~/go/src/k8s.io/kubernetes/cmd/genutils
  ~/go/src/k8s.io/kubernetes/cmd/clicheck
  ~/go/src/k8s.io/kubernetes/cmd/importverifier
  ~/go/src/k8s.io/kubernetes/cmd/kubeadm
  ~/go/src/k8s.io/kubernetes/cmd/yamlfmt
  ~/go/src/k8s.io/kubernetes/cmd/genswaggertypedocs
  ~/go/src/k8s.io/kubernetes/cmd/fieldnamedocscheck
  ~/go/src/k8s.io/kubernetes/cmd/gotemplate
  ~/go/src/k8s.io/kubernetes/cmd/kubectl
  ~/go/src/k8s.io/kubernetes/cmd/gendocs
  ~/go/src/k8s.io/kubernetes/cmd/prune-junit-xml
  ~/go/src/k8s.io/kubernetes/cmd/dependencyverifier
  ~/go/src/k8s.io/kubernetes/cmd/kubectl-convert


• with testing imports:

  ~/go/src/k8s.io/kubernetes/cmd/kubemark
  ~/go/src/k8s.io/kubernetes/cmd/genman
  ~/go/src/k8s.io/kubernetes/cmd/kube-controller-manager
  ~/go/src/k8s.io/kubernetes/cmd/kube-scheduler
  ~/go/src/k8s.io/kubernetes/cmd/genkubedocs
  ~/go/src/k8s.io/kubernetes/cmd/cloud-controller-manager
  ~/go/src/k8s.io/kubernetes/cmd/kubelet
  ~/go/src/k8s.io/kubernetes/cmd/kube-proxy
  ~/go/src/k8s.io/kubernetes/cmd/kube-apiserver

I checked the testing dependency in some of the packages to see if the results are incorrect:

• from the 'without testing imports' list:

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kubeadm | jq '.Deps[]' | grep testing
"k8s.io/client-go/testing"
"k8s.io/utils/clock/testing"

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kubectl | jq '.Deps[]' | grep testing
"k8s.io/utils/clock/testing"

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kubectl-convert | jq '.Deps[]' | grep testing
"k8s.io/utils/clock/testing"

• from the 'with testing imports' list:

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kubemark | jq '.Deps[]' | grep testing
"k8s.io/client-go/testing"
"k8s.io/component-helpers/node/util/sysctl/testing"
"k8s.io/cri-api/pkg/apis/testing"
"k8s.io/kubernetes/pkg/kubelet/cadvisor/testing"
"k8s.io/kubernetes/pkg/kubelet/container/testing"
"k8s.io/kubernetes/pkg/kubelet/prober/testing"
"k8s.io/kubernetes/pkg/util/iptables/testing"
"k8s.io/utils/clock/testing"
"k8s.io/utils/exec/testing"
"testing"

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kubelet | jq '.Deps[]' | grep testing
"k8s.io/client-go/testing"
"k8s.io/kubernetes/pkg/kubelet/container/testing"
"k8s.io/utils/clock/testing"
"testing"

-> % go list -json ~/go/src/k8s.io/kubernetes/cmd/kube-apiserver | jq '.Deps[]' | grep testing
"k8s.io/client-go/testing"
"k8s.io/utils/clock/testing"
"testing"

Currently, the script is checking for the testing pacakge dependency for all the packages under ./cmd.

Is the testing package from the golang standard library allowed? If yes, then the current method to detect the testing packages is not correct, the check has to be done in another way so "testing" should be allowed but something like "k8s.io/kubernetes/pkg/kubelet/container/testing" should not be allowed.

[liggitt]

I am a bit confused at the moment about what needs to be done for this task. I understand that no testing packages should be imported, however is the testing package from the golang standard library allowed?

The requirement is that the golang standard library testing package should not be linked into these built binaries (should not be a dependency with any level of recursion):

• cloud-controller-manager
• kube-apiserver
• kube-controller-manager
• kube-proxy
• kube-scheduler
• kubectl
• kubectl-convert
• kubelet

[vlasebian]

Thank you for the clarification. I limited the search for the testing import only to the packages listed above. The .Deps field should contain all the dependencies of the specified package, it does a depth-first traversal. Currently, the packages containing the testing package as a dependency are the following:

Testing package imported in:
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/cloud-controller-manager
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kube-apiserver
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kube-controller-manager
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kube-proxy
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kube-scheduler
	/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kubelet

So besides:

/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kubectl
/Users/vladvitan/go/src/k8s.io/kubernetes/cmd/kubectl-convert

All the above have the testing dependency. To double check, I built kubectl and kube-apiserver, then checked the binaries:

-> % make WHAT="cmd/kubectl"

go version go1.21.4 darwin/arm64
+++ [1115 20:15:59] Building go targets for darwin/arm64
    k8s.io/kubernetes/cmd/kubectl (non-static)

-> % nm _output/bin/kubectl | grep testing
00000001030688f0 B _crypto/ecdsa.testingOnlyRejectionSamplingLooped
000000010306c960 B _crypto/tls.testingOnlyForceClientHelloSignatureAlgorithms
00000001030acf2b S _crypto/tls.testingOnlyForceDowngradeCanary
0000000101fdeff0 S _go:link.pkghash.k8s.io/utils/clock/testing
0000000101a9abf8 S _go:link.pkghashbytes.k8s.io/utils/clock/testing
00000001030acf08 S _os.testingForceReadDirLstat

-> % make WHAT="cmd/kube-apiserver"
go version go1.21.4 darwin/arm64
+++ [1115 20:16:05] Building go targets for darwin/arm64
    k8s.io/kubernetes/cmd/kube-apiserver (static)

-> % nm _output/bin/kube-apiserver | grep testing
0000000107199f88 b _crypto/ecdsa.testingOnlyRejectionSamplingLooped
000000010719faf0 b _crypto/tls.testingOnlyForceClientHelloSignatureAlgorithms
00000001071e47cb s _crypto/tls.testingOnlyForceDowngradeCanary
00000001071e47a8 s _os.testingForceReadDirLstat
00000001070cd590 s _testing..inittask
000000010155d360 t _testing.init
000000010719bb60 b _testing.supportedTypes

For kube-apiserver it seems that the testing.init symbol is in the binary, I also tried with kube-proxy too and it's there:

-> % make WHAT="cmd/kube-proxy"
go version go1.21.4 darwin/arm64
+++ [1115 20:19:37] Building go targets for darwin/arm64
    k8s.io/kubernetes/cmd/kube-proxy (static)

-> % nm _output/bin/kube-proxy | grep testing
0000000103201640 b _crypto/ecdsa.testingOnlyRejectionSamplingLooped
0000000103205f20 b _crypto/tls.testingOnlyForceClientHelloSignatureAlgorithms
0000000103247acd s _crypto/tls.testingOnlyForceDowngradeCanary
0000000103247aa8 s _os.testingForceReadDirLstat
000000010315baa0 s _testing..inittask
00000001011e05d0 t _testing.init
0000000103201e60 b _testing.supportedTypes

[liggitt]

it looks like two links to the stdlib testing package had crept into our binaries - fixed in #121913

if you want to rebase on that and see what your script reports, that would be good

[vlasebian]

Sure, will do.

[vlasebian]

@liggitt I cherry picked the commit from your pr to run the pull-kubernetes-verify job to check the script. I'll remove the cherry-picked commit afterwards. I tried it locally and no package is reported anymore as having a testing import.

[vlasebian]

/test pull-kubernetes-verify

[dashpole]

/triage accepted
/assign
for instrumentation

[bart0sh]

/priority important-longterm
/retest

[dashpole]

approval for component-base/metrics

[bart0sh]

@liggitt FWIW, the script doesn't report anything on this PR branch rebased on top of the latest master:

(121735) $ ./hack/verify-testing-import.sh && echo ok
ok

would it make sense to merge it?

[liggitt]

/lgtm
/approve

thanks!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 9e22e3ade2c04ae7984135fa630d6c267e6ff115

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, liggitt, vlasebian

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:

• hack/OWNERS [liggitt]
• pkg/kubelet/OWNERS [liggitt]
• staging/src/k8s.io/component-base/metrics/OWNERS [dashpole,liggitt]

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