-
Notifications
You must be signed in to change notification settings - Fork 262
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
Add sock_accept()
to snapshot1
#458
Conversation
A few examples of possible uses for this functionality. Simple echo serverClient connects to listening WASI process over TCP, process accepts connection, client sends data, process echoes data back to client. HTTP(S) serverSimilar to echo server, but with HTTP(S) support. Database serverDatabase implemented in WASI, accepts connections from clients, accepts queries (reads and writes), updates database and/or serves data to client. Remote logging serverRemote logging protocol implemented in WASI, accepts connections from logging agents, adds to log, writing periodically. |
If posix_spawn was also implemented, this should be enough to run cargo in wasi: |
@programmerjake While I'm not opposed to that, it isn't exactly low-hanging fruit. I would also like to avoid scope creep on this discussion. |
Thanks for posting this! One of the big questions is whether it makes sense to do this kind of "snapshot2" at this time, using the old witx-based tooling and shapshot scheme. We have a lot of experience and feedback on the limitations of the old witx-based tooling and proposal repo format, and the underlying interface types design has evolved significantly from when witx was designed, so it's worth asking whether it makes sense to take time out of the efforts to work towards better tools. The key observation that I see here is that
Could you be more specific about the motivation for Also, Re: |
I'm all for working on the better tools. My concern is that snapshot1 has basically been frozen for over a year with no development. The new tools aren't ready yet. I get that. But it would seem to me that the best thing for consumers of WASI is to have limited, intentional evolution of the "old" system until the "new" system becomes usable in many/most of the applications of the old system. OTOH, shutting down the "old" system with out something to replace it is pretty much the precise recipe for forks (That's not a threat! Promise!). What I'm hoping for is just enough evolution to keep people engaged with WASI until it is time to rebase on the "new" system.
👍
👍 I'm on board with this. To be clear Pipe is used to manually interact with main loop control functions. There are several alternatives. For example, Linux offers
That was my initial thought too. It isn't a bad idea. We could always do |
I'm going to remove the request for I think the major question that remain are:
|
I will be changing the documents in future commits. This ensures they are up to date before making any changes. Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>
This function allows a server to accept incoming connections. A few notes are in order. Although there are legitimate questions about the capabilities model in numerous networking APIs, this particular API is both immensely useful and not a violation of the capabilities model. There is still no way to create an arbitrary socket. The only thing that WASM can do is accept incoming connections on a predefined listening socket. Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>
Per the discussion today, I have updated the pull request to remove the IP addressing functionality of accept. I believe this is now ready for merge. I will provide a paragraph of documentation containing recommended details for implementors in a subsequent comment. I am now marking this PR as ready for review. |
Recommendations for ImplementorsThe WASI Return EINVALOne possible approach is to return Return an Invalid or Reserved Address and PortA better approach is to return a reserved address and port. Under IPv4, the address DrawbacksAlthough the compatibility of this approach is high, it could have some drawbacks.
|
@sunfishcode Can you please review the above text? Feel free to tag any other reviewers you deem relevant. |
sock_accept()
to snapshot1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me!
;;; The listening socket. | ||
(param $fd $fd) | ||
;;; The desired values of the file descriptor flags. | ||
(param $flags $fdflags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the purpose of accept a fdflags value here to have an equivalent to the SOCK_NONBLOCK
flag? If so, it'd be good to mention that flag in the comment above, and document that accept
doesn't recognize any of the other $fdflags
flags.
I agree, and I think it'd be simpler if we just don't mention the
What would you think about using |
I think, setting one specific value is pointless, as the returned value would be useless for the end user. In regular But, I think the address can be easily computed, and returned without exposing any real information to wasm module. We can describe a deterministic one-way function that takes a real client address and translates it to some CIDR like
Such a result could be useful to wasm module end-user, at the same time it doesn't expose real information. Limiting the output CIDR or port range, could be beneficial, as it prevents malicious users trying to reverse the one-way function to get a real address. |
@cardil I'm strongly against returning a valid-but-incorrect IP address to the application.
|
@sunfishcode If we are going to recommend the synthesizing of an IP address in |
This only works for
The problem here is that |
This is why I proposed |
At @PiotrSikora:
This is why I propose to use limited CIDR. For example, using CIDR
Yes. It is simple address translation. Host knows both addresses, real and crafted. Wasm module knows only the crafted one. This should be enough from monitoring purposes.
I don't think so. Docker/Podman/Kubernetes also uses crafted addresses, and hostnames. They are valid within the system, giving users an abstraction layer, but they are essentially fake. They do not work outside the system. Here similarly, the crafted addresses could be valid within system - across different WASM modules, but be invalid outside, where specific translation is required. This is just a NAT. A private, separated network. And the router is doing the translation. In our case, the WASM host can act as network gateway. At @npmccallum:
That's a bummer. So, at least for this implementation, we need a static value, then. The thing I'm proposing could be possible with full implementation, right? In generalI'm not strongly after this addresses translation, at least in scope of this PR. But, I just have got an idea, that this could help create sandbox abstraction for networking in general. What if we allow the host to define CIDR mount-points?! With it, host could say that, for ex.: real addresses |
Given that the proposal Its not clear to me we even need to make such recommendations, given that libc/library implementations can choose whatever solution they want, regardless. |
@sunfishcode Do you agree with the proposal from @sbc100 ? I think we have general consensus that:
|
@npmccallum Yes, that sounds right to me. |
can we call it |
Merging, as discussed in the meeting, and with overall consensus here.
This is an interesting space to be thinking about. There are several folks talking about networking proposals, and as these progress, it will be useful to bring these ideas up. And if future proposals need a new
@devsnek Since this is just a special one-time extension of snapshot_preview1, and not WASI's last work on the topic of sockets, we don't need to worry too much about the aesthetics of the name here :-). |
Update
Today (2022-01-13) we discussed this PR in the WASI subgroup call. There was general agreement that updating snapshot1 with an
accept()
call was a valuable endeavor. I have updated the PR as I believe it is now ready for merge.Although this proposal previously suggested the publication of a
snapshot2
, the consensus on the call was that implicitly updatingsnapshot1
is the preferred route.This proposal needs to be adopted and implemented in the
libc
implementation before transitioning to streams so that consumers can lock to an older version (per @sunfishcode).Overview
WASI snapshot1 is very broadly implemented. It is also fairly usable for a number of cases. However, for networking its use is quite limited. This is particularly because there are a few omissions that create real blockers in downstream users of this API. For this reason while there has been extensive demand to support WASI in the Rust ecosystem, real penetration of support in the crate ecosystem remains elusive for major networking libraries.
This PR sets out to add
accept()
, which makes it possible to accept distinct connections inside of wasm.Supporting Accept
All network APIs need to support calling
accept()
on pre-opened listening sockets. For example, this is widely implemented insystemd
and other init systems.For example, in Rust you can create a
TcpListener
from aRawFd
on Unix platforms and a parallel exists for Windows. This doesn't work on WASI, but only because we are missing theaccept()
call. Enabling the creation of aTcpListener
in Rust from a pre-opened listening socket would substantially increase the usability of WASM and would incentivize the adoption by frameworks liketokio
.Proposal
While I'm aware of the great strides being taken to modularize WASI, I am essentially proposing an incremental
snapshot2
aimed at improving the existing toolchain support.CC
@tschneidereit @lukewagner @sunfishcode @wgwoods @MikeCamel