libct/specconf: CreateDevices nits - #5255
Conversation
|
Alternative view: the second commit actually reveals a (subtle, corner case) bug and it needs to be fixed. |
Commit 0709202 added TestCreateDevices which, among the other things, checks that defaultDevs, as returned by createDevices, does not contain a *second* entry for /dev/tty in case user-supplied rules also have an entry to /dev/tty. Since defaultDevs is a subset of AllowedDevices, having two entries is not possible at all, making this check useless. What it should probably check is that defaultDevs does not contain an entry for /dev/tty (as the user-supplied one prevails). Fix the test appropriately. While at it, do some minor cleanup. The alternative to this is to remove this check entirely. Fixes: 0709202 Signed-off-by: Kir Kolyshkin <[email protected]>
Since commit 0709202 ("Remove runc default devices that overlap with spec devices.") runc removes the default cgroup device access rule from the default set in case a device with the same path is also listed in container spec. Judging by the commit description, this was not the intention, and yet this is what we have. As the behavior is now part of runc (since v1.0-rc93), it makes sense to at least test it, to ensure it won't be broken in the future. Note that the above behavior is only for rootful runc (rootless does not use cgroup device access rules and bind-mounts host devices instead). In addition, the test case serves as a demo how to limit the container device access to a subset of default AllowedDevices, which we thought was not possible before. Turns out it is possible, in a way (and for rootful runc only). Signed-off-by: Kir Kolyshkin <[email protected]>
The usage of device.Rule is not very obvious, so explain it. Signed-off-by: Kir Kolyshkin <[email protected]>
| for _, configDev := range conf.Devices { | ||
| if configDev.Path == "/dev/tty" { | ||
| if configDev.Path == ttyPath { | ||
| wantDev := &devices.Device{ |
There was a problem hiding this comment.
nit: looks like we don't need this to be a pointer?
There was a problem hiding this comment.
I think it was there for symmetry -- so the comparison below is *configDev != *wantDev not *configDev != wantDev. Surely this can be changed but I don't think it's worth increasing the diff size.
| FileMode: 0o666, | ||
| Uid: 1000, | ||
| Gid: 1000, | ||
| Path: ttyPath, |
There was a problem hiding this comment.
TBH; I slightly liked the fixed values more; it prevents having to look up what's checked here; also having fixed values slightly reduces risk of a test continue passing if things are changed.
That last bit is perhaps the intent, so a bit of a pro/con (having fixed values means changing adds more friction, making it a deliberate choice)
There was a problem hiding this comment.
Are you talking about Path/ttyPath or about all of those (including FileMode, Uid, Gid)?
For ttyPath, I just did not like repeated "/dev/tty" everywhere in this test.
For Uid and Gid, we set this to 1000 above and check that it's still 1000 here. Makes sense to use a constant (which is a variable here as we have to take a pointer to it, and new to get a pointer to a const only appeared in Go 1.26 so we can't use it here yet).
So, I am not with you on this. The variables here are just to remove repetition.
| ttyGid := uint32(1000) | ||
| fm := os.FileMode(0o666) | ||
| ttyMode := os.FileMode(0o666) | ||
| ttyPath := "/dev/tty" |
There was a problem hiding this comment.
nit: added const here while we're at it.
2 participants
A couple of followups to PR #2522.
libct/specconv: fixup TestCreateDevices
Commit 0709202 added
TestCreateDeviceswhich, among the other things,checks that
defaultDevs, as returned bycreateDevices, does not containa second entry for /dev/tty, in case user-supplied rules also have an
entry to /dev/tty.
Since
defaultDevsis a subset ofAllowedDevices, having two entriesis not possible at all, making this check useless.
What it should probably check is that
defaultDevsdoes not contain anentry for /dev/tty (as the user-supplied one prevails).
Fix the test appropriately. While at it, do some minor code cleanup.
The alternative to this is to remove this check entirely.
tests/int: demo default device access rule removal
Since commit 0709202 ("Remove runc default devices that overlap with
spec devices.") runc removes the default cgroup access rule for a device
in case a device with the same path is listed in container spec.
Judging by the commit description, this was not the intention, and yet
this is what we have.
As the behavior is now part of runc (since v1.0-rc93), it makes sense
to at least test it, to ensure it won't be broken in the future.
Note that the above behavior is only for rootful runc (rootless does not
use cgroup device access rules and bind-mounts host devices instead).
In addition, the test case serves as a demo how to limit the container
device access to a subset of default AllowedDevices, which we thought
was not possible before. Turns out it is possible, in a way (and for
rootful runc only).