Skip to content
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

tls,https: respect address family when connecting #6654

Merged
merged 2 commits into from
May 28, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 9, 2016

Respect the { family: 6 } address family property when connecting to
a remote peer over TLS.

Fixes: #6440

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

@bnoordhuis bnoordhuis added tls Issues and PRs related to the tls subsystem. https Issues or PRs related to the https subsystem. lts-watch-v4.x labels May 9, 2016
@jasnell
Copy link
Member

jasnell commented May 9, 2016

CI failed to start. https://ci.nodejs.org/job/node-test-pull-request/2549/console

LGTM if CI comes up green

@bnoordhuis
Copy link
Member Author

@bnoordhuis
Copy link
Member Author

Added a fix-up for the CI hosts where localhost doesn't have an IPv6 address, PTAL.

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

@bnoordhuis
Copy link
Member Author

That only made it worse... @nodejs/build Can this be fixed on the buildbots?

@jbergstroem
Copy link
Member

@bnoordhuis to be clear you want ::1 localhost into /etc/hosts rite?

@jbergstroem
Copy link
Member

..or is it just whatever ::1 entry? debian8 on do has this: ::1 ip6-localhost ip6-loopback

@bnoordhuis
Copy link
Member Author

@jbergstroem Ideally, I'd like AAAA queries for localhost to resolve to ::1 everywhere.

We currently have this list but trying them in order still doesn't make the tests pass on all machines, see e.g. debian8-x86 (but not debian8-64, oddly enough.)

@jbergstroem
Copy link
Member

@bnoordhuis the reason they differ is because one is from DO and the other from softlayer. providers usually populate differently.

I'll look at adding ::1 localhost to all hosts via the ansible-playbooks; but it might take a small while.

@bnoordhuis
Copy link
Member Author

I'm marking the tests as flaky on Linux for now, pending resolution of nodejs/build#415.

New CI: https://ci.nodejs.org/job/node-test-pull-request/2815/

const https = require('https');

if (!common.hasIPv6) {
console.log('1..0 # Skipped: no IPv6 support');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you switch to the new common.skip().

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2016

LGTM with a couple comments.

@bnoordhuis
Copy link
Member Author

bnoordhuis commented May 28, 2016

Good points. Updated with feedback. New CI: https://ci.nodejs.org/job/node-test-pull-request/2838/

EDIT: Make that https://ci.nodejs.org/job/node-test-pull-request/2839/ - I just landed #7037 to get the CI in a good state again.

@cjihrig
Copy link
Contributor

cjihrig commented May 28, 2016

Still EADDRINUSE errors in https://ci.nodejs.org/job/node-test-commit-arm/3507/nodes=armv7-ubuntu1404/console. I guess the port could have still been in use from older CI runs.

@bnoordhuis
Copy link
Member Author

Should be better now. New CI: https://ci.nodejs.org/job/node-test-pull-request/2840/

@cjihrig
Copy link
Contributor

cjihrig commented May 28, 2016

All green now.

Respect the `{ family: 6 }` address family property when connecting to
a remote peer over TLS.

Fixes: nodejs#4139
Fixes: nodejs#6440
PR-URL: nodejs#6654
Reviewed-By: Colin Ihrig <[email protected]>
Update parallel/test-http-agent-getname to use assert.strictEqual()
consistently and const-ify variables while we're here.

PR-URL: nodejs#6654
Reviewed-By: Colin Ihrig <[email protected]>
@bnoordhuis bnoordhuis deleted the fix6440 branch May 28, 2016 20:50
@bnoordhuis bnoordhuis merged commit ead6c2d into nodejs:master May 28, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
Respect the `{ family: 6 }` address family property when connecting to
a remote peer over TLS.

Fixes: nodejs#4139
Fixes: nodejs#6440
PR-URL: nodejs#6654
Reviewed-By: Colin Ihrig <[email protected]>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
Update parallel/test-http-agent-getname to use assert.strictEqual()
consistently and const-ify variables while we're here.

PR-URL: nodejs#6654
Reviewed-By: Colin Ihrig <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Respect the `{ family: 6 }` address family property when connecting to
a remote peer over TLS.

Fixes: #4139
Fixes: #6440
PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Update parallel/test-http-agent-getname to use assert.strictEqual()
consistently and const-ify variables while we're here.

PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 29, 2016
Respect the `{ family: 6 }` address family property when connecting to
a remote peer over TLS.

Fixes: #4139
Fixes: #6440
PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jun 29, 2016
Update parallel/test-http-agent-getname to use assert.strictEqual()
consistently and const-ify variables while we're here.

PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Respect the `{ family: 6 }` address family property when connecting to
a remote peer over TLS.

Fixes: #4139
Fixes: #6440
PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Update parallel/test-http-agent-getname to use assert.strictEqual()
consistently and const-ify variables while we're here.

PR-URL: #6654
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
kegsay added a commit to matrix-org/matrix-appservice-irc that referenced this pull request Oct 31, 2016
Was previously 4.x for ES6 goodies. Now v4.5 to fix issues with TLS and IPv6
not playing nicely together, which meant that you couldn't connected to IRC
networks over TLS with IPv6.

See:
 - nodejs/node#6654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants