feat: add unixSocket option and +unix suffix support to protocol

[robertsLando]

Fixes #1873

[ytimenkov]

I finally managed to look a bit at url parsing...

I think it is broken in nodejs: nodejs/node-v0.x-archive@a94ffda#diff-3755baff71802c66753e9be67b9b7c0e219dd0b61fda072a0009f807a3
Master: https://github.com/nodejs/node/blob/main/lib/url.js#L430

It hallucinates (trendy word now) a path part of URL for ipv6 addresses.

> url.parse("mqtt://localhost")
Url {
  protocol: 'mqtt:',
  slashes: true,
  auth: null,
  host: 'localhost',
  port: null,
  hostname: 'localhost',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'mqtt://localhost'
}
> url.parse("mqtt://127.0.0.1")
Url {
  protocol: 'mqtt:',
  slashes: true,
  auth: null,
  host: '127.0.0.1',
  port: null,
  hostname: '127.0.0.1',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'mqtt://127.0.0.1'
}
url.parse("mqtt://[::1]")
Url {
  protocol: 'mqtt:',
  slashes: true,
  auth: null,
  host: '[::1]',
  port: null,
  hostname: '::1',
  hash: null,
  search: null,
  query: null,
  pathname: '/',
  path: '/',
  href: 'mqtt://[::1]/'
}

So I agree that stripping path for non-web-socket urls (ws://, wss:/) makes sense.

But I would still be very cautious about what is sent to createConnection function... The validation / simulation of overloads looks very weak to me (https://github.com/nodejs/node/blob/main/lib/net.js#L287): Any string parameter which doesn't convert to number results in Unix address (as if path was specified).
So adding a trailing slash can result in Unix socket being used (e.g. for mqtt://localhost/).

Therefore I would move choice between port/host vs path closer to where createConnection is called...

[robertsLando]

@ytimenkov Thanks for your research, I think so that the only reliable way to fix this is add a boolean option like unixSocket that if true considers the path in tcp connect otherwise it doesn't. What about this?

[ytimenkov]

Not sure how boolean option will help.

Curl's option is a string, because you have 2 paths: the first one is address (goes into Host: header), and the second one is in the protocol (goes into the request line).

I wonder if curl sets Host: to path-address or to localhost placeholder... 🤔. (or to whatever host which was specified in the URL).

This placeholder reminds me about another project Ansible - I think it also recognizes "localhost" and executes tasks in place rather than going via ssh.

This is if you want to provide complete feature-parity between inet(6) and local address for all protocols (kid of hook into the name resolver).

A smaller change would be sacrificing functionality (ws over unix sockt) and use something like "mqtt+unix". This is close to a boolean option you suggested and what is already implemented - for !ws, check if protocol ends with "+unix" and then take either path part or port-host.

With or without that url parsing inconsistency in nodejs, mqtt protocol doesn't care about path, does it? So I think it's good to be a bit more explicit and have clear distinction between these 2 paths, rather than "pass-through everything and hope it works somehow".

How did you see the boolean option?

[robertsLando]

How did you see the boolean option?

I don't like it as I would prefer not adding other options but let's think about the possible cases:

• mqtt/mqtts:// protocol: should parse the path only if the boolean flag is true (false by default)
• ws/wss:// protocol: should always parse the path

I think that those cases should cover all the possible conflicts while parsing the url.

But I would still be very cautious about what is sent to createConnection function... The validation / simulation of overloads looks very weak to me (nodejs/node@main/lib/net.js#L287): Any string parameter which doesn't convert to number results in Unix address (as if path was specified).

About this, this is only valid when calling the create connection with two args, in our case the first argument is an object with all the options set (both for tcp and tls)

The only thing that makes me prefer the mqtt+unix: solution is that it would not require to specify an option and I know that on some projects they only allow to specify a connection string so that could be more handy. Let me draft something that uses both solutions

[mcollina]

lgtm