Add a relative_network_cgroups test as one of the integration tests #2986

moz-sec · 2024-11-12T05:56:31Z

This implements the relative_network_cgroups validation in #361 .
I wrote it based on linux_cgroups_relative_network.go from opencontainers/runtime-tools and tests/cgroups/network.rs from youki-dev/youki.

YJDoc2 · 2024-11-12T13:04:01Z

Hey, thanks for the PR :)
I'll need some time to get to this, will comment once the review is done.

YJDoc2 · 2024-11-25T05:02:06Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+    test_outside_container(spec, &|data| {
+        test_result!(check_container_created(&data));
+        TestResult::Passed
+    })


Hey, here along with checking if the container is created, we also need validation for the created network cgroup resources - In the original test we call this function which does the validation, so need that here as well.

Thank you.
Added validation for the created network cgroup resources.

Hey, I don't think it is fixed yet. Let me clarify in case there is any confusion -

In the original go test, at line https://github.com/opencontainers/runtime-tools/blob/master/validation/linux_cgroups_relative_network/linux_cgroups_relative_network.go#L24C1-L24C77, in the test_outside_container , they are passing util.ValidateLinuxResourcesNetwork function, which will do the validation that ok, the runtime has actually setup the relative network correctly.

The util.ValidateLinuxResourcesNetwork function defined at https://github.com/opencontainers/runtime-tools/blob/master/validation/util/linux_resources_network.go#L12 does the checking and validation of relative network cgroup.

The change you did in the last commit you pushed is actually almost a no-op. The original way of just calling the test_outside_container was correct, but also needs the cgroup checking logic as mentioned above.

I added a new validate_network() and changed the program to validate net_cls.classid and net_prio.ifpriomap.
However, network.rs only verifies check_container_created(&data).
Am I understanding this wrong?

YJDoc2

validation of this test using runc is failing, can you check?
If I'm correct, this is applicable only to cgroups v1, right?

YJDoc2 · 2024-12-13T15:15:46Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+    let test_result = test_outside_container(spec.clone(), &|data| {
+        test_result!(check_container_created(&data));
+        test_result!(validate_network(cgroup_name, &spec));
+        TestResult::Passed
+    });
+    if let TestResult::Failed(_) = test_result {
+        return test_result;
+    }
+
+    TestResult::Passed


You can just return the test_outside_container value like this -

Suggested change

let test_result = test_outside_container(spec.clone(), &|data| {

test_result!(check_container_created(&data));

test_result!(validate_network(cgroup_name, &spec));

TestResult::Passed

});

if let TestResult::Failed(_) = test_result {

return test_result;

}

TestResult::Passed

test_outside_container(spec.clone(), &|data| {

test_result!(check_container_created(&data));

test_result!(validate_network(cgroup_name, &spec));

TestResult::Passed

})

The if let part here is basically a no-op

Unnecessary programs have been removed.
ca7ab449

YJDoc2 · 2024-12-13T15:27:44Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+    let cgroup_path = PathBuf::from(CGROUP_ROOT)
+        .join("net_cls,net_prio/runtime-test")
+        .join(cgroup_name);


Is it ok to hard-code this here? I think in the can_run below you mention the cgroup can be at a couple of different points.

Address multiple network cgroup mount points.
Changed where to get paths for net_cls.classid and net_prio.ifpriomap.
ca7ab449

Signed-off-by: moz-sec <m0253c@gmail.com>

….ifpriomap Signed-off-by: moz-sec <m0253c@gmail.com>

Signed-off-by: moz-sec <m0253c@gmail.com>

moz-sec · 2024-12-16T08:14:54Z

@YJDoc2
Thanks for the review.
I modified the program and verified that test_linux_cgroups_network is ok with just validate-contest-runc.
Could you please check the code?

moz-sec · 2024-12-23T09:26:50Z

@YJDoc2
I just talked with @utam0k offline, and we discussed whether it would be better to write the equivalent of ValidateLinuxResourcesNetwork in runtime-tools in network.rs and call that method in this relative-network.rs.
What do you think about this?

YJDoc2 · 2024-12-26T04:28:39Z

Hey @moz-sec , that makes sense. I have been busy for some time, and wasn't able to take more detailed look after your changes, apologies. Please go ahead with the implementation as discussed with utam0k. Do you plan to do it in this PR only, or a separate PR (I'm fine with either). Thanks :)

utam0k · 2024-12-26T23:45:02Z

@YJDoc2
I have a room to review this PR. If you don't mind, please assign me instead of you.

Signed-off-by: moz-sec <m0253c@gmail.com>

moz-sec · 2025-01-20T14:13:20Z

@YJDoc2
I have proceeded with this PR.
Add validate_network, the equivalent of ValidateLinuxResourcesNetwork in runtime-tools, to network.rs and call it from relative-network.rs as well.
Please review when you have time.

YJDoc2 · 2025-01-22T14:09:00Z

Hey @moz-sec , apologies I haven't been able to take a look at this. I had been quite busy, and then got a bit sick, and currently just have a lot of things I need to work on 😅 I'll try my best to take a look at this as soon as possible.

Sincere apologies, I should have done better managing this 🙏

@utam0k if I may ask of you, it would be great if you could help with reviewing this PR. Otherwise I'll get to this as soon as I can.

utam0k · 2025-01-27T12:07:39Z

@utam0k if I may ask of you, it would be great if you could help with reviewing this PR. Otherwise I'll get to this as soon as I can.

Sure!

utam0k

Thanks for your PR. I left some introductory comments. I'll return this week to review more details.

utam0k · 2025-01-27T12:19:34Z

tests/contest/contest/src/tests/cgroups/network.rs

+    let (net_cls_path, net_prio_path) = if Path::new("/sys/fs/cgroup/net_cls/net_cls.classid")
+        .exists()
+        && Path::new("/sys/fs/cgroup/net_prio/net_prio.ifpriomap").exists()
+    {
+        (
+            net_cls_path(PathBuf::from(CGROUP_ROOT).join("net_cls"), cgroup_name),
+            net_prio_path(PathBuf::from(CGROUP_ROOT).join("net_prio"), cgroup_name),
+        )
+    } else if Path::new("/sys/fs/cgroup/net_cls,net_prio/net_cls.classid").exists()
+        && Path::new("/sys/fs/cgroup/net_cls,net_prio/net_prio.ifpriomap").exists()
+    {
+        (
+            net_cls_path(
+                PathBuf::from(CGROUP_ROOT).join("net_cls,net_prio"),
+                cgroup_name,
+            ),
+            net_prio_path(
+                PathBuf::from(CGROUP_ROOT).join("net_cls,net_prio"),
+                cgroup_name,
+            ),
+        )
+    } else if Path::new("/sys/fs/cgroup/net_prio,net_cls/net_cls.classid").exists()
+        && Path::new("/sys/fs/cgroup/net_prio,net_cls/net_prio.ifpriomap").exists()
+    {
+        (
+            net_cls_path(
+                PathBuf::from(CGROUP_ROOT).join("net_prio,net_cls"),
+                cgroup_name,
+            ),
+            net_prio_path(
+                PathBuf::from(CGROUP_ROOT).join("net_prio,net_cls"),
+                cgroup_name,
+            ),
+        )
+    } else {
+        return Err(anyhow::anyhow!("Required cgroup paths do not exist"));
+    };


Please make each condition have a meaning, not only checking if the path exists.

Each conditional branch will set net_cls_path to /sys/fs/cgroup/net_cls/net_cls.classid/test_network_cgroups or /sys/fs/cgroup/net_cls,net_prio/net_cls.classid/test_network_cgroup or /sys/fs/cgroup/net_prio,net_cls/net_cls.classid/network_cgroups.
But looking at this alone, I too find this program confusing.
How can I write the same thing more neatly?

I think it would be simplest to just use sys/fs/cgroup/net_cls,net_prio/, but looking at #39, do we still need to identify which directory it is mounted in and process it accordingly?

utam0k · 2025-01-27T12:20:19Z

tests/contest/contest/src/tests/cgroups/network.rs

@@ -115,8 +116,12 @@ fn test_network_cgroups() -> TestResult {
    ];

    for spec in cases.into_iter() {
-        let test_result = test_outside_container(spec, &|data| {
+        let test_result = test_outside_container(spec.clone(), &|data| {


Why is this change needed?

In the case of spec, ownership is passed to test_outside_container and I used spec.clone() since I cannot use spec on line 123.

utam0k · 2025-01-27T12:23:39Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+    let cgroup_name = "test_relative_network_cgroups";
+
+    let id = 255;
+    let prio = 10;
+    let if_name = "lo";


Please use consts instead of normal values.

For example, is it correct to write “const CGROUP_NAME: &str = ‘test_relative_network_cgroups’;`”?

utam0k · 2025-01-27T12:24:19Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+
+// Gets the loopback interface if it exists
+fn get_loopback_interface() -> Option<String> {
+    let interfaces = interfaces();


I don't think this binding is needed.

utam0k · 2025-01-27T12:30:49Z

tests/contest/contest/src/tests/cgroups/relative_network.rs

+fn create_spec(cgroup_name: &str, class_id: u32, prio: u32, if_name: &str) -> Result<Spec> {
+    // Create the Linux Spec
+    let linux_spec = LinuxBuilder::default()
+        .cgroups_path(Path::new("testdir/runtime-test/container").join(cgroup_name))


testdir/runtime-test/container is duplicated with L66 in this file. How about passing cgroup path?

I think it is possible if I define it as a relative path (e.g. testdir/runtime-test/container/test_relative_network_cgroups) in the cgroup_name variable.
However, looking at other existing test code, the cgroup path consists of /runtime + cgroup_name variable, and I wrote this to match.
Is there a cleaner way to write this?

utam0k

Thanks for your PR. I left some introductory comments. I'll return this week to review more details.

utam0k

Thanks for your PR. I left some introductory comments. I'll return this week to review more details.

utam0k · 2025-01-27T12:34:18Z

For some reason, multiple copies were sent 😭

moz-sec force-pushed the add-test-relative-network branch from 4696ba0 to b332eb0 Compare November 12, 2024 06:34

moz-sec changed the title ~~test: add relative_network_cgroups test~~ Add a relative_network_cgroups test as one of the integration tests Nov 12, 2024

YJDoc2 self-requested a review November 12, 2024 13:02

YJDoc2 added the kind/test label Nov 12, 2024

YJDoc2 reviewed Nov 25, 2024

View reviewed changes

moz-sec requested a review from YJDoc2 November 25, 2024 08:41

YJDoc2 reviewed Dec 13, 2024

View reviewed changes

moz-sec added 4 commits December 16, 2024 16:15

test: add relative_network_cgroups test

d477ee7

Signed-off-by: moz-sec <m0253c@gmail.com>

test: validate network cgroup resources

4255ed9

Signed-off-by: moz-sec <m0253c@gmail.com>

test: add validate_network() to validate net_cls.classid and net_prio…

6280f34

….ifpriomap Signed-off-by: moz-sec <m0253c@gmail.com>

fix: support different mount points in network cgroup

ca7ab44

Signed-off-by: moz-sec <m0253c@gmail.com>

moz-sec force-pushed the add-test-relative-network branch from 2a8b217 to ca7ab44 Compare December 16, 2024 07:16

moz-sec requested a review from YJDoc2 December 23, 2024 08:03

moz-sec added 2 commits January 20, 2025 13:11

test: add checks for classid and net_prio to network.rs

d87abac

Signed-off-by: moz-sec <m0253c@gmail.com>

test: call validate_network() in network.rs from relative_netwrok.rs

be19fee

Signed-off-by: moz-sec <m0253c@gmail.com>

utam0k self-requested a review January 27, 2025 12:07

utam0k approved these changes Jan 27, 2025

View reviewed changes

utam0k requested changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a relative_network_cgroups test as one of the integration tests #2986

Add a relative_network_cgroups test as one of the integration tests #2986

moz-sec commented Nov 12, 2024

YJDoc2 commented Nov 12, 2024

YJDoc2 Nov 25, 2024

moz-sec Nov 25, 2024

YJDoc2 Nov 25, 2024

moz-sec Dec 2, 2024

YJDoc2 left a comment

YJDoc2 Dec 13, 2024

moz-sec Dec 16, 2024

YJDoc2 Dec 13, 2024

moz-sec Dec 16, 2024

moz-sec commented Dec 16, 2024

moz-sec commented Dec 23, 2024

YJDoc2 commented Dec 26, 2024

utam0k commented Dec 26, 2024

moz-sec commented Jan 20, 2025

YJDoc2 commented Jan 22, 2025

utam0k commented Jan 27, 2025

utam0k left a comment

utam0k Jan 27, 2025

moz-sec Feb 1, 2025

moz-sec Feb 1, 2025

utam0k Jan 27, 2025

moz-sec Feb 1, 2025

utam0k Jan 27, 2025

moz-sec Feb 1, 2025

utam0k Jan 27, 2025

utam0k Jan 27, 2025

moz-sec Feb 1, 2025

utam0k left a comment

utam0k left a comment

utam0k commented Jan 27, 2025

Add a relative_network_cgroups test as one of the integration tests #2986

Are you sure you want to change the base?

Add a relative_network_cgroups test as one of the integration tests #2986

Conversation

moz-sec commented Nov 12, 2024

YJDoc2 commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YJDoc2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moz-sec commented Dec 16, 2024

moz-sec commented Dec 23, 2024

YJDoc2 commented Dec 26, 2024

utam0k commented Dec 26, 2024

moz-sec commented Jan 20, 2025

YJDoc2 commented Jan 22, 2025

utam0k commented Jan 27, 2025

utam0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utam0k left a comment

Choose a reason for hiding this comment

utam0k left a comment

Choose a reason for hiding this comment

utam0k commented Jan 27, 2025