add support for normal Unix domain socket

containerd / ttrpc-rust

Rust implementation of ttrpc (GRPC for low-memory environments)

Apache License 2.0

195 stars 45 forks source link

add support for normal Unix domain socket #116

Closed liubin closed 2 years ago

liubin commented 2 years ago

To let Linux os use normal Unix domain socket, this commit extends the Domain enum and control the socket type by the address pattern passed in:

vsock://xxx:xx -> Vsock
unix:///run/aaa.sock -> normal Unix domain sock
unix://@/run/abc.sock -> abstract Unix sock
other:///run/d.sock -> error

Fixes: #115

Signed-off-by: bin liu bin@hyper.sh

liubin commented 2 years ago

@jodh-intel @mxpv @Tim-Zhang

I tried another method to achieve it. This method depends on the format of the address passed by the user. The specific rules are as follows:

vsock://xxx:xx -> Vsock unix:///run/aaa.sock -> normal Unix domain sock /run/abc.sock -> abstract Unix sock other:///run/d.sock -> error

This method has a bigger problem, that is, compatibility. Users of the original version will get an abstract socket if they use unix:///run/a.sock, but will get a normal Unix socket in this version. To use abstract Unix socket, users must use /run/abs.sock as the address parameter.

liubin commented 2 years ago

Thanks for @teawater , here are some other patterns for abstract socket:

1: unix://@/run/aaa.sock -> abstract socket: /run/aaa.sock
2: @unix:///run/aaa.sock -> abstract socket: /run/aaa.sock
3: @/run/aaa.sock -> abstract socket: /run/aaa.sock

The @ mark is what we can see from netstat command for abstract sockets.

/cc @lifupan

Tim-Zhang commented 2 years ago

I vote for option 1

lifupan commented 2 years ago

I'd prefer 3

jodh-intel commented 2 years ago

I'd vote for 1 (as we already use that format for https://github.com/kata-containers/kata-containers/tree/main/src/tools/agent-ctl here: https://github.com/kata-containers/kata-containers/blob/main/src/tools/agent-ctl/src/client.rs#L456..L463 ;)

Regarding compatability, you could bump the semver major, but that won't help much given the API hasn't changed. So maybe add a #[deprecated(since="a.b.c", note = "please use new_func() instead")] but maintain the abstract compat in that function for a few releases on the original function, but introduce a new API that understands the new @ syntax.

teawater commented 2 years ago

I vote for option 1