brendandburns commented on issue #5929:
@pchickey I think this is ready for an initial high-level review.
Everything is implemented in terms of the
wit-bindgen
interfaces, and then an adapter is introduced which maps from the wit-bindgen to the currentwasmtime::Linker
implementation.This means that it is very forward compatible with the component model linker going forward.
If you can take a quick look at the rough layout and see if this is on the path to merging that would be great.
After we get the rough layout right, we can move on to a more detailed review.
Thanks!
brendandburns commented on issue #5929:
There's also a pretty extensive example that can be used to test this implementation in
crates/wasi-http/example
brendandburns commented on issue #5929:
Comments addressed except the macro vs fn comment because I'm not clear how to resolve the compile error.
Please take another look.
brendandburns edited a comment on issue #5929:
Comments addressed ~except the macro vs fn comment because I'm not clear how to resolve the compile error.~
Please take another look.
brendandburns commented on issue #5929:
Ok, this was switched to hyper. I'm not sure it really saved you very many dependencies, but if there are other ways to reduce the dependency count, let me know. Hyper doesn't implement TLS, so I had to pull in
hyper_native_tls
I think this is ready to go, let me know if there is a good way to add tests, it seems the other modules don't really have tests.
brendandburns commented on issue #5929:
@pchickey
Ok, I implemented timeouts. I think that the client side implementation is now basically feature complete. Ready for a more detailed review whenever you have time.
Thanks!
brendandburns commented on issue #5929:
Oh, and I plan on starting the server side implementation in a different branch.
pchickey commented on issue #5929:
I left some fixes last night in https://github.com/brendandburns/wasmtime/pull/1 but it looks like they now conflict
pchickey edited a comment on issue #5929:
I left some fixes last night in https://github.com/brendandburns/wasmtime/pull/1 but it looks like they now conflict - I'll rebase that PR on the current state of this branch
pchickey edited a comment on issue #5929:
I left some fixes last night in https://github.com/brendandburns/wasmtime/pull/1 but it looks like they now conflict - I'll rebase that PR on the current state of this branch (edit: done now!)
pchickey commented on issue #5929:
from my point of view we still need to fix 2 architectural issues before we can merge:
- wasmtime should be running in the tokio executor, rather than using block_on inside a Func.
- rather than examples provided in C, we need to rewrite those to be test-programs written in Rust, because our CI environment doesn't have a C compiler installed.
I'll work on both of those things right now, and send them as PRs like the above:
eduardomourar commented on issue #5929:
@pchickey , I believe you can borrow code from here for your Rust example.
brendandburns commented on issue #5929:
@pchickey thanks for the PR, I merged it. Let me know if you want me to take on either of those tasks.
eduardomourar edited a comment on issue #5929:
@pchickey , I believe you can borrow code from here for your Rust example.
brendandburns commented on issue #5929:
@pchickey unless you've already started on this, I can take care of those two items.
brendandburns commented on issue #5929:
Ok, I switched the executor on the
Host
to usetokio::Runtime::block_on
instead ofexecutors
.I don't think it is required to build out the full async support in this first PR, but if that's really a blocker, I will.
I also added an example implemented in rust for use as a test or just another example.
Please take another look.
brendandburns edited a comment on issue #5929:
Ok, I switched the executor on the
Host
to usetokio::Runtime::block_on
instead ofexecutors
.I don't think it is required to build out the full async support in this first PR, but if that's really a blocker, I will.
We can't just use
spawn_task
andawait
in that function because it is not declaredasync
I also added an example implemented in rust for use as a test or just another example.
Please take another look.
pchickey commented on issue #5929:
I just finished writing basically the same rust example this morning but I didn't push it yet because I need to hook its build and execution into the
test-programs
machinery. I'll wrap that up tomorrow and push it to you.And yeah lets just do async after we land this PR. We'll have to string async through everywhere, including into the wasmtime cli to run things under tokio when the http module is enabled.
brendandburns commented on issue #5929:
@pchickey sounds good, I'm happy to delete my Rust example and just use yours if that is easier.
pchickey commented on issue #5929:
https://github.com/brendandburns/wasmtime/pull/2 finishes up the testing changes. I am working on the supply chain stuff today, I will send a PR with any changes needed here but most of those I'll have to submit to wasmtime in a separate PR that will merge before this one.
brendandburns commented on issue #5929:
Cool. I squashed/merged that PR. Let me know if you need anything else from me.
pchickey commented on issue #5929:
https://github.com/bytecodealliance/wasmtime/pull/6121 will need to land and merge to make the deny and vet CI steps pass. Then, we expect the rest of the steps will pass, since they did locally, but windows and mac can surprise me sometimes.
We will need to remove the contents of
crates/wasi-http/example/
, and at that point I think we are good to go, I'll make one pass with fresh eyes tomorrow.
pchickey edited a comment on issue #5929:
https://github.com/bytecodealliance/wasmtime/pull/6121 will need to land and merge to make the deny and vet CI steps pass. Then, we expect the rest of the steps will pass, since they did locally, but windows and mac can surprise me sometimes.
We will need to remove the contents of
crates/wasi-http/example/
: we don't want any code in the tree which isn't checked by CI, because it is bound to get out of date. At that point I think we are good to go, I'll make one more pass with fresh eyes tomorrow.
pchickey commented on issue #5929:
https://github.com/brendandburns/wasmtime/pull/3 will make the cargo vet step pass
brendandburns commented on issue #5929:
- PRs merged (thanks!)
- example directory removed
I think this is ready to go.
pchickey commented on issue #5929:
It looks like I missed the vet of some stuff I introduced in #2, I'll get those if you can fix the warnings I sloppily left in the test
pchickey commented on issue #5929:
in order to make CI faster, we don't run every step until its time to go in the merge queue. https://github.com/brendandburns/wasmtime/pull/4 fixes the publishing issues.
eduardomourar commented on issue #5929:
@pchickey , I know this is a little bit late, but can we change this package name to
wasmtime-wasi-http
, please? I feel that the current name there will eventually be used for the guest bindings and its high-level basic implementation.
pchickey commented on issue #5929:
@eduardomourar I think that is a good idea but lets get things merged and then rename it - we have a few more days before the release gets cut.
brendandburns commented on issue #5929:
Merged the PR. I will send a couple of follow on PRs (including the rename) this evening.
eduardomourar commented on issue #5929:
Here is an initial stab at the guest binding: https://github.com/bytecodealliance/wasi/pull/69
jeffparsons commented on issue #5929:
Is it easy/possible to play with these WASI module implementations from Rust as they land? E.g. if I copy a snapshot of the relevant wit files locally and then build a component using cargo-component and its adapter magic, could I then run that in Wasmtime and expect something to happen?
Just having a bit of trouble understanding what pieces already work together, and which need other pieces (new WASI target in rustc, completed component model support in Wasmtime) to land first.
jeffparsons edited a comment on issue #5929:
Is it easy/possible to play with these WASI module implementations from Rust as they land? E.g. if I copy a snapshot of the relevant wit files locally and then build a component using cargo-component and its adapter magic (edit: that attempts to use wasi-http), could I then run that in Wasmtime and expect something to happen?
Just having a bit of trouble understanding what pieces already work together, and which need other pieces (new WASI target in rustc, completed component model support in Wasmtime) to land first.
brendandburns commented on issue #5929:
@jeffparsons yes, it's pretty easy to play with the modules.
You need to:
Run
wit-bindgen rust wasmtime/crates/wasi-http/wit
this will generate the Rust client side bindings (you can substitutewit-bindgen c ...
if you want the c bindings instead.Once you have that, here's a small example program that you can compile:
use crate::proxy::types::new_fields; use crate::proxy::types::new_outgoing_request; use crate::proxy::types::MethodParam; use crate::proxy::types::SchemeParam; pub mod proxy; fn request(method: MethodParam, scheme: Option<SchemeParam>, host: &str, path: &str) { let headers = new_fields(&[ ("Content-type", "text/plain"), ("User-agent", "wasm32-wasi-rust"), ]); let req = new_outgoing_request(method, path, "", scheme, host, headers); let fut = crate::proxy::default_outgoing_http::handle(req, None); let res = crate::proxy::types::future_incoming_response_get(fut) .unwrap() .unwrap(); let code = crate::proxy::types::incoming_response_status(res); let response_headers = crate::proxy::types::incoming_response_headers(res); let stream = crate::proxy::types::incoming_response_consume(res).unwrap(); let body = crate::proxy::streams::read(stream, 60 * 1024).unwrap().0; println!("Status is {}", code); println!("Headers are:"); let entries = crate::proxy::types::fields_entries(response_headers); for tuple in entries.iter() { println!("{}: {}", tuple.0, tuple.1); } println!("{}", String::from_utf8(body).unwrap()); } fn main() { request( MethodParam::Get, Some(SchemeParam::Https), "postman-echo.com", "/get", ); request( MethodParam::Post, Some(SchemeParam::Https), "postman-echo.com", "/post", ); request( MethodParam::Put, Some(SchemeParam::Http), "postman-echo.com", "/put", ); }
brendandburns edited a comment on issue #5929:
@jeffparsons yes, it's pretty easy to play with the modules, it doesn't require any of the component stuff (yet)
You need to:
Run
wit-bindgen rust wasmtime/crates/wasi-http/wit
this will generate the Rust client side bindings (you can substitutewit-bindgen c ...
if you want the c bindings instead.Once you have that, here's a small example program that you can compile:
use crate::proxy::types::new_fields; use crate::proxy::types::new_outgoing_request; use crate::proxy::types::MethodParam; use crate::proxy::types::SchemeParam; pub mod proxy; fn request(method: MethodParam, scheme: Option<SchemeParam>, host: &str, path: &str) { let headers = new_fields(&[ ("Content-type", "text/plain"), ("User-agent", "wasm32-wasi-rust"), ]); let req = new_outgoing_request(method, path, "", scheme, host, headers); let fut = crate::proxy::default_outgoing_http::handle(req, None); let res = crate::proxy::types::future_incoming_response_get(fut) .unwrap() .unwrap(); let code = crate::proxy::types::incoming_response_status(res); let response_headers = crate::proxy::types::incoming_response_headers(res); let stream = crate::proxy::types::incoming_response_consume(res).unwrap(); let body = crate::proxy::streams::read(stream, 60 * 1024).unwrap().0; println!("Status is {}", code); println!("Headers are:"); let entries = crate::proxy::types::fields_entries(response_headers); for tuple in entries.iter() { println!("{}: {}", tuple.0, tuple.1); } println!("{}", String::from_utf8(body).unwrap()); } fn main() { request( MethodParam::Get, Some(SchemeParam::Https), "postman-echo.com", "/get", ); request( MethodParam::Post, Some(SchemeParam::Https), "postman-echo.com", "/post", ); request( MethodParam::Put, Some(SchemeParam::Http), "postman-echo.com", "/put", ); }
brendandburns edited a comment on issue #5929:
@jeffparsons yes, it's pretty easy to play with the modules, it doesn't require any of the component stuff (yet)
You need to:
Run
wit-bindgen rust wasmtime/crates/wasi-http/wit
this will generate the Rust client side bindings (you can substitutewit-bindgen c ...
if you want the c bindings instead.Once you have that, here's a small example program that you can compile:
use crate::proxy::types::new_fields; use crate::proxy::types::new_outgoing_request; use crate::proxy::types::MethodParam; use crate::proxy::types::SchemeParam; pub mod proxy; fn request(method: MethodParam, scheme: Option<SchemeParam>, host: &str, path: &str) { let headers = new_fields(&[ ("Content-type", "text/plain"), ("User-agent", "wasm32-wasi-rust"), ]); let req = new_outgoing_request(method, path, "", scheme, host, headers); let fut = crate::proxy::default_outgoing_http::handle(req, None); let res = crate::proxy::types::future_incoming_response_get(fut) .unwrap() .unwrap(); let code = crate::proxy::types::incoming_response_status(res); let response_headers = crate::proxy::types::incoming_response_headers(res); let stream = crate::proxy::types::incoming_response_consume(res).unwrap(); let body = crate::proxy::streams::read(stream, 60 * 1024).unwrap().0; println!("Status is {}", code); println!("Headers are:"); let entries = crate::proxy::types::fields_entries(response_headers); for tuple in entries.iter() { println!("{}: {}", tuple.0, tuple.1); } println!("{}", String::from_utf8(body).unwrap()); } fn main() { request( MethodParam::Get, Some(SchemeParam::Https), "postman-echo.com", "/get", ); request( MethodParam::Post, Some(SchemeParam::Https), "postman-echo.com", "/post", ); request( MethodParam::Put, Some(SchemeParam::Http), "postman-echo.com", "/put", ); }
once you compile that to
main.wasm
(or whatever) you can run:wasmtime --wasi-modules=experimental-wasi-http main.wasm` and it should work correctly. ~~~
pchickey commented on issue #5929:
@jeffparsons we still need to land the preview 1 adapter and a component model implementation of the rest of wasi before this becomes usable from components. the preview 1 adapter means we don't have to worry about a new wasi target in the rust toolchain at this time.
brendandburns commented on issue #5929:
@pchickey looks like the merge failed b/c libssl isn't installed on some of the test environments :(
How do you want to handle fixing this?
pchickey commented on issue #5929:
I am a bit stumped on how to fix this.
To maximize linux libc compatibility, we perform our linux release builds on centos 7 (https://github.com/bytecodealliance/wasmtime/blob/main/ci/docker/x86_64-linux/Dockerfile). If we were to install the default libssl from centos 7 in the builder, it would mean all of our wasmtime linux users would need openssl 1.0.2 installed, which has been out of support since the start of 2020, so that isn't an acceptable solution.
I don't know enough about the linux and openssl ecosystem to know what minimum version is acceptable, but clearly it reduce our binary compatibility to take on this dep. I also don't know if there are any knobs we can tweak in the build to lazily load the openssl dylib, so users are opting in to needing it by passing the
--wasi-modules=experimental-wasi-http
flag.Finally, I don't know if there are alternatives we can swap in for needing openssl on linux. I am aware that
rustls
exists, but I don't know if it is a complete alternative. I'll ask some of my colleagues who have spent more time in this space.
rvolosatovs commented on issue #5929:
I am a bit stumped on how to fix this.
To maximize linux libc compatibility, we perform our linux release builds on centos 7 (
main
/ci/docker/x86_64-linux/Dockerfile). If we were to install the default libssl from centos 7 in the builder, it would mean all of our wasmtime linux users would need openssl 1.0.2 installed, which has been out of support since the start of 2020, so that isn't an acceptable solution.I don't know enough about the linux and openssl ecosystem to know what minimum version is acceptable, but clearly it reduce our binary compatibility to take on this dep. I also don't know if there are any knobs we can tweak in the build to lazily load the openssl dylib, so users are opting in to needing it by passing the
--wasi-modules=experimental-wasi-http
flag.Finally, I don't know if there are alternatives we can swap in for needing openssl on linux. I am aware that
rustls
exists, but I don't know if it is a complete alternative. I'll ask some of my colleagues who have spent more time in this space.FWIW, we have used
rustls
in https://github.com/enarx/enarx to provide transparent TLS to guests by wrapping the wasmtime WASI TCP traits and it was perfectly sufficient for that particular use case, which seems quite related.Depending on
rustls
would be best for both compatibility and simplicity of development. Definitely would vote for depending onrustls
instead ofopenssl
This would also make (perhaps eventually) static builds much easier
rvolosatovs edited a comment on issue #5929:
I am a bit stumped on how to fix this.
To maximize linux libc compatibility, we perform our linux release builds on centos 7 (
main
/ci/docker/x86_64-linux/Dockerfile). If we were to install the default libssl from centos 7 in the builder, it would mean all of our wasmtime linux users would need openssl 1.0.2 installed, which has been out of support since the start of 2020, so that isn't an acceptable solution.I don't know enough about the linux and openssl ecosystem to know what minimum version is acceptable, but clearly it reduce our binary compatibility to take on this dep. I also don't know if there are any knobs we can tweak in the build to lazily load the openssl dylib, so users are opting in to needing it by passing the
--wasi-modules=experimental-wasi-http
flag.Finally, I don't know if there are alternatives we can swap in for needing openssl on linux. I am aware that
rustls
exists, but I don't know if it is a complete alternative. I'll ask some of my colleagues who have spent more time in this space.FWIW, we have used
rustls
in https://github.com/enarx/enarx to provide transparent TLS to guests by wrapping the wasmtime WASI TCP traits and it was perfectly sufficient for that particular use case, which seems quite related.Depending on
rustls
would be best for both compatibility and simplicity of development. Definitely would vote for depending onrustls
instead ofopenssl
This would also make (perhaps eventually) static builds much easier
The only drawback I see is that
rustls
has not been through the same audits asopenssl
has, but IMO that is acceptable
rvolosatovs edited a comment on issue #5929:
I am a bit stumped on how to fix this.
To maximize linux libc compatibility, we perform our linux release builds on centos 7 (
main
/ci/docker/x86_64-linux/Dockerfile). If we were to install the default libssl from centos 7 in the builder, it would mean all of our wasmtime linux users would need openssl 1.0.2 installed, which has been out of support since the start of 2020, so that isn't an acceptable solution.I don't know enough about the linux and openssl ecosystem to know what minimum version is acceptable, but clearly it reduce our binary compatibility to take on this dep. I also don't know if there are any knobs we can tweak in the build to lazily load the openssl dylib, so users are opting in to needing it by passing the
--wasi-modules=experimental-wasi-http
flag.Finally, I don't know if there are alternatives we can swap in for needing openssl on linux. I am aware that
rustls
exists, but I don't know if it is a complete alternative. I'll ask some of my colleagues who have spent more time in this space.FWIW, we have used
rustls
in https://github.com/enarx/enarx to provide transparent TLS to guests by wrapping the wasmtime WASI TCP traits and it was perfectly sufficient for that particular use case (both incoming and outbound), which seems quite related.Depending on
rustls
would be best for both compatibility and simplicity of development. Definitely would vote for depending onrustls
instead ofopenssl
This would also make (perhaps eventually) static builds much easier
The only drawback I see is that
rustls
has not been through the same audits asopenssl
has, but IMO that is acceptable
pchickey commented on issue #5929:
Thanks, I asked around and we don't use rustls inside fastly for reasons that aren't relevant to this case. @brendandburns are you up for switching things to use rustls? I can do whatever supply chain audits that requires as soon as we know which crates it will take.
I guess it is a good time to rename the
wasi-http
crate towasmtime-wasi-http
in this PR since we'll be squeaking in before the release is cut next week.
pchickey commented on issue #5929:
I was just told we use rustls in https://github.com/fastly/viceroy and it has worked great for that use case.
brendandburns commented on issue #5929:
I will try switching to
rustls
I believe that it should be a pretty easy switch. I have a commit for the rename queued up also, so I will add that to this PR too.
brendandburns commented on issue #5929:
@pchickey this is switched to
rustls
and the rename (and a few minor cleanups) are added also.Please take another look. I think you may have to vet a couple more packages :(
pchickey commented on issue #5929:
I'm not a license lawyer at all but it appears the config our team selected for cargo-deny doesn't permit the use of
webpki-roots
:
https://github.com/bytecodealliance/wasmtime/actions/runs/4579627092/jobs/8087593223?pr=5929#step:5:160
Could we swap it out to use https://github.com/rustls/rustls-native-certs instead?
pchickey edited a comment on issue #5929:
I'm not a license lawyer at all but it appears the config our team selected for cargo-deny doesn't permit the use of
webpki-roots
:
https://github.com/bytecodealliance/wasmtime/actions/runs/4579627092/jobs/8087593223?pr=5929#step:5:160
Could we swap it out to use https://github.com/rustls/rustls-native-certs instead?There is a related warning about
ring
, but it appears from the license file that it contains code with MIT, ISC, and the OpenSSL license https://github.com/briansmith/ring/blob/main/LICENSE. The only one there we don't currently permit is OpenSSL. I will have to seek guidance from the Bytecode Alliance TSC or board to see if we can allow that.
pchickey edited a comment on issue #5929:
I'm not a license lawyer at all but it appears the config our team selected for cargo-deny doesn't permit the use of
webpki-roots
:
https://github.com/bytecodealliance/wasmtime/actions/runs/4579627092/jobs/8087593223?pr=5929#step:5:160
Could we swap it out to use https://github.com/rustls/rustls-native-certs instead?There is a related warning about
ring
, but it appears from the license file that it contains code with MIT, ISC, and the OpenSSL license https://github.com/briansmith/ring/blob/main/LICENSE. The only one there we don't currently permit is OpenSSL. I will have to seek guidance from the Bytecode Alliance TSC or board to see if we can allow that, cc @tschneidereit @ricochet @fitzgen
tschneidereit commented on issue #5929:
If our cargo deny config doesn't accept MPL I'd argue we should change it. The MPL 2.0 is one of the most well designed and carefully legally vetted licenses out there, and has all the key properties we need, in particular only per-file copyleft and a patent grant
pchickey commented on issue #5929:
@brendandburns please merge
main
and this should pass CI, fingers crossed
brendandburns commented on issue #5929:
@pchickey thanks, rebased on
main
this looks like it's ready to go (again :))
brendandburns commented on issue #5929:
@pchickey I missed the rename in
scripts/publish.rs
so this got bounced out of the queue.This is now fixed, hopefully it will pass the merge queue this time...
pchickey commented on issue #5929:
Unfortunately, it appears
ring
does not support riscv64. I think we will have to juggle some cargo feature flags around to disable including thewasmtime-wasi-http
crate intowasmtime-cli
on that platform.Also, I just learned that you can use the magic string
prtest:full
in a commit message, and github will run the complete CI instead of just the automatically-estimated subset. That will at least get these issues to show up more eagerly, rather than waiting to enter the main merge queue.
brendandburns commented on issue #5929:
@pchickey ok, I will see about flipping the feature flags the right way (and thanks for the tip on the prtest:full, that will definitely enable quicker iteration.
brendandburns commented on issue #5929:
@pchickey I conditionalized tls to disable it on
riscv64gc
I tried the
prtest:full
thing but it didn't seem to work for me, maybe I did it wrong...Anyhow, perhaps we can give this another try? If there's a way for me to trigger the full test runs w/o you adding it to the merge queue, let me know what I need to change in my commit message.
Thanks!
brendandburns edited a comment on issue #5929:
@pchickey I conditionalized tls to disable it on
riscv64gc
~I tried the
prtest:full
thing but it didn't seem to work for me, maybe I did it wrong...~~Anyhow, perhaps we can give this another try? If there's a way for me to trigger the full test runs w/o you adding it to the merge queue, let me know what I need to change in my commit message.~
Turns out the gh status was just cached and lagging,
prtest:full
appears to work correctly.Turns out that
ring
doesn't work ons390x
either, so I conditionalized that also. We may want to investigate how we can go back to native SSL, but that's for another PR...
brendandburns commented on issue #5929:
cc @pchickey
Ok, next issue hit:
The
wasmtime::component::bindgen!
macro expects a folder namedwit
in the same directory as theCargo.toml
The challenge is that if we want to pull the
wasi-http
repository in as a git submodule, then the directory structure is messed up, so I used a symbolic link to linkwit
->wasi-http/wit
but it looks like those symbolic links don't work properly for Windows.I can get rid of the git submodule, and just check in the
wit
files directly into this repo, it will make it a little more annoying to keep the wit files in-sync but it's do-able.Let me know what you think.
`
brendandburns commented on issue #5929:
It looks like _maybe_ setting
git config --global core.symlinks true
in our builds would fix things? But I'm not certain where those builds are configured, I can't seem to find them inmain.yml
cfallin commented on issue #5929:
Is it possible to modify the macro to have a configurable path? I think we'd probably prefer that over most any directory-structure or build-time hack (symlinks, copying files, or the like).
eduardomourar commented on issue #5929:
I believe the macro does accept a path property and you can check it here: https://github.com/bytecodealliance/wasmtime/blob/main/crates/component-macro/tests/codegen.rs
brendandburns commented on issue #5929:
@eduardomourar thanks for the pointer (I don't think that's documented anywhere :() I updated this to use
path
and I removed the symlink.crosses fingers
brendandburns commented on issue #5929:
@pchickey This passed complete CI, but it needed a rebase. I'm pretty sure that it will pass the merge queue this time.
Thanks for the patience!
brendandburns commented on issue #5929:
:clink: :confetti: :clink:
Thanks for the patience here, I'll send in some follow-on PRs.
Last updated: Dec 23 2024 at 13:07 UTC