Don't PublishAllPorts AND specify the same IP bindings, else we get "address already in use" reliably on docker v28 #776
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ult gateway These options were introduced in v28. They are ignored by older Docker client versions.
This was required as of moby v28: moby/moby#49541
|
Next issue to contend with: I plan to split the go1.22 -> 1.23 update into a separate PR once CI is passing. |
Looks like this is just about matching the minor version: |
kegsay
changed the title
MadLittleMods
added a commit
that referenced
this pull request
May 26, 2025
…server `BaseURL` Regressed in #776 which started using the Docker default (`0.0.0.0`). This caused downstream issues in some of our out-of-repo Complement tests which stress some SSO/OIDC login flows and expect to be using the canonical `public_baseurl` configured in Synapse.
1 task
MadLittleMods
added a commit
that referenced
this pull request
May 29, 2025
…server `BaseURL` (#781) Regressed in #776 which started using the Docker default `HostIP` (`0.0.0.0`) because we removed the specific `HSPortBindingIP` port bindings for `8008` and `8448` (see "Root cause" section below). ### Problem This caused downstream issues in some of our out-of-repo Complement tests (Element internal) which stress some SSO/OIDC login flows with Synapse. Before #776, [`SsoRedirectServlet`](https://github.com/element-hq/synapse/blob/33ba8860c43d4770ea119a09a4fcbbf366f3b32e/synapse/rest/client/login.py#L645-L683) received a request like this `http://127.0.0.1:8008/_matrix/client/v3/login/sso/redirect/oidc-test_idp?redirectUrl=http%3A%2F%2Fapp.my-fake-client.io%2Fclient-callback`. But now it's seeing `http://0.0.0.0:8008/_matrix/client/v3/login/sso/redirect/oidc-test_idp?redirectUrl=http%3A%2F%2Fapp.my-fake-client.io%2Fclient-callback` and tries to redirect to the canonical `http://127.0.0.1:8008/` address which would then go to the step we expect in the tests (authorization endpoint). Basically, the `host` header in the request used to be set to `127.0.0.1:8008` but now it's `0.0.0.0:8008`. Synapse wants to use the canonical URL so the cookies are available so it redirects you to the canonical `public_baseurl` version first. Relevant Synapse code that cares about using the canonical URL to get cookies back -> [`SsoRedirectServlet`](https://github.com/element-hq/synapse/blob/33ba8860c43d4770ea119a09a4fcbbf366f3b32e/synapse/rest/client/login.py#L645-L683) We could alternatively, update our out-of-repo tests to account for this extra redirect but it seems more appropriate to update Complement to use the the configured `HSPortBindingIP` as expected. ### Root cause The host name used in the requests during the Complement tests is from the `client.BaseURL` which is sourced from the [Docker container port mapping/bindings](https://github.com/matrix-org/complement/blob/28a09014afd1f9c75a3549b4cd9d8fc480ba1149/internal/docker/deployer.go#L521) in Complement. In #776, we removed the explicit port bindings for `8008` with `HSPortBindingIP` ([`127.0.0.1`](https://github.com/matrix-org/complement/blob/28a09014afd1f9c75a3549b4cd9d8fc480ba1149/config/config.go#L180)) so now it uses Docker's default `0.0.0.0`. ```patch diff --git a/internal/docker/deployer.go b/internal/docker/deployer.go index cdcf51a..a186b30 100644 --- a/internal/docker/deployer.go +++ b/internal/docker/deployer.go @@ -382,20 +380,8 @@ func deployImage( }, &container.HostConfig{ CapAdd: []string{"NET_ADMIN"}, // TODO : this should be some sort of option PublishAllPorts: true, - PortBindings: nat.PortMap{ - nat.Port("8008/tcp"): []nat.PortBinding{ - { - HostIP: cfg.HSPortBindingIP, - }, - }, - nat.Port("8448/tcp"): []nat.PortBinding{ - { - HostIP: cfg.HSPortBindingIP, - }, - }, - }, ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Docker v28 refactored the networking code. Complement was driving the networking API incorrectly, by using
PublishAllPortsin addition to specifying specificPortBindings. This was because a niche use for Homerunner needed the homeservers to bind to a specific address, see #560 However, Docker was happy with this up until v28 where it would produce "address already in use" errors. This then created unrelated problems because the error occurred duringContainerStart. We never cleaned up the container properly, thus subsequent attempts to create a container with the same name would produceConflict. The container name "/complement_crypto_dirty_hs1" is already in use by container "ef141595c2188aa4d0ece3aa5f74fdb43a7ba276cf95a00076ad387881d7b24a". You have to remove (or rename) that container to be able to reuse that nameOld theory:
See the explanation in moby/moby#49935.
Also see the Docker v28 release notes:
Closes #775