-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix /proc/net/tcp* check in InternalCommandPortListeningCheck t… #2195
Fix /proc/net/tcp* check in InternalCommandPortListeningCheck t… #2195
Conversation
Interesting - out of curiosity is this causing you an actual issue, or have you just noticed this is incorrect? Happy to fix it either way! The brace expansion does look to be a bash-ism which will fail in However, I'm unsure that changing the entire port check to run under bash is the right thing to do; we don't have bash available in many containers, so this is going to cause the internal port check to always fail. Indeed, it looks like this is exactly what's happening in CI right now. What if instead we keep things running under It would still be good to check both |
0cd81b9
to
a965020
Compare
The check works but we were seeing some TCP connections being performed to our (Tomcat-based, Java backend) service which listens to some arbitrary ports inside the container. In our use case, a TCP connection being opened and then just dropped is an error (we expect some data to be transmitted over a connection to our service); we log this as a warning/error. I presumed this was coming from the port check; the The |
...main/java/org/testcontainers/containers/wait/internal/InternalCommandPortListeningCheck.java
Show resolved
Hide resolved
Bump @bsideup or @rnorth, would be great to get this moving forward. #2195 (comment) describes why adding a test for the change isn't straightforward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking a while to respond @perlun, but I'm happy to approve.
@rnorth Thanks. Looking forward to see it merged. 🙂 |
testcontainers/testcontainers-java#2195 Testcontainer .start() command was unable to connect to docker image otherwise.
I noted this when running this code locally; the first part of the check didn't work correctly. After looking more closely, it turned out that we use bashisms with
/bin/sh
which might work but in many cases (e.g. Debian, Ubuntu-based containers)/bin/sh
is adash
binary.Here is the error. The first invocation illustrates how the statement works properly with bash and the second shows how it fails with dash:
I added a check to ensure that nothing is printed to
stderr
from this check, sinceresult.getExitCode() == 0
seems to betrue
even in cases where parts of this command line is failing. Digging deeper, it's the fallback to his part that typically covers scenarios like this.I can consider reverting the
if (!result.getStderr().equals(""))
part if it's considered too dangerous, but IMHO it's good to have it there to spot errors like this in the future. I'll leave it up to the project maintainers to decide if this is safe or too risky.I thought more about the
stderr
part; it will probably not work since I think in the lattermost fallback (/bin/bash -c '</dev/tcp/localhost/%d
), an error will actually be printed to stderr if the port is being used. So I'm fine with removing that part, but will wait with doing so until I hear more feedback on the PR in general.