This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
The following commit(s) were added to refs/heads/master by this push:
new 13000010e [devices] Add CgroupDeviceAccess::create helper which checks
normalization.
13000010e is described below
commit 13000010e07bf6e2cad5c04cf58546f18bb66e3a
Author: Jason Zhou <[email protected]>
AuthorDate: Fri Jul 26 18:19:25 2024 -0400
[devices] Add CgroupDeviceAccess::create helper which checks normalization.
Currently, CgroupDeviceAccess instances can be directly constructed
without verifying that its allow and deny lists are normalized.
To codify our normalization constraints, CgroupDeviceAccess can now only
be created with a create() helper.
Review: https://reviews.apache.org/r/75116/
---
.../device_manager/device_manager.cpp | 48 +++++++++++++---
.../device_manager/device_manager.hpp | 10 ++++
src/tests/device_manager_tests.cpp | 67 ++++++++++++++--------
3 files changed, 93 insertions(+), 32 deletions(-)
diff --git a/src/slave/containerizer/device_manager/device_manager.cpp
b/src/slave/containerizer/device_manager/device_manager.cpp
index c25af0adc..9c3e6c00c 100644
--- a/src/slave/containerizer/device_manager/device_manager.cpp
+++ b/src/slave/containerizer/device_manager/device_manager.cpp
@@ -99,8 +99,13 @@ public:
}
}
- device_access_per_cgroup[cgroup].allow_list = allow_list;
- device_access_per_cgroup[cgroup].deny_list = deny_list;
+ auto result = device_access_per_cgroup.emplace(
+ cgroup,
+ CHECK_NOTERROR(
+ DeviceManager::CgroupDeviceAccess::create(allow_list, deny_list)));
+ if (!result.second) {
+ return Failure("cgroup entry already exists");
+ }
Try<Nothing> commit = commit_device_access_changes(cgroup);
if (commit.isError()) {
@@ -131,10 +136,19 @@ public:
}
}
- device_access_per_cgroup[cgroup] = DeviceManager::apply_diff(
- device_access_per_cgroup[cgroup],
- non_wildcard_additions,
- non_wildcard_removals);
+ auto it = device_access_per_cgroup.find(cgroup);
+ if (it != device_access_per_cgroup.end()) {
+ it->second = DeviceManager::apply_diff(
+ it->second, non_wildcard_additions, non_wildcard_removals);
+ } else {
+ auto result = device_access_per_cgroup.emplace(
+ cgroup,
+ DeviceManager::apply_diff(
+ CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {})),
+ non_wildcard_additions,
+ non_wildcard_removals));
+ CHECK(result.second);
+ }
Try<Nothing> commit = commit_device_access_changes(cgroup);
if (commit.isError()) {
@@ -156,7 +170,7 @@ public:
{
return device_access_per_cgroup.contains(cgroup)
? device_access_per_cgroup.at(cgroup)
- : DeviceManager::CgroupDeviceAccess();
+ : CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create({}, {}));
}
private:
@@ -366,6 +380,26 @@ bool DeviceManager::CgroupDeviceAccess::is_access_granted(
return allowed() && !denied();
}
+
+DeviceManager::CgroupDeviceAccess::CgroupDeviceAccess(
+ const std::vector<cgroups::devices::Entry>& _allow_list,
+ const std::vector<cgroups::devices::Entry>& _deny_list)
+ : allow_list(_allow_list), deny_list(_deny_list) {}
+
+
+Try<DeviceManager::CgroupDeviceAccess>
+DeviceManager::CgroupDeviceAccess::create(
+ const vector<Entry>& allow_list,
+ const vector<Entry>& deny_list)
+{
+ if (!cgroups2::devices::normalized(allow_list)
+ || !(cgroups2::devices::normalized(deny_list))) {
+ return Error("Failed to create CgroupDeviceAccess:"
+ " The allow or deny list is not normalized");
+ }
+ return CgroupDeviceAccess(allow_list, deny_list);
+}
+
} // namespace slave {
} // namespace internal {
} // namespace mesos {
diff --git a/src/slave/containerizer/device_manager/device_manager.hpp
b/src/slave/containerizer/device_manager/device_manager.hpp
index c0ffcc20f..6d5a7693b 100644
--- a/src/slave/containerizer/device_manager/device_manager.hpp
+++ b/src/slave/containerizer/device_manager/device_manager.hpp
@@ -72,6 +72,16 @@ public:
// A device access is granted if it is encompassed by an allow entry
// and does not have access overlaps with any deny entry.
bool is_access_granted(const cgroups::devices::Entry& entry) const;
+
+ // Returns an error if it the allow or deny lists are not normalized.
+ static Try<CgroupDeviceAccess> create(
+ const std::vector<cgroups::devices::Entry>& allow_list,
+ const std::vector<cgroups::devices::Entry>& deny_list);
+
+ private:
+ CgroupDeviceAccess(
+ const std::vector<cgroups::devices::Entry>& allow_list,
+ const std::vector<cgroups::devices::Entry>& deny_list);
};
static Try<DeviceManager*> create(const Flags& flags);
diff --git a/src/tests/device_manager_tests.cpp
b/src/tests/device_manager_tests.cpp
index da1ef64fe..aebf9014c 100644
--- a/src/tests/device_manager_tests.cpp
+++ b/src/tests/device_manager_tests.cpp
@@ -410,49 +410,66 @@ INSTANTIATE_TEST_CASE_P(
TEST(DeviceManagerCgroupDeviceAccessTest, IsAccessGrantedTest)
{
- DeviceManager::CgroupDeviceAccess cgroup_device_access;
-
- // Character devices with major minor numbers 1:3 can do write only:
- cgroup_device_access.allow_list = {*devices::Entry::parse("c 1:3 w")};
+ // Character devices with major minor numbrs 1:3 can do write only:
+ DeviceManager::CgroupDeviceAccess cgroup_device_access =
+ CHECK_NOTERROR(DeviceManager::CgroupDeviceAccess::create(
+ {*devices::Entry::parse("c 1:3 w")}, {}
+ ));
EXPECT_TRUE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))
+ );
EXPECT_FALSE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw"))
+ );
EXPECT_FALSE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("b 1:3 w")));
-
- // Character devices with minor number 3 can do write only:
- cgroup_device_access.allow_list = {*devices::Entry::parse("c *:3 w")};
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("b 1:3 w"))
+ );
EXPECT_TRUE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 4:3 w")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 4:3 w"))
+ );
EXPECT_TRUE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c *:3 w")));
-
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c *:3 w"))
+ );
// Character devices with major number 5 can do write only:
- cgroup_device_access.allow_list = {(*devices::Entry::parse("c 5:* w"))};
+ cgroup_device_access = CHECK_NOTERROR(
+ DeviceManager::CgroupDeviceAccess::create(
+ {(*devices::Entry::parse("c 5:* w"))}, {}
+ ));
EXPECT_TRUE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:* w")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:* w"))
+ );
EXPECT_TRUE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:2 w")));
-
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 5:2 w"))
+ );
// All devices will match the catch-all and can perform all operations:
- cgroup_device_access.allow_list = {*devices::Entry::parse("a *:* rwm")};
+ cgroup_device_access = CHECK_NOTERROR(
+ DeviceManager::CgroupDeviceAccess::create(
+ {*devices::Entry::parse("a *:* rwm")}, {}
+ ));
+
EXPECT_TRUE(
cgroup_device_access.is_access_granted(*devices::Entry::parse("c 6:2 w")));
-
// Deny all accesses to character device with major and numbers 1:3.
- cgroup_device_access.deny_list = {*devices::Entry::parse("c 1:3 rwm")};
+ cgroup_device_access = CHECK_NOTERROR(
+ DeviceManager::CgroupDeviceAccess::create(
+ {}, {*devices::Entry::parse("c 1:3 rwm")}
+ ));
EXPECT_FALSE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3 w"))
+ );
EXPECT_FALSE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw"))
+ );
// Entry should be denied if encompassed by an entry in allow_list and
// overlaps with entry in deny_list.
- cgroup_device_access.allow_list = {*devices::Entry::parse("c 1:3 rw")};
- cgroup_device_access.deny_list = {*devices::Entry::parse("c 1:3 w")};
+ cgroup_device_access = CHECK_NOTERROR(
+ DeviceManager::CgroupDeviceAccess::create(
+ {*devices::Entry::parse("c 1:3 rw")}, {*devices::Entry::parse("c 1:3 w")}
+ ));
EXPECT_FALSE(
- cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw")));
+ cgroup_device_access.is_access_granted(*devices::Entry::parse("c 1:3
rw"))
+ );
}
} // namespace tests {
