tls: accept `lookup` option for `tls.connect()` #12839

indutny · 2017-05-04T23:12:10Z

net.connect() and consequently http.Agent support custom DNS
lookup option. However, as we move to https.Agent - this option no
longer works because it is not proxied by tls.connect.

Fix this inconsistency by passing it down to net.connect.

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

Affected core subsystem(s)

tls

`net.connect()` and consequently `http.Agent` support custom DNS `lookup` option. However, as we move to `https.Agent` - this option no longer works because it is not proxied by `tls.connect`. Fix this inconsistency by passing it down to `net.connect`.

indutny · 2017-05-04T23:12:16Z

cc @nodejs/collaborators

indutny · 2017-05-04T23:12:22Z

cc @nodejs/crypto

indutny · 2017-05-04T23:12:59Z

@MylesBorins I wonder how far this can be backported? This looks like a very big oversight on our end, that could be fixed for our LTS users.

benjamingr · 2017-05-04T23:18:16Z

@indutny the change looks like there shouldn't be any trouble backporting it to 6.x and 4.x, based on how _tls_wrap looks like at these branches.

addaleax · 2017-05-04T23:20:26Z

I don’t think this could be backported to v4.x since it’s in Maintenance mode now, but for v6.x it should make sense as part of one of the regular semver-minor updates?

indutny · 2017-05-04T23:26:44Z

I'd love to see it backported to v4.x, if this is not in contradiction to our promises. My company is still moving to v6, and v4 upgrade will be much much easier to roll out. Otherwise, I'm going to write some terrible hacky code 😢

addaleax · 2017-05-04T23:37:48Z

@nodejs/lts Thoughts? I think we can consider it a bugfix, it we want to.

subeeshcbabu-zz · 2017-05-04T23:38:25Z

It would be awesome, if this fix lands in v4.x as well. While we are migrating all our code to pick up latest LTS, having this option enabled in v4.x helps us a lot to maintain and phase out the upgrade process.

lpinca · 2017-05-05T06:06:38Z

test/parallel/test-tls-lookup.js

+
+const expectedError = /^TypeError: "lookup" option should be a function$/;
+
+['foobar', 1, {}, []].forEach((input) => connectThrows(input));


Nit: how about passing connectThrows directly instead of wrapping it in another function?

This is practically a copy-paste of test-net-lookup.js FWIW.

lpinca · 2017-05-05T06:08:09Z

test/parallel/test-tls-lookup.js

+  }, expectedError);
+}
+
+connectDoesNotThrow(common.mustCall(common.noop));


Nit: in light of #12822 I'm not sure if it makes sense to use () => {} here.

@lpinca You mean don't use common.noop here? Instead use () => {}? (If so: 👍)

@Trott yes.

sam-github

One suggested reword of docs, otherwise LGTM

sam-github · 2017-05-05T14:10:47Z

doc/api/tls.md

@@ -809,6 +809,7 @@ changes:
    `tls.createSecureContext()`. *Note*: In effect, all
    [`tls.createSecureContext()`][] options can be provided, but they will be
    _completely ignored_ unless the `secureContext` option is missing.
+  * `lookup`: {Function} Custom lookup function. Defaults to [`dns.lookup()`][].


nit: s/Custom/Optional/, for consistency with above.

Should I change net.md as well? This is a copy-paste from it.

Style varies between different files in node. Here, I was looking at the next property up "Optional TLS context..." and thought this function should be internally consistent, but I opened the diff further, there is no consistency, so just leave the current wording as is (or change it if you want). But net is (hopefully) consistent already in how it docs its properties.

MylesBorins · 2017-05-06T00:19:51Z

if this lands as a patch it can likely be included in a future maintenance release... I don't think it would be appropriate to do another minor.

That being said there are some other minors that could offer better consistency across our release lines. While I am reluctant I support LTS discussing this... I've tagged this issue

bnoordhuis

LGTM if you fix https://github.com/nodejs/node/pull/12839/files#r115776198.

thefourtheye · 2017-05-15T16:47:46Z

test/parallel/test-tls-lookup.js

+['foobar', 1, {}, []].forEach(function connectThrows(input) {
+  const opts = {
+    host: 'localhost',
+    port: common.PORT,


There is an ongoing effort to avoid the usage of common.PORT. cc @Trott Can you please change the port number to zero, if it is applicable?

It is a copy-paste of a similar net test FWIW

indutny · 2017-05-15T21:26:08Z

Landed in e600fbe, thank you everyone!

`net.connect()` and consequently `http.Agent` support custom DNS `lookup` option. However, as we move to `https.Agent` - this option no longer works because it is not proxied by `tls.connect`. Fix this inconsistency by passing it down to `net.connect`. PR-URL: #12839 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

`net.connect()` and consequently `http.Agent` support custom DNS `lookup` option. However, as we move to `https.Agent` - this option no longer works because it is not proxied by `tls.connect`. Fix this inconsistency by passing it down to `net.connect`. PR-URL: nodejs#12839 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2017-09-19T21:21:51Z

LTS WG agreed that this can be backported

indutny · 2017-09-19T21:24:15Z

Hooray!

`net.connect()` and consequently `http.Agent` support custom DNS `lookup` option. However, as we move to `https.Agent` - this option no longer works because it is not proxied by `tls.connect`. Fix this inconsistency by passing it down to `net.connect`. PR-URL: #12839 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

MylesBorins · 2018-01-11T18:06:35Z

It looks like we agreed to backport but never did! I just landed on v6.x-staging

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

indutny added the tls Issues and PRs related to the tls subsystem. label May 4, 2017

indutny requested review from sam-github, bnoordhuis and shigeki May 4, 2017 23:12

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 4, 2017

benjamingr approved these changes May 4, 2017

View reviewed changes

addaleax approved these changes May 4, 2017

View reviewed changes

addaleax added lts-watch-v4.x semver-minor PRs that contain new features and should be released in the next minor version. and removed lts-watch-v4.x labels May 4, 2017

refack approved these changes May 4, 2017

View reviewed changes

lpinca reviewed May 5, 2017

View reviewed changes

lpinca approved these changes May 5, 2017

View reviewed changes

cjihrig approved these changes May 5, 2017

View reviewed changes

jasnell approved these changes May 5, 2017

View reviewed changes

joyeecheung approved these changes May 5, 2017

View reviewed changes

sam-github approved these changes May 5, 2017

View reviewed changes

MylesBorins added the lts-agenda label May 6, 2017

bnoordhuis approved these changes May 15, 2017

View reviewed changes

thefourtheye reviewed May 15, 2017

View reviewed changes

fix

a95f6ea

indutny closed this May 15, 2017

indutny deleted the feature/tls-lookup branch May 15, 2017 21:26

gabrielcsapo mentioned this pull request May 18, 2017

v4.x backport change https://github.com/nodejs/node/pull/12839 #13105

Closed

4 tasks

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

bitinn mentioned this pull request Nov 5, 2018

Doc: Agent ctor options are not fully documented, such as lookup #24098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: accept `lookup` option for `tls.connect()` #12839

tls: accept `lookup` option for `tls.connect()` #12839

indutny commented May 4, 2017

indutny commented May 4, 2017

indutny commented May 4, 2017

indutny commented May 4, 2017

benjamingr commented May 4, 2017

addaleax commented May 4, 2017

indutny commented May 4, 2017

addaleax commented May 4, 2017

subeeshcbabu-zz commented May 4, 2017

lpinca May 5, 2017

indutny May 5, 2017

indutny May 6, 2017

lpinca May 5, 2017

Trott May 5, 2017 •

edited

Loading

lpinca May 5, 2017

indutny May 6, 2017

sam-github left a comment

sam-github May 5, 2017

indutny May 5, 2017

sam-github May 5, 2017

MylesBorins commented May 6, 2017

bnoordhuis left a comment

thefourtheye May 15, 2017

indutny May 15, 2017

indutny commented May 15, 2017

MylesBorins commented Sep 19, 2017 •

edited

Loading

indutny commented Sep 19, 2017

MylesBorins commented Jan 11, 2018


		const expectedError = /^TypeError: "lookup" option should be a function$/;

		['foobar', 1, {}, []].forEach((input) => connectThrows(input));

tls: accept lookup option for tls.connect() #12839

tls: accept lookup option for tls.connect() #12839

Conversation

indutny commented May 4, 2017

Checklist

Affected core subsystem(s)

indutny commented May 4, 2017

indutny commented May 4, 2017

indutny commented May 4, 2017

benjamingr commented May 4, 2017

addaleax commented May 4, 2017

indutny commented May 4, 2017

addaleax commented May 4, 2017

subeeshcbabu-zz commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented May 6, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented May 15, 2017

MylesBorins commented Sep 19, 2017 • edited Loading

indutny commented Sep 19, 2017

MylesBorins commented Jan 11, 2018

tls: accept `lookup` option for `tls.connect()` #12839

tls: accept `lookup` option for `tls.connect()` #12839

Trott May 5, 2017 •

edited

Loading

MylesBorins commented Sep 19, 2017 •

edited

Loading