search

opencontainers / runc

CLI tool for spawning and running containers according to the OCI specification
https://www.opencontainers.org/
Apache License 2.0
11.78k stars 2.1k forks source link

libct/userns: split userns detection from internal userns code #4331

Closed thaJeztah closed 3 months ago

thaJeztah commented 3 months ago

libct/userns: split userns detection from idmap code

Commit 4316df8b53cce2933d7f0f3cf4bf87da67459835 isolated RunningInUserNS to a separate package to make it easier to consume without bringing in additional dependencies, and with the potential to move it separate in a similar fashion as libcontainer/user was moved to a separate module in commit ca32014adbc5bcdd1ec5862672c35c7d4cbd849d. While RunningInUserNS is fairly trivial to implement, it (or variants of this utility) is used in many codebases, and moving to a separate module could consolidate those implementations, as well as making it easier to consume without large dependency trees (when being a package as part of a larger code base).

Commit 1912d5988bbb379189ea9ceb2e03945738c513dc and follow-ups introduced cgo code into the userns package, and code introduced in those commits are not intended for external use, therefore complicating the potential of moving the userns package separate.

This commit moves the new code to a separate package; some of this code was included in v1.1.11 and up, but I could not find external consumers of GetUserNamespaceMappings and IsSameMapping. The Mapping and Handles types (added in ba0b5e26989f39d0bdadeeff38182902df781df6) only exist in main and in non-stable releases (v1.2.0-rc.x), so don't need an alias / deprecation.

cyphar commented 3 months ago

I would prefer the code be moved to libcontainer/internal/userns instead of libcontainer/idmap. idmap isn't a good name for the new package IMHO, because the code you're moving is all about interacting with actual user namespaces, rather than just the idmap structure.

thaJeztah commented 3 months ago

Yes, that's fair; I was considering making it internal/ but wasn't 100% sure if we'd anticipate this to be used externally. I tried to come up with a different name, trying to avoid both packages being named the same (but one being internal) but perhaps that's fine, as I don't think we have locations where both are used.

I'll give that a quick go and push my changes to discuss further if we are ok with this move (and the potential extracting of the userns.RunningInUserNS() utility.

cyphar commented 3 months ago

Given #3028, I suspect we should move forward with the general principle that any new package for libcontainer should be internal by default and we can always expose it later.

thaJeztah commented 3 months ago

Given #3028, I suspect we should move forward with the general principle that any new package for libcontainer should be internal by default and we can always expose it later.

Yes, I think that's the way to go for most cases, or at least to help with how the post-modules situation.

Projects used to be quite relaxed public vs internal (Go-)APIs, other than through convention ("pkg") and documentation. And even for cases where packages were more "public", documentation may describe the promised stability ("these packages are used by us, but you may find them useful."). There was no formal contract on stability for such packages; users could use them at their own risk, and decide when to update/vendor a different version (making changes where needed), but with dependency graphs growing, and Go modules enforcing both SemVer and controlling the minimum version, it's much more complicated. Users no longer have a choice whether to update or not (if any dependency pushes new versions on them).

That means it's important to reduce the surface area of the public API; default to keep things private, unless there's a strong need to make it public; and for those, only if you're comfortable / being able to maintain that public API / signature stable.