-
Notifications
You must be signed in to change notification settings - Fork 333
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
Making verifying binary optional in wasi related code path #2220
Comments
I feel we should support the wasm use case in some way, but I am inclined against making any changes that will break OCI compatibility or make the integration tests fail. Is there a way to check if given entrypoint is wasm or not, and then conditionally do the check? |
Agreed. (I will do some test and reading the spec to make sure this. If it doesn't break assumptions in the spec, we can continue to explore if this is a good idea. Otherwise, this is a deal breaker.)
Perhaps, but I also want to be careful introducing wasm specific information into the libcontainer internals. So far, we have been successfully abstracting wasm related information to outside of the libcontainer. My biggest concern is that wasm is a fast moving targets and things in this space moves quite fast and unstable. On the other hand, libcontainer do need to support OCI which is reasonably stable and mature by now. Perhaps this means to need to introduce a configuration in the interface to skip the binary verification. At least, in this way, we surface the control knob up to the interface, instead of conceal the checking inside the implementation of libcontainer. Another idea is to change the executor interface to split off a Sigh... designing a good interface is hard... but at least now we have a customer we can look to for usecases and hints :) |
We are also exploring using OCI Artifacts for WASM. In this case the module was loaded from containerd's content store instead of within the container. The container file system in this case is just the files required for the module to run but the module itself is loaded by the wasm runtime. I got around this by disabling verification temporarily. |
of the two options, I like the "Provide a optional field in the libcontainer interface to make the verification optional" via the builder interface becuase of the downsides of running it at
|
🤦♂I forgot about it.
I love @jsturtevant 's idea. |
@jsturtevant Yes, looks like introducing the nob in the builder interface is the better approach here. I will get this implemented soon. |
The builder approach might have the drawback that if more than one executor is running it would apply to all of them? https://github.com/containerd/runwasi/pull/156/files#r1283810527 |
If we implement binary check logic in |
This is what I was hinting at to abstract the validation logic into the executor. If we head down this path, I do think we need to move where the |
This should be OK. I am changing the API to use a single executor and remove the executor list. The new scheme will favor composing multiple executor into a single executor. This should provider executor authors with greater control on how executors are implemented. |
I will prototype adding the |
👀 |
What are the implications of this change for shim authors? In particular, runwasi wasmtime | wasmedge shims are depending on the list of executors feature to enable running both Linux and wasm containers. |
That will still be possible. And as @utam0k mentioned, we will take care of any breaking changes on the runwasi side. |
Prototype in #2258 |
As a note, the containerd integration test expects the validation to happen before the end of Prototype that failed: #2280 |
In this case, I feel exposing a builder option to turn it off, and keeping it on by default would be a better idea than creating a separate validate step. |
Isn't this fixed by #2258 ? |
@yihuaf Is there anything you want to do? |
This is fixed. We just need to release and patch runwasi with the breaking changes. I am in the process to draft a PR for |
@yihuaf oh I've also started working on using the newest libcontainer from the main branch. To not duplicate the effort, I'll let you work on it. |
In the OCI container path, we do a few things to verify that the binary path pass in as part of the process block in the OCI spec exist and has the right permission. The path is usually an absolute path to a binary in the container rootfs. However,
wasi
andwasm
have syntax that calls to a specific module of a wasm file. So the path becomes/some/path/x.wasm#module
. This is not a valid path and will fail our check.Specifically see the issue from
runwasi
. containerd/runwasi#194There are two options I am contemplating:
I like the simplicity of moving the verification logic into executor, but there is one down side. The executor will not be executed until
youki start
andyouki create
will no longer fail if the binary doesn't exist. This may also break some existing integration test and even break OCI compliance.I have not decided on which path to take, but want this issue for tracking and discussion.
The text was updated successfully, but these errors were encountered: