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

libcontainer/system: move userns utilities, remove GetParentNSeuid, UIDMapInUserNS #2850

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Mar 13, 2021

libcontainer/system: remove unused GetParentNSeuid()

This function was added in f103de5 (#1688), but no longer used since 06f789c (#1862) (v1.0.0-rc6)

It is no longer used by runc itself, and only used by a single external project

libcontainer/system: un-export UIDMapInUserNS()

UIDMapInUserNS() is not used anywhere, only internally. so un-export it.

libcontainer/system: move userns utilities to separate package

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,
so adding a "Deprecated" alias for this.

@thaJeztah
Copy link
Member Author

Opening as draft; I'll create a PR in Moby to see if it has the desired result, then move it out of draft

var (
// RunningInUserNS detects whether we are currently running in a user namespace.
// Originally copied from github.com/lxc/lxd/shared/util.go
// Deprecated: use github.com/opencontainers/runc/libcontainer/userns.RunningInUserNS
Copy link
Member

Choose a reason for hiding this comment

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

We haven't been guaranteeing Go compatibility of libcontainer, so I think we can just drop these deprecated functions, but not a strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps keep only "RunninginUserNS" (the "popular" one) and drop the others

@thaJeztah thaJeztah changed the title libcontainer/system: move userns utilities to separate packaage libcontainer/system: move userns utilities to separate package Mar 22, 2021
@thaJeztah thaJeztah force-pushed the userns_check branch 2 times, most recently from f799e71 to c6f2e2c Compare March 22, 2021 13:52
@thaJeztah thaJeztah changed the title libcontainer/system: move userns utilities to separate package libcontainer/system: move userns utilities, remove GetParentNSeuid, UIDMapInUserNS Mar 22, 2021
@thaJeztah thaJeztah marked this pull request as ready for review March 22, 2021 14:03
@thaJeztah
Copy link
Member Author

I removed those functions that we don't use; reordered the commits to first remove these parts before moving the code. Moved this out of draft.

I also have a branch to isolate the userns package from the user package, as we only use the user package for a single function, and we likely don't need that; #2868

AkihiroSuda
AkihiroSuda previously approved these changes Mar 22, 2021
@thaJeztah
Copy link
Member Author

@kolyshkin ptal 🤗

@thaJeztah
Copy link
Member Author

@kolyshkin PTAL (I'll rebase #2868 after this)

kolyshkin
kolyshkin previously approved these changes Apr 2, 2021
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

@kolyshkin
Copy link
Contributor

close/reopen to kick ci

@kolyshkin kolyshkin closed this Apr 2, 2021
@kolyshkin kolyshkin reopened this Apr 2, 2021
@thaJeztah
Copy link
Member Author

looks like ci/centos7 doesn't start; let me try another rebase to force CI to run

@thaJeztah
Copy link
Member Author

close/reopen to trigger fedora CI

@thaJeztah thaJeztah closed this Apr 2, 2021
@thaJeztah thaJeztah reopened this Apr 2, 2021
This function was added in f103de5, but no
longer used since 06f789cf26774dd64cb2a9cc0b3c6a6ff832733bc (v1.0.0-rc6)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
`UIDMapInUserNS()` is not used anywhere, only internally. so un-export it.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
@thaJeztah
Copy link
Member Author

rebased again to kick centos7

@thaJeztah
Copy link
Member Author

All green now 🎉 @kolyshkin @AkihiroSuda ptal

@thaJeztah
Copy link
Member Author

@kolyshkin ptal 🤗

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

@kolyshkin kolyshkin merged commit b23315b into opencontainers:master Apr 8, 2021
@thaJeztah thaJeztah deleted the userns_check branch April 8, 2021 17:23
@thaJeztah
Copy link
Member Author

Thanks! I rebased #2868 and moved it out of draft

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

Successfully merging this pull request may close these issues.

3 participants