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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

moz-sec
Copy link

@moz-sec moz-sec commented Nov 12, 2024

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.

@moz-sec moz-sec force-pushed the add-test-relative-network branch from 4696ba0 to b332eb0 Compare November 12, 2024 06:34
@moz-sec 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 YJDoc2 self-requested a review November 12, 2024 13:02
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 12, 2024

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

Comment on lines 62 to 67
test_outside_container(spec, &|data| {
test_result!(check_container_created(&data));
TestResult::Passed
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

  1. 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.
  2. 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.
  3. 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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

@moz-sec moz-sec requested a review from YJDoc2 November 25, 2024 08:41
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 63 to 72
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Unnecessary programs have been removed.
ca7ab449

Comment on lines 77 to 79
let cgroup_path = PathBuf::from(CGROUP_ROOT)
.join("net_cls,net_prio/runtime-test")
.join(cgroup_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

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

moz-sec commented Dec 16, 2024

@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 moz-sec requested a review from YJDoc2 December 23, 2024 08:03
@moz-sec
Copy link
Author

moz-sec commented Dec 23, 2024

@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
Copy link
Collaborator

YJDoc2 commented Dec 26, 2024

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
Copy link
Member

utam0k commented Dec 26, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants