plugin: update/improve process lifecycle documentation

[neersighted]

Follow-up to #4905, making comment/documentation improvements I didn't want to block merge of that PR with.

[codecov-commenter]

Codecov Report

Merging #4960 (542e82c) into master (318911b) will increase coverage by 61.79%.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4960       +/-   ##
===========================================
+ Coverage        0   61.79%   +61.79%     
===========================================
  Files           0      289      +289     
  Lines           0    20267    +20267     
===========================================
+ Hits            0    12523    +12523     
- Misses          0     6858     +6858     
- Partials        0      886      +886     

[Benehiko]

LGTM

[thaJeztah]

Hm... and I just realised we may have picked a very wrong name for this env-var, or at least the GoDoc should probably be very clear that this is an internal env-var, used between the CLI (acting as "host" for plugins) and binaries running as CLI-plugin.

• We use the all-caps DOCKER_XXXXXX format for user-facing env-vars; in this case, we don't want users to (e.g.) DOCKER_CLI_PLUGIN_SOCKET=<whatever> /usr/libexec/docker/cli-plugins/docker-compose ....
• ☝️ I don't think the above would do anything if the plugin is running "standalone" (if it would it could possibly rm the given path if it's a macOS binary?)
• ☝️ BUT we should probably document that in the GoDoc regardless; explicitly call out this is an internal-use env-var, and not to be set by the user.
• ☝️ Paranoid me is even considering; do we need to actively unset the env-var when found? (To prevent any chance it leaks through somewhere?)

Should we change it to something more clearly meant for "internal"? Any good conventions for that?

• Perhaps lowercase? (docker_cli_plugin_socket)
• Perhaps prefix with _ / __ (__docker_cli_plugin_socket); could even have a _addr suffix to indicate it's the address (__docker_cli_plugin_socket_addr).
• ☝️ pedantic me was even considering "version it" (__docker_cli_plugin_socket_addr_v1), but I guess we can do so if we would need a v2
• Maybe something mentioning "internal" (__docker_cli_plugin_internal_socket_addr)

On topic of _addre; should we have included the scheme / protocol? (unix:///); would there ever be a reason for us to change this? (grpc://)

[krissetto]

I agree with these points. I was especially curious about including the scheme/protocol in the env var value as well, as i guess right now we just assume the socket is unix in the various plugins etc

[neersighted]

Re: naming, we haven't been strict in the past, e.g. DOCKER_TEST_ exists in several places in the daemon. I'd suggest we just go with something like @DOCKER as the convention for internal variables, as the @ makes it very hard to interact with these variables from shells.

Making that change now would be moderately breaking, but I think it would be more acceptable now rather than later (if we want to bite the bullet) as mismatched versions would only result in a reversion to the less-than-graceful lifecycle, instead of anything more drastic (e.g. ability to talk to a RPC server being broken).

Regarding the protocol, I don't think that makes sense, and I think it's just unnecessary complexity -- we control both the producer and the consumer here (in one codebase), and while cross-version compatibility is a concern, I think we just version the environment variable if we ever make a breaking change.

Adding e.g. gRPC or TTRPC here will not be breaking as older clients won't actually try to push anything over the socket. Also, unix:// would indicate a transport layer, while grpc:// would indicate a protocol -- so something like unix+grpc:// would be a lot more appropriate.

[thaJeztah]

Re: naming, we haven't been strict in the past, e.g. DOCKER_TEST_ exists in several places in the daemon.

So DOCKER_TEST was used for "CI-specific" env-vars, so "some consistency" there, but agreed, we haven't always been good at it. Still, DOCKER_ is common for user-facing ones, so what I want to avoid.

I'd suggest we just go with something like @docker as the convention for internal variables, as the @ makes it very hard to interact with these variables from shells.

I'm slightly concerned about using characters outside of the recommended character set https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html;

"Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2017 consist solely of uppercase letters, digits, and the ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit."

Hence thinking of lowercase (indicating "local" / "non exported"), and the underscore prefix.

Making that change now would be moderately breaking, but I think it would be more acceptable now rather than later (if we want to bite the bullet) as mismatched versions would only result in a reversion to the less-than-graceful lifecycle, instead of anything more drastic (e.g. ability to talk to a RPC server being broken).

Yup! This was a bit of an oversight I think, at least it didn't occur to me at the time.

w.r.t. compatibility; we can add a transitional situation; set both "old" and "new" env-var name for 1 release; this allows us to ship the new name fast (existing plugins continue to function, and don't even have to fall back), and then remove the "duplicate" env-var in the release after.

Adding e.g. gRPC or TTRPC here will not be breaking as older clients won't actually try to push anything over the socket. Also, unix:// would indicate a transport layer, while grpc:// would indicate a protocol -- so something like unix+grpc:// would be a lot more appropriate

Yes, I haven't given that a lot of thought yet, other then IF we decide to make this change, "now" may be the right moment to do so as well.

[neersighted]

Going outside the recommended character set would more or less be the point; the kernel/OS actually have very little in the way of opinions here outside the significance of '=' (see an example implementation in e.g. bionic libc), and the POSIX recommendations primarily relate to shell support.

With regard to compatibility, I think that's somewhat more optimistic than is realistic. I don't think that older plugins with a newer CLI are going to be that unusual, though it's not really an edge case I am too concerned about.

In any case, I'd rather not layer more complexity here... If we want to change the variable (and any changes should be in a separate PR), let's do that; but let's keep the simple 'sun_name of a Unix socket' pattern, and let's recognize that this may cause regressions for some users unless we maintain both variables in perpetuity (partially defeating the point).

Oh, and I forgot to address the 'what if the user sets this in standalone mode' question -- nothing, really. The unlink(2) logic is in the parent CLI, so the worst they can do is cause some spurious attempts to connect and log messages.

[laurazard]

Re: the overall point though, I don't see a need for changing the env-var right now. It looks kind of like a user-facing env var, which we could/should have avoided, but now that it's there, the downsides of changing it don't seem worth any positives we get from that? If it leaks through anywhere (where would that happen? it's set on the CLI process, and only used for the plugins it execs, and dies after) then there isn't really any consequences to that, since the socket is only used for signalling right now, and the socket gets cleaned up before the CLI exits regardless.
If in the future we make a change to use the socket for more things/communication, I think then would be a good time to reevaluate and maybe rename. Until then, I don't see a point.

[thaJeztah]

@neersighted But why take the risk? Do we really need to? Go stdlib also supports abstract sockets on macOS, and we've seen how well that went

[neersighted]

The Go stdlib does not support abstract sockets on macOS -- the convention of '@' to indicate a NUL byte at the start of sun_path is explicitly not part of the macOS implementation. It was our oversight/not checking what was supported that led to the creation of files with an '@' (and indeed, the 'skip unlink' is universal -- this was a shortcut taken by the Go team).

See https://cs.opensource.google/go/go/+/master:src/syscall/syscall_linux.go;l=569-574;drc=87056756141798b4dfd51dcaaa3e4ce63633a884;bpv=0;bpt=1 and https://cs.opensource.google/go/go/+/master:src/syscall/syscall_windows.go;l=870-875 for the Linux and Windows implementations.

I don't see the risk here -- by consulting the sources of libc and Go on the platforms of interest, the decades-old convention can be trivially verified.

[laurazard]

@laurazard you're looking at the socket listener code; I'm referring to the 'key' in the environment

OH DUH. Things got mixed up in my head, when talking about socket name vs socket ENV VAR name 😓 Yeah, "@" should be fine here.

I still do think that we don't need to change this right now (we'll be doing breaking changes to older plugins that are out there for virtually no benefit).

[laurazard]

(and indeed, the 'skip unlink' is universal -- this was a shortcut taken by the Go team)

Also, you say shortcut, I say oversight 😅 – it's a bug, since now it doesn't unlink real sockets on macOS because they start with "@".

[krissetto]

I agree with these points. I was especially curious about including the scheme/protocol in the env var value as well, as i guess right now we just assume the socket is unix in the various plugins etc

[neersighted]

I'm going to go ahead and bring this in, and open another PR to continue discussion/make a change I only now noticed is desirable.