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

Readonly paths #582

Merged
merged 12 commits into from
Jan 9, 2022
Merged

Readonly paths #582

merged 12 commits into from
Jan 9, 2022

Conversation

YJDoc2
Copy link
Collaborator

@YJDoc2 YJDoc2 commented Dec 31, 2021

Add readonly paths integration tests

@YJDoc2 YJDoc2 marked this pull request as draft December 31, 2021 12:51
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Dec 31, 2021

Hey @Furisto , @yihuaf can you take a look? This still has some things to be cleaned up, but I made this PR, as I had some questions regarding my implementation, as well as the original tests. I'll add them as comments, please check
Thanks

Remove unnecessary unsafe{}
Remove libc direct dependency and use nix instead
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #582 (8320f05) into main (636ecff) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #582   +/-   ##
=======================================
  Coverage   70.18%   70.18%           
=======================================
  Files          82       82           
  Lines       10909    10909           
=======================================
  Hits         7657     7657           
  Misses       3252     3252           

@YJDoc2 YJDoc2 marked this pull request as ready for review January 8, 2022 12:01
@YJDoc2 YJDoc2 requested review from yihuaf and Furisto January 8, 2022 12:11
@YJDoc2
Copy link
Collaborator Author

YJDoc2 commented Jan 8, 2022

This PR is adds an important Part of the integration tests : The test_inside_container function, which allows testing the restrictions from inside the container ; along with its counterpart the runtimetest . The PR contains the readonly_paths test, as well as a new crate for runtimetest, which is the binary which runs inside the container process and runs tests inside.

Apologies for such a big PR, but it needed to add both of the things together, as to make sure runtimetest works, we need a test which will run test_inside_container and to run a test_inside_container, we need runtiemtest, thus both of those ended up being in this same PR.

As the runtimetest tool and its configuration can be tricky, as well as I had several issues while figuring out how all works and fits together for the original Go tests, I have added detailed explanations in the Readme as well as have added docs for it as well.

Also I would like to apologies to @utam0k because this PR will likely make even more conflicts for #514 . If there is any way I can help with resolving the conflicts, or reducing the issue, please let me know I would like to help. If we need to wait for it to be merged before this, that is fine as well, we can review this and keep it in queue to be merged after that PR.

Please take a look, thanks :)

@utam0k
Copy link
Member

utam0k commented Jan 8, 2022

There is no problem. As this pr which I made is too large to merge, I'll try to make some smaller PRs.

Also I would like to apologies to @utam0k because this PR will likely make even more conflicts for #514 . If there is any way I can help with resolving the conflicts, or reducing the issue, please let me know I would like to help. If we need to wait for it to be merged before this, that is fine as well, we can review this and keep it in queue to be merged after that PR.

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.

Awesome!
However, this PR will inevitably have a large impact on other people's work. Therefore, I'll ignore the details and merge this once. Let me improve some of them on myend.

@utam0k utam0k merged commit 9dd1c9d into containers:main Jan 9, 2022
@YJDoc2 YJDoc2 deleted the readonly_paths branch October 7, 2022 05:00
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