fix: Improve confusing tunnel security messaging#246
Conversation
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
Signed-off-by: Bartek Mucha <bartosz.mucha@arm.com>
| cmd := exec.Command("curl", fmt.Sprintf("%s:%s", hostName, ct.Port), "--max-time", "1") | ||
| // curl failing is the success path here; suppress its output so a | ||
| // "Failed to connect" line doesn't appear in the deployment log. | ||
| cmd.Stdout = io.Discard | ||
| cmd.Stderr = io.Discard |
There was a problem hiding this comment.
I think this is actually a regression because we're suppressing diagnostic information, but we're not validating that the error is the one we're expecting. According to the curl man page, we can actually use the status codes to be more targeted and only expect status code 7 - "Failed to connect to host". This means host resolution or malformed URLs would be errors.
https://linux.die.net/man/1/curl
There was a problem hiding this comment.
I've suppressed the output, that is correct, however I'd refrain from calling it diagnostic information. For the end user this is meaningless. We could debug log what curl says instead.
But to clarify, I've not introduced a regression, this code was already broken, which is something I'm realising now. Curl will error when trying to poke a protocol it doesn't understand. In case of ssh forwarding it is simply TCP, and trying to curl the port I'm getting curl: (56) Recv failure: Connection reset by peer.
Now, I'm not sure if we can rely on status codes, because curl on windows is not what you think. Perhaps curl.exe behaves like curl with status codes. Will check when I get my hands on a windows box.
There was a problem hiding this comment.
so do you think we need to check the port security without using curl?
There was a problem hiding this comment.
Yes, I see your point. The registry uses HTTP, so I believe the curl test should work correctly if the registry is running (@yejseo01 should be able to confirm, she tested it). The next question becomes whether we should be testing pure TCP or coupling the check the registry running (probably just TCP?).
The thing I can't remember is why we're doing a curl test rather than using Go's networking libs - feels like it would be easier to guarantee the portability of that approach.
Let's integrate this PR with just the UX fixes and address the robustness separately.
There was a problem hiding this comment.
Yes. As long as the registry is running the curl check should be fine.
There was a problem hiding this comment.
The thing I can't remember is why we're doing a curl test rather than using Go's networking libs - feels like it would be easier to guarantee the portability of that approach.
Depending on CGO_ENABLED it may or may not be able to lookup the host using the hostname. I learned that yesterday after trying to do a simple tcp dial in golang.
Yes. As long as the registry is running the curl check should be fine.
As far as I can tell, there's nothing in the code that guarantees registry running before this curl executes. The operations are ordered correctly, but we run registry in the background, so who knows if it's listening or not.
I've tried setting GatewayPorts yes on remote and was not able to make this check fail. 🤷
There was a problem hiding this comment.
I'm surprised that happened. I thought the ordered operations would be good enough for most of the check to be fine. did you make some sort of artificial delay to hit it?
There was a problem hiding this comment.
Can you share how you've been testing this? Perhaps I'm doing something wrong.
Co-authored-by: James Cox-Morton <Th3james@fastmail.fm> Signed-off-by: Bartek Mucha <muchzill4@gmail.com>
3 participants
Changes