[WIP] Use docker oci spec.

[Random-Liu]

To keep the behavior consistent with docker, and avoid issues like:

• MQ does not run on containerd ibm-messaging/mq-container#207;
• Sandbox config - rlimit #515;
• Inclusion of /run tmpfs in default mounts breaks some images containerd#1494.

Let's use docker oci spec as the default. /cc @estesp @containerd/containerd-maintainers

Signed-off-by: Lantao Liu lantaol@google.com

[Random-Liu]

Here is a diff of the default spec diff before after:

78,79c78
<       },
<       "noNewPrivileges":true
---
>       }
88c87,92
<          "source":"proc"
---
>          "source":"proc",
>          "options":[
>             "nosuid",
>             "noexec",
>             "nodev"
>          ]
115,117c119,121
<          "destination":"/dev/shm",
<          "type":"tmpfs",
<          "source":"shm",
---
>          "destination":"/sys",
>          "type":"sysfs",
>          "source":"sysfs",
122,123c126,137
<             "mode=1777",
<             "size=65536k"
---
>             "ro"
>          ]
>       },
>       {
>          "destination":"/sys/fs/cgroup",
>          "type":"cgroup",
>          "source":"cgroup",
>          "options":[
>             "ro",
>             "nosuid",
>             "noexec",
>             "nodev"
137,139c151,153
<          "destination":"/sys",
<          "type":"sysfs",
<          "source":"sysfs",
---
>          "destination":"/dev/shm",
>          "type":"tmpfs",
>          "source":"shm",
144c158
<             "ro"
---
>             "mode=1777"
153a168,216
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":5,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":3,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":9,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":8,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":5,
>                "minor":0,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":5,
>                "minor":1,
>                "access":"rwm"
>             },
>             {
>                "allow":false,
>                "type":"c",
>                "major":10,
>                "minor":229,
>                "access":"rwm"
157d219
<       "cgroupsPath":"/k8s.io/816b543fa52f6fc616df52a5edba5134e3eb97aca2c726e6c5cb49ed8628aeab",
160c222
<             "type":"pid"
---
>             "type":"mount"
163c225
<             "type":"ipc"
---
>             "type":"network"
169c231
<             "type":"mount"
---
>             "type":"pid"
172c234
<             "type":"network"
---
>             "type":"ipc"
183,184c245,246
<          "/sys/firmware",
<          "/proc/scsi"
---
>          "/proc/scsi",
>          "/sys/firmware"

1. For cgroup mount, we mounted it in cri before. Now docker default includes it, we can get rid of the corresponding cri logic;
2. For /dev/shm, it doesn't matter we specify size in default or not. cri always overwrites it. https://github.com/containerd/cri/blob/master/pkg/server/container_create.go#L496
3. For cgroupsPath, we never use the default from containerd, cri always overwrites it. https://github.com/containerd/cri/blob/master/pkg/server/container_create.go#L419

Other diffs are what we want to keep behavior identical with Docker.

[Random-Liu]

/test pull-cri-containerd-build

[BenTheElder]

/retest

[fejta]

/retest

[mikebrow]

Agree keeping in sync with docker is a goal and I'm surprised at the number of diffs. But now we have a issue with users who are expecting us to be in sync with ourselves. Might be wise to consider config options for some of these behavior changes.

[dmcgowan]

Could this change be done more granularly and maybe with options like Mike said? We want to break away from importing from github.com/docker/docker and that package isn't large. Better to understand the changes and pull them in now rather than do this later when we want to break the dependency.

[Random-Liu]

Agree keeping in sync with docker is a goal and I'm surprised at the number of diffs. But now we have a issue with users who are expecting us to be in sync with ourselves. Might be wise to consider config options for some of these behavior changes.

Yeah, make sense. :(

Actually, besides the NoNewPrivileged fix, the only 2 things changed:

88c87,92
<          "source":"proc"
---
>          "source":"proc",
>          "options":[
>             "nosuid",
>             "noexec",
>             "nodev"
>          ]

The docker config seems more secure.

153a168,216
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":5,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":3,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":9,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":1,
>                "minor":8,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":5,
>                "minor":0,
>                "access":"rwm"
>             },
>             {
>                "allow":true,
>                "type":"c",
>                "major":5,
>                "minor":1,
>                "access":"rwm"
>             },
>             {
>                "allow":false,
>                "type":"c",
>                "major":10,
>                "minor":229,
>                "access":"rwm"

I think it makes sense to whitelist those devices https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#default-devices
And at least 10 229 /dev/fuse is not included in runc https://github.com/opencontainers/runc/blob/master/libcontainer/specconv/spec_linux.go#L45

Actually I'm not completely sure about the actual effect.

[Random-Liu]

Could this change be done more granularly and maybe with options like Mike said? We want to break away from importing from github.com/docker/docker and that package isn't large. Better to understand the changes and pull them in now rather than do this later when we want to break the dependency.

I can do it if that is preferred.

[Random-Liu]

Let's hold this PR for now. The most useful fix in this PR is the NoNewPrivileges default change.

However, I found that we are actually always overwriting containerd default in cri. https://github.com/containerd/cri/blob/master/pkg/server/container_create.go#L411 If that is the case, I don't quite understand why ibm-messaging/mq-container#207 happens.