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

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

Merged
merged 9 commits into from
May 28, 2024

Conversation

robertsLando
Copy link
Member

@robertsLando robertsLando commented May 24, 2024

Fixes #1873

@ytimenkov
Copy link

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

robertsLando commented May 28, 2024

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

@robertsLando robertsLando changed the title fix: consider path when protocol is ws/wss feat: add unixSocket option and +unix suffix support to protocol May 28, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Broker URL path is dropped in connect(), connections fail
3 participants