Remove opts.QuotedString implementation

[tianon]

This was originally added to solve a CLI usability issue related to docker-machine and docker (explicitly not dockerd) -- the "docker/cli" repository has a separate copy of this entire opts package, so isn't even using this implementation.

Refs #29761

[tianon]

(See https://github.com/docker/cli/blob/a32cd16160f1b41c1c4ae7bee4dac929d1484e59/opts/quotedstring.go for where the CLI has a separate copy of this code for their own needs, which continues to resolve the original issue -- daemon flags were only changed back then because it used to be shared code.)

[tianon]

This CI failure isn't related 😬

=== FAIL: libnetwork/networkdb TestNetworkDBCRUDTableEntries (7.82s)
    networkdb_test.go:303: Entry existence verification test failed for node1(27b044ef1f0f)

#42739

[samuelkarp]

Can the other format still be parsed? I wouldn't expect to see a unit test change if there's no change in behavior.

[tianon]

Sorry, I guess I should back up and explain this in more detail:

In #29761, it was discovered that after Docker changed from one flag parsing library to another, it lost a minor "feature" where it would strip quotes (outside whatever quote parsing the shell provides). In shell syntax, we're talking about docker '--flag="value"' ... (note the extra double quotes inside that get passed verbatim). The old flag library would quietly strip those, but the new flag library did not.

It turned out that docker-machine was relying on that double stripping for how it was passing the three --tls* options to the Docker CLI, so this file was created as a wrapper to strip the quotes, but only from those three CLI flags. In that original implementation, the CLI and Daemon were both sharing the same parsing code for those three TLS-related flags, so the daemon got support for stripping these quotes even though it wasn't actually needed on the daemon (docker-machine only passed them to the client).

Eventually, the CLI moved to a separate repository, and this code was copied over there, but stayed here as well even though it's not necessary in the daemon for the original bug, so I'm arguing that we should remove it.

The reason there's a unit test change here is because this test appears to be intended to make sure setting options works, but was also double-purposed to test that they would strip these extra double quotes. So for this change to move forward, either this test needs to strip the extra double quotes or validate that the value that comes in includes them (which if this PR goes forward would instead mean a relative path that literally starts with a double quote, so I don't think is terribly useful and I'd close this instead if that's the consensus).

[thaJeztah]

This was originally added to solve a CLI usability issue related to docker-machine and docker (explicitly not dockerd) -- the "docker/cli" repository has a separate copy of this entire opts package, so isn't even using this implementation.

So, digging in memory, I think we added this hack because users that had their systemd docker.unit (or other scripts) configured with quotes would end up with the daemon failing to start after we removed the quote handling (which probably never should've been there in the first place, as it's non-standard).

One option we could think of is to validate the value as part of startup, and to produce a useful error (value contains quotes, which must be removed)

This would also users to run dockerd --validate (added in #42393) to check if the options are ok (although in that case, they'd have to pass all command-line flags and --validate).

[tianon]

I think systemd has quote parsing similar to a shell (so this shouldn't be an issue there), but I'll do a little testing around that to verify.

Do you remember offhand whether dockerd --validate checks whether the provided certificates are valid? If so, that would already give us some pretty good validation IMO -- I'm not convinced this is really going to be common enough to be worth adding even more code we get to maintain just for a slightly better error message than "/path/to/workdir/"/path/to/cert" does not exist".

[tianon]

I went a little overboard on the test, but it illustrates reasonably, I think:

$ cat .config/systemd/user/test.service
[Service]
ExecStart=/bin/sh -c 'echo "$@"' -- --foo="bar" --bar='baz' --buzz=no"tianon"no

$ systemctl --user daemon-reload
$ systemctl --user start test.service
$ systemctl --user status test.service
● test.service
     Loaded: loaded (/home/tianon/.config/systemd/user/test.service; static)
     Active: inactive (dead)

Feb 17 11:02:33 adora systemd[2825]: Started test.service.
Feb 17 11:02:33 adora sh[23957]: --foo=bar --bar=baz --buzz=notianonno
Feb 17 11:02:33 adora systemd[2825]: test.service: Succeeded.

[samuelkarp]

LGTM

[tianon]

😞

root@311828fcf5ca:/usr/src/docker# ./bundles/dynbinary-daemon/dockerd --validate --tlscacert=/foo/cafile
configuration OK
root@311828fcf5ca:/usr/src/docker# ./bundles/dynbinary-daemon/dockerd --validate '--tlscacert="/foo/cafile"'
configuration OK

[tianon]

I guess we should probably add a new validateTLSOptions function/block inside daemon/config/config.go, and maybe only if config.TLS is true (since these are ignored otherwise)? (ie, dockerd --tls --tlsxxx=...)

Are there other "path" flags you think it would be useful to validate existence of as well?
(--init-path, --seccomp-profile, userland-auth-proxy-path ?)

[thaJeztah]

LGTM

let's look for validation in a follow up