Fix Complement not using HSPortBindingIP (127.0.0.1) for the homeserver 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,
-				},
-			},
-		},
```

Author: MadLittleMods

config/config.go

@@ -106,6 +106,14 @@ type Complement struct {
 	// disable this behaviour being added later, once this has stablised.
 	EnableDirtyRuns bool
 
+	// The IP that is used to connect to the running homeserver from the host.
+	//
+	// For Complement tests, this is always configured as `127.0.0.1` but can be
+	// overridden by homerunner to allow binding to a different IP address
+	// (`HOMERUNNER_HS_PORTBINDING_IP`).
+	//
+	// This field is used for the host-accessible homeserver URLs (as the hostname)
+	// so clients in your tests can access the homeserver.
 	HSPortBindingIP string
 
 	// Name: COMPLEMENT_POST_TEST_SCRIPT

internal/docker/builder.go

@@ -539,27 +539,59 @@ func printPortBindingsOfAllComplementContainers(docker *client.Client, contextSt
 	log.Printf("=============== %s : END ALL COMPLEMENT DOCKER PORT BINDINGS ===============\n\n\n", contextStr)
 }
 
-func endpoints(p nat.PortMap, csPort, ssPort int) (baseURL, fedBaseURL string, err error) {
-	csapiPort := fmt.Sprintf("%d/tcp", csPort)
-	csapiPortInfo, ok := p[nat.Port(csapiPort)]
-	if !ok {
-		return "", "", fmt.Errorf("port %s not exposed - exposed ports: %v", csapiPort, p)
+// endpoints transforms the homeserver ports into the base URL and federation base URL.
+func endpoints(p nat.PortMap, hsPortBindingIP string, csPort, ssPort int) (baseURL, fedBaseURL string, err error) {
+	csapiPortBinding, err := findPortBinding(p, hsPortBindingIP, csPort)
+	if err != nil {
+		return "", "", fmt.Errorf("Problem finding CS API port: %s", err)
 	}
-	if len(csapiPortInfo) == 0 {
-		return "", "", fmt.Errorf("port %s exposed with not mapped port: %+v", csapiPort, p)
+	baseURL = fmt.Sprintf("http://"+csapiPortBinding.HostIP+":%s", csapiPortBinding.HostPort)
+
+	ssapiPortBinding, err := findPortBinding(p, hsPortBindingIP, ssPort)
+	if err != nil {
+		return "", "", fmt.Errorf("Problem finding SS API port: %s", err)
 	}
-	baseURL = fmt.Sprintf("http://"+csapiPortInfo[0].HostIP+":%s", csapiPortInfo[0].HostPort)
+	fedBaseURL = fmt.Sprintf("https://"+ssapiPortBinding.HostIP+":%s", ssapiPortBinding.HostPort)
+	return
+}
 
-	ssapiPort := fmt.Sprintf("%d/tcp", ssPort)
-	ssapiPortInfo, ok := p[nat.Port(ssapiPort)]
+// findPortBinding finds a matching port binding for the given host/port in the `nat.PortMap`.
+//
+// This function will return the first port binding that matches the given host IP. If a
+// `0.0.0.0` binding is found, we will assume that it is listening on all interfaces,
+// including the `hsPortBindingIP`, and return a binding with the `hsPortBindingIP` as
+// the host IP.
+func findPortBinding(p nat.PortMap, hsPortBindingIP string, port int) (portBinding nat.PortBinding, err error) {
+	portString := fmt.Sprintf("%d/tcp", port)
+	portBindings, ok := p[nat.Port(portString)]
 	if !ok {
-		return "", "", fmt.Errorf("port %s not exposed - exposed ports: %v", ssapiPort, p)
-	}
-	if len(ssapiPortInfo) == 0 {
-		return "", "", fmt.Errorf("port %s exposed with not mapped port: %+v", ssapiPort, p)
+		return nat.PortBinding{}, fmt.Errorf("port %s not exposed - exposed ports: %v", portString, p)
+	}
+	if len(portBindings) == 0 {
+		return nat.PortBinding{}, fmt.Errorf("port %s exposed with not mapped port: %+v", portString, p)
+	}
+
+	for _, pb := range portBindings {
+		if pb.HostIP == hsPortBindingIP {
+			return pb, nil
+		} else if pb.HostIP == "0.0.0.0" {
+			// `0.0.0.0` means "all interfaces", so we can assume that this will be listening
+			// for connections from `hsPortBindingIP` as well.
+			return nat.PortBinding{
+				HostIP:   hsPortBindingIP,
+				HostPort: pb.HostPort,
+			}, nil
+		} else if pb.HostIP == "" && hsPortBindingIP == "127.0.0.1" {
+			// `HostIP` can be empty in certain environments (observed with podman v4.3.1). We
+			// will assume this is only a binding for `127.0.0.1`.
+			return nat.PortBinding{
+				HostIP:   hsPortBindingIP,
+				HostPort: pb.HostPort,
+			}, nil
+		}
 	}
-	fedBaseURL = fmt.Sprintf("https://"+csapiPortInfo[0].HostIP+":%s", ssapiPortInfo[0].HostPort)
-	return
+
+	return nat.PortBinding{}, fmt.Errorf("unable to find matching port binding for %s %s: %+v", hsPortBindingIP, portString, p)
 }
 
 type result struct {

internal/docker/deployer.go

@@ -313,9 +313,13 @@ func (d *Deployer) StartServer(hsDep *HomeserverDeployment) error {
 	}
 
 	// Wait for the container to be ready.
-	baseURL, fedBaseURL, err := waitForPorts(ctx, d.Docker, hsDep.ContainerID)
+	err = waitForPorts(ctx, d.Docker, hsDep.ContainerID, d.config.HSPortBindingIP)
 	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)
+	if err != nil {
+		return fmt.Errorf("failed to get host accessible homeserver URL's from container %s: %s", hsDep.ContainerID, err)
 	}
 	hsDep.SetEndpoints(baseURL, fedBaseURL)
 
@@ -381,7 +385,16 @@ func deployImage(
 			"complement_hs_name":   hsName,
 		},
 	}, &container.HostConfig{
-		CapAdd:          []string{"NET_ADMIN"}, // TODO : this should be some sort of option
+		CapAdd: []string{"NET_ADMIN"}, // TODO : this should be some sort of option
+		// We use `PublishAllPorts` because although Complement only requires the ports 8008
+		// and 8448 to be accessible in the image, other custom out-of-repo tests may use
+		// additional ports that are specific to their own application.
+		//
+		// Ideally, we would only bind to `cfg.HSPortBindingIP` but there isn't a way to
+		// specify the `HostIP` when using `PublishAllPorts`. And although, we could specify
+		// a manual port mapping, it's not compatible with also having `PublishAllPorts` set
+		// to true (we run into `address already in use` errors). Binding to all interfaces
+		// means we're also listening on `cfg.HSPortBindingIP` so it's good enough.
 		PublishAllPorts: true,
 		ExtraHosts:      extraHosts,
 		Mounts:          mounts,
@@ -441,10 +454,19 @@ func deployImage(
 		log.Printf("%s: Started container %s", contextStr, containerID)
 	}
 
-	baseURL, fedBaseURL, err := waitForPorts(ctx, docker, containerID)
+	// Wait for the container to be ready.
+	err = waitForPorts(ctx, docker, containerID, cfg.HSPortBindingIP)
 	if err != nil {
-		return stubDeployment, fmt.Errorf("%s : image %s : %w", contextStr, imageID, err)
+		return stubDeployment, fmt.Errorf("%s: failed to wait for ports on container %s: %w", contextStr, containerID, err)
+	}
+	baseURL, fedBaseURL, err := getHostAccessibleHomeserverURLs(ctx, docker, containerID, cfg.HSPortBindingIP)
+	if err != nil {
+		return stubDeployment, fmt.Errorf(
+			"%s: failed to get host accessible homeserver URL's from container %s: %s",
+			contextStr, containerID, err,
+		)
 	}
+
 	inspect, err := docker.ContainerInspect(ctx, containerID)
 	if err != nil {
 		return stubDeployment, fmt.Errorf("ContainerInspect: %s", err)
@@ -504,29 +526,104 @@ func copyToContainer(docker *client.Client, containerID, path string, data []byt
 	return nil
 }
 
-// Waits until a homeserver container has NAT ports assigned and returns its clientside API URL and federation API URL.
-func waitForPorts(ctx context.Context, docker *client.Client, containerID string) (baseURL string, fedBaseURL string, err error) {
+func assertHostnameEqual(inputUrl string, expectedHostname string) error {
+	parsedUrl, err := url.Parse(inputUrl)
+	if err != nil {
+		return fmt.Errorf("failed to parse URL %s: %s", inputUrl, err)
+	}
+	if parsedUrl.Hostname() != expectedHostname {
+		return fmt.Errorf("expected hostname %s in URL %s, got %s", expectedHostname, inputUrl, parsedUrl.Hostname())
+	}
+
+	return nil
+}
+
+// getHostAccessibleHomeserverURLs returns URLs that are accessible from the host
+// machine (outside the container) for the homeserver's client API and federation API.
+func getHostAccessibleHomeserverURLs(ctx context.Context, docker *client.Client, containerID string, hsPortBindingIP string) (baseURL string, fedBaseURL string, err error) {
+	inspectResponse, err := inspectContainer(ctx, docker, containerID)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to inspect ports: %w", err)
+	}
+
+	baseURL, fedBaseURL, err = endpoints(inspectResponse.NetworkSettings.Ports, hsPortBindingIP, 8008, 8448)
+
+	// Sanity check that the URLs match the expected configured binding IP. It's
+	// also important that we use the canonical publicly accessible hostname for the
+	// homeserver for some situations like SSO/OIDC login where important cookies are set
+	// for the domain.
+	err = assertHostnameEqual(baseURL, hsPortBindingIP)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to assert baseURL has the correct hostname: %w", err)
+	}
+	err = assertHostnameEqual(fedBaseURL, hsPortBindingIP)
+	if err != nil {
+		return "", "", fmt.Errorf("failed to assert fedBaseURL has the correct hostname: %w", err)
+	}
+
+	return baseURL, fedBaseURL, nil
+}
+
+// waitForPorts waits until a homeserver container has NAT ports assigned (8008, 8448).
+func waitForPorts(ctx context.Context, docker *client.Client, containerID string, hsPortBindingIP string) (err error) {
 	// We need to hammer the inspect endpoint until the ports show up, they don't appear immediately.
-	var inspect container.InspectResponse
 	inspectStartTime := time.Now()
 	for time.Since(inspectStartTime) < time.Second {
-		inspect, err = docker.ContainerInspect(ctx, containerID)
-		if err != nil {
-			return "", "", err
-		}
-		if inspect.State != nil && !inspect.State.Running {
-			// the container exited, bail out with a container ID for logs
-			return "", "", fmt.Errorf("container is not running, state=%v", inspect.State.Status)
+		inspectResponse, err := inspectContainer(ctx, docker, containerID)
+		if inspectionErr, ok := err.(*containerInspectionError); ok && inspectionErr.Fatal {
+			// If the error is fatal, we should not retry.
+			return fmt.Errorf("Fatal inspection error: %s", err)
 		}
-		baseURL, fedBaseURL, err = endpoints(inspect.NetworkSettings.Ports, 8008, 8448)
-		if err == nil {
+
+		// Check to see if we can see the ports yet
+		_, csPortErr := findPortBinding(inspectResponse.NetworkSettings.Ports, hsPortBindingIP, 8008)
+		_, ssPortErr := findPortBinding(inspectResponse.NetworkSettings.Ports, hsPortBindingIP, 8448)
+		if csPortErr == nil && ssPortErr == nil {
 			break
 		}
+
 	}
-	return baseURL, fedBaseURL, nil
+	return nil
+}
+
+type containerInspectionError struct {
+	// Error message
+	msg string
+	// Indicates whether the caller should stop retrying to inspect the container because
+	// it has already exited.
+	Fatal bool
+}
+
+func (e *containerInspectionError) Error() string { return e.msg }
+
+// inspectContainer inspects the container with the given ID and returns response.
+//
+// On failure, returns a `containerInspectionError` representing the underlying error and indicates
+// `err.Fatal: true` if the container is no longer running.
+func inspectContainer(
+	ctx context.Context,
+	docker *client.Client,
+	containerID string,
+) (inspectResponse container.InspectResponse, err error) {
+	inspectResponse, err = docker.ContainerInspect(ctx, containerID)
+	if err != nil {
+		return container.InspectResponse{}, &containerInspectionError{
+			msg:   err.Error(),
+			Fatal: false,
+		}
+	}
+	if inspectResponse.State != nil && !inspectResponse.State.Running {
+		// the container exited, bail out with a container ID for logs
+		return container.InspectResponse{}, &containerInspectionError{
+			msg:   fmt.Sprintf("container (%s) is not running, state=%v", containerID, inspectResponse.State.Status),
+			Fatal: true,
+		}
+	}
+
+	return inspectResponse, nil
 }
 
-// Waits until a homeserver deployment is ready to serve requests.
+// waitForContainer waits until a homeserver deployment is ready to serve requests.
 func waitForContainer(ctx context.Context, docker *client.Client, hsDep *HomeserverDeployment, stopTime time.Time) (iterCount int, lastErr error) {
 	iterCount = 0