kubernetes: unit tests should not require root

What happened:

  • Some of our unit tests fail when not run as root (locally), currently looking at cluster/gce/* but I suspect there are more of these

What you expected to happen:

  • Our unit tests should not fail as root, any test requiring this has gone well past being a “unit” test
  • Our unit tests should probably not run as root in CI to prevent regressions

How to reproduce it (as minimally and precisely as possible):

make test

/sig testing

cc @liggitt @spiffxp @vinayakankugoyal @cheftako

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Reactions: 3
  • Comments: 35 (33 by maintainers)

Most upvoted comments

THANK YOU! I keep trying to get to this and something else comes up. That change sounds totally correct: Please do file the PR and I’ll try to make sure we get it reviewed 🙏

audit unit test fix in #102610

thanks to @PushkarJ it’s time to roll over the job config and close the gap on this permanently 🎉

I would think most users are running the tests locally as non-root but I could be wrong 😅, for me just make test encounters the problems. If I’m wrong that would be great!

In CI we will hopefully switch to this there as soon after this last test is fixed.

In my local setup I was running as non-root user, but make test did not fail for me. Probably absence of user entry with a name is what (or one of the reasons that) causes the failure as we discussed here: https://github.com/kubernetes/kubernetes/pull/103127#discussion_r657505781

@BenTheElder I was able to reproduce this locally and got a much more readable result. It seems like user.Current() returns error, when current user is non-root, but the code skips error checking and uses the user value returned (which is nil), which induces the panic in the if condition for UID == 0 check:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x23d2334]

goroutine 3 [running]:
testing.tRunner.func1.2(0x2627500, 0x399d630)
	/usr/local/go/src/testing/testing.go:1143 +0x49f
testing.tRunner.func1(0xc000001980)
	/usr/local/go/src/testing/testing.go:1146 +0x695
panic(0x2627500, 0x399d630)
	/usr/local/go/src/runtime/panic.go:971 +0x499
k8s.io/kubernetes/pkg/volume/csi.TestAttacherMountDevice(0xc000001980)
	/tmp/_output/local/go/src/k8s.io/kubernetes/pkg/volume/csi/csi_attacher_test.go:1191 +0x10b4
testing.tRunner(0xc000001980, 0x29519d8)
	/usr/local/go/src/testing/testing.go:1193 +0x203
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1238 +0x5d8
FAIL	k8s.io/kubernetes/pkg/volume/csi	1.559s
FAIL
make: *** [Makefile:184: test] Error 1

After I added the error check, the tests passed. Will start working on PR to fix this, if there are no concerns

k8s.io/kubernetes/pkg/volume/csi: TestAttacherMountDevice still seems to produce no logs when it fails.

I noticed it has UID==0 specific codepath(s) but those seem to be to skip certain cases when UID is 0 https://github.com/kubernetes/kubernetes/blob/ea0764452222146c47ec826977f49d7001b0ea8c/pkg/volume/csi/csi_attacher_test.go#L1191

We will be adding some unit tests to test functionality that updates the file permissions and owners for kubeadm-rootless work, those will have to run as root.

don’t add new unit tests that require root permission to run

sure.

I think we should remove the unit tests in question since they run configure-helper.sh. configure-helper.sh requires running as root by design.