Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
May 29, 2025

Conversation

MadLittleMods
Copy link
Collaborator

@MadLittleMods MadLittleMods commented May 26, 2025

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.

  1. 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
    
  2. cd complement && go mod tidy && cd ..
  3. Run Complement how you normally would

Pull Request Checklist

…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.
Comment on lines 316 to 320
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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MadLittleMods MadLittleMods marked this pull request as ready for review May 26, 2025 22:33
@MadLittleMods MadLittleMods requested review from kegsay and a team as code owners May 26, 2025 22:33
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks and questions.

@MadLittleMods MadLittleMods requested a review from richvdh May 28, 2025 18:27
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

MadLittleMods and others added 2 commits May 29, 2025 09:30
Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
@MadLittleMods MadLittleMods merged commit 9b4c811 into main May 29, 2025
4 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/canonical-baseurl branch May 29, 2025 14:44
@MadLittleMods
Copy link
Collaborator Author

Thanks for the review @richvdh 🦘

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants