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 rootless for spec #49

Closed
wants to merge 9 commits into from

Conversation

chenyukang
Copy link
Contributor

No description provided.

@chenyukang
Copy link
Contributor Author

As discussed in containers/youki#77
Pair PR: containers/youki#236

@chenyukang
Copy link
Contributor Author

@saschagrunert my changes failed for cargo --all-features.
https://github.com/containers/oci-spec-rs/pull/49/checks?check_run_id=3444754339#step:5:96

Those attributes in Linux is private ...
How can i fix them?

@utam0k
Copy link
Member

utam0k commented Aug 28, 2021

This is probably caused by the fact that the field of the structure is private when there is a feature builder pattern. So you need to distinguish between the case where the feature builder is present and the case where it is not.

#[cfg(not(feature = "builder"))]
macro_rules! make_pub {
{
$(#[$outer:meta])*
struct $name:ident {
$(
$(#[$inner:ident $($args:tt)*])*
$field:ident: $t:ty,
)*
}
} => {
$(#[$outer])*
pub struct $name {
$(
$(#[$inner $($args)*])*
pub $field: $t,
)*
}
}
}
#[cfg(feature = "builder")]
macro_rules! make_pub {
{
$(#[$outer:meta])*
struct $name:ident {
$(
$(#[$inner:ident $($args:tt)*])*
$field:ident: $t:ty,
)*
}
} => {
$(#[$outer])*
pub struct $name {
$(
$(#[$inner $($args)*])*
$field: $t,
)*
}
}
}

@chenyukang
Copy link
Contributor Author

This is probably caused by the fact that the field of the structure is private when there is a feature builder pattern. So you need to distinguish between the case where the feature builder is present and the case where it is not.

#[cfg(not(feature = "builder"))]
macro_rules! make_pub {
{
$(#[$outer:meta])*
struct $name:ident {
$(
$(#[$inner:ident $($args:tt)*])*
$field:ident: $t:ty,
)*
}
} => {
$(#[$outer])*
pub struct $name {
$(
$(#[$inner $($args)*])*
pub $field: $t,
)*
}
}
}
#[cfg(feature = "builder")]
macro_rules! make_pub {
{
$(#[$outer:meta])*
struct $name:ident {
$(
$(#[$inner:ident $($args:tt)*])*
$field:ident: $t:ty,
)*
}
} => {
$(#[$outer])*
pub struct $name {
$(
$(#[$inner $($args)*])*
$field: $t,
)*
}
}
}

Emm, this will make code duplicated. I haven't got the benefits of getset.
Just read the PR: 06d0f77

@chenyukang
Copy link
Contributor Author

chenyukang commented Aug 28, 2021

Fixed it with some time.
The benefits may be to set fields in a struct as private in default and automatically create a builder for them.
Essentially, all the structs in oci-spec-rs is pure configurations, make all the fields to be public won't be a big issue?

I'm not a fan of OOP, and don't think it's more convenient with a builder.

And in the future, we will only keep the feature = "builder", right?
Otherwise, it's annoying to maintain two versions of code, even for the testcases.

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2021

Codecov Report

Merging #49 (e7aad6b) into main (0110ba4) will increase coverage by 0.33%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   26.93%   27.26%   +0.33%     
==========================================
  Files          19       19              
  Lines        1344     1401      +57     
  Branches      694      737      +43     
==========================================
+ Hits          362      382      +20     
- Misses        374      377       +3     
- Partials      608      642      +34     

@saschagrunert
Copy link
Member

saschagrunert commented Aug 30, 2021

Essentially, all the structs in oci-spec-rs is pure configurations, make all the fields to be public won't be a big issue?

I'm not a fan of OOP, and don't think it's more convenient with a builder.

A practical benefit of using the builder pattern is that end-users directly know when the struct is in "locked-up" in read-only mode. When creating the spec, we only pass around the builder. At the moment we call spec_builder.build() we can ensure that the interior mutability is hidden and exposed by the getters of their fields. This way it would be theoretically possible to provide the same API, while changing its internal implementation. The OCI runtime tools provides a generator, which maps to the same pattern.

Sure, we could also use mut and the fields directly, that's why I made it optional. :)

And in the future, we will only keep the feature = "builder", right?
Otherwise, it's annoying to maintain two versions of code, even for the testcases.

I don't know. I made it optional for folks to select on their personal preference and make the migration in youki easier.

@cgwalters
Copy link
Contributor

cgwalters commented Aug 30, 2021

I also got quite confused by this "fields are private when using the builder feature" thing.

A practical benefit of using the builder pattern is that end-users directly know when the struct is in "locked-up" in read-only mode.

I don't understand your statement. In Rust variables aren't mutable by default, so if I have e.g. let c = ConfigBuilder::new()...build() I can't mutate c afterwards without let mut c.

@utam0k
Copy link
Member

utam0k commented Aug 30, 2021

In the case of the builder pattern, the structure for building is different from the structure that is actually built by the builder pattern, so by setting the field of the structure to be built to private, you can't change it after the last build() in the structure for builder, even if you set it to mut.
https://docs.rs/derive_builder/0.10.2/derive_builder/

I thought that was what he meant. I'm sorry if I'm wrong.

@chenyukang
Copy link
Contributor Author

Anyway, the oci-spec-rs is mainly used by youki, right?
I think maybe we should choose one single style, and keep it.
Otherwise, maintain two copies of code and test cases with similar logic(different with styles) will be hard.

@saschagrunert
Copy link
Member

Anyway, the oci-spec-rs is mainly used by youki, right?

Right now, but the intention of this crate was to use it from other consumers as well.

I think maybe we should choose one single style, and keep it.

I'm open for that, do we wanna start a vote in a dedicated issue?

@cgwalters
Copy link
Contributor

Anyway, the oci-spec-rs is mainly used by youki, right?

ostreedev/ostree-rs-ext#70 FWIW

@Furisto
Copy link
Member

Furisto commented Aug 30, 2021

Personally I like the builder pattern, but it seems to be confusing for people. Maybe this can be solved with documentation? Right now this feature is not really explained. Adding a description of the feature and the rationale to the readme could help in that regard.

@chenyukang
Copy link
Contributor Author

@Furisto I notice you remove nix in oci-spec-rs,
Without nix, is there any other good way to replace geteuid and getegid in my PR?

@Furisto
Copy link
Member

Furisto commented Aug 31, 2021

@chenyukang You could pass it in as as argument for set_for_rootless. I would prefer it that way actually because currently if you calls this as root, container root will be mapped to host root which is usually not what you want.

@@ -31,6 +31,7 @@ serde_json = "1.0.66"
quickcheck = { version = "1.0.3", optional = true }
derive_builder = { version = "0.10.2", optional = true }
getset = { version = "0.1.1", optional = true }
path-clean = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

@saschagrunert What do you think about the introduction of this crate?
https://github.com/danreeves/path-clean

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make it optional and make rootless a feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to make it optional and make rootless a feature?

I think it is OK.

@yihuaf
Copy link
Contributor

yihuaf commented Sep 1, 2021

Do we have to implement the set_for_rootless in this crate? To me, we should limit what this crate does so it can be shared with more repos. Can we in the main Youki repo create a spec that's rootless?

Honestly, I am not sure how I feel about the builder pattern, but maintaining two version of the same code is not sustainable in my opinion.

@chenyukang
Copy link
Contributor Author

Do we have to implement the set_for_rootless in this crate? To me, we should limit what this crate does so it can be shared with more repos. Can we in the main Youki repo create a spec that's rootless?

Honestly, I am not sure how I feel about the builder pattern, but maintaining two version of the same code is not sustainable in my opinion.

I also have some considerations when implementing it. I thought put it in this repo because it seems more natural, out of oci-spec, we get a json and don't know much details about it. And we currently also have canonicalize_rootfs in spec.

I agree with two versions of the same code is not sustainable.

@utam0k
Copy link
Member

utam0k commented Sep 2, 2021

I think it would be easier to discuss the builder pattern here in the future, so what do you think? We are definitely looking for great feedback.
#60

@utam0k
Copy link
Member

utam0k commented Sep 2, 2021

@yihuaf @chenyukang
The canonicalize_rootfs is a remnant of the old youki. Sorry about that. How about extending it with a trait in youki, for example? It may be better to provide the extension as a utility in this repository instead of in youki. How about extending it with a trait in youki, for example?
https://github.com/containers/youki/blob/03cb29ef4a8031b7f508f1758f33d918797f5701/src/utils.rs#L17-L41

@yihuaf
Copy link
Contributor

yihuaf commented Sep 3, 2021

@chenyukang canonicalize_rootfs should be outside of this repo and inside the main repo as well. It was historical since we didn't take care of this when breaking this repo off from the Youki main repo. Depending on what we decide on #60, I will probably move that out of this repo as well. Is there any difficulties moving set_for_rootless into the main Youki repo? Maybe this is the purist in me speaking, but I think we should do that if we can.

@chenyukang
Copy link
Contributor Author

chenyukang commented Sep 3, 2021 via email

@yihuaf
Copy link
Contributor

yihuaf commented Sep 3, 2021

I haven't tried it. Youki currently hasn't synced with the head of oci-spec-rs. We'd better update it to the latest and then have a try. I am not sure whether all the data structures internals can be accessed outside of oci-spec-rs.

Hmm.. You have a good point. We should upgrade to the latest and see what needs to be changed. I will spend sometime to at least get a sense.

@chenyukang
Copy link
Contributor Author

I haven't tried it. Youki currently hasn't synced with the head of oci-spec-rs. We'd better update it to the latest and then have a try. I am not sure whether all the data structures internals can be accessed outside of oci-spec-rs.

Hmm.. You have a good point. We should upgrade to the latest and see what needs to be changed. I will spend sometime to at least get a sense.

Upgrading oci-spec-rs is tracked at this issue: containers/youki#225

@chenyukang
Copy link
Contributor Author

chenyukang commented Oct 1, 2021

@chenyukang canonicalize_rootfs should be outside of this repo and inside the main repo as well. It was historical since we didn't take care of this when breaking this repo off from the Youki main repo. Depending on what we decide on #60, I will probably move that out of this repo as well. Is there any difficulties moving set_for_rootless into the main Youki repo? Maybe this is the purist in me speaking, but I think we should do that if we can.

I have more investigation on this issue.
as the implementation in set_for_rootless , we need some details of Spec, such as modify it's linux fileds.
Currently, the linux is a private attributes of Spec.

If we need to move out of this repo, we need make those fields public.

Anyway, I updated my this PR with latest of oci-spec-rs. @utam0k

@chenyukang
Copy link
Contributor Author

@utam0k
Crrently youki is not with the latest oci-spec-rs, it is on this commit:

oci-spec = { git = "https://github.com/containers/oci-spec-rs",  rev = "5018f8e5b0355a82c08962cefa5ab07a05b930c6" }

It seems the not builder version of oci-spec-rs. If I work based on this, we need to more work since the latest oci-spec-rs has been moved to the builder version.

Anyone is working to update oci-spec-rs to latest for youki?

@guni1192
Copy link
Contributor

guni1192 commented Oct 1, 2021

@chenyukang Hi. I upgraded oci-spec-rs for youki. containers/youki#303
Now, youki use oci-spec-rs later than v0.5.1.
You can use the builder pattern in the main branch.

@chenyukang
Copy link
Contributor Author

@chenyukang Hi. I upgraded oci-spec-rs for youki. containers/youki#303 Now, youki use oci-spec-rs later than v0.5.1. You can use the builder pattern in the main branch.

Oh, thanks for reminding! I forget update my local repo with upstream. 😂

@yihuaf
Copy link
Contributor

yihuaf commented Oct 1, 2021

@chenyukang canonicalize_rootfs should be outside of this repo and inside the main repo as well. It was historical since we didn't take care of this when breaking this repo off from the Youki main repo. Depending on what we decide on #60, I will probably move that out of this repo as well. Is there any difficulties moving set_for_rootless into the main Youki repo? Maybe this is the purist in me speaking, but I think we should do that if we can.

I have more investigation on this issue. as the implementation in set_for_rootless , we need some details of Spec, such as modify it's linux fileds. Currently, the linux is a private attributes of Spec.

If we need to move out of this repo, we need make those fields public.

We don't necessarily need to make the fields public. Take a look at how tenent_builder.rs is updating/modifying the spec.

@chenyukang
Copy link
Contributor Author

@chenyukang canonicalize_rootfs should be outside of this repo and inside the main repo as well. It was historical since we didn't take care of this when breaking this repo off from the Youki main repo. Depending on what we decide on #60, I will probably move that out of this repo as well. Is there any difficulties moving set_for_rootless into the main Youki repo? Maybe this is the purist in me speaking, but I think we should do that if we can.

I have more investigation on this issue. as the implementation in set_for_rootless , we need some details of Spec, such as modify it's linux fileds. Currently, the linux is a private attributes of Spec.
If we need to move out of this repo, we need make those fields public.

We don't necessarily need to make the fields public. Take a look at how tenent_builder.rs is updating/modifying the spec.

Thanks! I will have a look.

@chenyukang
Copy link
Contributor Author

@chenyukang canonicalize_rootfs should be outside of this repo and inside the main repo as well. It was historical since we didn't take care of this when breaking this repo off from the Youki main repo. Depending on what we decide on #60, I will probably move that out of this repo as well. Is there any difficulties moving set_for_rootless into the main Youki repo? Maybe this is the purist in me speaking, but I think we should do that if we can.

I have more investigation on this issue. as the implementation in set_for_rootless , we need some details of Spec, such as modify it's linux fileds. Currently, the linux is a private attributes of Spec.
If we need to move out of this repo, we need make those fields public.

We don't necessarily need to make the fields public. Take a look at how tenent_builder.rs is updating/modifying the spec.

If we want to modify some fields on a Struct, we must clone the left fields of it like this?
Do we have any simpler solution.

 let mut spec_builder = SpecBuilder::default()
            .process(process)
            .version(spec.version())
            .linux(linux);

        spec_builder = match spec.root() {
            Some(root) => spec_builder.root(root.clone()),
            None => spec_builder,
        };
        spec_builder = match spec.mounts() {
            Some(mounts) => spec_builder.mounts(mounts.clone()),
            None => spec_builder,
        };
        spec_builder = match spec.hostname() {
            Some(hostname) => spec_builder.hostname(hostname.clone()),
            None => spec_builder,
        };
        spec_builder = match spec.hooks() {
            Some(hooks) => spec_builder.hooks(hooks.clone()),
            None => spec_builder,
        };
        spec_builder = match spec.annotations() {
            Some(annotations) => spec_builder.annotations(annotations.clone()),
            None => spec_builder,
        };

@yihuaf
Copy link
Contributor

yihuaf commented Oct 1, 2021

For the moment, that's the only way. That's the price we pay for making the struct immutable, which is a good trade off. The right solution to remove a lot of this boilerplate is to implement From trait allowing change from a Spec to a SpecBuilder. After that, we can refactor both the tenent_builder and this code. Since now, this oci crate is separated and we already see how a change in oci crate can block our progressive, I think the right thing is to implement this outside the crate in Youki, even it means a bit of more work.

containers/youki#351 tracking

@chenyukang
Copy link
Contributor Author

Closing this PR since we implement this option in youki: containers/youki#350

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

8 participants