Repository: moby/moby (language: go)

Issue:
fix: the logic of the protocol in loadListeners
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>

<!--
Please make sure you've read and understood our contributing guidelines;
https://github.com/moby/moby/blob/master/CONTRIBUTING.md

** Make sure all your commits include a signature generated with `git commit -s` **

For additional information on our contributing process, read our contributing
guide https://docs.docker.com/opensource/code/

If this is a bug fix, make sure your description includes "fixes #xxxx", or
"closes #xxxx"

Please provide the following information:
-->

**- What I did**

Here introduced a judgment in  https://github.com/moby/moby/commit/5008409b5cebe36690f653b71f171cf4e1ce114c


This will result in the following error.

```
➜  moby sudo /usr/bin/dockerd -H fd:// --containerd=/run/containerd/containerd.sock                                                                                                                       
INFO[2023-01-15T00:56:52.640211255+08:00] Starting up                                  
WARN[2023-01-15T00:56:52.640525792+08:00] Running experimental build                          
failed to load listeners: bad format fd://, expected PROTO://ADDR
```

The configuration `fd://` is the default configuration for docker.service.
we shouldn't break it
https://github.com/moby/moby/blob/master/contrib/init/systemd/docker.service#LL13C31-L13C36

**- How I did it**

**- How to verify it**

**- Description for the changelog**
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->


**- A picture of a cute animal (not mandatory but encouraged)**




Discussion / hints:
Actually, something fishy is going on:

https://github.com/moby/moby/blob/d62a88eb0d0358d593c0d8c43216f78d5104184c/opts/hosts_test.go#L30-L31

We have a test case explicitly for this -- so we need to investigate why that is failing to catch the regression.
Thanks! Even though we have this test case, it doesn't use the `parseHost` or `parseDaemonHost` functions at this point in my modification.

In the previous code, it is not judged whether `addr` is empty.
https://github.com/moby/moby/commit/5008409b5cebe36690f653b71f171cf4e1ce114c#diff-eab6997069d2fc9fd3e50c0721e88a363570b9d3ed03facd987373ab1ac9405bL665-L666


I think I should change the function called to `parseDaemonHost` to clean up a bit here, what do you think?
Could it be the subtests here not capturing the variable? https://github.com/moby/moby/blob/d62a88eb0d0358d593c0d8c43216f78d5104184c/opts/hosts_test.go#L50-L51
no, `fd://` is a valid configuration.
I'm personally fine with the check added in this PR (this was indeed an oversight of mine), or perhaps remove the check altogether (if we already validate these before this).

That said, we should refactor some of this (I recall I looked at this code as well, when working on https://github.com/moby/moby/pull/43459); I think there's too much  handling happening in `loadListeners()`; it would be better if we did all the parsing and handling before we reach that code, to allow validating the configuration _before_ we even try to start the daemon, and so that `dockerd --validate` already returns configuration errors (if we don't validate this specific case already); https://github.com/moby/moby/blob/803c21f4b20de6533d61a60cb9c359159e3e6a7a/cmd/dockerd/daemon.go#L83-L92

> no, `fd://` is a valid configuration.

Ah, yea, you're right; was wondering for a bit if the test was broken.
Looks like `fd://something` should _not_ be valid though; so some validation is missing in that part; https://github.com/moby/moby/blob/803c21f4b20de6533d61a60cb9c359159e3e6a7a/daemon/listeners/listeners_linux.go#L82-L90
Removed the cherry-pick label because it looks like https://github.com/moby/moby/commit/5008409b5cebe36690f653b71f171cf4e1ce114c (https://github.com/moby/moby/pull/44381) is only in master
Then I plan to do this:

* Modify the current PR, and remove the check on `addr`. Since the previous code didn't have this check either, here's a quick fix.  
Or leave this PR unchanged, since we really only need to cover `fd` for one scenario. 
 
* Create a new PR to refactor this:
   * Replace the processing logic here with `parseDaemonHost`
   * Add validation for `fd://something` 

WDYT?
I'd split the difference: use this PR, but change the implementation to the second, and add e2e testing of this area of code (not just `parseDaemonHost`).
OK, let me add some e2e test cases, thanks
> and add e2e testing of this area of code (not just parseDaemonHost).

Agreed; There's also quite a bit more to look at w.r.t. validation in general. Some quick thoughts on some of that;

- Make sure we're more consistent in where validation (needs to) happen
- Important there is that not _all_ validation has to be a _full_ validation (for better words). What I mean is that validating "format" and "valid input" is sometimes a separate concern; e.g. `https://hello.world/blabla/blablabla` is validly _formatted_ URL, but the URL may not _exist_. Same as `unix:///no/such/socket` being a valid formatted "unix" socket path, but may not exist.
- This distinction is sometimes important. For example, it's OK to validate for a valid formatted input in some areas (e.g. could be the `docker` CLI), but in some areas you don't want to (or are not _able to_) validate everything.
- Not validating _everything_ is fine as long as we catch errors (and ideally "as early as possible")/
- Some things will always remain a bit of a "grey" area (e.g. socket paths [may be "too long"](https://unix.stackexchange.com/questions/367008/why-is-socket-path-length-limited-to-a-hundred-chars), but the allowed length _may_ depend on how the host is configured. For such cases we must decide (probably on a per-case basis) wether _we_ (`dockerd`) should validate, or if that's something to delegate to the system itself.
Thanks for your reply, I will do some thinking.

I'll be back from vacation in two days.

Fix patch:
diff --git a/cmd/dockerd/daemon.go b/cmd/dockerd/daemon.go
index 86440502c2e40..f5f37c6f1defa 100644
--- a/cmd/dockerd/daemon.go
+++ b/cmd/dockerd/daemon.go
@@ -655,7 +655,7 @@ func loadListeners(cli *DaemonCli, tlsConfig *tls.Config) ([]string, error) {
 	for i := 0; i < len(cli.Config.Hosts); i++ {
 		protoAddr := cli.Config.Hosts[i]
 		proto, addr, ok := strings.Cut(protoAddr, "://")
-		if !ok || addr == "" {
+		if !ok {
 			return nil, fmt.Errorf("bad format %s, expected PROTO://ADDR", protoAddr)
 		}
 


Test patch:
diff --git a/cmd/dockerd/daemon_linux_test.go b/cmd/dockerd/daemon_linux_test.go
new file mode 100644
index 0000000000000..976ab626b7ecf
--- /dev/null
+++ b/cmd/dockerd/daemon_linux_test.go
@@ -0,0 +1,90 @@
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"os"
+	"strconv"
+	"testing"
+
+	"github.com/docker/docker/daemon/config"
+	"github.com/docker/docker/pkg/reexec"
+	"golang.org/x/sys/unix"
+	"gotest.tools/v3/assert"
+)
+
+const (
+	testListenerNoAddrCmdPhase1 = "test-listener-no-addr1"
+	testListenerNoAddrCmdPhase2 = "test-listener-no-addr2"
+)
+
+type listenerTestResponse struct {
+	Err string
+}
+
+func initListenerTestPhase1() {
+	os.Setenv("LISTEN_PID", strconv.Itoa(os.Getpid()))
+	os.Setenv("LISTEN_FDS", "1")
+
+	// NOTE: We cannot use O_CLOEXEC here because we need the fd to stay open for the child process.
+	_, err := unix.Socket(unix.AF_UNIX, unix.SOCK_STREAM, 0)
+	if err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	}
+
+	cmd := reexec.Command(testListenerNoAddrCmdPhase2)
+	if err := unix.Exec(cmd.Path, cmd.Args, os.Environ()); err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	}
+}
+
+func initListenerTestPhase2() {
+	cli := &DaemonCli{
+		Config: &config.Config{
+			CommonConfig: config.CommonConfig{
+				Hosts: []string{"fd://"},
+			},
+		},
+	}
+	_, err := loadListeners(cli, nil)
+	var resp listenerTestResponse
+	if err != nil {
+		resp.Err = err.Error()
+	}
+
+	if err := json.NewEncoder(os.Stdout).Encode(resp); err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	}
+}
+
+// Test to make sure that the listen specs without an address are handled
+// It requires a 2-phase setup due to how socket activation works (which we are using to test).
+// It requires LISTEN_FDS and LISTEN_PID to be set in the environment.
+//
+// LISTEN_PID is used by socket activation to determine if the process is the one that should be activated.
+// LISTEN_FDS is used by socket activation to determine how many file descriptors are passed to the process.
+//
+// We can sort of fake this without using extra processes, but it ends up not
+// being a true test because that's not how socket activation is expected to
+// work and we'll end up with nil listeners since the test framework has other
+// file descriptors open.
+//
+// This is not currently testing `tcp://` or `unix://` listen specs without an address because those can conflict with the machine running the test.
+// This could be worked around by using linux namespaces, however that would require root privileges which unit tests don't typically have.
+func TestLoadListenerNoAddr(t *testing.T) {
+	cmd := reexec.Command(testListenerNoAddrCmdPhase1)
+	stdout := bytes.NewBuffer(nil)
+	cmd.Stdout = stdout
+	stderr := bytes.NewBuffer(nil)
+	cmd.Stderr = stderr
+
+	assert.NilError(t, cmd.Run(), stderr.String())
+
+	var resp listenerTestResponse
+	assert.NilError(t, json.NewDecoder(stdout).Decode(&resp))
+	assert.Equal(t, resp.Err, "")
+}
diff --git a/cmd/dockerd/main_linux_test.go b/cmd/dockerd/main_linux_test.go
new file mode 100644
index 0000000000000..cab0914af66ff
--- /dev/null
+++ b/cmd/dockerd/main_linux_test.go
@@ -0,0 +1,16 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/docker/docker/pkg/reexec"
+)
+
+func TestMain(m *testing.M) {
+	reexec.Register(testListenerNoAddrCmdPhase1, initListenerTestPhase1)
+	reexec.Register(testListenerNoAddrCmdPhase2, initListenerTestPhase2)
+	if reexec.Init() {
+		return
+	}
+	m.Run()
+}