Fix /proc/net/tcp* check in InternalCommandPortListeningCheck t…

[perlun]

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 a dash 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:

$ denter tomcat
root@85217088bde8:/usr/local/tomcat# true &&  (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :2490)
00000000:2490

root@85217088bde8:/usr/local/tomcat# sh
# true &&  (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :2490)
cat: /proc/net/tcp{,6}: No such file or directory

I added a check to ensure that nothing is printed to stderr from this check, since result.getExitCode() == 0 seems to be true 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.

command += format("/bin/bash -c '</dev/tcp/localhost/%d'", internalPort);

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.

[rnorth]

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 sh etc.

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 sh but avoid the brace expansion?

It would still be good to check both /proc/net/tcp and /proc/net/tcp6, so perhaps we could just change that one line to run cat over /proc/net/tcp*? I could be wrong, but I think this would do the trick with very little effort or risk of regression. WDYT?

[perlun]

@rnorth

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 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 nc part of it will mean that we actually open a connection to the service inside the container. To get rid of this log noise, I started doing my own local copy of InternalCommandPortListeningCheck and noted that the check was actually failing. (Fixing the check also got rid of the noise in the logs)

The cat /proc/net/tcp{,6} part has the advantage in being much more "silent", compared to the netcat approach, which is why I wanted to get it working. But like you say, cat /proc/net/tcp* ought to be a much safer change. I updated the PR in accordance to this now, hopefully it'll pass the tests this time. 🙂

[perlun]

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.

[rnorth]

Sorry for taking a while to respond @perlun, but I'm happy to approve.

[perlun]

@rnorth Thanks. Looking forward to see it merged. 🙂

[perlun]

Ping @bsideup @kiview