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.

Sign up for GitHub

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

Jump to bottom

Support multiple uid/gid mappings #10722

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Support multiple uid/gid mappings #10722

merged 6 commits into from
Jan 17, 2025

Conversation

henry118
Copy link
Member

@henry118 henry118 commented Sep 24, 2024 •
edited
Loading

This is a follow-up PR of the previous #10307 to support multiple id mappings for ID mapped mounts.

What's changed

  • Added 'WithUserNSRemapperLabels()' to take multiple uid/gid mappings
  • Updated 'ctr' to call the new func in case of '--remap-labels'
  • Updated built-in overlay SN to handle labels with multiple mappings. It's also updated to create mount options for multiple mappings, e.g. 'uidmap=0:666:1000,1000:6666:64536'
  • Updated 'GetUsernsFD()' to parse a string of multiple mappings

What's not changed

  • CRI code is not changed
    • It still calls 'WithRemapperLabels()' which takes a single mapping entry
    • I believe it's okay because kubelet currently only passes a single mapping entry to the runtime
    • If required, CRI just needs to call 'WithUserNSRemapperLabels()' to handle multiple mappings

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@henry118 henry118 force-pushed the uidmap2 branch 3 times, most recently from ed47d9f to 0287d9b Compare September 25, 2024 17:23
@henry118 henry118 changed the title [WIP] Support multiple uid/gid mappings [2/2] Support multiple uid/gid mappings [2/2] Sep 25, 2024
@henry118 henry118 marked this pull request as ready for review September 25, 2024 18:52
@dosubot dosubot bot added the area/runtime Runtime label Sep 25, 2024
@henry118
Copy link
Member Author

@rata can you ptal?

rata
rata reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

@henry118 sorry for the delay, I was with urgent bugfixes for runc/containerd and then got sick and I'm just looking at this.

Would it be possible to split it in more atomic commits, so it is simpler to review? A 800+ lines change in a single commit is not the most trivial thing to do :)

Also, did you look at adding an e2e test for this? Or should this already be covered by the previous PR?

@henry118 henry118 force-pushed the uidmap2 branch 2 times, most recently from 323d07b to c9d8ef4 Compare November 13, 2024 19:58
@henry118
Copy link
Member Author

Hi @rata, sorry for the late update. I was busy with other stuff for the last week.

Would it be possible to split it in more atomic commits, so it is simpler to review? A 800+ lines change in a single commit is not the most trivial thing to do :)

Sure. I splitted the commit into multiple smaller ones. Hopefully it makes it easier to review :)

Also, did you look at adding an e2e test for this? Or should this already be covered by the previous PR?

I did update the integration test (see the last commit) to cover the multiple remapper label case. I didn't touch CRI test cases because I didn't update CRI to use this feature. I think CRI integration can be a follow up effort.

Please take another look at the changes. Thanks!

@estesp estesp added this to the 2.1 milestone Nov 13, 2024
@henry118
Copy link
Member Author

henry118 commented Dec 2, 2024

Kindly ping @rata, also @fuweid @cpuguy83 @dmcgowan @AkihiroSuda if you can take a look as well. It would be great to have this in the 2.1 milestone. Thanks!

fuweid
fuweid reviewed Dec 3, 2024
View reviewed changes
integration/client/container_idmapped_linux_test.go Outdated
)
defer cancel()

if ok, err := overlayutils.SupportsIDMappedMounts(); err != nil || !ok {
Copy link
Member

Choose a reason for hiding this comment

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

please consider putting this checker at the beginning of the function

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update this in the next revision.

@henry118 henry118 mentioned this pull request Dec 6, 2024
Merged
fuweid
fuweid approved these changes Dec 11, 2024
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp added this pull request to the merge queue Jan 17, 2025
Merged via the queue into containerd:main with commit 98af40b Jan 17, 2025
59 checks passed
@henry118 henry118 deleted the uidmap2 branch January 17, 2025 20:09
@dmcgowan dmcgowan changed the title Support multiple uid/gid mappings [2/2] Support multiple uid/gid mappings Mar 8, 2025
@dmcgowan dmcgowan mentioned this pull request Mar 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rata rata rata left review comments

@kzys kzys kzys approved these changes

@estesp estesp estesp approved these changes

@fuweid fuweid fuweid approved these changes

Assignees
No one assigned
Labels
area/runtime Runtime impact/changelog size/XXL
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
7 participants
@henry118 @k8s-ci-robot @kzys @rata @estesp @fuweid @dmcgowan