-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
inspector: proper WS URLs when bound to 0.0.0.0 #11755
Conversation
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.
Changes themselves LGTM but can you add a regression test?
@bnoordhuis Thank you for the review. I added a test, CI pending: https://ci.nodejs.org/job/node-test-pull-request/6776/ |
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.
LGTM. I wonder though, what happens in two tests try to start an inspector instance concurrently? It looks like one of them will lose out with a port conflict?
Rerun of CI failed on linuxone. CI 2: https://ci.nodejs.org/job/node-test-pull-request/6777/
|
@gibfahn - I fixed that https://ci.nodejs.org/job/node-test-pull-request/6785/ - test will skip if the system does not allow connecting to its external IP. LGTM was for the fixed version. |
@bnoordhuis I am planning to update the code so port :0 would be supported from the command line (right now it is command line options parser that explicitly prevents it) - but that change caused some failures in the legacy debugger tests so I will have to look into it. I will update inspector tests to use :0 once it is working. |
Landed as b170fb7 |
The test in
|
@italoacasas Will #11631 be backported or should I change the test? |
@eugeneo someone should take the lead of that backport, If you have the time available in the same PR you can backport both commits. |
JSON target list response will now return appropriate IP address for instances listening on 0.0.0.0. Refs: nodejs#11591 PR-URL: nodejs#11755 Reviewed-By: James Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@eugeneo I think this change breaks remote debugging by giving the remote debugger an IP address (host) which is internal to the system where Node is running. For example, if Node is running in a container the debugger might connect via the container host's name/address or via localhost over SSH (or both). The initial HTTP request for /json works, but since the debugger then uses the webSocketDebuggerUrl property from that response and that property has the internal IP address, the ensuing WebSocket connection fails. The following Wireshark capture may help illustrate the issue. Note that after the /json response the debugger tries to use the internal IP address (and gets nowhere). |
I'm afraid I am not familiar with containers... I can do one of the following:
|
@eugeneo will think more, it's a tricky problem. FWIW, when the address is reported as BTW, I confirmed my diagnosis by changing the base image in this Dockerfile from |
JSON target list response will now return appropriate IP address
for instances listening on 0.0.0.0.
Ref: #11591
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
inspector: HTTP handler now checks actual connection IP address.