Decouple pkg/archive from pkg/system by dmcgowan · Pull Request #49072 · moby/moby

dmcgowan · 2024-12-12T06:53:32Z

Decouples the pkg/archive and pkg/system to prepare pkg/archive for moving.

This also allowed quite a bit of cleanup due to unnecessary interfaces and workarounds for older versions of the unix package.

Related to Move pkg/archive out of main module/repository #49069
Related to Move pkg/archive and dependencies out into their own repositories #21700
Related to Find a place for /pkg #32989

pkg/archive/time_nonwindows.go

thaJeztah · 2024-12-12T07:47:36Z

pkg/archive/copy.go

 		}

-		if !system.IsAbs(linkTarget) {
+		if !filepath.IsAbs(linkTarget) {


Possibly we need to copy the system.IsAbs here for cross platform; see

moby/pkg/system/filesys.go

Lines 9 to 19 in 7faa4ec

// IsAbs is a platform-agnostic wrapper for filepath.IsAbs.

//

// On Windows, golang filepath.IsAbs does not consider a path \windows\system32

// as absolute as it doesn't start with a drive-letter/colon combination. However,

// in docker we need to verify things such as WORKDIR /windows/system32 in

// a Dockerfile (which gets translated to \windows\system32 when being processed

// by the daemon). This SHOULD be treated as absolute from a docker processing

// perspective.

func IsAbs(path string) bool {

return filepath.IsAbs(path) || strings.HasPrefix(path, string(os.PathSeparator))

}

☝️ looks like I wrote that up earlier today, but didn't submit. Per discussion in the call; we may not need this, or indeed can create a local copy of it.

We shouldn't need it here since we are not running it on non-sanitized input

pkg/archive/time_nonwindows.go

thaJeztah · 2024-12-15T20:22:02Z

pkg/archive/path_unix.go

+// lsetxattr sets the value of the extended attribute identified by attr
+// and associated with the given path in the file system.
+func lsetxattr(path string, attr string, data []byte, flags int) error {
+	return unix.Lsetxattr(path, attr, data, flags)
+}


Looks like this implementation of set/get xattr didn't preserve the improvements that were made to provide more information about the name of the attribute that failed;

Fail unpacking images with xattrs to filesystems without xattr support #45464

pkg/system: return even richer xattr errors #47174

Should we preserve that?

I'll put the path error back and just wrap the error there. Introducing a new error type is not needed and creates another dependency.

Yeah, I don't think the error-type was matched anywhere, but it was purely for formatting (?)

thaJeztah · 2024-12-16T15:07:22Z

did a quick rebase after #49098 was merged

thaJeztah

Looks like this needs to add back some platform-specific implementations that were provided by pkg/system (see comments)

thaJeztah · 2024-12-17T10:45:18Z

pkg/archive/archive_unix.go

-	return system.Mknod(path, mode, int(system.Mkdev(hdr.Devmajor, hdr.Devminor)))
+	return unix.Mknod(path, mode, int(unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))))


This needs differentiation, because the new implementation doesn't support (Free)BSD; moby/buildkit#5602

49.21 # github.com/docker/docker/pkg/archive 49.21 vendor/github.com/docker/docker/pkg/archive/archive_unix.go:111:32: cannot use int(unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))) (value of type int) as uint64 value in argument to unix.Mknod

Yes see also https://github.com/tonistiigi/fsutil/blob/bec5f0e1255330c04d8c05315b5a3cd3fa3e1590/diskwriter_freebsd.go#L12

thaJeztah · 2024-12-17T10:45:26Z

pkg/archive/path_unix.go

+		if errors.Is(err, unix.ENODATA) {
+			return nil, nil
+		}


Here as well; looks like this doesn't support (Free)BSD; moby/buildkit#5602

49.21 vendor/github.com/docker/docker/pkg/archive/path_unix.go:38:26: undefined: unix.ENODATA

Looks like FreeBSD has ENOATTR instead:

If the named attribute does not exist, or the process has no access to this attribute, errno is set to ENOATTR.

87 ENOATTR Attribute not found. The specified extended attribute does not exist.

thaJeztah · 2024-12-17T12:09:43Z

pkg/archive/path_unix.go

+		// Buffer too small, use zero-sized buffer to get the actual size
+		sz, err = unix.Lgetxattr(path, attr, []byte{})


More failures in the buildx repository, which also runs on OpenBSD (looks like xattrs are not supported there);

vendor: docker, docker/cli v28.0.0-rc.1 docker/buildx#2868

#25 [linux/amd64->openbsd/amd64 buildx-build 1/1] RUN --mount=type=bind,target=. --mount=type=cache,target=/root/.cache --mount=type=cache,target=/go/pkg/mod --mount=type=bind,from=buildx-version,source=/buildx-version,target=/buildx-version <<EOT (set -e...) #25 0.070 + CGO_ENABLED=0 go build -mod vendor -trimpath -ldflags '-X github.com/docker/buildx/version.Version=53dca9a -X github.com/docker/buildx/version.Revision=53dca9a8946feca5ba7a63cb96140d94f2067656 -X github.com/docker/buildx/version.Package=github.com/docker/buildx -s -w' -o /usr/bin/docker-buildx ./cmd/buildx #25 35.10 # github.com/docker/docker/pkg/archive #25 35.10 vendor/github.com/docker/docker/pkg/archive/path_unix.go:25:18: undefined: unix.Lgetxattr #25 35.10 vendor/github.com/docker/docker/pkg/archive/path_unix.go:29:18: undefined: unix.Lgetxattr #25 35.10 vendor/github.com/docker/docker/pkg/archive/path_unix.go:34:18: undefined: unix.Lgetxattr #25 35.10 vendor/github.com/docker/docker/pkg/archive/path_unix.go:38:26: undefined: unix.ENODATA #25 35.10 vendor/github.com/docker/docker/pkg/archive/path_unix.go:50:53: undefined: unix.Lsetxattr

Previously this code was in _linux.go file, so the xattrs were not used on BSD before. I think we can leave making the BSD work for a follow up.

Updated, it should match what is defined in golang.org/x/sys/unix now

dmcgowan · 2024-12-17T18:30:48Z

Updated, @thaJeztah can you bump the golang sys package in that buildx PR

thaJeztah · 2024-12-17T18:40:48Z

Updated;

vvoland

LGTM

thaJeztah

overall LGTM

Left one question (before blaming myself not asking), and some minor nits; no blockers AFAICS, just "touching up" in case you're doing another push.

thaJeztah · 2024-12-18T14:33:17Z

pkg/archive/time.go

+const maxSeconds = (1<<63 - 1) / int64(1e9)
+
+func sanitizeTime(t time.Time) time.Time {
+	if ut := t.Unix(); ut < 0 || ut > maxSeconds {
+		return time.Unix(0, 0)
+	}
+	return t
+}


Just to make sure I blame myself not asking; I recall there was some special handling for 32-bit; is that no longer needed, or was that only needed for Windows situations?

moby/pkg/system/chtimes.go

Lines 10 to 27 in bdfc384

// Used by Chtimes

var unixEpochTime, unixMaxTime time.Time

func init() {

unixEpochTime = time.Unix(0, 0)

if unsafe.Sizeof(syscall.Timespec{}.Nsec) == 8 {

// This is a 64 bit timespec

// os.Chtimes limits time to the following

//

// Note that this intentionally sets nsec (not sec), which sets both sec

// and nsec internally in time.Unix();

// https://github.com/golang/go/blob/go1.19.2/src/time/time.go#L1364-L1380

unixMaxTime = time.Unix(0, 1<<63-1)

} else {

// This is a 32 bit timespec

unixMaxTime = time.Unix(1<<31-1, 0)

}

}

I'm just going to pull in containerd's implementation for this, at some point we should have this unified

pkg/archive/xattr_unsupported.go

thaJeztah · 2024-12-18T14:42:13Z

pkg/archive/dev_freebsd.go

+
+import "golang.org/x/sys/unix"
+
+var mknod = unix.Mknod


More a "comment" than "need to change". I took this approach as well in some cases, but found that sometimes it became confusing, because this will be listed under variables in GoDoc, not under functions. Less relevant for non-exported functions, but for that reason I sometimes started to lean towards adding a wrapper function instead, i.e..

func mknod(path string, mode uint32, dev uint64)) error { return unix.Mknod(path, mode, dev) }

(no need to change, fine as-is IMO)

pkg/archive/time.go

pkg/archive/time_nonwindows.go

pkg/archive/time_windows.go

Update archive time logic to mirror containerd's Signed-off-by: Derek McGowan <[email protected]>

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

These were previously only used for pkg/archive Signed-off-by: Derek McGowan <[email protected]>

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

thaJeztah

LGTM

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

View reviewed changes

pkg/archive/time_nonwindows.go Show resolved Hide resolved

dmcgowan mentioned this pull request Dec 12, 2024

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

Closed

7 tasks

dmcgowan force-pushed the decouple-archive-system branch from d0a8986 to 670cc2d Compare December 12, 2024 07:38

dmcgowan marked this pull request as ready for review December 12, 2024 08:17

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

thaJeztah reviewed Dec 15, 2024

View reviewed changes

thaJeztah force-pushed the decouple-archive-system branch from 670cc2d to 61d01b1 Compare December 16, 2024 15:07

thaJeztah mentioned this pull request Dec 16, 2024

vendor: github.com/docker/docker ad6929339acd (master, v28.0.0-dev) docker/cli#5703

Merged

2 tasks

dmcgowan force-pushed the decouple-archive-system branch 2 times, most recently from eacfa48 to efd6a9f Compare December 16, 2024 23:21

thaJeztah mentioned this pull request Dec 17, 2024

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

Merged

8 tasks

thaJeztah requested changes Dec 17, 2024

View reviewed changes

thaJeztah mentioned this pull request Dec 17, 2024

vendor: docker, docker/cli v28.0.0-rc.1 docker/buildx#2868

Merged

1 task

thaJeztah reviewed Dec 17, 2024

View reviewed changes

dmcgowan force-pushed the decouple-archive-system branch from efd6a9f to 95a218f Compare December 17, 2024 18:20

vvoland approved these changes Dec 18, 2024

View reviewed changes

thaJeztah approved these changes Dec 18, 2024

View reviewed changes

dmcgowan added 3 commits December 19, 2024 10:13

Update archive to use time operations directly

1b4cbea

Update archive time logic to mirror containerd's Signed-off-by: Derek McGowan <[email protected]>

Update archive to use its own xattr funcs

35b9525

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

Update archive to use unix.Mknod directly

bb3e95d

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

dmcgowan added 3 commits December 19, 2024 10:13

Update archive to use fs.FileInfo over custom stat

12b2b56

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

Remove unused pkg/system functions

e4236c9

These were previously only used for pkg/archive Signed-off-by: Derek McGowan <[email protected]>

Remove import comments

315891d

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

dmcgowan force-pushed the decouple-archive-system branch from 95a218f to 315891d Compare December 19, 2024 18:16

thaJeztah approved these changes Dec 19, 2024

View reviewed changes

thaJeztah merged commit b677cf9 into moby:master Dec 19, 2024

thaJeztah deleted the decouple-archive-system branch December 19, 2024 19:40

thaJeztah mentioned this pull request Jan 3, 2025

golangci-lint: remove temporary exception for deprecated code #49211

Merged

	// IsAbs is a platform-agnostic wrapper for filepath.IsAbs.
	//
	// On Windows, golang filepath.IsAbs does not consider a path \windows\system32
	// as absolute as it doesn't start with a drive-letter/colon combination. However,
	// in docker we need to verify things such as WORKDIR /windows/system32 in
	// a Dockerfile (which gets translated to \windows\system32 when being processed
	// by the daemon). This SHOULD be treated as absolute from a docker processing
	// perspective.
	func IsAbs(path string) bool {
	return filepath.IsAbs(path) \|\| strings.HasPrefix(path, string(os.PathSeparator))
	}

		return system.Mknod(path, mode, int(system.Mkdev(hdr.Devmajor, hdr.Devminor)))
		return unix.Mknod(path, mode, int(unix.Mkdev(uint32(hdr.Devmajor), uint32(hdr.Devminor))))

		// Buffer too small, use zero-sized buffer to get the actual size
		sz, err = unix.Lgetxattr(path, attr, []byte{})

	// Used by Chtimes
	var unixEpochTime, unixMaxTime time.Time

	func init() {
	unixEpochTime = time.Unix(0, 0)
	if unsafe.Sizeof(syscall.Timespec{}.Nsec) == 8 {
	// This is a 64 bit timespec
	// os.Chtimes limits time to the following
	//
	// Note that this intentionally sets nsec (not sec), which sets both sec
	// and nsec internally in time.Unix();
	// https://github.com/golang/go/blob/go1.19.2/src/time/time.go#L1364-L1380
	unixMaxTime = time.Unix(0, 1<<63-1)
	} else {
	// This is a 32 bit timespec
	unixMaxTime = time.Unix(1<<31-1, 0)
	}
	}

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 16, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmcgowan commented Dec 17, 2024

Uh oh!

thaJeztah commented Dec 17, 2024

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmcgowan commented Dec 12, 2024 •

edited by thaJeztah

Loading

thaJeztah Dec 15, 2024 •

edited

Loading