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

net,dgram: add ipv6Only option for net and dgram #23798

Closed
wants to merge 1 commit into
base: master
from

Conversation

@oyyd
Copy link
Contributor

oyyd commented Oct 21, 2018

For our servers, the dual-stack support is enable by default, i.e. binding host "::" will also make "0.0.0.0" bound. This commit add ipv6Only option in net.Server.listen(), net.Socket.connect() and dgram.createSocket() methods which allows to disable dual-stack support.

Support for cluster module is also provided in this commit.

Fixes: #17664

/cc @silverwind

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@@ -266,6 +266,9 @@ added: v0.11.14
for all users. **Default:** `false`
* `writableAll` {boolean} For IPC servers makes the pipe writable
for all users. **Default:** `false`
* `ipv6only` {boolean} For TCP servers, setting `ipv6only` to `true` will
disable the dual-stack support, i.e., binding host `::` won't make
`0.0.0.0` be bound. **Default:** `false`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Oct 21, 2018

Member

Nit:

Suggested change Beta
`0.0.0.0` be bound. **Default:** `false`
`0.0.0.0` be bound. **Default:** `false`.
@@ -615,6 +618,8 @@ For TCP connections, available `options` are:
**Default:** `4`.
* `hints` {number} Optional [`dns.lookup()` hints][].
* `lookup` {Function} Custom lookup function. **Default:** [`dns.lookup()`][].
* `ipv6only` {boolean} Disable the dual-stack support when set to `true`.
**Default:** `false`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Oct 21, 2018

Member

Nit:

Suggested change Beta
**Default:** `false`
**Default:** `false`.
@@ -266,6 +266,9 @@ added: v0.11.14
for all users. **Default:** `false`
* `writableAll` {boolean} For IPC servers makes the pipe writable
for all users. **Default:** `false`
* `ipv6only` {boolean} For TCP servers, setting `ipv6only` to `true` will

This comment has been minimized.

@mscdex

mscdex Oct 21, 2018

Contributor

minor nit: perhaps this should be camel case (ipv6Only)?

This comment has been minimized.

@oyyd

oyyd Oct 22, 2018

Contributor

Though I prefer ipv6only here, it's better to follow camel case :)

if (!common.hasIPv6)
common.skip('no IPv6 support');

// This test ensures that dual-stack suuport is disabled when

This comment has been minimized.

@richardlau

richardlau Oct 21, 2018

Member
Suggested change Beta
// This test ensures that dual-stack suuport is disabled when
// This test ensures that dual-stack support is disabled when

@oyyd oyyd force-pushed the oyyd:net-ipv6only branch 2 times, most recently from 291bb1d to 91e21b2 Oct 22, 2018

port,
host: address,
// Currently, net module only supports `ipv6Only` option in `flags`.
ipv6Only: Boolean(flags | constants.UV_TCP_IPV6ONLY),

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 22, 2018

Member

This looks wrong. Boolean(flags | constants.UV_TCP_IPV6ONLY) is always true. Did you mean to use &?

This comment has been minimized.

@oyyd

oyyd Oct 22, 2018

Contributor

Yes, a terrible mistake. I'm going to update it and add a test of falsy ipv6Only scenario.

lib/net.js Outdated
@@ -98,6 +98,10 @@ const {

function noop() {}

function getFlags(ipv6Only = false) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 22, 2018

Member

Unused default argument.

This comment has been minimized.

@oyyd

oyyd Oct 22, 2018

Contributor

The default value of ipv6Only would be actually undefined rather than false which is how its document described. This is the only reason I set the default argument.

@oyyd oyyd changed the title net: add ipv6only option for net module net: add ipv6Only option for net module Oct 22, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Oct 22, 2018

PTAL. (Travis ci task for this halted..)

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Oct 22, 2018

Thanks for doing this. Would you mind als contributing the same option to UDP / dgram?

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Oct 23, 2018

Yes, that's possible. But the last discussion #17664 (comment) shows there is no need to provide the same option to dgram. How do you think about this now?

@@ -266,6 +266,9 @@ added: v0.11.14
for all users. **Default:** `false`
* `writableAll` {boolean} For IPC servers makes the pipe writable
for all users. **Default:** `false`
* `ipv6Only` {boolean} For TCP servers, setting `ipv6Only` to `true` will
disable the dual-stack support, i.e., binding host `::` won't make

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 23, 2018

Member

Just "disable dual-stack support", no "the." (And you'd normally say "binding to host", not "binding host".)

// This test ensures that the dual-stack support still works for cluster module
// when `ipv6Only` is not `true`.
const host = '::';
const WORKER_ACCOUNT = 3;

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 23, 2018

Member

Did you mean to write WORKER_COUNT? The name's a bit confusing to me.

This comment has been minimized.

@oyyd

oyyd Oct 24, 2018

Contributor

I should have been more careful.

const workers = new Map();
let address;

const countdown = new Countdown(WORKER_ACCOUNT, common.mustCall(() => {

This comment has been minimized.

@jasnell

jasnell Oct 23, 2018

Member

It's unnecessary to wrap the Countdown callback in a mustCall(). That's done automatically.

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Oct 24, 2018

I think I misunderstood the comment #17664 (comment). We could bind to and use unspecified address and it works as expected:

example
const dgram = require('dgram');

const socket = dgram.createSocket('udp6');
socket.on('message', (msg, rinfo) => {
  console.log('rinfo', rinfo)
});
socket.bind(0, '::', () => {
  const { port } = socket.address();
  const client = dgram.createSocket('udp4');
  // Ensure the `client` won't bind ipv6.
  client.bind(0, () => {
    client.send(Buffer.allocUnsafe(1), port, '0.0.0.0');
  });
});

So I would like to add the option to dgram too. Maybe I should open another PR?

@oyyd oyyd force-pushed the oyyd:net-ipv6only branch from cbafc39 to daa9abc Oct 24, 2018

@oyyd oyyd changed the title net: add ipv6Only option for net module net,dgram: add ipv6Only option for net and dgram Oct 24, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Oct 24, 2018

ipv6Only for dgram.createSocket() is added. PTAL.

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Oct 24, 2018

Yes, that's possible. But the last discussion #17664 (comment) shows there is no need to provide the same option to dgram. How do you think about this now?

I'd say still go for it. Even if the OS might not properly support the socket option, we can still expose it.

const workers = new Map();
let address;

const countdown = new Countdown(WORKER_ACCOUNT, common.mustCall(() => {

This comment has been minimized.

@jasnell

jasnell Oct 24, 2018

Member

nit: don't need the common.mustCall() wrapper here.

const workers = new Map();
let address;

const countdown = new Countdown(WORKER_ACCOUNT, common.mustCall(() => {

This comment has been minimized.

@jasnell

jasnell Oct 24, 2018

Member

nit: don't need the common.mustCall() wrapper here.

@oyyd oyyd force-pushed the oyyd:net-ipv6only branch from fe146de to aad0e5a Oct 25, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Oct 25, 2018

/ping @silverwind @vsemozhetbyt @mscdex @richardlau @jasnell It should look good now. PTAL.

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Oct 25, 2018

Docs LGTM.

Show resolved Hide resolved doc/api/dgram.md Outdated
@Trott

This comment has been minimized.

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 12, 2018

parallel/test-net-connect-ipv6only failed on AIX:

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: connect EHOSTUNREACH :::35876 - Local (undefined:undefined)
    at internalConnect (net.js:883:16)
    at defaultTriggerAsyncIdScope (internal/async_hooks.js:294:19)
    at defaultTriggerAsyncIdScope (net.js:974:9)
    at internalTickCallback (internal/process/next_tick.js:70:11)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    at Function.Module.runMain (internal/modules/cjs/loader.js:778:11)
    at startup (internal/bootstrap/node.js:300:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)
Emitted 'error' event at:
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at internalTickCallback (internal/process/next_tick.js:72:19)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    [... lines matching original stack trace ...]
    at bootstrapNodeJSCore (internal/bootstrap/node.js:833:3)

I have no idea why this happened though.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 12, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 15, 2018

I suggest removing the support for net.Socket.connect() in this PR as it fails on AIX and I guess we need to resolve it in libuv. The rest parts still make sense as the support for server is what we mostly care about.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 18, 2018

I suggest removing the support for net.Socket.connect() in this PR as it fails on AIX and I guess we need to resolve it in libuv. The rest parts still make sense as the support for server is what we mostly care about.

Will you be doing that and rebasing? If not, should we apply a help wanted label to this so someone else will do it?

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 18, 2018

I will remove the net.Socket.connect() and rebase again.

net,dgram: add ipv6Only option for net and dgram
For TCP servers, the dual-stack support is enable by default, i.e.
binding host "::" will also make "0.0.0.0" bound. This commit add
ipv6Only option in `net.Server.listen()` and `dgram.createSocket()`
methods which allows to disable dual-stack support. Support for
cluster module is also provided in this commit.

Fixes: #17664

@oyyd oyyd force-pushed the oyyd:net-ipv6only branch from 539ae7c to b047728 Nov 21, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 21, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 21, 2018

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 21, 2018

I have removed the support for net.Socket.connect() and the CI is green now.

/cc @bnoordhuis @jasnell Does this still LGTY?

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Nov 21, 2018

Yes, still LGTM

@oyyd

This comment has been minimized.

Copy link
Contributor

oyyd commented Nov 22, 2018

Landed in 33a25b2. Thanks!

@oyyd oyyd closed this Nov 22, 2018

oyyd added a commit that referenced this pull request Nov 22, 2018

net,dgram: add ipv6Only option for net and dgram
For TCP servers, the dual-stack support is enable by default, i.e.
binding host "::" will also make "0.0.0.0" bound. This commit add
ipv6Only option in `net.Server.listen()` and `dgram.createSocket()`
methods which allows to disable dual-stack support. Support for
cluster module is also provided in this commit.

Fixes: #17664

PR-URL: #23798
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

@oyyd oyyd added the dgram label Nov 22, 2018

targos added a commit that referenced this pull request Nov 24, 2018

net,dgram: add ipv6Only option for net and dgram
For TCP servers, the dual-stack support is enable by default, i.e.
binding host "::" will also make "0.0.0.0" bound. This commit add
ipv6Only option in `net.Server.listen()` and `dgram.createSocket()`
methods which allows to disable dual-stack support. Support for
cluster module is also provided in this commit.

Fixes: #17664

PR-URL: #23798
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

rvagg added a commit that referenced this pull request Nov 28, 2018

net,dgram: add ipv6Only option for net and dgram
For TCP servers, the dual-stack support is enable by default, i.e.
binding host "::" will also make "0.0.0.0" bound. This commit add
ipv6Only option in `net.Server.listen()` and `dgram.createSocket()`
methods which allows to disable dual-stack support. Support for
cluster module is also provided in this commit.

Fixes: #17664

PR-URL: #23798
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

@BridgeAR BridgeAR referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 6, 2018

2018-12-06, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

BridgeAR added a commit that referenced this pull request Dec 7, 2018

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

net,dgram: add ipv6Only option for net and dgram
For TCP servers, the dual-stack support is enable by default, i.e.
binding host "::" will also make "0.0.0.0" bound. This commit add
ipv6Only option in `net.Server.listen()` and `dgram.createSocket()`
methods which allows to disable dual-stack support. Support for
cluster module is also provided in this commit.

Fixes: nodejs#17664

PR-URL: nodejs#23798
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment