Skip to content
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

setup ExposeHostPorts forwards on container start #2815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hathvi
Copy link

@hathvi hathvi commented Oct 5, 2024

What does this PR do?

Changes the lifecycle hook for the ExposedHostPorts forwarding to happen on PreCreates of the testcontainer instead of PostReadies. Additionally updates the port forwarding tests to ensure the host ports are accessible on startup and that there's no issues even if the host isn't listening until PostCreates.

Why is it important?

Previously ExposedHostPorts would start an SSHD container prior to starting the testcontainer and inject a PostReadies lifecycle hook into the testcontainer in order to set up remote port forwarding from the host to the SSHD container so the testcontainer can talk to the host via the SSHD container

This would be an issue if the testcontainer depends on accessing the host port on startup ( e.g., a proxy server ) as the forwarding for the host access isn't set up until all the WiatFor strategies on the testcontainer have completed.

Related issues

How to test this PR

go test port_forwarding_test.go

Additionally, I've provided a minimal example of my use case where I ran into the issue trying to test my Caddy configuration as an API gateway using testcontainers.

package caddy_test

import (
	"bytes"
	"context"
	"fmt"
	"io"
	"net"
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"github.com/testcontainers/testcontainers-go"
	"github.com/testcontainers/testcontainers-go/wait"
)

const caddyFileContent = `
listen :80

reverse_proxy /api/* {
	to {$API_SERVER}

	health_uri /health
	health_status 200
	health_interval 10s
}
`

func TestCaddyfile(t *testing.T) {
	ctx := context.Background()

	apiServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, "Hello, World!")
	}))
	apiServerPort := apiServer.Listener.Addr().(*net.TCPAddr).Port

	caddyContainer, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
		ContainerRequest: testcontainers.ContainerRequest{
			Image:        "caddy:2.8.4",
			ExposedPorts: []string{"80/tcp"},
			WaitingFor:   wait.ForLog("server running"),
			Env: map[string]string{
				"API_SERVER": fmt.Sprintf("http://%s:%d", testcontainers.HostInternal, apiServerPort),
			},
			Files: []testcontainers.ContainerFile{
				{
					Reader:            bytes.NewReader([]byte(caddyFileContent)),
					ContainerFilePath: "/etc/caddy/Caddyfile",
				},
			},
			HostAccessPorts: []int{apiServerPort},
		},
		Started: true,
	})
	require.NoError(t, err)
	defer caddyContainer.Terminate(ctx)

	caddyURL, err := caddyContainer.PortEndpoint(ctx, "80/tcp", "http")
	require.NoError(t, err)

	resp, err := http.Get(caddyURL + "/api/test")
	require.NoError(t, err)
	defer resp.Body.Close()

	body, err := io.ReadAll(resp.Body)
	require.NoError(t, err)

	assert.Equal(t, http.StatusOK, resp.StatusCode)
	assert.Equal(t, "Hello, World!", string(body))

	lr, err := caddyContainer.Logs(ctx)
	assert.NoError(t, err)
	lb, err := io.ReadAll(lr)
	assert.NoError(t, err)
	fmt.Printf("== Caddy Logs ==\n%s================\n\n", string(lb))
}

Fixes testcontainers#2811

Previously ExposedHostPorts would start an SSHD container prior to
starting the testcontainer and inject a PostReadies lifecycle hook into
the testcontainer in order to set up remote port forwarding from the
host to the SSHD container so the testcontainer can talk to the host via
the SSHD container

This would be an issue if the testcontainer depends on accessing the
host port on startup ( e.g., a proxy server ) as the forwarding for the
host access isn't set up until all the WiatFor strategies on the
testcontainer have completed.

The fix is to move the forwarding setup to the PreCreates hook on the
testcontainer. Since remote forwarding doesn't establish a connection to
the host port until a connection is made to the remote port, this should
not be an issue even if the host isn't listening yet and ensures the
remote port is available to the testcontainer immediately.
@hathvi hathvi requested a review from a team as a code owner October 5, 2024 17:56
Copy link

netlify bot commented Oct 5, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit ad3f698
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/67017de89ef93400086b7076
😎 Deploy Preview https://deploy-preview-2815--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@stevenh
Copy link
Collaborator

stevenh commented Oct 7, 2024

Thanks for the PR, I've replied the issue, as it seems this is possible without using SSHD port forwarding, so wonder if that's the better way?

@cedric-appdirect
Copy link
Contributor

Thanks for the PR. As HostAccessPorts is part of the API, it is really expected that this work also at startup time, not just when doing Exec later on. I just lost half a day trying to figure out why this wasn't working and to end up here. Looking forward to see it merged.

@stevenh
Copy link
Collaborator

stevenh commented Oct 21, 2024

Thanks for the PR. As HostAccessPorts is part of the API, it is really expected that this work also at startup time, not just when doing Exec later on. I just lost half a day trying to figure out why this wasn't working and to end up here. Looking forward to see it merged.

Thanks for the feedback, sounds like it should be clearly documented or fixed so it works too.

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.

[Bug]: Unable to connect to HostAccessPorts on container startup
3 participants