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

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

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

thaJeztah
Copy link
Member

libct/userns: split userns detection from idmap code

Commit 4316df8 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 ca32014. 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 1912d59 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 ba0b5e2) only
exist in main and in non-stable releases (v1.2.0-rc.x), so don't need
an alias / deprecation.

@cyphar
Copy link
Member

cyphar commented Jun 30, 2024

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
Copy link
Member Author

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.

Commit 4316df8 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 ca32014. 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 1912d59 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 ba0b5e2) only
exist in main and in non-stable releases (v1.2.0-rc.x), so don't need
an alias / deprecation.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah changed the title libct/userns: split userns detection from idmap code libct/userns: split userns detection from internal userns code Jun 30, 2024
@thaJeztah thaJeztah added the kind/refactor refactoring label Jun 30, 2024
@thaJeztah thaJeztah self-assigned this Jun 30, 2024
@cyphar
Copy link
Member

cyphar commented Jul 1, 2024

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.

Copy link
Contributor

@kolyshkin kolyshkin 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

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.

Copy link
Member

@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, thanks!

@cyphar cyphar merged commit 81d63f2 into opencontainers:main Jul 3, 2024
39 checks passed
@thaJeztah thaJeztah deleted the separate_userns branch July 3, 2024 06:38
@lifubang lifubang mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/refactor refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants