Update copy.DirCopy to leave sockets in the file system

[anderbubble]

copy.DirCopy contains code to create named sockets matching those on the source; however, this operation closes the socket, removing it implicitly from the destination and causing later chown and chmod operations to fail.

This commit creates matching file system nodes directly without opening a socket or invoking the auto-removal semantics of a listener.

• Closes copy.DirCopy fails when encountering sockets #2113

[anderbubble]

If possible, I'd like to see this backported to 1.55 (before the shift to go 1.22) so that we could use it in Warewulf on EL 8 and 9.

[rhatdan]

@mtrmac @giuseppe @nalind @Luap99 PTAL

[mtrmac]

At a first glance this makes no sense to me as proposed: if we don’t close that socket, what is going to close it?

This code needs to work in long-running processes like CRI-O, so leaking file descriptors is not acceptable.

(And, au contraire, if Go’s garbage collector somehow caused the socket to be closed eventually, then that means that the problematic chown/chmod operations could also fail, depending on GC timing.)

---

It might well be the case that we need to keep the socket open longer, and close it elsewhere; I’m not saying that the current code is correct, I didn’t look into that at the detail, at this point I just took the 1 minute to notice that the PR as currently proposed looks incorrect.

[anderbubble]

I thought that this was just a difference in semantics between a TCP socket and a Unix domain socket; but turns out Go just mixes close with unlink. What I really want is SetUnlinkOnClose(false).

I'll fix it. Thanks for the review.

[rhatdan]

/approve
LGTM

[Luap99]

I am not familiar with this code but why are we copying socket files in the first place? I don't see a how any user could benefit from a not connected socket file in the target.
So would it not be simpler to skip socket files all together?

[anderbubble]

why are we copying socket files in the first place

I can’t speak to this: before I started investigating I expected the solution would be to skip sockets as well. But when I found code that was explicitly trying to copy them, but just failing to do so properly, I decided I would start by making the existing code work, rather than remove it, since I didn’t know the original intent.

in theory, I suppose the existence of the socket could be a sentinel to some other application. I also think the existing socket can be re-opened by later applications at the existing path.

[Luap99]

in theory, I suppose the existence of the socket could be a sentinel to some other application.

Maybe but this seems rather unlikely

I also think the existing socket can be re-opened by later applications at the existing path.

I do not see how this could work, binding an existing socket file will fail with EADDRINUSE even when there is no other program actively listening there. As such most programs do an explicit unlink() before calling bind().

Given the current version deletes the path again (and then fails to chown) it should be safe to assume that no user can depend on the behavior and I personally see skipping sockets to be much simpler than doing this here.

[anderbubble]

The original socket code was added by @giuseppe in 029896b. No objection from me removing it / altering it to skip sockets in stead; but maybe he has insight into why it was added in the first place.

Edit: I guess that's not quite right: he just converted it from mkfifo. I'll investigate further.

[anderbubble]

Looks like the first commit at 6a9fa6b includes sockets and was authored by @nalind, so he might have insight into the original intent as well.

[giuseppe]

I think this was done without thinking too much about its implications, so let's ignore them. Should we do the same with FIFOs?

[mtrmac]

In Docker, this exists all the way back in moby/moby#7619 .

---

Note the existence of

storage/drivers/fsdiff.go

Line 45 in 4bf3f07

func (gdw *NaiveDiffDriver) Diff(id string, idMappings *idtools.IDMappings, parent string, parentMappings *idtools.IDMappings, mountLabel string) (arch io.ReadCloser, err error) {

; just the presence of files might actually make a difference to the reported layer contents.

I don’t know that the files would be useful, but if a pull+push changed the filesystem contents, that might be unexpected.

[mtrmac]

(Let’s decide whether we need these files at all, first.)

[Luap99]

I did take a quick look what gnu cp does. With cp -r it will copy the socket file, however it does not the socket(), bind(), close() thing we are doing here.
Instead it just calls mknodat() looking at the strace output
mknodat(AT_FDCWD, "/tmp/test2/mysock", S_IFSOCK|0700) = 0

relevant source code: https://github.com/coreutils/coreutils/blob/e71b24cedc2ff640f94911298e53b24506f13062/src/copy.c#L3090-L3099

Thus I would say we should prefer a mknod() call here over the current fix.

---

Though this still doesn't answer if we have to keep socket files during copy.

I don’t know that the files would be useful, but if a pull+push changed the filesystem contents, that might be unexpected.

I guess if this is a practical concern then keeping them makes sense.

[giuseppe]

I don’t know that the files would be useful, but if a pull+push changed the filesystem contents, that might be unexpected.

I guess if this is a practical concern then keeping them makes sense.

AFAICS a tarball cannot handle unix sockets (https://pkg.go.dev/archive/tar#pkg-constants), so that shouldn't be a concern.

I did take a quick look what gnu cp does. With cp -r it will copy the socket file, however it does not the socket(), bind(), close() thing we are doing here. Instead it just calls mknodat() looking at the strace output mknodat(AT_FDCWD, "/tmp/test2/mysock", S_IFSOCK|0700) = 0

Using mknod seems like a good idea to fix the current code without changing its behavior

[mtrmac]

AFAICS a tarball cannot handle unix sockets (https://pkg.go.dev/archive/tar#pkg-constants)

Thanks, TIL.

[rhatdan]

@anderbubble what is going on with this PR?

[anderbubble]

@rhatdan I'll update it based on the conversation here; I just got pulled into some other things for a bit.

[anderbubble]

@rhatdan I updated the pr to use mknod rather than opening a dangling socket.

I think it's good to go now; but let me know if you have any questions or concerns. Sorry for the delay!

[rhatdan]

@giuseppe @Luap99 @mtrmac PTAL

[giuseppe]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anderbubble, giuseppe, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

/lgtm
Thanks @anderbubble

[anderbubble]

Will this be automatically backported to 1.55, or do I need to file a separate PR for that?

[giuseppe]

it needs to be backported

[anderbubble]

Backport PR at #2159. Let me know if that needs any adjustment.