Skip to content
This repository was archived by the owner on May 14, 2024. It is now read-only.

Progress on supporting IPv6 #805

Merged
jsumners merged 3 commits into from
Jun 8, 2022
Merged

Progress on supporting IPv6 #805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
467dcf5
Progress on supporting IPv6
MaxLeiter Jun 1, 2022
b6689dc
Apply suggestions from code review
Jun 2, 2022
8875d92
tests: add IPv6 URL format test to server.test.js
MaxLeiter Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,15 @@ Object.defineProperties(Server.prototype, {
} else {
str = 'ldap://'
}
str += this.host + ':' + this.port

let host = this.host
// Node 18 switched family from returning a string to returning a number
// https://nodejs.org/api/net.html#serveraddress
if (addr.family === 'IPv6' || addr.family === 6) {
host = '[' + this.host + ']'
}
Comment on lines +515 to +517
Copy link
Copy Markdown
Member

@jsumners jsumners Jun 2, 2022

Choose a reason for hiding this comment

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

We need a test to check for IPv6 connectivity. Since we don't yet have v18 in the testing matrix, we don't need to cover the branch. I don't want to add v18 to the testing matrix until I can cut the next major release.

Copy link
Copy Markdown
Member

@jsumners jsumners Jun 7, 2022

Choose a reason for hiding this comment

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

@MaxLeiter are you able to add this test?

Copy link
Copy Markdown
Author

@MaxLeiter MaxLeiter Jun 7, 2022

Choose a reason for hiding this comment

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

Yeah sorry, I misunderstood what you meant by “dont need to cover this test”

Copy link
Copy Markdown
Author

@MaxLeiter MaxLeiter Jun 7, 2022

Choose a reason for hiding this comment

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

Test pushed. Lmk if that's what you meant.

Copy link
Copy Markdown
Author

@MaxLeiter MaxLeiter Jun 7, 2022

Choose a reason for hiding this comment

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

Suites:   ​54 passed​, ​54 of 54 completed​
Asserts:  ​​​2782 passed​, ​of 2782​


str += host + ':' + this.port
return str
},
configurable: false
Expand Down
17 changes: 14 additions & 3 deletions test/server.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ tap.test('basic create', function (t) {
tap.test('connection count', function (t) {
const server = ldap.createServer()
t.ok(server)
server.listen(0, 'localhost', function () {
server.listen(0, '127.0.0.1', function () {
t.ok(true, 'server listening on ' + server.url)

server.getConnections(function (err, count) {
Expand Down Expand Up @@ -62,6 +62,17 @@ tap.test('properties', function (t) {
})
})

tap.test('IPv6 URL is formatted correctly', function (t) {
const server = ldap.createServer()
t.equal(server.url, null, 'url empty before bind')
server.listen(0, '::1', function () {
t.ok(server.url)
t.equal(server.url, 'ldap://[::1]:' + server.port)

server.close(() => t.end())
})
})

tap.test('listen on unix/named socket', function (t) {
const server = ldap.createServer()
server.listen(t.context.sock, function () {
Expand Down Expand Up @@ -437,7 +448,7 @@ tap.test('multithreading support via external server', function (t) {
config: serverOptions
}
t.ok(server)
fauxServer.listen(5555, 'localhost', function () {
fauxServer.listen(5555, '127.0.0.1', function () {
t.ok(true, 'server listening on ' + server.url)

t.ok(fauxServer)
Expand All @@ -459,7 +470,7 @@ tap.test('multithreading support via hook', function (t) {
const server = ldap.createServer(serverOptions)
const fauxServer = ldap.createServer(serverOptions)
t.ok(server)
fauxServer.listen(0, 'localhost', function () {
fauxServer.listen(0, '127.0.0.1', function () {
t.ok(true, 'server listening on ' + server.url)

t.ok(fauxServer)
Expand Down