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

fix(rpc): cross-platform support for /unix/ socket maddrs in Addresses.API #10019

Merged
merged 6 commits into from
Aug 20, 2024

Conversation

djdv
Copy link
Contributor

@djdv djdv commented Jul 14, 2023

closes: #10018

This simply removes the need to dial the domain by specifying the dialer in the transport.
And removes any concerns in regards to the socket's path containing characters that are not valid HTTP URLs.
We make requests to a made up domain http://unix which the client is happy to engage with.

There may be a better way to do this but I couldn't figure one out.
I considered trying to avoid calling manet.DialArgs since it will be called twice for non-UDS maddrs (once here and again in NewApiWithClient).
But if we do something like multiaddr.ValueForProtocol(multiaddr.P_UNIX), this will cause issues on platforms like Windows.
This is because multiaddr requires / delimiters (/unix/C:/somewhere), while net.Dial and the OS want C:\somewhere.
ValueForProtocol will return C:/somewhere while manet.DialArgs returns C:\somewhere.
So I just stuck with manet.Dial.

As an aside, I don't really understand why NewApiWithClient is converting from a multiaddr, to a string, then back to a multiaddr. But this is out of scope.

@djdv
Copy link
Contributor Author

djdv commented Mar 12, 2024

Some other commit was causing a conflict with the import statements, so I rebased and force pushed. No changes to the actual code in this PR though.

@gammazero gammazero self-assigned this Aug 15, 2024
@gammazero gammazero added the skip/changelog This change does NOT require a changelog entry label Aug 20, 2024
test/cli/rpc_unixsocket_test.go Outdated Show resolved Hide resolved
@lidel lidel changed the title rpc: fix - Unix domain socket maddrs used with NewApi fix(rpc): cross-platform support for /unix/ socket maddrs in Addresses.API Aug 20, 2024
docs/changelogs/v0.30.md Outdated Show resolved Hide resolved
Co-authored-by: djdv <ddvpublic@Gmail.com>
@gammazero gammazero merged commit c5b0428 into ipfs:master Aug 20, 2024
14 checks passed
@djdv djdv deleted the uds-client branch September 10, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does NOT require a changelog entry
Projects
No open projects
Status: 🥞 Todo
Development

Successfully merging this pull request may close these issues.

rpc: NewAPI constructs faulty HTTP client when using Unix Domain sockets
3 participants