Skip to content

Conversation

@YamasouA
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it:

We support idmapped mount
youki-dev/youki#2307

Which issue(s) this PR fixes:

Closed. #295

Special notes for your reviewer:

ref

Does this PR introduce a user-facing change?

 It adds support for the uidMappings/gidMappings fields on Mount, exposing them in the JSON schema so users can express ID‑mapped mounts.

@saschagrunert
Copy link
Contributor

@YamasouA thank you for the PR, do you mind signing-off the commit?

Signed-off-by: YamasouA <akiakiskyhand@gmail.com>
Signed-off-by: Akiyama <akiakiskyhand@gmail.com>
@YamasouA
Copy link
Contributor Author

@saschagrunert
Thanks for your review, I signing-off the commit.

Comment on lines 80 to 95

#[serde(
default,
skip_serializing_if = "Option::is_none",
rename = "uidMappings"
)]
/// UID mappings used for changing file owners w/o calling chown, fs should support it. Every mount point could have its own mapping.
uid_mappings: Option<Vec<LinuxIdMapping>>,

#[serde(
default,
skip_serializing_if = "Option::is_none",
rename = "gidMappings"
)]
/// GID mappings used for changing file owners w/o calling chown, fs should support it. Every mount point could have its own mapping.
gid_mappings: Option<Vec<LinuxIdMapping>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the OCI spec, uidMappings and gidMappings must be specified together. The current implementation allows setting one without the other.

I'd say we could add validation in the builder or as a separate validation method, like:

  • Add a custom build() validation in derive_builder
  • Add a validation method on Mount to check consistency
  • Document this requirement in the field comments

skip_serializing_if = "Option::is_none",
rename = "uidMappings"
)]
/// UID mappings used for changing file owners w/o calling chown, fs should support it. Every mount point could have its own mapping.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about being a bit more verbose here:

Suggested change
/// UID mappings used for changing file owners w/o calling chown, fs should support it. Every mount point could have its own mapping.
/// UID mappings for ID-mapped mounts (Linux 5.12+).
///
/// Specifies how to map UIDs from the source filesystem to the destination mount point.
/// This allows changing file ownership without calling chown.
///
/// **Important**: If specified, gid_mappings MUST also be specified.
/// The mount options SHOULD include "idmap" or "ridmap".
///
/// See: https://github.com/opencontainers/runtime-spec/blob/main/config.md#posix-platform-mounts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saschagrunert
Sorry for the late response.
I've fixed your review comments.

Signed-off-by: Akiyama <akiakiskyhand@gmail.com>
@utam0k
Copy link
Member

utam0k commented Dec 29, 2025

@YamasouA May I ask you to check the failed CI?

Signed-off-by: Akiyama <akiakiskyhand@gmail.com>
Signed-off-by: Akiyama <akiakiskyhand@gmail.com>
@YamasouA
Copy link
Contributor Author

@utam0k
Thank you for your review!
I fix your review comment!

@utam0k
Copy link
Member

utam0k commented Dec 29, 2025

Thanks!

@utam0k utam0k merged commit d21e23b into youki-dev:main Dec 29, 2025
16 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.

3 participants