cli-plugins: terminate plugin when CLI exits

[laurazard]

- What I did

Previously, long lived CLI plugin processes weren't properly handled (see: #4402) resulting in plugin processes being left behind running, after the CLI process exits.

- How I did it

Change the plugin handling code to open an abstract unix socket before running the plugin and passing it to the plugin process, and changes the signal handling on the CLI side to close this socket which tells the plugin that it should exit.

The 3 SIGINTs/SIGTERMs logic is copied over/kept from cmd/docker/internal/appcontext (previously github.com/moby/buildkit/util/appcontext) (see #4457).

This implementation makes use of sockets instead of simply setting PDEATHSIG on the plugin process so that it will work on most platforms (including Windows :'))

- How to verify it

Before:

Screen.Recording.2023-10-12.at.21.49.02.mov

After:

Screen.Recording.2023-10-12.at.22.04.25.mov

Or, build your plugin with the CLI code from my branch :)

- Description for the changelog

Fix issue with long lived CLI plugin processes not being terminated after the CLI exits in some cases.

- A picture of a cute animal (not mandatory but encouraged)

[laurazard]

Have to check some CI failures, a lot of tests calling Compose breaking (not sure if due to parallel tests running into the issue described above or due to other issues with listening on an abstract namespace sockets in CI).

[codecov-commenter]

Codecov Report

Merging #4599 (5e394d0) into master (a46f850) will decrease coverage by 0.44%.
Report is 162 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4599      +/-   ##
==========================================
- Coverage   59.71%   59.27%   -0.44%     
==========================================
  Files         288      285       -3     
  Lines       24858    24858              
==========================================
- Hits        14843    14735     -108     
- Misses       9128     9237     +109     
+ Partials      887      886       -1     

[neersighted]

🐩

[thaJeztah]

We really need to order a bork® T-shirt

[thaJeztah]

(And a børk® 🇸🇪 for David)

[laurazard]

🐶

[neersighted]

Overall looks really promising, I'm glad that my ramble on solving this using a socket was helpful.

For others, Win32 job control can technically be used to solve this on Windows, but macOS still exists.

We could poll getppid(2), but we'd need a busy loop, spam system calls (causing context switching), and it's full of edge-cases like PR_SET_CHILD_SUBREAPER or the fact that Solaris and Android have different init PIDs in some cases.

Letting the kernel notify us when the parent has either died, or has asked us to exit (solving the non-interactive SIGINT/SIGTERM case) seems best to me.

I'm still not sure if the 'three signal then exit' logic should apply in the non-interactive case, but we do just fine with it in dockerd as @laurazard pointed to me. Similarly, I'm not sure if we should handle SIGINT ourselves when there is a TTY in the loop, but I think this should be good enough as-is, and we can figure out further edge-cases once we get this in and have others kick the tires.

[thaJeztah]

Failures in e2e;

got a weird error! accept unix @docker_cli_9bd0ef8a-2f24-441f-85d9-fcab0b79d535: use of closed network connection

erm... @neersighted would this be because of ...

• seccomp: block socket calls to AF_VSOCK in default profile moby/moby#44562

[neersighted]

No, this is AF_UNIX in the abstract domain (read: not a path, NUL bytes don't delimit the socket name, instead the addrlen defines when the socket name ends), not AF_VSOCK.

I'm not sure why we're getting a net.ErrClosed here; we should get EINVAL if the socket isn't listening.

[laurazard]

I was going to say, it seems to be failing only when running inside of a container. I still want to change that code to fallback to the previous behaviour when we can't establish a connection, so that should be okay after those changes, but I'd like to figure out why it's not working.

[thaJeztah]

Yeah, had my wires crossed on the AF_VSOCK; ended up on that as I was wondering if there's something different when running in a container 🥹☺️

[neersighted]

Not sure what's going on there either, it will take some digging into. containerd's v1 shim API used abstract sockets until containerd/containerd@126b35c, so it should work in the general case DinD...

[laurazard]

Adding this here as it seems like the most relevant place:

At the last maintainers call, it was suggested that a more elegant solution to this could be built, relying on another OS primitive – unnamed pipes – for communication between the CLI and the plugin process. In this implementation, instead of generating a random abstract socket name/passing it as an environment variable, we'd create an unnamed pipe on the side of the CLI process, and set the FD 3 (or another non-0/1/2 FD) of the plugin process as that pipe, which could then use it to be alerted to exit.

---

Note

This is my current understanding of the issue, which is definitely incomplete as my knowledge of Windows internals is extremely limited 😅

This would be a great solution, but in practice it falls short as Go does not have built-in support for inheriting file descriptors on Windows. In truth, "file descriptors" aren't a concept on Windows, and although this should be possible to do using file handles, there's a lot of complexity regarding how to pass the desired file handle to the child process.

While, on Unixy platforms, we can do:

reader, writer, err :=  os.Pipe()
if err == nil {
    defer writer.Close()
    plugincmd.ExtraFiles = append(plugincmd.ExtraFiles, reader)
}

and then simply access FD 3 on the child process, a Windows implementation would have to look like:

reader, writer, err :=  os.Pipe()
if err == nil {
    defer writer.Close()
    plugincmd.SysProcAttr = &syscall.SysProcAttr{AdditionalInheritedHandles: []syscall.Handle{syscall.Handle(reader.Fd())}}
}

and we would still have to pass the file handle as an environment variable/some other way so that the child process knows where to access it, as we can't just "access FD 3".

This is because cmd.ExtraFiles is not supported on Windows – more on this issue in golang/go#21085 (comment).

(For the more curious, check out https://go-review.googlesource.com/c/go/+/288299, the Go CL where os.Pipes were made inheritable on Windows, and how the feature is tested – it involves passing the pipe handle as an argument when running the child command)

We could implement this feature that way (more likely, passing the handle as an environment variable), but then we end up on the same different implementation for each platform place that we're trying to avoid.

I think, taking this into account, the abstract socket/AF_UNIX implementation is preferable.

(cc @corhere @cpuguy83 @thaJeztah)

---

Other alternatives, for future reference:

• Named pipes suffer from much the same issue, as we'd have to write our own implementation/rely on go-winio for them on Windows
• PDEATHSIG works on Unix, but not Windows, and the same/similar behaviour can be obtained on Windows with Job Objects, but then we still need a solution for macOS.

[corhere]

and we would still have to pass the file handle as an environment variable/some other way so that the child process knows where to access it, as we can't just "access FD 3".

Yeah, it is annoying that we can't predict the Windows handle value at compile-time, unlike the tricks which can be played with POSIX file descriptors. We would have to have separate implementations of anonymous pipe IPC for Windows and Unix, but at least Linux and macOS could share one implementation.

One distinct advantage of anonymous pipes over abstract UNIX sockets is their anonymity: there is no name by which unrelated processes could reference it, intentionally or accidentally. However this is not a dealbreaker for abstract sockets at this time as the IPC channel is only being used for lifecycle management. The worst a malicious process could reasonably do is DoS a plugin. I'd suggest keeping the anonymous-pipe design in your back pocket just in case abstract sockets don't work out for some reason. Until then, I completely agree with Keeping It Simple using AF_UNIX.

[thaJeztah]

We could implement this feature that way (more likely, passing the handle as an environment variable), but then we end up on the same different implementation for each platform place that we're trying to avoid.

Let me interject for a moment. What you guys are referring to as Linux .. (edit: sorry, damn you, autocomplete)

While I think an identical implementation would be "great", I would not consider it to be a strict requirement.

I know there's been many cases we (and the Go team itself for stdlib funcionality) have tried to provide a platform-agnostic approach, this also has resulted in many situations where trying to shoehorn Windows into Unix semantics resulted in poor choices.

While I think it's still preferable if we implement library functions (it's great if we and can provide a platform-agnostic API / signature), we should not limit / force ourselves to do so if that means a more complicated and/or less idiomatic and/or less optimized/stable implementation.

In some cases there are significant differences between Windows and Unix ("non-Windows"), and for some of those, it may be better to admit those differences, and choose the best / most stable solution for each.

Given that this particular code is an implementation and not a "library package" (or at least: it's an internal package, and does not require a public Go API), I'm not against having platform-specific implementations if that means we can optimize each with platform-specific semantics (potentially making it more stable).

I think the goal here is to provide the same / similar experience from a user's perspective; if that requires a different implementation under the hood for each, that's an implementation detail.

(All my $0.02c 😅)

[thaJeztah]

LGTM! ❤️

[thaJeztah]

Looks like we missed this during review ☺️ 🙈 ;

• socket: return from loop after EOF #4805