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

config/config.go

@@ -106,7 +106,15 @@ type Complement struct {
 	// disable this behaviour being added later, once this has stablised.
 	EnableDirtyRuns bool
 
-	// The hostname that will be used to bind the homeserver ports to, e.g. `127.0.0.1`
+	// The IP that will be used to bind the homeserver ports to, e.g. `127.0.0.1`  (only
+	// allow localhost access), `0.0.0.0` (bind to all available interfaces).
+	//
+	// 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 also 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,7 +539,7 @@ func printPortBindingsOfAllComplementContainers(docker *client.Client, contextSt
 	log.Printf("=============== %s : END ALL COMPLEMENT DOCKER PORT BINDINGS ===============\n\n\n", contextStr)
 }
 
-// Transform the homeserver ports into the base URL and federation base URL.
+// 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 {
@@ -555,7 +555,7 @@ func endpoints(p nat.PortMap, hsPortBindingIP string, csPort, ssPort int) (baseU
 	return
 }
 
-// Find a matching port binding for the given host/port in the `nat.PortMap`.
+// 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,
@@ -591,7 +591,7 @@ func findPortBinding(p nat.PortMap, hsPortBindingIP string, port int) (portBindi
 		}
 	}
 
-	return portBindings[0], nil
+	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,11 +313,11 @@ func (d *Deployer) StartServer(hsDep *HomeserverDeployment) error {
 	}
 
 	// Wait for the container to be ready.
-	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 wait for ports on container %s: %s", hsDep.ContainerID, err)
 	}
-	baseURL, fedBaseURL, err := getHostAccessibleHomeserverUrls(ctx, d.Docker, hsDep.ContainerID, d.config.HSPortBindingIP)
+	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)
 	}
@@ -385,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,
@@ -446,11 +455,11 @@ func deployImage(
 	}
 
 	// Wait for the container to be ready.
-	err = waitForPorts(ctx, docker, containerID)
+	err = waitForPorts(ctx, docker, containerID, cfg.HSPortBindingIP)
 	if err != nil {
 		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)
+	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",
@@ -529,17 +538,17 @@ func assertHostnameEqual(inputUrl string, expectedHostname string) error {
 	return nil
 }
 
-// Returns URL's 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 := inspectPortsOnContainer(ctx, docker, containerID)
+// 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 URL's match the expected configured binding hostname. It's
+	// 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.
@@ -555,49 +564,57 @@ func getHostAccessibleHomeserverUrls(ctx context.Context, docker *client.Client,
 	return baseURL, fedBaseURL, nil
 }
 
-// Waits until a homeserver container has NAT ports assigned.
-func waitForPorts(ctx context.Context, docker *client.Client, containerID string) (err error) {
+// 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.
 	inspectStartTime := time.Now()
 	for time.Since(inspectStartTime) < time.Second {
-		_, err = inspectPortsOnContainer(ctx, docker, containerID)
-		if err == nil {
-			break
-		}
-
-		if inspectionErr, ok := err.(*ContainerInspectionError); ok && inspectionErr.Fatal {
+		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)
 		}
+
+		// 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 nil
 }
 
-type ContainerInspectionError struct {
+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 }
+func (e *containerInspectionError) Error() string { return e.msg }
 
-func inspectPortsOnContainer(
+// inspectContainer inspects the container with the given ID and returns response.
+//
+// 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{
+		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{
+		return container.InspectResponse{}, &containerInspectionError{
 			msg:   fmt.Sprintf("container (%s) is not running, state=%v", containerID, inspectResponse.State.Status),
 			Fatal: true,
 		}
@@ -606,7 +623,7 @@ func inspectPortsOnContainer(
 	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