Stream: git-wasmtime

Topic: wasmtime / PR #7705 Abstract out IpNameLookup core functi...

Wasmtime GitHub notifications bot (Dec 19 2023 at 15:48):

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 a String to a list of IP addresses. Another trait, WasiIpNameLookupView is used to configure which concrete type that implements IpNameLookup will be used in the actual IpNameLookup host implementation. Lastly. the SystemIpNameLookup 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 in SystemIpNameLookup. If there were default associated types for traits, we could get rid of WasiIpNameLookupView and make ip_name_lookup a function on WasiView that defaults to the the SystemIpNameLookup. But alas that's not available to us.

Wasmtime GitHub notifications bot (Dec 19 2023 at 15:48):

rylev requested alexcrichton for a review on PR #7705.

Wasmtime GitHub notifications bot (Dec 19 2023 at 15:48):

rylev requested wasmtime-core-reviewers for a review on PR #7705.

Wasmtime GitHub notifications bot (Dec 19 2023 at 15:49):

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 a String to a list of IP addresses. Another trait, WasiIpNameLookupView is used to configure which concrete type that implements IpNameLookup will be used in the actual IpNameLookup host implementation. Lastly. the SystemIpNameLookup 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 in SystemIpNameLookup. If there were default associated types for traits, we could get rid of WasiIpNameLookupView and make ip_name_lookup a function on WasiView that defaults to the the SystemIpNameLookup. But alas that's not available to us.

r? @badeend

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:00):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:00):

badeend created PR review comment:

With these changes, can network.allow_ip_name_lookup now be removed?

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:04):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:04):

badeend created PR review comment:

I'm curious; why isn't resolve_addresses placed directly on WasiIpNameLookupView? Is there something the extra indirection buys us?

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:08):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:08):

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 through WasiCtx/WasiCtxBuilder.

Though it's probably somewhat confusing to implement WasiIpNameLookupView and then not have name lookups work because you forgot to flip the bit on WasiCtx.

Perhaps we remove the allow_ip_name_lookup and provide an additional implementation of IpNameLookup: BlockedIpNameLookup so that users can easily turn off IP name lookup if they like. Thoughts?

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:10):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:10):

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 call resolve_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?

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:15):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:38):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:38):

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 into network.allow_ip_name_lookup on its own.

It might be an idea to provide an additional implementation of IpNameLookup based on WasiCtx, that dynamically checks allowed_network_uses. This could supersede the BlockedIpNameLookup you suggested

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:50):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 16:50):

badeend created PR review comment:

Alright. Was just curious.
No strong preference

Wasmtime GitHub notifications bot (Dec 19 2023 at 17:28):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2023 at 17:28):

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 for allowed_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.

Wasmtime GitHub notifications bot (Dec 19 2023 at 17:28):

rylev edited PR review comment.

Wasmtime GitHub notifications bot (Dec 19 2023 at 17:28):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Dec 19 2023 at 18:16):

badeend commented on PR #7705:

All in all, looks good to me :rocket:

Wasmtime GitHub notifications bot (Dec 19 2023 at 19:20):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Jan 04 2024 at 13:32):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Jan 04 2024 at 13:51):

rylev commented on 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 previous IpNameLookupView 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 and network_view_mut which return references to a dyn 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 methods WasiNetworkView 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 with SystemNetwork (the default implementation of WasiNetworkView)

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?

Wasmtime GitHub notifications bot (Jan 04 2024 at 16:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 16:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 16:53):

alexcrichton created PR review comment:

This might be best to model as an async function rather than returning an AbortOnDropJoinHandle?

Wasmtime GitHub notifications bot (Jan 04 2024 at 16:53):

alexcrichton created PR review comment:

(then it might be neat to have default implementations of fn network_view perhaps?)

Wasmtime GitHub notifications bot (Jan 04 2024 at 16:53):

alexcrichton created PR review comment:

Could this perhaps manifest as a impl WasiNetworkView for WasiCtx to avoid a second type?

Wasmtime GitHub notifications bot (Jan 04 2024 at 17:42):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 17:42):

rylev created PR review comment:

This is indeed very annoying... The value returned from resolve_address must be Send + Sync as it is used in ResolveAddressStream which impls Subscribe which requires the type implementing it to be Send + Sync.

The #[async_trait] macro puts a Send bound on the return type but not Sync and there is no way to instruct it to do so. We could move to using Box<dyn Future> but that means we loose the developer experience of async_trait. We could potentially move to using impl Future but that requires the latest version of Rust, and I'm not sure if that's ok.

Wasmtime GitHub notifications bot (Jan 04 2024 at 17:55):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Jan 04 2024 at 17:55):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 17:55):

rylev created PR review comment:

Great idea!

Wasmtime GitHub notifications bot (Jan 04 2024 at 18:23):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:28):

alexcrichton created PR review comment:

Could this be removed in lieu of implementing WasiNetworkView for WasiCtx itself? (given that WasiCtx is needed to create this structure anyway it may not buy a whole lot)

Wasmtime GitHub notifications bot (Jan 04 2024 at 20:28):

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.

Wasmtime GitHub notifications bot (Jan 05 2024 at 11:18):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 05 2024 at 11:18):

rylev created PR review comment:

We could take a page out of WasiHttpView's playbook and have the function return a wasmtime::Result<Resource<ResolveAddressStream>> just like WasiHttpView::send_request returns a wasmtime::Result<Resource<HostFutureIncomingResponse>>. I don't really love this either, but at least it's consistent?

Wasmtime GitHub notifications bot (Jan 05 2024 at 11:21):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 05 2024 at 11:21):

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 the SystemNetwork. 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 a WasiCtx
• Leave it as is

I think I'd be most in favor of the last two options.

Wasmtime GitHub notifications bot (Jan 06 2024 at 12:53):

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
    }
}

Wasmtime GitHub notifications bot (Jan 06 2024 at 16:02):

badeend submitted PR review.

Wasmtime GitHub notifications bot (Jan 06 2024 at 16:02):

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

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:55):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:55):

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 like fn from_future<T: Future + Send + Sync + 'static> (...) on the ResolveAddressStream type. That way you could pass an async block which would have bounds generated correctly and additioanlly the handle from spawn_blocking could also be passed in? That way we might be able to avoid async traits altogether and rely entirely on that.

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 15:57):

alexcrichton created PR review comment:

Hm I'm not sure I understand? Given impl WasiNetworkView for WasiCtx then if users are themselves implementing WasiNetworkView for their own custom type then conditionally using the default behavior would be calling self.ctx.the_method_name() so I'm not sure where the need for having this separate SystemNetwork structure arises?

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:02):

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 from WasiView (although it having a default impl returning self.ctx_mut() I think is pretty nifty and makes it almost hidden.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:17):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:17):

rylev created PR review comment:

Thinking about this more, returning Resource<ResolveAddressStream> would mean that the implementor would have to also implement WasiCtx or otherwise have multiple access to the ResourceTable. Right now we don't require that from the implementor of WasiNetworkView, and I'm not sure we want to.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:25):

rylev submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:25):

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 the ip_name_lookup module that can handle name lookups, but you're right that we can just implement that logic directly on WasiCtx. It does make the code a wee bit simpler.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 08 2024 at 16:35):

alexcrichton created PR review comment:

ResolveAddressStream itself could be returned though leaving the table management bits to the glue?

Wasmtime GitHub notifications bot (Jan 08 2024 at 17:20):

rylev commented on PR #7705:

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 to SystemNetwork or DefaultNetwork and only changing what they do care about. The only issue I can see with this is that if the Network trait grows large (which I think we anticipate it will), then there will be a large amount of boiler plate of delegating Network methods to the SystemNetwork or DefaultNetwork. This could be really annoying.

Wasmtime GitHub notifications bot (Jan 08 2024 at 17:38):

rylev updated PR #7705.

Wasmtime GitHub notifications bot (Jan 08 2024 at 17:40):

rylev commented on 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 a ResolveAddressStream.

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?

Wasmtime GitHub notifications bot (Jan 08 2024 at 20:02):

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 to DefaultNetwork

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

Wasmtime GitHub notifications bot (Jan 09 2024 at 15:27):

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.

Wasmtime GitHub notifications bot (Jan 09 2024 at 15:27):

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.

Wasmtime GitHub notifications bot (Jan 09 2024 at 15:27):

alexcrichton created PR review comment:

I think the + Sized may not be necessary here?

Wasmtime GitHub notifications bot (Jan 09 2024 at 17:00):

rylev commented on PR #7705:

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.

Wasmtime GitHub notifications bot (Jan 16 2024 at 08:54):

rylev updated PR #7705.

Last updated: Nov 22 2024 at 16:03 UTC