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

linux_mount_label integration test #2688

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Gekko0114
Copy link
Contributor

Created linux_mount_label integration test.

Referred this linux_mount_label test.
https://github.com/opencontainers/runtime-tools/tree/master/validation/linux_mount_label

#361

@Gekko0114 Gekko0114 force-pushed the integration/mount_label branch from 977ca3a to 7005b28 Compare February 18, 2024 08:29
@Gekko0114
Copy link
Contributor Author

Hi @utam0k,
Could you take a look?
Though I think I've implemented this PR same as the integration test of runtime-tools, got an error below on my codespace environment. I've tried to fix it, but couldn't find how to fix it

# Start group linux_mount_label
1 / 1 : linux_mount_label : not ok
	container stderr was not empty, found : �[31mERROR�[0m �[2mlibcontainer::process::container_init_process�[0m�[2m:�[0m failed to mount path as masked using tempfs �[3mpath�[0m�[2m=�[0m"/proc/acpi" �[3merr�[0m�[2m=�[0mNix(EINVAL)
�[31mERROR�[0m �[2mlibcontainer::process::container_init_process�[0m�[2m:�[0m failed to set masked path �[3merr�[0m�[2m=�[0mMountPathMasked(Nix(EINVAL)) �[3mpath�[0m�[2m=�[0m"/proc/acpi"
�[31mERROR�[0m �[2mlibcontainer::process::container_intermediate_process�[0m�[2m:�[0m failed to initialize container process: failed to mount path as masked
�[31mERROR�[0m �[2mlibcontainer::process::container_main_process�[0m�[2m:�[0m failed to wait for init ready: failed to receive. "waiting for init ready". BrokenChannel
�[31mERROR�[0m �[2mlibcontainer::container::builder_impl�[0m�[2m:�[0m failed to run container process �[3merr�[0m�[2m=�[0mChannel(ReceiveError { msg: "waiting for init ready", source: BrokenChannel })
�[31mERROR�[0m �[2myouki�[0m�[2m:�[0m error in executing command: failed to receive. "waiting for init ready". BrokenChannel

Caused by:
    channel connection broken
Error: failed to receive. "waiting for init ready". BrokenChannel

Caused by:
    channel connection broken

# End group linux_mount_label

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 19, 2024

@Gekko0114 It seems there is some issue with the /proc/acpi path due to which we are getting EINVAL error. Can you check this path, its perms and related stuff? Thanks!

@Gekko0114 Gekko0114 force-pushed the integration/mount_label branch from 7005b28 to bd11f7d Compare February 24, 2024 13:36
@utam0k
Copy link
Member

utam0k commented Feb 25, 2024

@Gekko0114 May I ask you to commit with your sign?
https://github.com/containers/youki/pull/2688/checks?check_run_id=21940370118

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.41%. Comparing base (919ff4a) to head (1d77cf7).
Report is 232 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2688   +/-   ##
=======================================
  Coverage   65.40%   65.41%           
=======================================
  Files         133      133           
  Lines       16942    16942           
=======================================
+ Hits        11081    11082    +1     
+ Misses       5861     5860    -1     

@Gekko0114 Gekko0114 force-pushed the integration/mount_label branch from 1940783 to f8a1610 Compare February 25, 2024 13:50
Signed-off-by: Hiroyuki Moriya <[email protected]>
@Gekko0114 Gekko0114 force-pushed the integration/mount_label branch from f8a1610 to 1d77cf7 Compare February 25, 2024 14:36
@Gekko0114
Copy link
Contributor Author

Hi @YJDoc2 @utam0k
Though I have some questions, I created a PR and passed a test case. Could you review it?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 26, 2024

Though I have some questions, I created a PR and passed a test case. Could you review it?

Hey, thanks! I'll take a look and answer the questions 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 26, 2024

Hey @utam0k , a couple of things :

  1. Currently our integration test validation CI is broken, opened Fix integration test validation CI, make io_priority test conditional #2707 for that. In that the test added in this PR is failing on Runc as well , see a run done in my branch : https://github.com/YJDoc2/youki/actions/runs/8050503893/job/21986255509
  2. I have some question regarding the MountLabel implementation itself : As per the oci spec, the MountLabel field marks the selinux label to be used for that mount. Thus in runc https://github.com/opencontainers/runc/blob/d5e4c33001d74176222fe8f48a323f3e8ad89999/libcontainer/rootfs_linux.go#L536 , they use the selinux package and set the label using that. However in youki, we are passing that from https://github.com/containers/youki/blob/main/crates/libcontainer/src/rootfs/rootfs.rs#L90 to the syscall implementation, which finally reaches https://github.com/containers/youki/blob/2681f9c63f2b70ef636c088222a92cccb9252995/crates/libcontainer/src/rootfs/mount.rs#L484 and then goes https://github.com/containers/youki/blob/main/crates/libcontainer/src/syscall/linux.rs#L471 . Where it finally calls nix::mount. I'm not sure how that is supposed to correspond to selinux. Can you check if there is some issue with out implementation , or am I doing some wrong code follow?

@Gekko0114 May I ask you to wait for sometime until the above are a bit more clear? Thanks :)

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 26, 2024

On further testing, the original oci go tests fails on runc as well here : https://github.com/YJDoc2/youki/actions/runs/8052111550/job/21991533730?pr=263 (ignore the passing status, I have removed all exit 1 to make sure all tests run ; check the not ok instead)

I think we need se linux setup and configured for this to work at all. Also another thing is that I'm not sure if the original oci tools tests are used anywhere currently (runc,crun etc) probably because they being broken and out of sync.

@utam0k
Copy link
Member

utam0k commented Feb 27, 2024

🤦‍♂Our implementation is wrong.

@utam0k
Copy link
Member

utam0k commented Feb 27, 2024

@saschagrunert Do you know any good crate for SELinux without libselinux? I don't like adding a dependency.

@saschagrunert
Copy link
Collaborator

@saschagrunert Do you know any good crate for SELinux without libselinux? I don't like adding a dependency.

Which functionality do you need? I assume all of them require libselinux somehow.

@utam0k
Copy link
Member

utam0k commented Feb 27, 2024

Which functionality do you need? I assume all of them require libselinux somehow.

I want a crate like opencontainers/selinux. It doesn't need libselinux, does it?
https://github.com/opencontainers/selinux

It seems we need following features:

			if err := label.Validate(m.Relabel); err != nil {
				return err
			}
			shared := label.IsShared(m.Relabel)
			if err := label.Relabel(m.Source, mountLabel, shared); err != nil {
				return err
			}

If there isn't in the world for now, is there any motivation to implement it in containers org? I think you are a specialist in this field.

@saschagrunert
Copy link
Collaborator

@utam0k that does not look like much code, do we want to create a module here and then consider moving it into a new crate?

@utam0k
Copy link
Member

utam0k commented Feb 27, 2024

Yes, I think something like oci-spec-rs would be good. How about first implementing it in this repository to some extent, and then taking it to new repository in containers org? Maybe it can be used for other repositories/projects than youki?

@saschagrunert
Copy link
Collaborator

Yes, I think something like oci-spec-rs would be good. How about first implementing it in this repository to some extent, and then taking it to new repository in containers org? Maybe it can be used for other repositories/projects than youki?

Yes this sounds like a good idea. 👍

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Feb 27, 2024

How about first implementing it in this repository to some extent,
... Maybe it can be used for other repositories/projects than youki?

After seeing the go implementation code, I was thinking along the same lines. Even if not a complete API, I think we can port the functions we need into youki, and use them 👍

I can take a try at a basic initial implementation around the weekend, or if someone else want to try before that, let me know if you need any help!

@utam0k
Copy link
Member

utam0k commented Feb 28, 2024

@Gekko0114 Are you interested in this? If possible, I want you to give it a challenge with @YJDoc2 as a mentor and @saschagrunert as an advisor.

Yes, I think something like oci-spec-rs would be good. How about first implementing it in this repository to some extent, and then taking it to new repository in containers org? Maybe it can be used for other repositories/projects than youki?

@Gekko0114
Copy link
Contributor Author

@utam0k
Thanks for asking me! Yes, I would like to work on it. I will start working on this issue from this weekend.
Also I will create a new issue for this.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Mar 1, 2024

Thanks for asking me! Yes, I would like to work on it. I will start working on this issue from this weekend.
Also I will create a new issue for this.

That's great! Feel free to ping me on github or discord if you need any help :)

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.

5 participants