rylev opened PR #7705 from rylev:lookup-override
to bytecodealliance:main
:
This follows the design outlined in bytecodealliance/wasmtime#7681 for IpNameLookup.
A new trait
IpNameLookup
is introduced that factors out the conversion from aString
to a list of IP addresses. Another trait,WasiIpNameLookupView
is used to configure which concrete type that implementsIpNameLookup
will be used in the actual IpNameLookup host implementation. Lastly. theSystemIpNameLookup
is provided as a default implementation.Some questions that might guide tweaks:
- Users are now forced to implement
WasiIpNameLookupView
- it would be nice if there was a to default to the built inSystemIpNameLookup
. If there were default associated types for traits, we could get rid ofWasiIpNameLookupView
and makeip_name_lookup
a function onWasiView
that defaults to the theSystemIpNameLookup
. But alas that's not available to us.
rylev requested alexcrichton for a review on PR #7705.
rylev requested wasmtime-core-reviewers for a review on PR #7705.
rylev edited PR #7705:
This follows the design outlined in bytecodealliance/wasmtime#7681 for IpNameLookup.
A new trait
IpNameLookup
is introduced that factors out the conversion from aString
to a list of IP addresses. Another trait,WasiIpNameLookupView
is used to configure which concrete type that implementsIpNameLookup
will be used in the actual IpNameLookup host implementation. Lastly. theSystemIpNameLookup
is provided as a default implementation.Some questions that might guide tweaks:
- Users are now forced to implement
WasiIpNameLookupView
- it would be nice if there was a to default to the built inSystemIpNameLookup
. If there were default associated types for traits, we could get rid ofWasiIpNameLookupView
and makeip_name_lookup
a function onWasiView
that defaults to the theSystemIpNameLookup
. But alas that's not available to us.r? @badeend
badeend submitted PR review.
badeend created PR review comment:
With these changes, can
network.allow_ip_name_lookup
now be removed?
badeend submitted PR review.
badeend created PR review comment:
I'm curious; why isn't
resolve_addresses
placed directly onWasiIpNameLookupView
? Is there something the extra indirection buys us?
rylev submitted PR review.
rylev created PR review comment:
It could be potentially. Right now the default is to not allow ip name lookups, so if we remove this, the default will flip. Additionally, the new way to turn off name lookups would be through a trait implementation (i.e.,
impl IpNameLookup for BlockedIpNameLookup
or similar) and not throughWasiCtx
/WasiCtxBuilder
.Though it's probably somewhat confusing to implement
WasiIpNameLookupView
and then not have name lookups work because you forgot to flip the bit onWasiCtx
.Perhaps we remove the
allow_ip_name_lookup
and provide an additional implementation ofIpNameLookup
:BlockedIpNameLookup
so that users can easily turn off IP name lookup if they like. Thoughts?
rylev submitted PR review.
rylev created PR review comment:
I'm not sure it gives a ton given that we initialize the type that implements
IpNameLookup
right before we callresolve_addresses
. The reason I initially went this direction is that it mirrors how the UDP and TCP implementations will work. I can remove it though. Shall I?
rylev updated PR #7705.
badeend submitted PR review.
badeend created PR review comment:
Hmm. Interesting.
I think the WASI translation layer (preview2/ip_name_lookup.rs
in this case) should only depend on a single source of truth:WasiIpNameLookupView
. It should not _also_ go peeking intonetwork.allow_ip_name_lookup
on its own.It might be an idea to provide an additional implementation of
IpNameLookup
based onWasiCtx
, that dynamically checksallowed_network_uses
. This could supersede theBlockedIpNameLookup
you suggested
badeend submitted PR review.
badeend created PR review comment:
Alright. Was just curious.
No strong preference
rylev submitted PR review.
rylev created PR review comment:
This does have the downside that the WasiCtx setting can be ignored by users. If their implementation of
IpNameLookup
doesn't check forallowed_network_uses
, that option will simply be ignored. I guess it seems fine that the user can ignore that option (they're the ones setting it anyway), but it does seem somewhat error prone.
rylev edited PR review comment.
rylev updated PR #7705.
badeend commented on PR #7705:
All in all, looks good to me :rocket:
rylev updated PR #7705.
rylev updated PR #7705.
@badeend @alexcrichton
I've moved the implementation towards having a single
WasiNetworkView
trait where all network based overrides will be implemented. I'm happier with this solution than the previousIpNameLookupView
version, but I'm still not convinced we've arrived at a good solution.The current solution has the following charactertistics:
- Two new methods on
WasiView
:network_view
andnetwork_view_mut
which return references to adyn WasiNetworkView
- This prevents all the methods necessarily being on
WasiView
which might get crowded over time. It also makes it easy for users who want to use an off-the-shelf solution to get implement 2 methods instead of the N methodsWasiNetworkView
will have.- The use of dynamic dispatch is new to WasiView but seems appropriate here.
- The various implementors in Wasmtime of
WasiView
gain a new field withSystemNetwork
(the default implementation ofWasiNetworkView
)I would said, we move forward with this for now and we can adjust as we add TCP and UDP support (and of course continue to evolve over time). Thoughts?
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This might be best to model as an
async
function rather than returning anAbortOnDropJoinHandle
?
alexcrichton created PR review comment:
(then it might be neat to have default implementations of
fn network_view
perhaps?)
alexcrichton created PR review comment:
Could this perhaps manifest as a
impl WasiNetworkView for WasiCtx
to avoid a second type?
rylev submitted PR review.
rylev created PR review comment:
This is indeed very annoying... The value returned from
resolve_address
must beSend + Sync
as it is used inResolveAddressStream
which implsSubscribe
which requires the type implementing it to beSend + Sync
.The
#[async_trait]
macro puts aSend
bound on the return type but notSync
and there is no way to instruct it to do so. We could move to usingBox<dyn Future>
but that means we loose the developer experience ofasync_trait
. We could potentially move to usingimpl Future
but that requires the latest version of Rust, and I'm not sure if that's ok.
rylev updated PR #7705.
rylev submitted PR review.
rylev created PR review comment:
Great idea!
rylev updated PR #7705.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this be removed in lieu of implementing
WasiNetworkView for WasiCtx
itself? (given thatWasiCtx
is needed to create this structure anyway it may not buy a whole lot)
alexcrichton created PR review comment:
Hm that's true, and yeah I see how there's no great way to manage this right now.
I suppose this can stay for now, but I'm still worried that this will inevitably cause some pain down the road. The best alternative I can think of is to return a
oneshot::Receiver<T>
instead but that's still pretty cumbersome to work with and wouldn't work great here.
rylev submitted PR review.
rylev created PR review comment:
We could take a page out of
WasiHttpView
's playbook and have the function return awasmtime::Result<Resource<ResolveAddressStream>>
just likeWasiHttpView::send_request
returns awasmtime::Result<Resource<HostFutureIncomingResponse>>
. I don't really love this either, but at least it's consistent?
rylev submitted PR review.
rylev created PR review comment:
If a user wants to have some logic where they conditionally switch between either their own network implementation and
SystemNetwork
, they need some way of constructing theSystemNetwork
. Without this constructor, they would not be able to do so. We could do the following:
- make all fields of
SystemNetwork
public- Make the constructor just take a simple
bool
instead of aWasiCtx
- Leave it as is
I think I'd be most in favor of the last two options.
badeend commented on PR #7705:
Having thought about it some more I would like to offer one more suggestion. See code below.
This touches multiple discussions above, so tagging @alexcrichton.The main difference is that this setup doesn't modify WasiView at all, but instead dispatches through WasiCtx::network
@rylev: This does have the downside that the WasiCtx setting can be ignored by users.
In setup below that can't happen.
The strict separation between the "System" and "Default" structs below might be overkill for this PR because of the relative trivial "System" and "Default" implementations. However, for TCP/UDP sockets the implementation won't be so trivial anymore.
/// Trait for any WASI-compliant network implementation. trait Network { fn resolve_addresses(&mut self, ) -> ...; // fn new_tcp_socket(family) -> ... // fn new_udp_socket(family) -> ... } /// A WASI-compliant "base" implementation. /// Without any opinions on how to do filtering, permission checks or whatever. struct SystemNetwork {} impl SystemNetwork { fn new() -> Self {} // Probably no parameters } impl Network for SystemNetwork { fn resolve_addresses(&mut self, ) -> { // Perform the syscalls without any additional permission checks } } /// A "good enough" default implementation for Wasmtime users, _with_ permission checks. struct DefaultNetwork { sys: SystemNetwork, allowed: bool } impl DefaultNetwork { /// Whatever parameters we want. /// Currently just a simple boolean, but could be extended to any fn new(allowed: bool) -> Self {} } impl Network for DefaultNetwork { fn resolve_addresses(&mut self, ) -> { if self.allowed { self.sys.resolve_addresses() } else { Err(PermissionDenied) } } } pub struct WasiCtx { pub(crate) random: Box<dyn RngCore + Send + Sync>, pub(crate) insecure_random: Box<dyn RngCore + Send + Sync>, pub(crate) insecure_random_seed: u128, pub(crate) wall_clock: Box<dyn HostWallClock + Send + Sync>, pub(crate) monotonic_clock: Box<dyn HostMonotonicClock + Send + Sync>, pub(crate) env: Vec<(String, String)>, pub(crate) args: Vec<String>, pub(crate) preopens: Vec<(Dir, String)>, pub(crate) stdin: Box<dyn StdinStream>, pub(crate) stdout: Box<dyn StdoutStream>, pub(crate) stderr: Box<dyn StdoutStream>, // Remove `socket_addr_check` // Remove `allowed_network_uses` // Add: pub(crate) network: Box<dyn Network>, } pub struct WasiCtxBuilder { // (...) network: Box<dyn Network>, } impl WasiCtxBuilder { pub fn new() -> Self { Self { // (...) network: DefaultNetwork::new(/*allowed: */false), } } pub fn allow_network(&mut self, enable: bool) -> &mut Self { self.network = DefaultNetwork::new(enable); self } pub fn custom_network(&mut self, net: Box<dyn Network>) -> &mut Self { self.network = net; self } }
badeend submitted PR review.
badeend created PR review comment:
Of all the options mentioned so far, the
Box<dyn Future>
route seems the least worst to me.The fact that we currently use a quick&dirty implementation using spawn_blocking should be an implementation detail of
SystemNetwork
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Returning a
Resource<ResolveAddressStream>
would be nice yeah but it seems like that would have all the same design questions around "how do you actually create one of those".Although as I say that we could perhaps get around
async trait
by having something likefn from_future<T: Future + Send + Sync + 'static> (...)
on theResolveAddressStream
type. That way you could pass anasync
block which would have bounds generated correctly and additioanlly the handle fromspawn_blocking
could also be passed in? That way we might be able to avoid async traits altogether and rely entirely on that.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Hm I'm not sure I understand? Given
impl WasiNetworkView for WasiCtx
then if users are themselves implementingWasiNetworkView
for their own custom type then conditionally using the default behavior would be callingself.ctx.the_method_name()
so I'm not sure where the need for having this separateSystemNetwork
structure arises?
alexcrichton commented on PR #7705:
@badeend your sketch makes sense yeah, but my thoughts at https://github.com/bytecodealliance/wasmtime/issues/7694#issuecomment-1858351478 showcase how something like that breaks down unless all callbacks about configuration go through
trait Network
, so for example we'd also need to route IP address checks through that trait as well. (not a problem of course, just pointing out that once things are behind a trait everything needs to be behind that trait to share state between callbacks).Otherwise though I think it'd be reasonable to configure a trait object on
WasiCtx
vs returning a&mut dyn Trait
fromWasiView
(although it having a default impl returningself.ctx_mut()
I think is pretty nifty and makes it almost hidden.
rylev submitted PR review.
rylev created PR review comment:
Thinking about this more, returning
Resource<ResolveAddressStream>
would mean that the implementor would have to also implementWasiCtx
or otherwise have multiple access to theResourceTable
. Right now we don't require that from the implementor ofWasiNetworkView
, and I'm not sure we want to.
rylev submitted PR review.
rylev created PR review comment:
You're right that
SystemNetwork
isn't strictly necessary. It felt nice to have a distinct type that lives in theip_name_lookup
module that can handle name lookups, but you're right that we can just implement that logic directly onWasiCtx
. It does make the code a wee bit simpler.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
ResolveAddressStream
itself could be returned though leaving the table management bits to the glue?
I like @badeend's suggestion. I initially shared the same concern that @alexcrichton pointed out, but I think it should be relatively easy for users to implement the
Network
trait by delegating all the parts they don't care about toSystemNetwork
orDefaultNetwork
and only changing what they do care about. The only issue I can see with this is that if theNetwork
trait grows large (which I think we anticipate it will), then there will be a large amount of boiler plate of delegatingNetwork
methods to theSystemNetwork
orDefaultNetwork
. This could be really annoying.
rylev updated PR #7705.
@alexcrichton @badeend I've pushed a new change that moves more towards @badeend's design as well as changing the return type of
resolve_addresses
to be aResolveAddressStream
.I think the only way for us to be sure about this design is to keep going and implement UDP and TCP on top of it. With that in mind, I think we should merge this PR and continue the design exploration in a follow up. Thoughts?
badeend commented on PR #7705:
something like that breaks down unless all callbacks about configuration go through trait Network, so for example we'd also need to route IP address checks through that trait as well.
Correct. In my mind that was exactly the goal of this (and upcoming) PRs.
socket_addr_check
&allowed_network_uses
would most likely be moved toDefaultNetwork
I think the only way for us to be sure about this design is to keep going and implement UDP and TCP on top of it.
Agree.
With that in mind, I think we should merge this PR and continue the design exploration in a follow up. Thoughts?
I'm not familiar with wasmtime's release process, but if a release gets cut partway through these changes it could be weird for consumers when e.g. IP lookup & TCP use the Network trait but UDP still uses an old & completely different customization mechanism. I'll leave this up for @alexcrichton
alexcrichton submitted PR review:
Looks reasonable to me!
Unless this is particularly urgent though I would personally agree with @badeend in that I'd prefer to see a more complete picture before landing if possible. If this is needed to unblock work I think it's ok to land, but if you're ok working with a fork for now though I think it'd be best to sketch out the other checks you're thinking of implementing to be able to see them altogether at once.
alexcrichton submitted PR review:
Looks reasonable to me!
Unless this is particularly urgent though I would personally agree with @badeend in that I'd prefer to see a more complete picture before landing if possible. If this is needed to unblock work I think it's ok to land, but if you're ok working with a fork for now though I think it'd be best to sketch out the other checks you're thinking of implementing to be able to see them altogether at once.
alexcrichton created PR review comment:
I think the
+ Sized
may not be necessary here?
I'm fine with not landing this on main. I just think we need to decide which fork/branch is the integration point so that if @badeend and I continue the work, we can bring that all together into one unified branch that will ultimately be merged into main.
rylev updated PR #7705.
Last updated: Dec 23 2024 at 12:05 UTC