Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

syscall: TestGroupCleanupUserNamespace failure on linux-s390x-ibm #52088

Closed
bcmills opened this issue Mar 31, 2022 · 7 comments
Closed

syscall: TestGroupCleanupUserNamespace failure on linux-s390x-ibm #52088

bcmills opened this issue Mar 31, 2022 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.19

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 31, 2022 

--- FAIL: TestGroupCleanupUserNamespace (0.00s)
    exec_linux_test.go:311: id command output: "uid=0(root) gid=0(root) groups=0(root) context=system_u:system_r:unconfined_service_t:s0", expected one of ["uid=0(root) gid=0(root) groups=0(root)" "uid=0(root) gid=0(root) groups=0(root),65534(nobody)" "uid=0(root) gid=0(root) groups=0(root),65534(nogroup)" "uid=0(root) gid=0(root) groups=0(root),65534" "uid=0(root) gid=0(root) groups=0(root),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody),65534(nobody)" "uid=0(root) gid=0(root) groups=0(root) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023" "uid=0(root) gid=0(root) groups=0(root),65534(nobody) context=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023"]
FAIL
FAIL	syscall	0.924s

greplogs --dashboard -md -l -e 'FAIL: TestGroupCleanupUserNamespace'

2022-03-31T20:02:55-f990b0f/linux-s390x-ibm

This test is empirically fragile and non-portable: it has broken (and had to be updated) in at least #16224, #16303, #19938, #34547, and #46752. With that many hard-coded special cases, it is all but certain that we have missed some.

It isn't clear to me from reading the test what property it is actually testing or what invariants it expects (it is quite sparse on commentary). Given the empirical fragility of the test, and given that the syscall package is essentially frozen at this point anyway, I suggest that we delete the test outright. Barring that, if someone feels strongly enough about keeping the test to make it more robust, perhaps it would be reasonable for the test to actually parse the output string, strip the context field, and ensure that the groups in the reported list are all identical to nobody or nogroup.
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 31, 2022
@bcmills bcmills added this to the Go1.19 milestone Mar 31, 2022
@bcmills bcmills self-assigned this Mar 31, 2022
@gopherbot
Copy link

Change https://go.dev/cl/397316 mentions this issue: syscall: relax output check in TestGroupCleanupUserNamespace

@jonathan-albrecht-ibm
Copy link
Contributor

Thanks @bcmills for fixing this. I was adding a new builder on Thurs which caused the failure only when run from systemd. I've figured out how to reproduce the error locally on the new builder so I can confirm it fixes the problem. The new builder is at a newer version of RHEL 8 but I haven't figured out why the selinux info is different on the old builders vs new new one. All have selinux enabled and are run from systemd.

Could this be backported to 1.18 and 1.17 so those branches will also be fixed? Would older release branches need it as well or are they guaranteed not to be built anymore? If it can't be backported everywhere its needed I'll probably disable selinux because I don't think I'll find a fix.

@bcmills
Copy link
Contributor Author

bcmills commented Apr 4, 2022

@gopherbot, please backport to Go 1.17 and 1.18. This is a test-only fix for an issue exposed by a builder upgrade.

This was referenced Apr 4, 2022
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #52148 (for 1.17), #52149 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@bcmills
Copy link
Contributor Author

bcmills commented Apr 4, 2022

We won't need to backport to branches before 1.17 because they're no longer running tests. It's ultimately up to the release team to decide on the 1.17 and 1.18 backports but TBH I think it's a shoo-in.

@gopherbot
Copy link

Change https://go.dev/cl/398234 mentions this issue: [release-branch.go1.18] syscall: relax output check in TestGroupCleanupUserNamespace

@gopherbot
Copy link

Change https://go.dev/cl/398235 mentions this issue: [release-branch.go1.17] syscall: relax output check in TestGroupCleanupUserNamespace

@dmitshur dmitshur mentioned this issue Apr 11, 2022
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 11, 2022
gopherbot pushed a commit that referenced this issue May 4, 2022
@dmitshur
[release-branch.go1.18] syscall: relax output check in TestGroupClean…
a286c7d 
…upUserNamespace

“If you have a procedure with ten parameters, you probably missed some.”
― attr. Alan J. Perlis

I argue that the same is true for hard-coded special cases.

In TestGroupCleanupUserNamespace, instead of a curated list of strings
observed in the wild we now check for a prefix, as was done for
TestGroupCleanup in CL 24670.

Updates #52088.
Fixes #52149.

Change-Id: I59c5b0c048113e306996c0f8247e09c714d2423a
Reviewed-on: https://go-review.googlesource.com/c/go/+/397316
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 434b2a5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/398234
Reviewed-by: Russ Cox <rsc@golang.org>
gopherbot pushed a commit that referenced this issue May 4, 2022
@dmitshur
[release-branch.go1.17] syscall: relax output check in TestGroupClean…
2150be1 
…upUserNamespace

“If you have a procedure with ten parameters, you probably missed some.”
― attr. Alan J. Perlis

I argue that the same is true for hard-coded special cases.

In TestGroupCleanupUserNamespace, instead of a curated list of strings
observed in the wild we now check for a prefix, as was done for
TestGroupCleanup in CL 24670.

Updates #52088.
Fixes #52148.

Change-Id: I59c5b0c048113e306996c0f8247e09c714d2423a
Reviewed-on: https://go-review.googlesource.com/c/go/+/397316
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 434b2a5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/398235
Reviewed-by: Ian Lance Taylor <iant@google.com>
@rsc rsc unassigned bcmills Jun 22, 2022
@golang golang locked and limited conversation to collaborators Jun 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
4 participants
@dmitshur @bcmills @gopherbot @jonathan-albrecht-ibm