When windows layerFolder is null parsing of the runtime spec fails

[jsturtevant]

If windows layerFolders in null loading of the runtime spec fails. This happens when containerd serializes Windows{} portion of the spec due to go's serialization of lists

Example:

{
        "ociVersion": "1.0.2-dev",
        "process": {
            "user": {
                "username": "ContainerUser"
            },
            "args": [
                "/wasi-demo-app.wasm"
            ],
            "cwd": ""
        },
        "root": {
            "path": ""
        },
        "windows": {
            "layerFolders": null,
            "ignoreFlushesDuringBoot": true,
            "network": {
                "allowUnqualifiedDNSQuery": true
            }
        }
    }

let spec = Spec::load(path)?;
thread 'sandbox::oci::ocitests::test_oci_parse' panicked at 'called `Result::unwrap()` on an `Err` value: Oci(SerDe(Error("invalid type: null, expected a sequence", line: 16, column: 32)))', crates\containerd-shim-wasm\src\sandbox\oci.rs:221:49    

I was able to work around this by modifying containerd's ctr initialization to make a blank slice:

Windows: &specs.Windows{
			LayerFolders: []string{},
		},

which results in

"windows": {
            "layerFolders": [],
            "ignoreFlushesDuringBoot": true,
            "network": {
                "allowUnqualifiedDNSQuery": true
            }
        }

[saschagrunert]

I think making the implementation being able to parse the null field to avoid the panic seems reasonable to me. Do you have any thoughts on that @utam0k?

[jsturtevant]

I was looking into this a bit more and the schema and spec for windows state this should be a min of 1 item:

{
    "windows": {
        "description": "Windows platform-specific configurations",
        "type": "object",
        "properties": {
            "layerFolders": {
                "type": "array",
                "items": {
                    "$ref": "defs.json#/definitions/FilePath"
                },
                "minItems": 1

layerFolders (array of strings, REQUIRED) specifies a list of layer folders the container image relies on. The list is ordered from topmost layer to base layer with the last entry being the scratch. layerFolders MUST contain at least one entry.

There was recently work to enable Host Process containers for Windows and a scratch image was created for it. When running that image the runtime config doesn't have a layer folder:

nerdctl run --isolation host -it --rm mcr.microsoft.com/oss/kubernetes/windows-host-process-containers-base-image:v1.0.0 powershell

--- in separate terminal---

cat C:\ProgramData\containerd\state\io.containerd.runtime.v2.task\default\7e41788b49ac0d19003fc126ffb1ced68fbe9ac6cfb355a4bcc3056fc5b6b994\config.json

{
    "ociVersion": "1.1.0-rc.1",
    "process": {
        "terminal": true,
        "user": {
            "uid": 0,
            "gid": 0
        },
        "args": [
            "powershell"
        ],
        "env": [
            "PATH="
        ],
        "cwd": ""
    },
    "root": {
        "path": ""
    },
    "hostname": "7e41788b49ac",
.... snip....
    },
    "annotations": {
        "nerdctl/name": "windows-host-process-containers-base-image-7e417",
        "nerdctl/namespace": "default",
        "nerdctl/networks": "[\"nat\"]",
        "nerdctl/platform": "windows/amd64",
....snip....
    },
    "windows": {
        "layerFolders": null,
        "ignoreFlushesDuringBoot": true,
        "network": {
            "allowUnqualifiedDNSQuery": true
        }
    }
}

It looks like this is a case where the implementation outpaced the spec and because of go's serialization behavior it didn't fail.

[utam0k]

I think making the implementation being able to parse the null field to avoid the panic seems reasonable to me. Do you have any thoughts on that @utam0k?

@saschagrunert Sorry for the late replay. How about keeping watching the discussion on this issue?
opencontainers/runtime-spec#1185

[saschagrunert]

@utam0k sounds good to me!

[jsturtevant]

@utam0k @saschagrunert after a bit of discussion I think we can move forward with making this take the null to prevent the panic.

would using a serde default or deserialize_with work?