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

integrate libcontainer/userns into moby/sys/user #140

Merged
merged 10 commits into from
Jul 25, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 16, 2024


integrate libcontainer/userns into moby/sys/user

This integrates the userns package from libcontainer (runc) at commit;
3778ae603c706494fd1e2c2faf83b406e38d687d (1).

Code was originally added through runc@c0ad40c (2), and located in the
libcontainer/system package; history of this code from before it was moved
can be found through 3 and 4.

Migration was done using the following steps:

# install filter-repo (https://github.com/newren/git-filter-repo/blob/main/INSTALL.md)
brew install git-filter-repo

# create a temporary clone of docker
cd ~/Projects
git clone https://github.com/opencontainers/runc.git runc_temp
cd runc_temp

# commit taken from
git rev-parse --verify HEAD
3778ae603c706494fd1e2c2faf83b406e38d687d

git filter-repo --analyze

# remove all code, except for 'libcontainer/userns/', and rename to /user/userns
git filter-repo \
  --path 'libcontainer/userns/userns.go' \
  --path 'libcontainer/userns/userns_linux.go' \
  --path 'libcontainer/userns/userns_fuzzer.go' \
  --path 'libcontainer/userns/userns_linux_fuzzer.go' \
  --path 'libcontainer/userns/userns_linux_test.go' \
  --path 'libcontainer/userns/userns_unsupported.go' \
  --path-rename libcontainer/userns:user/userns

# go to the target github.com/moby/sys repository
cd ~/go/src/github.com/moby/sys

# create a branch to work with
git checkout -b integrate_libcontainer_userns_take2

# add the temporary repository as an upstream and make sure it's up-to-date
git remote add libcontainer_userns ~/Projects/runc_temp
git fetch libcontainer_userns

# merge the upstream code
git merge --allow-unrelated-histories --signoff -S libcontainer_userns/main

@thaJeztah
Copy link
Member Author

/cc @AkihiroSuda @kolyshkin @cyphar @lifubang PTAL if this LGTY (related PR in runc is opencontainers/runc#4350)

Makefile Outdated
@@ -16,14 +16,18 @@ clean:
test: test-local
set -eu; \
for p in $(PACKAGES); do \
(cd $$p; go test $(RUN_VIA_SUDO) -v .); \
if $p = user && go version | grep -qv go1.17; then \
Copy link
Member

Choose a reason for hiding this comment

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

No need to support Go <= 1.20

Copy link
Member Author

Choose a reason for hiding this comment

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

The other modules still have go1.17 as minimum version; I didn't want to change that as part of this PR, but we can revisit that in a follow up


// runningInUserNS detects whether we are currently running in a user namespace.
//
// Originally copied from https://github.com/lxc/incus/blob/e45085dd42f826b3c8c3228e9733c0b6f998eafe/shared/util.go#L678-L700.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we should probably refer to runc, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a mention of runc (both here, and in the package description)

@kolyshkin
Copy link
Collaborator

It looks like git history here begins when the code was moved in runc, and contains some drastic changes (like additon and removal of a big portion of C code). IOW it's not very useful (I tried to answer simple questions like "why do we use bytes.Buffer?" or "who added the constant 4294967295?" using git blame and git log, and it's not possible to do so).

Given that git history serves a main purpose of figuring out why (or when, or by whom) things were written the way they are written, and that this git history is:

  1. quite lengthy (esp. compared to the amount of code added), yet
  2. doesn't work for the above purpose,

I'm not sure it makes sense to retain it.

I think it's better to just copy and commit the code @ specific commit with a clean git history, and refer to older runc history in the docs (like we did when it was copied from lxc).

@thaJeztah thaJeztah force-pushed the integrate_libcontainer_userns branch 2 times, most recently from 7b76d4a to aa1d8ad Compare July 21, 2024 13:15
@thaJeztah
Copy link
Member Author

@kolyshkin I gave this another go, and used more specific filters so that the unrelated commits don't get included. History now contains all history since the code was moved, but the merge-commit contains a link to the commits that moved it.

Let me know if this looks better 🤞

@thaJeztah thaJeztah force-pushed the integrate_libcontainer_userns branch from aa1d8ad to 6d16ea0 Compare July 22, 2024 07:46
@thaJeztah thaJeztah requested a review from kolyshkin July 22, 2024 16:37
@thaJeztah
Copy link
Member Author

@kolyshkin ptal 🤗

Copy link
Contributor

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 24, 2024

oh! need to rebase after #142 was merged done! ✅

thaJeztah and others added 7 commits July 24, 2024 10:10
Moving these utilities to a separate package, so that consumers of this
package don't have to pull in the whole "system" package.

Looking at uses of these utilities (outside of runc itself);

`RunningInUserNS()` is used by [various external consumers][1],
so adding a "Deprecated" alias for this.

[1]: https://grep.app/search?current=2&q=.RunningInUserNS

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
For files that end with _linux.go or _linux_test.go, there is no need to
specify linux build tag, as it is assumed from the file name.

In addition, rename libcontainer/notify_linux_v2.go -> libcontainer/notify_v2_linux.go
for the file name to make sense.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go 1.17 introduce this new (and better) way to specify build tags.
For more info, see https://golang.org/design/draft-gobuild.

As a way to seamlessly switch from old to new build tags, gofmt (and
gopls) from go 1.17 adds the new tags along with the old ones.

Later, when go < 1.17 is no longer supported, the old build tags
can be removed.

Now, as I started to use latest gopls (v0.7.1), it adds these tags
while I edit. Rather than to randomly add new build tags, I guess
it is better to do it once for all files.

Mind that previous commits removed some tags that were useless,
so this one only touches packages that can at least be built
on non-linux.

Brought to you by

        go1.17 fmt ./...

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This makes libcontainer/userns self-dependent, largely returning to
the original implementation from lxc. The `uiMapInUserNS` is kept as
a separate function for unit-testing and fuzzing.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Removed pre-go1.17 build-tags with go fix;

    go fix -mod=readonly ./...

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was a poor decision on my side; ab29593
moved this utility to a separate package, and split the exported function
from the implementation (and stubs). Out of convenience, I used an alias
for the latter part, but there's two downsides to that;

- `RunningInUserNS` being an exported var means that (technically) it can
  be replaced by other code; perhaps that's a "feature", but not one we
  intended it to be used for.
- `RunningInUserNS` being implemented through a var / alias means it's
  also documented as such on [pkg.go.dev], which is confusing.

This patch changes it to a regular function, acting as a wrapper for
the underlying implementations. While at it, also slightly touching
up the GoDoc to describe its functionality / behavior.

[pkg.go.dev]: https://pkg.go.dev/github.com/opencontainers/runc@v1.1.13/libcontainer/userns#RunningInUserNS

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The fuzzer for this only runs on Linux; rename the file to be Linux-only
so that we don't have to stub out the uidMapInUserNS function.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Now that we dropped support for go < 1.21, we can use this; moving
the sync.once out of the runningInUserNS() implementation would also
allow for it to be more easily tested if we'd decide to.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the integrate_libcontainer_userns branch from 6d16ea0 to 5cd502c Compare July 24, 2024 08:12
@thaJeztah thaJeztah self-assigned this Jul 24, 2024
Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

@laurazard laurazard merged commit 86870e7 into moby:main Jul 25, 2024
19 checks passed
@thaJeztah thaJeztah deleted the integrate_libcontainer_userns branch July 25, 2024 08:53
@tianon
Copy link
Member

tianon commented Jul 25, 2024

I'm a little late to the party (sorry 😭), but I'm actually a bit confused by this. As far as I can tell, the only thing the "user" and "userns" packages have in common is the name, so this seems like an odd grouping IMO

@tianon
Copy link
Member

tianon commented Jul 29, 2024

I'm also missing the justification for the bump to go1.21? 5cd502c#r144637064

(and in fact, #140 (comment) seems to directly contradict doing so 🤔)

@thaJeztah
Copy link
Member Author

@tianon Arf, sorry for ghosting you; I saw your comment on my phone, and wanted to bring it up in one of the calls, but I didn't make both of them, then forgot.

I'm a little late to the party (sorry 😭), but I'm actually a bit confused by this. As far as I can tell, the only thing the "user" and "userns" packages have in common is the name, so this seems like an odd grouping IMO

That, hm, is a valid point. I think I initially put it as a separate module, but then (maybe incorrectly?) recalled that there was a discussion to move this, as well as possibly the idtools bit into the same module. But maybe I my memory fails me here (it's been leaky a bit recently).

FWIW; to my knowledge none of the current consumers (moby, buildkit, containerd) have included this change in a release, so there's still a potential to change this to a separate module if we think that's better

I'm also missing the justification for the bump to go1.21? 5cd502c#r144637064

The code in userns uses sync.OnceFunc, which requires go1.21

(and in fact, #140 (comment) seems to directly contradict doing so 🤔)

Ah, right, so the comment was about the added check in make test; GitHub actions runs on go1.18, go1.21.x and go1.22.x; the lines added are to skip the user module on go1.18, but still test the other modules (mount, mountinfo, sequential, signal, symlink) on that version.

@tianon
Copy link
Member

tianon commented Jul 30, 2024

The context for why I even care is that I'm a downstream user of github.com/moby/sys/user who will never touch userns, so this change has me strongly considering the Go Proverb "A little copying is better than a little dependency." (but "a little dependency" is part of why github.com/moby/sys/user was created / exists in the first place) 😅 🙈

@tianon
Copy link
Member

tianon commented Jul 30, 2024

So that's the long way of saying I'm very much in favor of a git mv user/userns userns

@thaJeztah
Copy link
Member Author

I opened a PR to move this to a separate module;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

7 participants