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

Introduce seccomp feature for libcontainer with musl #1484

Merged
merged 2 commits into from Mar 23, 2023

Conversation

krisnova
Copy link
Contributor

This is a better PR than #1472

This pull request introduces the following Cargo feature to libcontainer:

[features]
default = ["systemd", "v2", "v1", "libseccomp"]
libseccomp = ["dep:libseccomp"]

And turns on libseccomp dependency by default.

For situations such as building with musl we can remove libseccomp dependeny and log a warn!() during privileged operations where seccomp is not available.

We can address the seccomp dependency with musl libc in a follow up pull request, I do not believe that warn!() is secure, however that can be an additional feature.

Signed-off-by: Kris Nóva kris@nivenly.com

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jan 17, 2023

Hey @krisnova can I ask you to add tests for this similar to others in https://github.com/containers/youki/blob/main/scripts/features_test.sh ?

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #1484 (0b08bf0) into main (3f3c052) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1484      +/-   ##
==========================================
- Coverage   68.78%   68.68%   -0.11%     
==========================================
  Files         120      120              
  Lines       13082    13101      +19     
==========================================
- Hits         8999     8998       -1     
- Misses       4083     4103      +20     

@krisnova
Copy link
Contributor Author

can I ask you to add tests for this similar to others in

Hey @YJDoc2! Yes I will add tests. Give me some time to send a follow up commit. Thank you.

@krisnova
Copy link
Contributor Author

I have pushed more commits, can we please check this approach?

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

I left one nit comment, but the code itself looks good to me.

@utam0k utam0k requested review from YJDoc2 and Furisto January 23, 2023 00:10
@krisnova
Copy link
Contributor Author

Yes I will update shortly with the requested changes. Please give me some time to send updated changes from your requests.

For further validation I am able to compile Aurae runtime project with this branch and create a single static binary with musl which can be used to run aurae/youki recursively inside of itself. https://github.com/aurae-runtime/aurae/blob/pod-impl/auraed/Cargo.toml#L56

libcontainer = { git = "https://github.com/krisnova/youki", branch = "musl-libcontainer", features = ["v2"], default-features = false }

@utam0k
Copy link
Member

utam0k commented Feb 14, 2023

@krisnova May ask you to add your signature to each commit and rebase from the main branch?

@yihuaf
Copy link
Collaborator

yihuaf commented Mar 1, 2023

Fixes #382

@yihuaf
Copy link
Collaborator

yihuaf commented Mar 17, 2023

I would like to see this merged. @krisnova May I have your permission to pick this PR up and continue the work to make sure this gets merged soon?

@krisnova
Copy link
Contributor Author

Hey @yihuaf its all yours! Please make any changes you see best fit and lets get it merged.

@yihuaf yihuaf self-assigned this Mar 21, 2023
Signed-off-by: Kris Nóva <kris@nivenly.com>
Signed-off-by: Eric Fang <yihuaf@unkies.org>
@yihuaf yihuaf merged commit bef32e0 into containers:main Mar 23, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants