cmd/docker-proxy: re-add SO_REUSEADDR by akerouanton · Pull Request #48567 · moby/moby

akerouanton · 2024-10-02T11:10:54Z

Related to:

Create docker-proxy TCP/UDP listener sockets in the daemon #48132

- What I did

Since commit b3fabed, the Engine creates the listening sockets used by docker-proxy by making raw syscalls (ie. socket, setsockopt, bind). Before that commit, those sockets were created by docker-proxy through Go's net.ListenX functions.

Unlike net.ListenX functions, the raw syscall code doesn't set the SO_REUSEADDR option. This option is typically used by TCP servers to make sure that they can be restarted even if there are client sockets referencing that sport (eg. in TIME_WAIT state, or any other state).

Citing UNIX Network Programming, Section 7.5 (p210):

By default, when the listening server is restarted by calling socket,
bind, and listen, the call to bind fails because the listening server
is trying to bind a port that is part of an existing connection [...]
All TCP servers should specify this socket option to allow the
server to be restarted in this situation.

- How to verify it

The new integration test TestRestartUserlandProxyUnder2MSL should be enough.

- Description for the changelog

- Fix a bug that was preventing containers exposing a TCP port on the host to be restarted if it was accessed by another container (or from the host) shortly before.

robmry

LGTM - just some ignorable suggestions ...

integration/networking/port_mapping_linux_test.go

robmry · 2024-10-02T11:48:40Z

integration/networking/port_mapping_linux_test.go

+		container.WithName(sanitizeCtrName(t.Name()+"-server")),
+		container.WithExposedPorts("80/tcp"),
+		container.WithPortMap(nat.PortMap{"80/tcp": {{HostPort: "1780"}}}),
+		container.WithCmd("httpd", "-f"),
+		container.WithNetworkMode("nat-time-wait"))


Could make a []func(*TestContainerConfig) of these options and use it in both Run calls - just to make it more obvious that the networks are the same, and in case something needs to change one day?

thaJeztah · 2024-10-07T18:21:54Z

This one ready for review?

akerouanton · 2024-10-07T19:28:21Z

@thaJeztah Yes! I was waiting for the CI to be green.

thaJeztah · 2024-10-08T08:19:42Z

integration/networking/port_mapping_linux_test.go

+	assert.NilError(t, c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true}))
+
+	serverID = container.Run(ctx, t, c, ctrOpts...)
+	defer c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true})


This one doesn't have to be a defer;

Suggested change

defer c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true})

assert.NilError(t, c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true}))

Right, actually the call to ContainerRemove doesn't even need to be made here. At this point, the 1st defer will take care of deleting that container (if the container name is passed).

integration/networking/port_mapping_linux_test.go

thaJeztah · 2024-10-08T08:32:05Z

integration/networking/port_mapping_linux_test.go

+	httpClient := &http.Client{Timeout: 3 * time.Second}
+	resp, err := httpClient.Get("http://127.0.0.1:1780")
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal(resp.StatusCode, 404))


I was actually curious if this would pass if no container was created, but looks like that gives a dial tcp 127.0.0.1:1780: connect: connection refused

Regardless; wondering if we should have something that's more clearly "it works", because I had trouble understanding the test here, and thought 404 was meant to indicate "should not return a result".

I guess we could either add some file to serve, or use some file that's expected to be present (e.g.) http://127.0.0.1:1780/etc/resolv.conf (but maybe that's confusing).

I guess we could either add some file to serve, or use some file that's expected to be present (e.g.) http://127.0.0.1:1780/etc/resolv.conf (but maybe that's confusing).

There are other tests doing exactly that in this suite. Generally speaking, we don't care at all about what's happening in L7. It's just an easy way to realistically test L4 (full TCP connection lifecycle, exchange a message back-and-forth, etc...).

I don't think we need to make things more complicated, like serving a real file, etc... That wouldn't add any value. But I added an inline comment stating that we don't care about the 404 response itself, and that we're just looking for a full-duplex TCP connection that we know is working correctly.

Yeah, I'm fine with keeping the 404 for now. Perhaps we could find some other minimal approach. My main train of thought here was that usually a 404 means "something went wrong!" which could be confusing (at least it initially confused me).

nit: Actually wondering now if the response needs to be checked at all for this; i.e., the err returned will be non-nil when failing to connect.

Right, we don't need to check the status code. I'll open a follow-up PR to fix the few tests that use this pattern.

Since commit b3fabed, the Engine creates the listening sockets used by docker-proxy by making raw syscalls (ie. socket, setsockopt, bind). Before that commit, those sockets were created by docker-proxy through Go's `net.ListenX` functions. Unlike `net.ListenX` functions, the raw syscall code doesn't set the `SO_REUSEADDR` option. This option is typically used by TCP servers to make sure that they can be restarted even if there are client sockets referencing the server port as their sport (eg. in TIME_WAIT state, or any other state). Citing UNIX Network Programming, Section 7.5 (p210): > By default, when the listening server is restarted by calling socket, > bind, and listen, the call to bind fails because the listening server > is trying to bind a port that is part of an existing connection [...] > _All_ TCP servers should specify this socket option to allow the > server to be restarted in this situation. Signed-off-by: Albin Kerouanton <[email protected]>

thaJeztah

LGTM, thanks!

akerouanton added status/2-code-review area/networking Networking impact/changelog kind/bugfix PR's that fix bugs area/networking/portmapping Networking process/cherry-pick/27.x labels Oct 2, 2024

akerouanton added this to the 28.0.0 milestone Oct 2, 2024

akerouanton requested review from neersighted and robmry October 2, 2024 11:10

akerouanton self-assigned this Oct 2, 2024

akerouanton marked this pull request as draft October 2, 2024 11:11

robmry approved these changes Oct 2, 2024

View reviewed changes

akerouanton force-pushed the add-SO_REUSEADDR-to-docker-proxy branch 2 times, most recently from d764f48 to d2aa364 Compare October 2, 2024 20:57

akerouanton changed the title ~~libnet/d/bridge: proxy: re-add SO_REUSEADDR~~ cmd/docker-proxy: re-add SO_REUSEADDR Oct 2, 2024

akerouanton force-pushed the add-SO_REUSEADDR-to-docker-proxy branch 2 times, most recently from 729769a to 2badf35 Compare October 7, 2024 16:02

akerouanton marked this pull request as ready for review October 7, 2024 19:27

akerouanton removed the process/cherry-pick/27.x label Oct 8, 2024

thaJeztah reviewed Oct 8, 2024

View reviewed changes

akerouanton force-pushed the add-SO_REUSEADDR-to-docker-proxy branch from 2badf35 to 77e5165 Compare October 8, 2024 16:08

akerouanton requested a review from thaJeztah October 8, 2024 16:08

thaJeztah approved these changes Oct 8, 2024

View reviewed changes

akerouanton merged commit aafdd33 into moby:master Oct 8, 2024

akerouanton deleted the add-SO_REUSEADDR-to-docker-proxy branch October 8, 2024 19:30

akerouanton added status/4-merge and removed status/2-code-review labels Oct 8, 2024

robmry added version/28.0 and removed impact/changelog labels Jan 21, 2025

robmry mentioned this pull request May 22, 2025

Avoid selecting duplicate host ports for mappings to 0.0.0.0 and specific addresses #50054

Merged

akerouanton mentioned this pull request Aug 8, 2025

libnet/pa: don't set SO_REUSEADDR on UDP sockets #50669

Merged

	defer c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true})
	assert.NilError(t, c.ContainerRemove(ctx, serverID, containertypes.RemoveOptions{Force: true}))

Conversation

akerouanton commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robmry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robmry Oct 2, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 7, 2024

Uh oh!

akerouanton commented Oct 7, 2024

Uh oh!

thaJeztah Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thaJeztah Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

akerouanton Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton commented Oct 2, 2024 •

edited

Loading

akerouanton Oct 8, 2024 •

edited

Loading