Skip to content

inspector: proper WS URLs when bound to 0.0.0.0#11755

Merged
eugeneo merged 0 commit intonodejs:masterfrom
eugeneo:0000-address
Mar 13, 2017
Merged

inspector: proper WS URLs when bound to 0.0.0.0#11755
eugeneo merged 0 commit intonodejs:masterfrom
eugeneo:0000-address

Conversation

@eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Mar 8, 2017

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: HTTP handler now checks actual connection IP address.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x labels Mar 8, 2017
bnoordhuis
bnoordhuis requested changes Mar 9, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a 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?

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 9, 2017

@bnoordhuis Thank you for the review. I added a test, CI pending: https://ci.nodejs.org/job/node-test-pull-request/6776/

bnoordhuis
bnoordhuis approved these changes Mar 10, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a 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?

@gibfahn
Copy link
Member

gibfahn commented Mar 10, 2017

Rerun of CI failed on linuxone.

CI 2: https://ci.nodejs.org/job/node-test-pull-request/6777/

not ok 1329 inspector/test-inspector-ip-detection
  ---
  duration_ms: 1.209
  severity: fail
  stack: |-
    [err] Debugger listening on port 9229.
    [err] Warning: This is an experimental feature and could change at any time.
    [err] To start debugging, open the following URL in Chrome:
    [err]     chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=0.0.0.0:9229/13648cdd-2bf2-4982-9772-0c1a25986326
    [err] 
    Error: connect EHOSTUNREACH 148.100.110.64:9229
        at Object.exports._errnoException (util.js:990:11)
        at exports._exceptionWithHostPort (util.js:1013:20)
        at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1105:14)
  ...

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 10, 2017

@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.

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 10, 2017

@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.

@eugeneo eugeneo closed this Mar 13, 2017
@eugeneo eugeneo deleted the 0000-address branch March 13, 2017 17:44
@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 13, 2017

Landed as b170fb7

@eugeneo eugeneo merged commit b170fb7 into nodejs:master Mar 13, 2017
@italoacasas
Copy link

italoacasas commented Mar 14, 2017

The test in /test/inspector/test-inspector-ip-detection is failing in v7 staging because the usage of common.skipIfInspectorDisabled which dosen't exist in v7 at the moment.

common.skipIfInspectorDisabled is not in v7 because the commit adding the functionality need backport #11631

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 14, 2017

@italoacasas Will #11631 be backported or should I change the test?

@eugeneo eugeneo mentioned this pull request Mar 14, 2017
Merged
3 tasks
@italoacasas
Copy link

italoacasas commented Mar 14, 2017

@eugeneo someone should take the lead of that backport, If you have the time available in the same PR you can backport both commits.

jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@jungx098
inspector: proper WS URLs when bound to 0.0.0.0
3aed9cd 
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>
@joshgav
Copy link
Contributor

joshgav commented Mar 28, 2017

@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).

screen shot 2017-03-28 at 6 49 45 pm

@eugeneo
Copy link
Contributor Author

eugeneo commented Mar 28, 2017

I'm afraid I am not familiar with containers...

I can do one of the following:

  1. Revert the patch - /json will then report 0.0.0.0 that does not work in the remote debugging case.
  2. Try to detect when 0.0.0.0 or similar is specified from the command line and only report the detected IP address only in those cases. Otherwise the IP address provided from the command line will be reported.
  3. Any suggestions on how to detect the address other then by checking the TCP connection?

@joshgav
Copy link
Contributor

joshgav commented Mar 28, 2017

@eugeneo will think more, it's a tricky problem. FWIW, when the address is reported as 0.0.0.0 things work in Chrome DevTools and VS Code. What is the case where 0.0.0.0 doesn't work?

BTW, I confirmed my diagnosis by changing the base image in this Dockerfile from node:latest to node:7.7.3, which made remote debugging work again.

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@joshgav joshgav mentioned this pull request Apr 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@eugeneo @gibfahn @italoacasas @joshgav @bnoordhuis @jasnell @MylesBorins @nodejs-github-bot