Fix Complement not using `HSPortBindingIP` (`127.0.0.1`) for the homeserver `BaseURL` by MadLittleMods · Pull Request #781 · matrix-org/complement

MadLittleMods · 2025-05-26T21:50:06Z

Fix Complement not using config.HSPortBindingIP (127.0.0.1) for the homeserver BaseURL.

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 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

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 in Complement.

In #776, we removed the explicit port bindings for 8008 with HSPortBindingIP (127.0.0.1) so now it uses Docker's default 0.0.0.0.

diff --git a/internal/docker/deployer.go b/internal/docker/deployer.go
index cdcf51a5..a186b30d 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,
-				},
-			},
-		},

Dev notes

Locally link Complement package into your tests

See https://thewebivore.com/using-replace-in-go-mod-to-point-to-your-local-module/

So you can edit the Complement source code and use it in the tests.

Add the following to complement/go.mod

// TODO: Remove as this is only for local development
replace github.com/matrix-org/complement => /home/eric/Documents/github/element/complement

cd complement && go mod tidy && cd ..
Run Complement how you normally would

Pull Request Checklist

Pull request includes a sign off

…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.

MadLittleMods · 2025-05-26T22:30:57Z

internal/docker/deployer.go

+	err = waitForPorts(ctx, d.Docker, hsDep.ContainerID)
 	if err != nil {
-		return fmt.Errorf("failed to get ports for container %s: %s", hsDep.ContainerID, err)
+		return fmt.Errorf("failed to wait for ports on container %s: %s", hsDep.ContainerID, err)
+	}
+	baseURL, fedBaseURL, err := getHostAccessibleHomeserverUrls(ctx, d.Docker, hsDep.ContainerID, d.config.HSPortBindingIP)


To clean things up, I've refactored waitForPorts(...) to only do the waiting part as described. We now assemble BaseURL and FedBaseUrl via getHostAccessibleHomeserverUrls(...)

@richvdh

As discovered by @richvdh Spawning from - #779 (comment) - #779 (comment)

richvdh

A few nitpicks and questions.

internal/docker/deployer.go

config/config.go

internal/docker/builder.go

internal/docker/deployer.go

config/config.go

See #781 (comment)

This reverts commit da98b90.

See #781

richvdh

LGTM otherwise

config/config.go

internal/docker/deployer.go

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

MadLittleMods · 2025-05-29T14:45:18Z

Thanks for the review @richvdh 🦘

…server `BaseURL` (matrix-org#781) Regressed in matrix-org#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 matrix-org#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 matrix-org#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, - }, - }, - }, ```

MadLittleMods added 2 commits May 26, 2025 16:43

Better docstrings

cdeb5ec

MadLittleMods commented May 26, 2025

View reviewed changes

MadLittleMods marked this pull request as ready for review May 26, 2025 22:33

MadLittleMods requested review from a team and kegsay as code owners May 26, 2025 22:33

MadLittleMods mentioned this pull request May 27, 2025

Fixes for running under podman #779

Merged

Incorporate empty HostIP podman edge case

5077e33

As discovered by @richvdh Spawning from - #779 (comment) - #779 (comment)

richvdh reviewed May 28, 2025

View reviewed changes

MadLittleMods added 10 commits May 28, 2025 10:27

URL grammar

1bfe471

Clarify HSPortBindingIP better

64147f1

Add docs for ContainerInspectionError being returned

39edab5

See #781 (comment)

Make ContainerInspectionError private

e17f4db

See #781 (comment)

Use idiomatic Go doc comments

4eb1cb6

See #781 (comment)

Remove incorrect fallback when we can't find a matching port binding

39c2ac2

See #781 (comment)

Update to waitForContainer

da98b90

See #781 (comment)

Revert "Update to waitForContainer"

e33cbc8

This reverts commit da98b90.

Actually look for ports in waitForPorts

cb2586f

See #781

Fix new args missing

b91e723

MadLittleMods requested a review from richvdh May 28, 2025 18:27

richvdh approved these changes May 29, 2025

View reviewed changes

config/config.go Outdated Show resolved Hide resolved

internal/docker/deployer.go Outdated Show resolved Hide resolved

MadLittleMods and others added 2 commits May 29, 2025 09:30

Returns error on failure

50ea132

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

HSPortBindingIP doc-comment usage nuance

616981f

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

MadLittleMods merged commit 9b4c811 into main May 29, 2025
4 checks passed

MadLittleMods deleted the madlittlemods/canonical-baseurl branch May 29, 2025 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Complement not using `HSPortBindingIP` (`127.0.0.1`) for the homeserver `BaseURL`#781

Fix Complement not using `HSPortBindingIP` (`127.0.0.1`) for the homeserver `BaseURL`#781
MadLittleMods merged 15 commits intomainfrom
madlittlemods/canonical-baseurl

MadLittleMods commented May 26, 2025 •

edited

Loading

Uh oh!

MadLittleMods May 26, 2025

Uh oh!

richvdh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods commented May 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

MadLittleMods commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Dev notes

Locally link Complement package into your tests

Pull Request Checklist

Uh oh!

MadLittleMods May 26, 2025

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MadLittleMods commented May 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MadLittleMods commented May 26, 2025 •

edited

Loading