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.

Sign up for GitHub

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

Jump to bottom

tls,https: respect address family when connecting #6654

Merged
merged 2 commits into from
May 28, 2016
Merged

tls,https: respect address family when connecting #6654

merged 2 commits into from
May 28, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 9, 2016 •
edited
Loading

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
@bnoordhuis bnoordhuis mentioned this pull request May 9, 2016
Closed
@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

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

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

@jbergstroem jbergstroem mentioned this pull request May 11, 2016
Closed
@bnoordhuis bnoordhuis mentioned this pull request May 27, 2016
Closed
@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/

cjihrig
cjihrig reviewed May 27, 2016
View reviewed changes
test/parallel/test-https-connect-address-family.js
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 •
edited
Loading

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.

@bnoordhuis
tls,https: respect address family when connecting
fb19b80 
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 <cjihrig@gmail.com>
@bnoordhuis
test: use strictEqual consistently in agent test
ead6c2d 
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 <cjihrig@gmail.com>
@bnoordhuis bnoordhuis closed this May 28, 2016
@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
@bnoordhuis @Fishrock123
tls,https: respect address family when connecting
2ab7139 
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 <cjihrig@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 30, 2016
@bnoordhuis @Fishrock123
test: use strictEqual consistently in agent test
0230fe3 
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 <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@bnoordhuis @rvagg
tls,https: respect address family when connecting
257e54b 
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 <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
@bnoordhuis @rvagg
test: use strictEqual consistently in agent test
8c289df 
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 <cjihrig@gmail.com>
@gibfahn gibfahn mentioned this pull request Jun 13, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Jun 29, 2016
@bnoordhuis
tls,https: respect address family when connecting
c7c5a78 
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 <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 29, 2016
@bnoordhuis
test: use strictEqual consistently in agent test
8cbcea0 
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 <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@bnoordhuis
tls,https: respect address family when connecting
c807287 
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 <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
@bnoordhuis
test: use strictEqual consistently in agent test
e3f9bc8 
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 <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
Merged
kegsay added a commit to matrix-org/matrix-appservice-irc that referenced this pull request Oct 31, 2016
@kegsay
Supported on Node v4.5+
340b8b9 
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
@DoobleD DoobleD mentioned this pull request Sep 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
https Issues or PRs related to the https subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bnoordhuis @jasnell @jbergstroem @cjihrig @MylesBorins