Split idtools to an internal package and package to be moved by dmcgowan · Pull Request #49087 · moby/moby

dmcgowan · 2024-12-13T06:40:18Z

The core part of utils which is used by pkg/archive and externally is kept in pkg/idtools and the rest moved to internal/usergroup. pkg/idtools should be ready to move to github.com/moby/sys after this.

supersedes, closes [WIP] Split idtools to an internal package and package to be moved (changed order of commits) #49225

pkg/idtools/idtools.go

thaJeztah · 2024-12-13T09:48:56Z

pkg/idtools/idtools_unix.go

-// LoadIdentityMapping takes a requested username and
-// using the data from /etc/sub{uid,gid} ranges, creates the
-// proper uid and gid remapping ranges for that user/group pair
-func LoadIdentityMapping(name string) (IdentityMapping, error) {


Can you add a idtools_deprecated_unix.go file with aliases for the exported functions, and deprecate those, or won't that work due to a circular import in that case? (we might still be able to after idtools is moved to a separate module.

// LoadIdentityMapping takes a requested username and // using the data from /etc/sub{uid,gid} ranges, creates the // proper uid and gid remapping ranges for that user/group pair. // // Deprecated: this function was only used internally and will be removed in the next release. func LoadIdentityMapping(name string) (IdentityMapping, error) { return usergroup. LoadIdentityMapping(name) }

Can add back later if we need. Who is that deprecation message for though? I think we could have documentation about what is breaking with each release but if we require stretching every refactor to a two release cycle then we won't it will make doing all the valuable way more time consuming. IMO we are already doing major version bumps and we should cleanup as much as we can in v28, switch to go.mod, then actually semver the go sdk.

Who is that deprecation message for though?

Always hard to tell.

The Deprecated annotations are effectively the documentation here, and those are picked up by linters and IDEs, so help users find their replacement. Updating references is not always under control by the user, because go modules may force them to use dependencies that perhaps have not yet switched over. We for sure won't be able to do this for all changes, but if it's only adding a file with aliases, that's not a ton of things to maintain, and can go a long way not having to deal with the aftermath.

☝️ OK: identified at least one; BuildKit breaks because of this;

0.137 + xx-go build '-gcflags=' -ldflags '-X github.com/moby/buildkit/version.Version=v0.18.0-55-g80600495c -X github.com/moby/buildkit/version.Revision=80600495cbc8c0c439d71fc77f146cb26fabcfa5 -X github.com/moby/buildkit/version.Package=github.com/moby/buildkit -extldflags '"'"'-static'"'" -tags 'osusergo netgo static_build seccomp ' -o /usr/bin/buildkitd ./cmd/buildkitd 71.51 # github.com/moby/buildkit/cmd/buildkitd 71.51 cmd/buildkitd/util_linux.go:25:27: undefined: idtools.LoadIdentityMapping

see vendor: github.com/docker/docker, github.com/docker/cli v28.0.0-rc.1 buildkit#5602 (comment)

pkg/idtools/idtools_unix.go

pkg/idtools/idtools_windows.go

internal/usergroup/const_windows.go

internal/usergroup/parser.go

thaJeztah · 2024-12-21T13:50:18Z

pkg/idtools/idtools_windows.go

 func mkdirAs(path string, _ os.FileMode, _ Identity, _, _ bool) error {
-	return system.MkdirAll(path, 0)
+	return os.MkdirAll(path, 0)
 }


I went looking if the system.MkdirAll was needed at all anywhere (for the special handling of windows GUID volume paths), and it looks like it's no longer needed since go1.22, so I opened a PR to deprecate it, and remove its use;

pkg/system: deprecate MkdirAll and remove custom volume GUID handling #49162

☝️ this was the last use go pkg/system in BuildKit, so removing this line makes BuildKit no longer depend on it.

Did a quick rebase after that one was merged

dmcgowan · 2025-01-04T02:10:25Z

I added back an implementation of LoadIdentityMapping which directly uses github.com/moby/sys/user. I kept the original internal one since that has the getent logic that Docker uses to load. If we want other projects to use that logic then we should add an implementation in github.com/moby/sys/user.

I think we should probably copy the package over directly to github.com/moby/sys/user (or a subpackage of it like usermap or something). Then consolidate the mapping types IDMap, Identity, IdentityMapping to github.com/moby/sys/user types.

pkg/idtools/idtools_unix.go

pkg/idtools/idtools_windows.go

pkg/idtools/idtools_unix.go

thaJeztah

Gave it a try what it'd look like with changing the order of things;

#49225
(first commit of the above); #49226

thaJeztah · 2025-01-07T10:48:02Z

pkg/idtools/idtools_unix.go

+	uidstr := strconv.Itoa(usr.Uid)
+	rangeList, err := user.ParseSubIDFileFilter(path, func(sid user.SubID) bool {
+		return sid.Name == uidstr
+	})
 	if err != nil {
 		return nil, err
 	}
 	if len(rangeList) == 0 {
-		rangeList, err = parseSubuid(usr.Name)
+		rangeList, err = user.ParseSubIDFileFilter(path, func(sid user.SubID) bool {
+			return sid.Name == usr.Name
+		})
 		if err != nil {
 			return nil, err
 		}
 	}


Had to double-check this one, because the first filter was matching numeric Uid with SubID.Name, but looks like moby/sys/user does the same, and checking the man-page; https://man7.org/linux/man-pages/man5/subuid.5.html

Each line in /etc/subuid contains a user name and a range of
subordinate user ids that user is allowed to use. This is
specified with three fields delimited by colons (“:”). These
fields are:

• login name or UID
• numerical subordinate user ID
• numerical subordinate user ID count

One thing we could do is to take the same approach as is used there, and to combine them to a single filter;

moby/vendor/github.com/moby/sys/user/lookup_unix.go

Lines 132 to 141 in b2450ff

func currentUserSubIDs(fileName string) ([]SubID, error) {

u, err := CurrentUser()

if err != nil {

return nil, err

}

filter := func(entry SubID) bool {

return entry.Name == u.Name || entry.Name == strconv.Itoa(u.Uid)

}

return ParseSubIDFileFilter(fileName, filter)

}

pkg/idtools/idtools_windows.go

internal/usergroup/parser_test.go

internal/usergroup/parser.go

internal/usergroup/add_linux.go

Separare idtools functionality that is used internally from the functionlality used by importers. The `pkg/idtools` package is now much smaller and more generic. Signed-off-by: Derek McGowan <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Derek McGowan <[email protected]>

thaJeztah

LGTM

dmcgowan requested a review from tonistiigi as a code owner December 13, 2024 06:40

dmcgowan force-pushed the split-idtools-internal branch 2 times, most recently from 7c4962e to d854eb6 Compare December 13, 2024 07:02

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code area/go-sdk labels Dec 13, 2024

thaJeztah added this to the 28.0.0 milestone Dec 13, 2024

thaJeztah reviewed Dec 13, 2024

View reviewed changes

dmcgowan force-pushed the split-idtools-internal branch from d854eb6 to b42c6ad Compare December 13, 2024 16:56

dmcgowan mentioned this pull request Dec 14, 2024

Move pkg/archive out of main module/repository #49069

Closed

7 tasks

github-advanced-security bot found potential problems Dec 19, 2024

View reviewed changes

internal/usergroup/parser.go Fixed Show fixed Hide fixed

internal/usergroup/parser.go Fixed Show fixed Hide fixed

thaJeztah mentioned this pull request Dec 21, 2024

Find a place for /pkg #32989

Open

79 tasks

thaJeztah reviewed Dec 21, 2024

View reviewed changes

thaJeztah mentioned this pull request Dec 23, 2024

pkg/system: deprecate MkdirAll and remove custom volume GUID handling #49162

Merged

thaJeztah force-pushed the split-idtools-internal branch from 75a8139 to cb7934c Compare December 23, 2024 12:02

This was referenced Dec 23, 2024

vendor: github.com/docker/docker, github.com/docker/cli v28.0.0-rc.1 moby/buildkit#5602

Merged

vendor: github.com/docker/docker 69687190936d (master, v28.0-dev) docker/cli#5708

Merged

thaJeztah force-pushed the split-idtools-internal branch 2 times, most recently from 1823ba5 to a2d1047 Compare January 1, 2025 15:30

dmcgowan force-pushed the split-idtools-internal branch from a2d1047 to 376f122 Compare January 4, 2025 02:05

thaJeztah reviewed Jan 6, 2025

View reviewed changes

pkg/idtools/idtools_unix.go Outdated Show resolved Hide resolved

pkg/idtools/idtools_windows.go Show resolved Hide resolved

pkg/idtools/idtools_unix.go Show resolved Hide resolved

pkg/idtools/idtools_unix.go Show resolved Hide resolved

dmcgowan force-pushed the split-idtools-internal branch from 376f122 to 0f356a5 Compare January 7, 2025 06:25

This was referenced Jan 7, 2025

[WIP] Split idtools to an internal package and package to be moved (changed order of commits) #49225

Closed

pkg/idtools: rewrite to use moby/sys/user #49226

Merged

thaJeztah reviewed Jan 7, 2025

View reviewed changes

dmcgowan added 2 commits January 7, 2025 11:18

Update internal use of idtools to usergroup

3fa5e7e

Signed-off-by: Derek McGowan <[email protected]>

dmcgowan force-pushed the split-idtools-internal branch from 0f356a5 to 3fa5e7e Compare January 7, 2025 19:21

thaJeztah approved these changes Jan 7, 2025

View reviewed changes

thaJeztah merged commit 6c523af into moby:master Jan 7, 2025
142 checks passed

thaJeztah mentioned this pull request Feb 6, 2025

pkg/idtools: un-deprecate Windows consts for now #49405

Merged

	func currentUserSubIDs(fileName string) ([]SubID, error) {
	u, err := CurrentUser()
	if err != nil {
	return nil, err
	}
	filter := func(entry SubID) bool {
	return entry.Name == u.Name \|\| entry.Name == strconv.Itoa(u.Uid)
	}
	return ParseSubIDFileFilter(fileName, filter)
	}

Comments

Conversation

dmcgowan commented Dec 13, 2024 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah Dec 21, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Jan 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmcgowan commented Dec 13, 2024 •

edited by thaJeztah

Loading