Fix file capabilites droping in Dockerfile

[abdulrahimiliasu]

fixes #42655

- What I did
moved copyXattr function out of doCopyXattrs function, so that security capabilities are copied
- How I did it
doCopyXattrs() never reached due to copyXattrs boolean being false, as a result file capabilities not being copied.

- How to verify it

Test Case

FROM registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
RUN apk --no-cache add libcap && setcap cap_net_admin=eip /sbin/apk
RUN setcap -v cap_net_admin=eip /sbin/apk

Test Result

Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
latest: Pulling from library/alpine
a0d0a0d46f8b: Pull complete
Digest: sha256:e1c082e3d3c45cccac829840a25941e679c25d438cc8412c2fa221cf1a824e6a
Status: Downloaded newer image for registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
 ---> 14119a10abf4
Step 2/3 : RUN apk --no-cache add libcap && setcap cap_net_admin=eip /sbin/apk
 ---> Running in 923b46395907
fetch https://dl-cdn.alpinelinux.org/alpine/v3.14/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.14/community/x86_64/APKINDEX.tar.gz
(1/1) Installing libcap (2.50-r0)
Executing busybox-1.33.1-r3.trigger
OK: 6 MiB in 15 packages
Removing intermediate container 923b46395907
 ---> 97d6ec51d4b3
Step 3/3 : RUN setcap -v cap_net_admin=eip /sbin/apk
 ---> Running in 33260d42c77d
/sbin/apk: OK
Removing intermediate container 33260d42c77d
 ---> 5fc3c8660150
Successfully built 5fc3c8660150
Successfully tagged test:cap

- Description for the changelog
Fixed issue of file capabilities dropping when moving to next command in Dockerfile during image building.

[thaJeztah]

doCopyXattrs() never reached due to copyXattrs boolean being false

A mentioned on #42655 (comment), the copyXattrs option was explicitly added at some point. This change looks to work around that by ignoring the option, which feels incorrect;

I see this line explicitly disables copyXattrs (last argument);

moby/daemon/graphdriver/vfs/copy_linux.go

Line 6 in 7f5e39b

return copy.DirCopy(srcDir, dstDir, copy.Content, false)

That was added in d2b71b2 (part of #35537). Before that it seems like it was enabled by default.

Perhaps @sargun or @cpuguy83 recalls why it's disabled for vfs; perhaps the expectation was that the copy_file_range would/should take care of this?

If this code must be called for VFS, I guess the boolean should be set to true, but we should have an understanding on why it was disabled instead of ignoring

[abdulrahimiliasu]

That's correct the Boolean should be set to true for those capabilities to be copied, but this change only makes sure that the
"security-capability" attribute are copied. Everything else stay the same. But I do not know the specific reason for why copyXattrs default to false.

[szubersk]

@thaJeztah I can't speak for @sargun but the feeling I get is that copying xattrs has been removed to speed copying up. d2b71b2 Have VFS graphdriver use accelerated in-kernel copy introduces two major optimizations: copy_file_range usage to transfer file content without switching from kernel- to userspace and back and xattrs ignoring.

copy_file_range can't take care of file attributes (or any other metadata) as it operates only on file content.

ssize_t copy_file_range(int fd_in, off64_t *off_in,
                        int fd_out, off64_t *off_out,
                        size_t len, unsigned int flags);

copyXattr requires one getxattr(2) call per file/directory + memory allocation (sometimes two) to store the result. No wonder @sargun was looking for an optimization there.

moby/daemon/graphdriver/copy/copy.go

Lines 90 to 91 in c09789c

	func copyXattr(srcPath, dstPath, attr string) error {
	data, err := system.Lgetxattr(srcPath, attr)

moby/pkg/system/xattrs_linux.go

Lines 8 to 11 in c09789c

	func Lgetxattr(path string, attr string) ([]byte, error) {
	// Start with a 128 length byte array
	dest := make([]byte, 128)
	sz, errno := unix.Lgetxattr(path, attr, dest)

I'd lean towards suggestion of unconditionally enabling xattrs copying as current implementation is simply incorrect.

[abdulrahimiliasu]

I've updated the commit by changing copyXattrs to true instead of moving the block

moby/daemon/graphdriver/copy/copy.go

Lines 259 to 261 in 33a3680

	if err := copyXattr(srcPath, dstPath, "security.capability"); err != nil {
	return err
	}

out of

moby/daemon/graphdriver/copy/copy.go

Line 258 in 33a3680

func doCopyXattrs(srcPath, dstPath string) error {

[sargun]

You should only copy the security.capability xattr in the VFS graphdriver, and not the trusted.overlay.opaque one.

[szubersk]

@sargun @thaJeztah Is the PR in current form ok with you?

[szubersk]

@sargun @thaJeztah
Is there an issue that prevent merging this PR?

[mikebrow]

nit.. comments for the modified DirCopy() func and field name copyXattrs should be modified to reflect the boolean is just for switching off copying xattr trusted.overlay.opaque

[mikebrow]

LGTM functionally.. just the nit comment regarding variable naming and existing api // comments

[thaJeztah]

I rebased the PR, renamed the copyXattrs argument to copyOpaqueXattrs, and updated the GoDoc for CopyDir()

[thaJeztah]

LGTM, thanks! And sorry for the long delay; this one dropped of my radar, and I saw the PR was still marked with "missing DCO", but that looks to have been resolved since.

[mikebrow]

LGTM