Env of native containers is not being set

[toobeeh]

In a commit some time ago, it had been changed how env variables are being passed to the container: 76f436f
The change expects the shim implementation to set the env during run_wasi and therefore skip libcontainer DefaultExecutor.setup_envs using a no-op implementation.

This misses to handle the case of native Linux containers, where simply no env is set, because obviously, there is no responsible shim.
Environment is for example crucial when running a Linux container sidecar of a WASM container pod in Knative (queue-proxy), which fails in the current state because of the described behaviour.

A fix for this would be deciding in the LibcontainerExecutor implementation whether to skip env setup based on the container type.
To determine the container type, the OCI spec needs to be available. (https://github.com/youki-dev/youki/blob/8ce850c2bba192c5170c3c51ce1ea65c1c057231/crates/libcontainer/src/workload/mod.rs#L81)
This would require a change in the youki/libcontainer API, to provide an additional parameter in DefaultExecutor.setup_envs.
If this (breaking?) libcontainer change is considered viable, I can create a PR there.

An alternative approach without libcontainer changes would be:

• not replacing the DefaultExecutor setup_envs with a no op
• therefore enabling process env setup by default
• clearing env before executing run_wasi in exec
  This commit shows how it would look like:
  toobeeh@04f1a5b

Personally I don't think this internal fix should be preferred, since set vars -> clear vars is unnecessary overhead for the default behaviour of running a WASM container.
If it should be good enough for a hotfix/temporary workaround, I may open a PR though.
I tested the version at mentioned commit and it solves the problem I am having with knative (queue-proxy sidecar).

[Mossaka]

It looks like the first option would be a better approach. @utam0k can you please evaluate if a breaking change in libcontainer is considered viable?

[utam0k]

It's worth considering, but could we discuss in more detail what the interface will look like?

[toobeeh]

@utam0k for the use case of checking of the container type (native/wasm), a function signature providing the container specs would be perfectly fine:
fn setup_envs(&self, envs: HashMap<String, String>, spec: &Spec) -> Result<(), ExecutorSetEnvsError>

this is similar to "exec", in which runwasi currently also checks the container type (

runwasi/crates/containerd-shim-wasm/src/sys/unix/container/executor.rs

Line 54 in 5c75842

fn exec(&self, spec: &Spec) -> Result<(), LibcontainerExecutorError> {

)

[utam0k]

Would validate be insufficient for your use case?
https://github.com/youki-dev/youki/blob/9109758fa56c8aaa02c8c1b3cdcfca0aa2f2ec41/crates/libcontainer/src/workload/mod.rs#L69-L73

[toobeeh]

from my point of view, validate could be utilized in two ways:

• set the environment variables here
• cache the config for usage in setup_envs later

in my opinion the implications are not desirable:

• side effects in validate
• introduce state in the executor for this
• rely on validate being called before setup_envs

but i am new to runwasi, youki and rust at all so maybe @Mossaka can provide a more valuable opinion.

[utam0k]

I'm not entirely clear on your suggestion, but perhaps we could check in setup_env if environment variables need to be set?
https://github.com/youki-dev/youki/blob/8ce850c2bba192c5170c3c51ce1ea65c1c057231/crates/youki/src/workload/wasmer.rs#L15-L17

[jprendes]

I think we can rely on validate being called before setup_env, and we should hard fail if that's not the case.
We should also include a test for this, so that we can identify if the that precondition is broken when we bump youki's version.
That would imply keeping state in the excecutor, but that's ok, we are already keeping that state, we just need to expose a way of accessing it without providing the spec (and hard fail, i.e., abort, if it's not there).
We should add something like this to the executor:

    // `inner` renamed as `get_or_init_inner`
    fn get_or_init_inner(&self, spec: &Spec) -> &InnerExecutor {
        self.inner.get_or_init(|| {
            .. // current impl of `inner`
        })
    }

    fn get_inner(&self) -> Option<&InnerExecutor> {
        self.get()
    }

and in setup_envs we do

    fn setup_envs(
        &self,
        _: HashMap<String, String>,
    ) -> std::result::Result<(), ExecutorSetEnvsError> {
        let inner = self.get_inner().expect("Failed precondition: `setup_envs` should be called after `validate`.")
        match inner {
            ..
        }
    }