feat: Add wait strategy to check external (TCP) port availability#1495
feat: Add wait strategy to check external (TCP) port availability#1495HofmeisterAn merged 23 commits intotestcontainers:developfrom
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Mmh, I just noticed, that testcontainers-java implemented this check in the general waitForHostPort Strategy. |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Mmh, I just noticed, that testcontainers-java implemented this check in the general waitForHostPort Strategy. Maybe this would be also a idea for us? Then it wouldn't be as bad, that the check from host doesn't work with some host configurations, as we would still have the Check from inside of the container.
Aligning with the other TC language implementations sounds good and is something we try to follow. I had a quick look, and I agree. It makes sense to follow both the internal and external listening checks. Ideally, we implement a new wait strategy (HostPortWaitStrategy) and mark the old one obsolete? WDYT?
…ut doesn't accept connections
|
As the WaitStrategy for the internal Port depends on the ContainerOS I couldn't think of a way to have a WaitStrategy for both OS. So I implemented one class for now and let the I noticed that the current internal port waiting strategies only work with tcp (same for java implementation), so I decided to name the strategy to make that clear. When we remove the internal only WaitingStrategies, we can refactor this without having duplicated code. Should I mark the existing Is there a way to output more specific errors in WaitStrategies? I forgot to map a port myself and needed to add debugging Console outputs until I figured that out :D |
You can pass the internal wait strategy, which depends on the OS, to the wait strategy using dependency injection:
Throw an exception. Don't swallow the exception from |
|
Oh yea, it's my fault that the exception is gone🙈. That's fixed now! I thought about using dependency injection to require a Should I implement it via constructor, another way or leave it like it is? |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Thanks for the adjustments, the PR looks good! I just have a quick question to make sure I've understood everything correctly.
UntilHostTcpPortAvailable(int) checks the remote connection (host to container), but not the internal connection (container to actual service), right?
we would invoke the internal Wait check every time again when the external wait check fails and the Wait is retried.
I was considering adding two wait strategies to the list. Once the first one succeeds, it will not run again. Something like:
return AddCustomWaitStrategy(new Internal...()).AddCustomWaitStrategy(new External...());But this is probably unnecessary, and we can just rely on checking the remote connection.
src/Testcontainers/Configurations/WaitStrategies/UntilHostTcpPortIsAvailable.cs
Outdated
Show resolved
Hide resolved
tests/Testcontainers.Platform.Windows.Tests/WindowsContainerTest.cs
Outdated
Show resolved
Hide resolved
|
I'm also wondering if we should choose different names that are more meaningful and better describe what they check (mark the old one obsolete in Edit: Maybe something like |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Thanks for the PR and your contribution.
What does this PR do?
Using the TcpClient the new WaitStrategy allows to test if a port ist open from the host perspective.
Why is it important?
As discussed in #1350 it may be helpful to verify, that a Port is not only listening from inside the container but also from the host.
Related issues
How to test this PR
I added tests for the new waiting strategy. The only downside is, that one of the negative tests (that the wait fails when a port is not listening) fails on some of my systems as the proxy is listening on any mapped port. So you may get a error (for me with Rancher on Mac & Windows)