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

libct/int: add device update test #3000

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

kolyshkin
Copy link
Contributor

... and remove the one from tests/integration.

The idea is similar to the one for the test case being removed -- try
updating device rules many times to make sure we are not leaking eBPF
programs after every update/Set(). This is better though as we can
really change the device rules every time (which "runc update" can't)
and check that the rule is applied.

The test works, but I see the following warning (on Fedora 34, cgroup v2):

WARN[0000] found more than one filter (2) attached to a cgroup -- removing extra filters! 

@kolyshkin kolyshkin requested a review from cyphar June 4, 2021 22:16
@kolyshkin
Copy link
Contributor Author

Same in Fedora CI:

=== RUN   TestUpdateDevices
time="2021-06-04T22:28:40Z" level=warning msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
time="2021-06-04T22:28:40Z" level=warning msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
...
time="2021-06-04T22:28:54Z" level=warning msg="found more than one filter (2) attached to a cgroup -- removing extra filters!"
--- PASS: TestUpdateDevices (14.49s)
PASS

I changed the test like this:

diff --git a/libcontainer/integration/update_test.go b/libcontainer/integration/update_test.go
index 4a83ed41..0ec1dca1 100644
--- a/libcontainer/integration/update_test.go
+++ b/libcontainer/integration/update_test.go
@@ -69,14 +69,18 @@ func TestUpdateDevices(t *testing.T) {
 
                // Now flip the access permission
                isAllowed = !isAllowed
-               config.Cgroups.Resources.Devices = []*devices.Rule{
-                       {
-                               Type:        devices.CharDevice,
-                               Major:       1,
-                               Minor:       7,
-                               Permissions: "rwm",
-                               Allow:       isAllowed,
-                       },
+               if isAllowed {
+                       config.Cgroups.Resources.Devices = []*devices.Rule{
+                               {
+                                       Type:        devices.CharDevice,
+                                       Major:       1,
+                                       Minor:       7,
+                                       Permissions: "rwm",
+                                       Allow:       true,
+                               },
+                       }
+               } else {
+                       config.Cgroups.Resources.Devices = []*devices.Rule{}
                }
                if err := container.Set(*config); err != nil {
                        t.Fatal(err)

and it's still emitting the same warning.

@kolyshkin
Copy link
Contributor Author

Any way I modify the devices list, the warning is here.

Modified the test to

  • add systemd
  • add a second check (for /dev/null)
  • instead of grant/remove access to just one device, alternate between default and empty lists

@kolyshkin
Copy link
Contributor Author

So I have tried the following ways of modifying device access:

  1. Start from the default list, then use the list with a single /dev/full entry, alternating between Allow: true and Allow: false.
  2. Start from the default list, then alternate between the empty list and the list with a single /dev/full entry.
  3. Start from the default list, then alternate between empty and the default list.

The only method I haven't tried yet is modifying device permissions (say remove/re-add w from /dev/full`).

@cyphar

This comment has been minimized.

@kolyshkin
Copy link
Contributor Author

I think it's easier for you to cherry-pick this test to PR #2986 (or just merge this and then expand this test to cover more cases).

... and remove the one from tests/integration.

The idea is similar to the one for the test case being removed -- try
updating device rules many times to make sure we are not leaking eBPF
programs after every update/Set(). This is better though as we can
really change the device rules every time (which "runc update" can't)
and check that the rule is applied.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin modified the milestones: post-1.0, 1.0.0 Jun 8, 2021
@kolyshkin
Copy link
Contributor Author

@cyphar let me know if you're OK to merge this as is, or is this test totally useless, or there is something else I should address.

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is a pretty good smoke test and actually exercises the "update" aspect as well, so device rule updates can't be a no-op either.

@cyphar cyphar requested review from AkihiroSuda and a team June 11, 2021 06:37
@mrunalp mrunalp merged commit a3813ca into opencontainers:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants