libct/specconf: CreateDevices nits#5255
Open
kolyshkin wants to merge 3 commits intoopencontainers:mainfrom
Open
libct/specconf: CreateDevices nits#5255kolyshkin wants to merge 3 commits intoopencontainers:mainfrom
kolyshkin wants to merge 3 commits intoopencontainers:mainfrom
Conversation
Contributor
Author
|
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 <kolyshkin@gmail.com>
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 <kolyshkin@gmail.com>
The usage of device.Rule is not very obvious, so explain it. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).