[cri] don't clear base security settings

[crosbymichael]

When a base runtime spec is being used, admins can configure defaults for the
spec so that default ulimits or other security related settings get applied for
all containers launched.

Signed-off-by: Michael Crosby michael@thepasture.io

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 5m 45s (non-voting)

[mikebrow]

Thinking out loud.. The opt is to remove some defaults we didn't want to be there..(no default seccomp/apparmor is specified, and Remove default rlimits). Hmm.. With base runtime spec in play we need to know the source of the defaulting before making that decision. If on the deep copy of the BaseRuntimeSpec these fields are included then we shouldn't remove that makes sense. But if they are not included in the BaseRuntimeSpec I think we still need to remove them?

Maybe add a WithBaseRuntimeSpec.. so we can control the timing of the deep copy?

[crosbymichael]

I believe the timing part is fine where it's currently located. If you look at the "if" statement, we basically only add this opt if the base runtime spec is empty. Could you comment on the code lines where you think this will be an issue?

[mikebrow]

Could you comment on the code lines where you think this will be an issue?

Here's the original code..
https://github.com/containerd/cri/pull/516/files#diff-12e90e2ad0c834b3c0815d728b0435550f5cad157635031683517e42220bc0ffR743-R753

I believe you replaced it with:
https://github.com/containerd/containerd/blob/master/pkg/cri/opts/spec_linux.go#L97

Here's the original issue for the rlimits filter..
containerd/cri#515

And the rlimits:
https://github.com/containerd/containerd/blob/master/oci/spec.go#L156-L162

may be ok on the seccomp/apparmor defaults... that might have carried over from when we were using oci runtime tools to generate the spec? https://github.com/opencontainers/runtime-tools/blob/master/generate/generate.go#L238

[crosbymichael]

Yep, all your links are correct. I think maybe you are miss understanding when this isn't applied. We still clear these like normal, for the normal use case. My change is that, when you have the config.toml changed to use a different base spec, than the one that is from Go, by settings the base_runtime_spec config option, then we don't clear out the settings on that spec. This is because, when using this option, you WANT your defaults applied as you explicitly set them.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 5m 54s (non-voting)

[mikebrow]

Yep, all your links are correct. I think maybe you are miss understanding when this isn't applied. We still clear these like normal, for the normal use case. My change is that, when you have the config.toml changed to use a different base spec, than the one that is from Go, by settings the base_runtime_spec config option, then we don't clear out the settings on that spec. This is because, when using this option, you WANT your defaults applied as you explicitly set them.

ah.. you are correct. I was thinking (wrongly) that any fields not touched in the base spec would be as set in the default spec... I see now that it's an either base file or default go settings thing. Thx!

[mikebrow]

LGTM

[crosbymichael]

No worries, that file and method are really big, they should probably be refactored sometime as it's hard to follow along with the code.

[fuweid]

LGTM