@yujuhong Add support for Seccomp sounds like a new feature. The milestone should be reset to next-candidate, correct?

@grodrigues3 not really. The current implementation supports seccomp already. If it's missing critical information to function properly, this would be a bug.

@yujuhong are we blocking releases for CRI in 1.5? i was not sure, so marking this as non-release-blocker and leaving 1.5 milestone. please correct if i am mistaken.

I don't get why this is a 1.5 targetted thing myself.

Yes we want CRI to be in a good state in 1.5, but afaik we have zero intention of anyone actually using (at the very least because of the lack of exec auth regressing kubelet security).

@yujuhong why should this be part of 1.5?

Yes we want CRI to be in a good state in 1.5, but afaik we have zero intention of anyone actually using (at the very least because of the lack of exec auth regressing kubelet security).

First, this is a small enough change that allows non-docker integrations to properly support seccomp. Second, if "we have zero intention of anyone actually using" is a valid reason, we should never fix any bugs in CRI. We do want people to try using it (perhaps not in production yet). However, given that seccomp is only in alpha, I am ok with dropping this and just continue the docker support for now.

this is a small enough change

Perhaps I misunderstood the meaning of code freeze, but I thought it was only bug-fixes regardless of size of change (or exceptions that have gone through some process)

Second, if "we have zero intention of anyone actually using" is a valid reason, we should never fix any bugs in CRI

What I meant is we don't intend this to be used generally as part of the 1.5 release. Because of that, I don't think we should consider bugs in CRI bugs for the purpose of the 1.5 release freeze. My statement is just that we shouldn't violate code-freeze for this. We should totally fix this right after the 1.5 freeze window ends, and saying that I'm arguing we should never fix anything is disingenuous and taking it to an absurd extreme.

@euank the only thing we don't seem to agree on is whether seccomp support was considered to be supported for CRI in 1.5. My interpretation is that we assumed seccomp would be supported until we found out that it wouldn't for other runtimes due to the lack of profile path. That's the only reason why I considered this a bug. Of course we could say that this is not really an important feature in CRI, so we can drop it, but we can be more clear on that.

If there's a bug in a feature no one uses, does fixing it make a sound?

I think what we're not agreeing on is the role of CRI in 1.5.

I understood it as something that is not generally usable. We can't recommend a customer use it, even as an experimental feature, due to security aspects (if nothing else). Correct me if I'm wrong.

It's a developer integration point, and developers integrating with it are doing so with the kubernetes master branch, not with some release.

Those reasons together result in my opinion that CRI is still not really a customer/release feature, and thus bugs or features related to it should not be exceptions to the freeze window, but rather should targeted at master as soon as the freeze lifts.

If this were already post-freeze, would we be cherry-picking this over to 1.5?

If there's a bug in a feature no one uses, does fixing it make a sound?

@euank This is not true. cri-o is depending on this fix for apparmor support.

@feiskyer is it depending on the 1.5 release branch directly, or on master?

@euank I think it's better to make this in 1.5, so cri-o could work with first release with experimental CRI. It helps to encourage people to use CRI and find features misses or bugs.

Modifying the user-specified annotation is a little bit unsettling. I think adding a new annotation explicitly for the path would be better. Or, we should just add an explicit field for this.

I spoke to @euank and @timstclair separately. I still think this is a bug fix, but we may want to be more careful about the fix. Given the time constraint, let's just list this as a known issue and work on it for 1.6.

@yujuhong OK, let's move this to next release and think carefully about seccomp feature besides this:

• kubelet may just pass the profile as string via CRI; may assume the profile locate in a file in a known format and pass path to the runtime; or maybe some other solutions. We need to clearly define it.
• define profile format clearly

Please document known issues for 1.5 in #37134

(copied from #39128)

I did a little more investigation, and as far as I can tell a seccomp root and the associated file structure is a docker specific (invented here) concept.

If we wanted to make this container runtime agnostic, the Kubelet could convert from that json to protobuf structs which clearly specify the exact details.

I think this is attractive for a number of reasons. Most importantly, the current specification is relying on incidental communication of state via the filesystem (passing references to files over an API is basically always an anti-pattern).

There are also things, such as no-new-privileges, which interact in some way with certain seccomp options, and so it makes sense for the kubelet to resolve a seccomp specification fully itself prior to reaching out to the runtime so that the kubelet may make any additional decisions that must be made.

That would also reduce the number of annotations that have magic powers, which is always a good thing in my book.

And

@euank - I agree with you, but that's also not how seccomp works today. I'd like to be able to source seccomp (and AppArmor) profiles from ConfigMaps, and maybe at that point we could deprecate local profiles.

@euank @timstclair let's move here to discuss how to pass profiles to runtime.

Related with #39128.

This is issue is related with two level APIs:

1. In CRI, since we may also want to support profiles from ConfigMaps, I think we should pass full profile contents instead of path to runtime.
2. This issues is also related with seccomp api in kube-apiserver, it is based on annotations today. We should define seccomp profile format clearly (see Proposal: spec the seccomp profile format #39128) and decide whether to add struct types to pod spec or just continue the annotations way. I think it's more clear to add specific struct types to pod spec.

Agreed - the path stuff is really a crutch (and implementation-specific) - I think #39128 is the answer here.

In CRI, since we may also want to support profiles from ConfigMaps, I think we should pass full profile contents instead of path to runtime.

I don't disagree, but it's not strictly a requirement for supporting ConfigMaps (kubelet could create a file with the contents). An open question is whether to pass it as a raw string, or wait for #39128 and pass the structured data.

decide whether to add struct types to pod spec or just continue the annotations way

The annotations way is deprecated (#30819). I think the bigger question is where/how to put the data. E.g. structured data in the API, unstructured (string type) data in the API, reference to another object (e.g. ConfigMap). If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

The annotations way is deprecated (#30819).

Yes, already opened #39130 to replace annotations with typed structs.

I think the bigger question is where/how to put the data. E.g. structured data in the API, unstructured (string type) data in the API, reference to another object (e.g. ConfigMap). If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

I personally prefer a structured data type in both kubernetes API and CRI. @yujuhong @euank WDYT?

I personally prefer a structured data type in both kubernetes API and CRI. @yujuhong @euank WDYT?

A counter argument is that as the spec grows or new features are added, we need to keep updating the Kubernetes API to maintain parity, or update the code in each CRI implementation to translate the Kubernetes spec. OTOH, if we use unstructured data we can use 3rd party libraries to parse out the data we need (assuming the spec evolves in a backwards compatible way).

A counter argument is that as the spec grows or new features are added,

What spec?

Whichever seccomp profile spec is being implemented by runtimes. E.g. if we decide to base the Kubernetes seccomp profile API off the OCI seccomp profile spec, and OCI adds some new features, at some point we'd probably want to add those features to Kubernetes. It also means that if different runtimes support different feature sets, we'll be stuck either tracking only the common features, or specifying features that may be ignored by the runtime. This may be a non-issue if the seccomp feature set rarely changes.

Well, yes, but that applies to everything in the CRI, and is one of the tradeoffs we ma[d|k]e by introducing this abstraction in the first place.

My $0.02:

Moving the existing logic/information for seccomp into a first-class field for CRI (as in #46332) is productive because of the known problems with the annotation approach.

My initial proposal for seccomp support included a resource similar to what @jessfraz has proposed in kubernetes/community#660 and we were unable to come to an agreement on the issue @timstclair refers to here: #36997 (comment)

I do not necessarily expect that we will be able to come to an agreement about the seccomp resource in time for 1.7, so I think we should proceed with #46332 and do backward compatible CRI changes later if we need to inject the content of the seccomp profile instead of the path.

If the Kubelet needs to parse the spec, I think that's a good argument for a structured data type (either a standalone object or embedded in the pod API).

I think we would deeply regret embedding API surface to describe seccomp profiles in the Pod API :)