Fix internal port check when other ports are opened as well on the target container #2363
Conversation
|
Hi @codablock, Could you please add tests for it? |
|
@bsideup I added 3 commits to this PR.
|
core/src/test/resources/internal-port-check-dockerfile/Dockerfile-bash
Outdated
Show resolved
Hide resolved
core/src/test/resources/internal-port-check-dockerfile/Dockerfile-tcp
Outdated
Show resolved
Hide resolved
.../java/org/testcontainers/containers/wait/internal/InternalCommandPortListeningCheckTest.java
Show resolved
Hide resolved
|
Handled review comments. I also noticed that I forgot to commit an important change in |
|
Test failures are not reproducible locally. And it's unfortunately the new test that fails, so it's likely related. It's also strange that they succeed on circleci. Any ideas? |
|
@codablock I just triggered a re-run, let's see how it goes |
|
Tests failed again. But I think I found it: The |
|
The port check tests are green now. Other test failures on Azure Pipelines are unrelated ( |
|
@codablock it seems there was an issue with the disk space on Azure, I've submitted #2376 to fix it. Let's update your PR and do another run once that PR is merged to ensure that this change does not bring any side effects 👍 |
This one is never used and probably causes some garbage on the docker side.
… check When at least one process is listening for a port that is significantly higher then the port we're checking, this check will fail as the checked port will contain leading zeros in the output of "cat /proc/net/tcp". This commit fixes this by changing the grep to ":0*XXX".
Otherwise unnecessary shell expansion is tried
…ssible checks This commit refactors InternalCommandPortListeningCheckTest to work with 3 different nginx based containers, each being able to only succeed one of the 3 tests performed in InternalCommandPortListeningCheck. Each test is now executed agains all 3 containers to ensure that all tests work as expected.
This test should fail without the leading zeros fix found a few commits before this one.
It's not only port 8080 anymore, so better not have it in the name.
|
@codablock merged, thanks! 🎉 |
|
This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉 Thanks for the contribution! |
…rget container (testcontainers#2363) * Remove redundant execCreateCmd call This one is never used and probably causes some garbage on the docker side. * Allow leading zeros in /proc/net/tcp* when doing internal port listen check When at least one process is listening for a port that is significantly higher then the port we're checking, this check will fail as the checked port will contain leading zeros in the output of "cat /proc/net/tcp". This commit fixes this by changing the grep to ":0*XXX". * Surround grep parameter with ' Otherwise unnecessary shell expansion is tried * Refactor/Rewrite InternalCommandPortListeningCheckTest to test all possible checks This commit refactors InternalCommandPortListeningCheckTest to work with 3 different nginx based containers, each being able to only succeed one of the 3 tests performed in InternalCommandPortListeningCheck. Each test is now executed agains all 3 containers to ensure that all tests work as expected. * Add test for low+high ports This test should fail without the leading zeros fix found a few commits before this one. * Check for bash existence instead of only mentioning it in a comment * Rename nginx_on_8080.conf to nginx.conf It's not only port 8080 anymore, so better not have it in the name. * Make container a @rule again by moving initialization into contructor * Wait a few seconds for InternalCommandPortListeningCheck to pass
This PR contains 2 commits. The first one is a trivial cleanup of a redundant execCreateCmd call. The second commit contains the actual fix. Description from the second commit: