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

UNIX sockets support for RPC #9919

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@laanwj
Copy link
Member

laanwj commented Mar 4, 2017

Add functionality for RPC over UNIX sockets, on platforms that support UNIX sockets. This is specified with :unix:/path/to/socket to -rpcbind and -rpcconnect. By default the socket path is relative to the data directory.

For the server side this works as-is.

For the client side (bitcoin-cli) this requires a small patch to libevent to be able to pass in a file descriptor of an existing connection (libevent/libevent#479).

Even without the client change this could be useful though. For example the Python RPC tests could connect through UNIX socket to avoid port collisions (TODO: make a Python example). This has been implemented.

Also the overhead of using UNIX sockets should be lower than using local TCP (see https://stackoverflow.com/questions/14973942/performance-tcp-loopback-connection-vs-unix-domain-socket).

Example

Server:

src/bitcoind -printtoconsole -datadir=/store/tmp/testbtc -connect=0 -debug=rpc -debug=http -rpcbind=:unix:socket
...
2017-03-05 10:29:31 Binding RPC on UNIX socket /store/tmp/testbtc/socket
...

Client:

src/bitcoin-cli -rpcconnect=":unix:socket" -datadir=/store/tmp/testbtc getinfo
{
  "version": 149900,
  ...
}

@laanwj laanwj added the RPC/REST/ZMQ label Mar 4, 2017

laanwj added a commit to laanwj/libevent that referenced this pull request Mar 4, 2017

http: Make it possible to pass in a pre-existing connection
This patch adds functionality to pass a pre-existing connection
as a bufferevent to `evhttp_connection_base_bufferevent_new`.

When the bufferevent has an existing fd, the evcon starts
in state `EVCON_IDLE` so that requests can be made immediately.

I am using this in Bitcoin Core for doing HTTP requests over
a UNIX socket (bitcoin/bitcoin#9919).

It could also be useful in sandboxed environments such as cloudabi
that cannot directly make connections but can pass around fds.
@@ -377,6 +381,9 @@ bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPN

# bitcoin-cli binary #
bitcoin_cli_SOURCES = bitcoin-cli.cpp
if BUILD_EVUNIX

This comment has been minimized.

@laanwj

laanwj Mar 4, 2017

Author Member

@theuni This is pretty ugly, but I don't quite know how to do this otherwise. My first intuition was to add the file to libbitcoin_util_a_SOURCES, however these can't depend on libevent as that is shared between all executables. There is no library that is shared between just bitcoind and bitcoin-cli and adding one just for this seems overkill.

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

I think this is OK. I think we'll end up with a few wrappers for libevent, so moving those to their own internal helper lib will make sense.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch 2 times, most recently Mar 4, 2017

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 4, 2017

Neat.

@paveljanik

This comment has been minimized.

Copy link
Contributor

paveljanik commented Mar 4, 2017

This does not compile with libevent2-2.0.22 which is OK with the current master.

bitcoin-cli.cpp:214:13: error: use of undeclared identifier `evhttp_connection_base_bufferevent_new'
            evhttp_connection_base_bufferevent_new(base.get(), __null, bev, "", 0)
            ^
1 error generated.

https://github.com/libevent/libevent/blob/e7ff4ef2b4fc950a765008c18e74281cdb5e7668/whatsnew-2.1.txt#L422

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 4, 2017

@paveljanik Yes, that needs a version #ifdef, that call doesn't exist in older versions of libevent, and on newer ones (until my patch lands) it ignores the current connection and tries to open a new one so that trick doesn't work. I'd recommend testing with my patched libevent. Alternatively comment out the bitcoin-cli change and test connecting with something else:

  • nc -U <socket> works, for example
  • curl also has a --unix-socket parameter, but haven't tried it yet
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Mar 4, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 4, 2017

For merge we should probably just not have the client version, then?

Would prefer putting it behind #ifdef EXPERIMENTAL_LIBEVENT or such, with a comment that it can be replaced with the appropriate version comparison when it lands in a version. That way, people that compile their own stuff can use it already.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 5, 2017

Still, this is cool - lots of better auth controls can be applied to
sockets than IPs :)

Indeed. Default permissions of the socket will be 700 due to our umask setting (can be overridden with -sysperms), which seems okay:

srwx------  1 user user          0 Mar  5 11:40 socket
@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 6, 2017

This is great!
Concept ACK.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch Mar 6, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 6, 2017

Added a commit that makes the tests' RPC run over UNIX sockets on platforms that support it, except for a few tests that make explicit assumptions about running over TCP/IP.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch 3 times, most recently Mar 6, 2017

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 6, 2017

I've been trying to test this locally by running the qa tests. Something that I do quite often is to run the tests with --noshutdown, wait for the test to fail, and use bitcoin-cli to interactively run RPCs on the test nodes:

bitcoin.conf -conf=<test temp directory>/node<x>/bitcoin.conf

With this PR, I'm not able to do that anymore because the conf file doesn't contain the username/password, and even when I add them back in, I get the error:

/tmp/user/1000/test_6o70u2b/28404/node0
→ ~/bitcoin/src/bitcoin-cli -rpcconnect=":unix:rpc_socket" -datadir=/tmp/user/1000/test_6o70u2b/28404/node0 help
[warn] evhttp_connection_connect_: connection to "" failed: No such file or directory
error: couldn't connect to server: unknown (code -1)
(make sure server is running and you are connecting to the correct RPC port)
/tmp/user/1000/test_6o70u2b/28404/node0
→ ll
total 16
drwxrwxr-x 3 ubuntu ubuntu 4096 Mar  6 23:31 ./
drwxrwxr-x 4 ubuntu ubuntu 4096 Mar  6 23:11 ../
-rw-rw-r-- 1 ubuntu ubuntu   79 Mar  6 23:20 bitcoin.conf
drwx------ 5 ubuntu ubuntu 4096 Mar  6 23:26 regtest/
srwx------ 1 ubuntu ubuntu    0 Mar  6 23:11 rpc_socket=

Do you have a better way of connecting to a test node during qa testing?

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 7, 2017

With this PR, I'm not able to do that anymore because the conf file doesn't contain the username/password, and even when I add them back in, I get the error:

Right, I could change it to add the options (including -rpcconnect) to the bitcoin.conf.

And even when I add them back in, I get the error:

The above should work, though. That's exactly how I've been testing it. The connection failure is strange. Did you patch libevent so that bitcoin-cli is able to connect to UNIX sockets? (you did add LIBEVENT_EXPERIMENTAL otherwise you'd be getting a RPC was asked to connect to a UNIX socket. However, the version of libevent that this utility was compiled with has no support for that. error when specifying :unix:)

Do you have a better way of connecting to a test node during qa testing?

I'll add an environment variable to force running the RPC tests over TCP.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 7, 2017

@jnewbery Okay, added two commits:

  • add BITCOIN_TEST_RPC_TCP environment variable to force tests to use TCP instead of UNIX sockets for RPC
  • write connection info to bitcoin.conf instead of providing it on the command line. -conf should just work, also for UNIX sockets, if your client supports it.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch 2 times, most recently Mar 7, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 9, 2017

Needed rebase, so rebased and squashed the squasme: commits.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 9, 2017

F.I.Y:
I wanted to test this via binaries built in gitian.
I could start the daemon (wanted to test in conjunction with Q): ./bitcoin-0.14.99/bin/bitcoin-qt -debug=rpc -debug=http -rpcbind=nix:socket --regtest

However the CLI responded with:
error: RPC was asked to connect to a UNIX socket. However, the version of libevent that this utility was compiled with has no support for that.

Maybe we should bump libevent in the dependencies directory?

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 9, 2017

However the CLI responded with:

Yes, that's expected. bitcoin-cli support needs this patch: https://github.com/laanwj/libevent/commit/403b793771341460b262edb1abc31fae267174bd.patch as well as defining -DLIBEVENT_EXPERIMENTAL

Without, you should still be able to use the socket with other clients, for example from Python or with curl. The server-side support is not dependent on that patch.

This could be done as part of the depends system, but rather not in this PR as it is a wholly separate discussion.

@laanwj laanwj added this to the 0.15.0 milestone Mar 10, 2017

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch 2 times, most recently Mar 12, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 12, 2017

Rebased (RPC tests logging changes #9768)

laanwj added some commits Mar 4, 2017

rpc: UNIX sockets support
Add functionality for RPC over UNIX sockets, on platforms that support
UNIX sockets. This is specified with `:unix:/path/to/socket` to
`-rpcbind` and `-rpcconnect`. By default the socket path is relative to the
data directory.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch Mar 12, 2017

rpc: More robust localhost-UNIX detection
evhttp has no direct support for UNIX sockets thus
is not able to recognize UNIX peers. Add custom code in
HTTPRequest::GetPeer and evunix to recognize these connections
as coming from localhost.

The previous solution did not work on some libevent versions.

@laanwj laanwj force-pushed the laanwj:2017_03_unix_socket branch to 695e854 Mar 12, 2017

@laanwj laanwj referenced this pull request Mar 18, 2017

Open

Unix Domain Sockets #5029

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Apr 5, 2017

Oops, I accidentally did my review of this code on the other PR. See here #9979 (review)

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 695e854, though code is pretty out of date

if (!sockaddr_from_path(&addr, path)) {
return -1;
}
int fd = socket(AF_UNIX, SOCK_STREAM, 0);

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: UNIX sockets support"

Maybe add a sock_from_path function to dedup code between evunix_bind_fd & evunix_connect_fd a little more.

addr.sun_family = AF_UNIX; ]])],
[ AC_MSG_RESULT(yes);
AC_DEFINE(HAVE_SOCKADDR_UN, 1,[Define this symbol if the sockaddr_un is available])
AM_CONDITIONAL([BUILD_EVUNIX],[true])

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: UNIX sockets support"

Naming of BUILD_EVUNIX seems a little unusual. Would expect something more like ENABLE_UNIX_SOCKETS (to be similar to ENABLE_WALLET, ENABLE_QT, ENABLE_ZMQ, etc).

Maybe more to the point though, since BUILD_EVUNIX is only used to control building of a single source file, maybe consider not defining it as build variable at all and just using #if HAVE_SOCKADDR_UN in the c++ source. I think this makes sense since you are already using HAVE_SOCKADDR_UN to avoid calling evunix functions, so you might as well use the same variable to avoid defining those functions.


// If path is not complete, it is interpreted relative to the data directory.
if (!name.is_complete()) {
name = GetDataDir(true) / name;

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: UNIX sockets support"

Maybe consider deduping the path manipulation code with the same code in bitcoin-cli

/* Special: will get here for UNIX sockets. As we have no way to indicate that,
* just pass localhost IPv6.
*/
address = "::1";

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: UNIX sockets support"

Seems like this instead of emulating an NET_IPV6 address, this should have it's own network type in netaddress.h similar to NET_TOR.

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

+1. This seems like another good use-case for NET_INTERNAL from #10446 (or something similar). As a bonus, that would allow us to use associative containers for unix socket addresses, since each path would have its own hash.

{
std::string name = path.string();
if (name.size() >= sizeof(addr->sun_path)) {
/* Name too long */

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: UNIX sockets support"

Would it be possible to throw an exception here? Since the path comes from the user's configuration it seems like it would be better to provide a specific error about the path than a generic connect or bind error.

{
struct sockaddr_un peer_unix;
socklen_t peer_unix_len = sizeof(peer_unix);
if (getpeername(fd, (sockaddr*)&peer_unix, &peer_unix_len) == 0) {

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "rpc: More robust localhost-UNIX detection"

Would it be better to use getsockname than getpeername for this? Seems like maybe that would work even on a socket that isn't connected.

'''
raise NotImplementedError

class RPCConnectInfoTCP:

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "tests: Run RPC tests optionally over UNIX socket"

Probably should inherit from RPCConnectInfo. Same with RPCConnectInfoUNIX class below. Doesn't really affect anything, but might make generated documentation / help messages a little better.

"""
Args:
url (str): URL of the RPC server to call
node_number (int): the node number (or id) that this calls to
urlinfo (RPCConnectInfo): Connection info of the RPC server to call

This comment has been minimized.

@ryanofsky

ryanofsky May 8, 2017

Contributor

In commit "tests: Run RPC tests optionally over UNIX socket"

s/urlinfo/conninfo/

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented May 9, 2017

I've had a quick read through the changes to the functional tests, but I'll defer doing a full review until this is rebased. A few general comments:

  • I'd prefer a command line argument over and environment variable for BITCOIN_TEST_RPC_TCP, partly because adding a command line argument means the help text will be updated and the option will be better documented.
  • there's a response to your libevent PR here: libevent/libevent#479. It'd be good to the functionality merged into libevent so we can avoid the LIBEVENT_EXPERIMENTAL stuff
  • I also think it would be better to make the changes to the functional test framework after TestNode changes in #10082 are in. I really dislike having state for nodes floating around in util.py (eg the new conninfo_for() function)

ghazel added a commit to clostra/Libevent that referenced this pull request May 20, 2017

http: Make it possible to pass in a pre-existing connection
This patch adds functionality to pass a pre-existing connection
as a bufferevent to `evhttp_connection_base_bufferevent_new`.

When the bufferevent has an existing fd, the evcon starts
in state `EVCON_IDLE` so that requests can be made immediately.

I am using this in Bitcoin Core for doing HTTP requests over
a UNIX socket (bitcoin/bitcoin#9919).

It could also be useful in sandboxed environments such as cloudabi
that cannot directly make connections but can pass around fds.
@theuni
Copy link
Member

theuni left a comment

Concept ACK. I added a few comments, and @ryanofsky beat me to several others.

@@ -377,6 +381,9 @@ bitcoind_LDADD += $(BOOST_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPN

# bitcoin-cli binary #
bitcoin_cli_SOURCES = bitcoin-cli.cpp
if BUILD_EVUNIX

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

I think this is OK. I think we'll end up with a few wrappers for libevent, so moving those to their own internal helper lib will make sense.

/* Special: will get here for UNIX sockets. As we have no way to indicate that,
* just pass localhost IPv6.
*/
address = "::1";

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

+1. This seems like another good use-case for NET_INTERNAL from #10446 (or something similar). As a bonus, that would allow us to use associative containers for unix socket addresses, since each path would have its own hash.

close(fd);
return -1;
}
evutil_make_socket_nonblocking(fd);

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

need to disable SIGPIPE here too?

#ifdef HAVE_SOCKADDR_UN
//! Clean up UNIX sockets on shutdown
for (const auto& path: cleanupUNIXSockets) {
evunix_remove_socket(path);

This comment has been minimized.

@theuni

theuni Jun 2, 2017

Member

should probably .clear() in case this gets called twice.

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Jun 24, 2017

Closing this for now. I'll probably pick it up again for 0.16.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 26, 2018

Concept ACK. c-lightning does this too FWIW.

This was briefly discussed during the meeting today. Tag as Up for Grabs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.