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

[#50] Consider Docker host IP during port mapping resolution #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rvem
Copy link

@rvem rvem commented Apr 18, 2024

In some cases, 'localhost' might be resolved to IPv6 '::1', while 'containerPort' will grab IPv4 host IP port mapping by default, when Docker maps different ports for host's IPv4 and IPv6 addresses, this might cause timeout errors.

To solve this issue, the Docker host is resolved from 'localhost' at container creation time, host IP protocol version is later used to deduce the corresponding port in 'containerPort' function.

Additionally, helper 'containerAddress', as well as 'TraceOpenSocket' and 'TraceHttpCall' now operate 'IP' addresses instead of plain 'Text' domains. This should be safe since we only actually consider domains within some Docker network or 'localhost' at the moment.

My solution is quite hacky since I was a bit afraid to break backward compatibility, would be happy to hear your feedback

Somewhat resolves #50.

…ution

In some cases, 'localhost' might be resolved to IPv6 '::1', while
'containerPort' will grab IPv4 host IP port mapping by default,
when Docker maps different ports for host's IPv4 and IPv6 addresses,
this might cause timeout errors.

To solve this issue, the Docker host is resolved from 'localhost' at
container creation time, host IP protocol version is later used to
deduce the corresponding port in 'containerPort' function.

Additionally, helper 'containerAddress', as well as 'TraceOpenSocket'
and 'TraceHttpCall' now operate 'IP' addresses instead of
plain 'Text' domains. This should be safe since we only actually
consider domains within some Docker network or 'localhost' at the
moment.
@rvem rvem requested a review from alexbiehl as a code owner April 18, 2024 11:06
@alexbiehl
Copy link
Collaborator

alexbiehl commented Apr 20, 2024

Nice, thanks for the contribution.

Did you research how Java, Ruby and Go testcontainers approach the problem?

That being said, the only way this breaks backward compat is by introducing the IP type where it was a Text before? That part of the change can be undone easily, no? Would love to avoid breaking API changes.

@rvem
Copy link
Author

rvem commented Apr 22, 2024

That part of the change can be undone easily, no?

Yeah, but I considered avoiding Text -> IP -> Text -> IP conversions. Additionally, for waitForHttp the manager expects the IPv6 host address to be wrapped with [] while getAddrInfo expects IPv6 without them. Perhaps we can introduce something like

data ContainerAddress = ContainerIP IP | ContainerAlias Text

with the Show/FromString instances. Though, this looks a bit redundant because AFAIU currently we can always deduce the container IP from inspect output

@rvem
Copy link
Author

rvem commented Apr 22, 2024

Did you research how Java, Ruby and Go testcontainers approach the problem?

testcontainers-go has a similar issue, but it was closed, see testcontainers/testcontainers-go#551. They suggest exposing the port via IPv4 0.0.0.0 explicitly. For testcontainers-node this is also somewhat known issue, see https://github.com/testcontainers/testcontainers-node/blob/main/docs/supported-container-runtimes.md#usage-1

@rvem
Copy link
Author

rvem commented Apr 22, 2024

They suggest exposing the port via IPv4 0.0.0.0 explicitly

I also have a branch with changes that allow exposing a container port on a specific host IP, see serokell@5d5ca97. Let me know if you consider them worth another PR 😅

However, these changes don't directly solve the root problem since ryuk port is hardcoded at the moment, and I wasn't brave enough to change it to 0.0.0.0::8080 instead of ::8080

@rvem
Copy link
Author

rvem commented Apr 22, 2024

Yeah, but I considered avoiding Text -> IP -> Text -> IP conversions

OTOH, it's quite hard to avoid them, we do Text -> IP -> Text anyway

@alexbiehl
Copy link
Collaborator

Sorry for the delay, I’ll take care of this during zurihac.t

@rvem
Copy link
Author

rvem commented May 17, 2024

Yeah, sure, no hurry. At the moment I'm ok with using testcontainers-hs from the fork

@npopov8
Copy link

npopov8 commented Sep 27, 2024

@alexbiehl any chance this can get revisited?

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.

Make it possible to customize 'ryuk' reaper config
3 participants