bmahler commented on code in PR #538:
URL: https://github.com/apache/mesos/pull/538#discussion_r1543341425
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
+
+ pid_t pid = ::fork();
+ ASSERT_NE(-1, pid);
+
+ if (pid == 0) {
+ // Check that we can only do the "allowedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_ERROR(os::open(access.first, access.second));
+ }
+
+ ASSERT_SOME(ebpf::cgroups2::detach(path, attached->at(0)));
+
+ // Check that we can do both the "allowedAccesses" and "blockedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+
+ // Wait for kill signal.
+ while (true) { sleep(1); }
+
+ SAFE_EXIT(
+ EXIT_FAILURE, "Error, child should be killed before reaching here");
+ }
+
+ // Kill the child process.
+ ASSERT_NE(-1, ::kill(pid, SIGKILL));
+
+ AWAIT_EXPECT_WTERMSIG_EQ(SIGKILL, process::reap(pid));
+}
+
+
+INSTANTIATE_TEST_CASE_P(
+ DeviceControllerTestParams,
+ DeviceControllerTestFixture,
+ ::testing::Values<DeviceControllerTestParams>(
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{*devices::Entry::parse("c *:* rwm")},
+ vector<Access>{},
+ vector<Access>{{os::DEV_NULL, O_RDWR}}
+ },
+ DeviceControllerTestParams{
+ // allow /dev/null
+ vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
+ vector<devices::Entry>{},
+ // read-write allowed
+ vector<Access>{{os::DEV_NULL, O_RDWR}},
+ vector<Access>{}
+ },
+ DeviceControllerTestParams{
+ // allow /dev/null
+ vector<devices::Entry>{*devices::Entry::parse("c 1:3 r")},
+ vector<devices::Entry>{},
+ // read-only allowed
+ vector<Access>{{os::DEV_NULL, O_RDONLY}},
+ // read-write is blocked
+ vector<Access>{{os::DEV_NULL, O_RDWR}}
+ },
+ DeviceControllerTestParams{
+ vector<devices::Entry>{*devices::Entry::parse("a 1:3 w")},
+ vector<devices::Entry>{},
+ // write-only allowed
+ vector<Access>{{os::DEV_NULL, O_WRONLY}},
+ // read is blocked
+ vector<Access>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ },
+ DeviceControllerTestParams{
+ vector<devices::Entry>{*devices::Entry::parse("b 1:3 r")},
+ vector<devices::Entry>{},
+ vector<Access>{},
+ // /dev/null is blocked
+ vector<Access>{{os::DEV_NULL, O_RDWR}, {os::DEV_NULL, O_RDONLY}}
+ }
Review Comment:
Is this what we're testing?
```
// Allow access to /dev/null but with the wrong device type, therefore
access is denied.
```
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
+
+ pid_t pid = ::fork();
+ ASSERT_NE(-1, pid);
+
+ if (pid == 0) {
+ // Check that we can only do the "allowedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_ERROR(os::open(access.first, access.second));
+ }
+
+ ASSERT_SOME(ebpf::cgroups2::detach(path, attached->at(0)));
+
+ // Check that we can do both the "allowedAccesses" and "blockedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+
+ // Wait for kill signal.
+ while (true) { sleep(1); }
+
+ SAFE_EXIT(
+ EXIT_FAILURE, "Error, child should be killed before reaching here");
+ }
+
+ // Kill the child process.
+ ASSERT_NE(-1, ::kill(pid, SIGKILL));
+
+ AWAIT_EXPECT_WTERMSIG_EQ(SIGKILL, process::reap(pid));
+}
+
+
+INSTANTIATE_TEST_CASE_P(
+ DeviceControllerTestParams,
+ DeviceControllerTestFixture,
+ ::testing::Values<DeviceControllerTestParams>(
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{*devices::Entry::parse("c *:* rwm")},
+ vector<Access>{},
+ vector<Access>{{os::DEV_NULL, O_RDWR}}
+ },
+ DeviceControllerTestParams{
+ // allow /dev/null
+ vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
+ vector<devices::Entry>{},
+ // read-write allowed
+ vector<Access>{{os::DEV_NULL, O_RDWR}},
+ vector<Access>{}
+ },
+ DeviceControllerTestParams{
+ // allow /dev/null
+ vector<devices::Entry>{*devices::Entry::parse("c 1:3 r")},
+ vector<devices::Entry>{},
+ // read-only allowed
+ vector<Access>{{os::DEV_NULL, O_RDONLY}},
+ // read-write is blocked
+ vector<Access>{{os::DEV_NULL, O_RDWR}}
+ },
+ DeviceControllerTestParams{
Review Comment:
what's this one? This?
```
// Allow write-only access to /dev/null using any device type.
```
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
Review Comment:
needs to be an ASSERT_SOME or the next line can crash the code dereferencing
the Try
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
Review Comment:
hm.. it's a little weird to define another Access here when there's already
a devices::Entry::Access we could be using, maybe something like this instead?
```
// Combination of a path and access flags for open()ing the device.
typedef pair<string, int> OpenTest;
using DeviceControllerTestParams = tuple<
vector<devices::Entry>,
vector<devices::Entry>,
vector<OpenTest>, // Opens that should be allowed.
vector<OpenTest>>; // Opens that should be denied.
```
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
Review Comment:
these can be refs, since the rhs is not an rvalue reference this pattern
definitely copies the vectors, we take the references off when the rhs returns
an rvalue reference
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
Review Comment:
This is an unnecessary guard? The fixture is supposed to ensure we enter
with a clean slate?
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
+
+ pid_t pid = ::fork();
+ ASSERT_NE(-1, pid);
+
+ if (pid == 0) {
+ // Check that we can only do the "allowedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_ERROR(os::open(access.first, access.second));
+ }
+
+ ASSERT_SOME(ebpf::cgroups2::detach(path, attached->at(0)));
+
+ // Check that we can do both the "allowedAccesses" and "blockedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+
+ // Wait for kill signal.
+ while (true) { sleep(1); }
+
+ SAFE_EXIT(
+ EXIT_FAILURE, "Error, child should be killed before reaching here");
+ }
+
+ // Kill the child process.
+ ASSERT_NE(-1, ::kill(pid, SIGKILL));
+
+ AWAIT_EXPECT_WTERMSIG_EQ(SIGKILL, process::reap(pid));
+}
+
+
+INSTANTIATE_TEST_CASE_P(
+ DeviceControllerTestParams,
+ DeviceControllerTestFixture,
+ ::testing::Values<DeviceControllerTestParams>(
+ DeviceControllerTestParams{
+ vector<devices::Entry>{},
+ vector<devices::Entry>{*devices::Entry::parse("c *:* rwm")},
+ vector<Access>{},
+ vector<Access>{{os::DEV_NULL, O_RDWR}}
+ },
+ DeviceControllerTestParams{
+ // allow /dev/null
+ vector<devices::Entry>{*devices::Entry::parse("c 1:3 rwm")},
+ vector<devices::Entry>{},
+ // read-write allowed
+ vector<Access>{{os::DEV_NULL, O_RDWR}},
Review Comment:
just fyi there's several lines here where your editor left in trailing
whitespace, doesn't highlight red on github like it does on reviewboard
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
Review Comment:
Ditto here, we don't want to do attached->at(0) if this fails because it's
empty?
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
+
+ pid_t pid = ::fork();
+ ASSERT_NE(-1, pid);
Review Comment:
ideally we'd do something that's errno aware here
##########
src/tests/containerizer/cgroups2_tests.cpp:
##########
@@ -210,6 +221,119 @@ TEST_F(Cgroups2Test, ROOT_CGROUPS2_EnableAndDisable)
EXPECT_EQ(0u, enabled->count("cpu"));
}
+// Combination of a path and access flags.
+typedef pair<string, int> Access;
+
+using DeviceControllerTestParams = tuple<
+ vector<devices::Entry>,
+ vector<devices::Entry>,
+ vector<Access>,
+ vector<Access>>;
+
+class DeviceControllerTestFixture :
+ public Cgroups2Test,
+ public ::testing::WithParamInterface<DeviceControllerTestParams> {};
+
+
+TEST_P(DeviceControllerTestFixture, ROOT_CGROUPS2_DeviceController) {
+ const string& cgroup = TEST_CGROUP;
+
+ auto params = GetParam();
+ const vector<devices::Entry> allow = std::get<0>(params);
+ const vector<devices::Entry> deny = std::get<1>(params);
+ const vector<Access> allowedAccesses = std::get<2>(params);
+ const vector<Access> blockedAccesses = std::get<3>(params);
+
+ if (!cgroups2::exists(cgroup)) {
+ ASSERT_SOME(cgroups2::create(cgroup));
+ }
+ string path = cgroups2::path(cgroup);
+
+ ASSERT_SOME(devices::configure(cgroup, allow, deny));
+ Try<vector<uint32_t>> attached = ebpf::cgroups2::attached(path);
+ EXPECT_SOME(attached);
+ EXPECT_EQ(1u, attached->size());
+
+ pid_t pid = ::fork();
+ ASSERT_NE(-1, pid);
+
+ if (pid == 0) {
+ // Check that we can only do the "allowedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_ERROR(os::open(access.first, access.second));
+ }
+
+ ASSERT_SOME(ebpf::cgroups2::detach(path, attached->at(0)));
+
+ // Check that we can do both the "allowedAccesses" and "blockedAccesses".
+ foreach(const Access& access, allowedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
+ foreach(const Access& access, blockedAccesses) {
+ ASSERT_SOME(os::open(access.first, access.second));
+ }
Review Comment:
We can't use ASSERT in the child process, as they won't fail the test. Looks
like the strategy in cgroups_tests.cpp is to use a pipe and communicate back to
the parent process to indicate whether the child process' tests succeeded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
