Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KEP for promoting seccomp to GA #1148
KEP for promoting seccomp to GA #1148
Changes from 4 commits
0e21d25
82f393e
8d37151
cc0e931
098ac44
26b8a4f
da67380
da1bbc6
81059c3
b32550c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this intended to be embedded rather than something like
Profile SeccompProfile
? what other options do you anticipate?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I modeled it off the
Volume
type.The only option that comes to mind is the fallback strategy. Currently, if the seccomp options can't be enforced the node silently ignores them. In contrast, when AppArmor can't be enforced the pod is held in a
Blocked
state.From a security perspective, silently failing could leave a potential security hole. From a portability perspective, it's nice to be able to choose the best practices options with a best-effort approach. Hence it might make sense to give the user the choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens for nodes/runtimes that don't enable seccomp is worth documenting, at least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just
securityContext.seccompProfile
instead of embedding/nesting (can prefix other seccomp fields withseccomp
if they are needed in the futureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @apelisse ... don't we want to add a discriminator here? what's the current approach to doing that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are discriminators required if the structure is immutable? I guess we'd still need it for pod templates...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the discriminator field need to be a 1:1 mapping with union fields? If not, the API could look like this:
In other words, it seems redundant to have a the type indicate a boolean value that must be set to true. In those cases, the discriminator should be sufficient to indicate the option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, a union type without an associated member field is permitted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify format limits (length, valid characters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I go through this, I'm second guessing this field. AFAIK there aren't any runtimes that support multiple built in profiles, and this feature has never been requested.
If we dropped this field for now (just assume it's runtime/default), how bad would it be to add it back at some point in the future if it was needed? As long as we defaulted the new field to "default" and it's immutable, it doesn't seem like it would be that problematic? Or am I forgetting something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do the current annotations contain sufficient detail to know whether they should convert to a runtime or localhost profile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, currently the legal values for the annotations are:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec out how annotation -> field conversion would work in the docker/default case (and how we'd validate annotation and field matched in that case... probably a special case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we use API defaulting to set RuntimeProfile to "default" if type is set to "Runtime"? if so, would we clear RuntimeProfile on update if Type was changed to something other than "Runtime"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the design review, we discussed not wanting to add the LocalhostProfile field if the intention is to deprecate and eventually remove that feature. Here's an alternative proposal for how to handle localhost:
Drop the
LocalhostProfile *string
field. Keep theSeccompProfileLocalhost SeccompProfileType
, but optionally change its value toLocalhostDeprecated
.When creating a pod, the profile type can only be set to "Localhost" if the annotation is also set to localhost. When the kubelet goes to enforce the localhost profile, it fetches the path from the annotation.
The one gotcha is how to handle annotation update in this case. I'm tempted to say allow the update, and if the kubelet goes to enforce it and the annotation isn't set to localhost anymore, just treat it as an invalid localhost path (fail the pod).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use
Config
object for storing profiles across the nodes? Can be another KEP though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree this would be useful, but implementing it is a big project that I don't want to block GA on. See non-goals:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the expected behavior if the CRI runtime doesn't support seccomp? The pod should be created without seccomp (as in the current alpha annotation) or should result in an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to maintain the backwards compatible behavior of ignoring it (I'll add a line to be explicit about this). Going forward, I left space in the API for a
FailureStrategy
which could dictate how to handle that situation (e.g. some applications may depend on seccomp application to run securely, while others might accept a best-effort best-practices approach).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No application should depend on a policy; a policy can be applied as a non privileged operation by any code, so if it requires it then it can just apply it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the proposed field is more expressive than the annotation (allows runtime profiles other than "default")... need to resolve if/how those would be converted to annotations that would not cause 1.17-level API servers to choke in validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this to say "when the oldest supported kubelet is 1.18", since we're thinking through extending support to 4 releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we also considering extending version-skew support to 4 releases? Or just that we would carry patches for 4 releases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're considering the latter. the current kubelet/apiserver skew was set so the oldest kubelet would work with the newest apiserver, to avoid needing to do multiple nodepool recreations in a single cluster upgrade cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think we should just keep this for at least 4 versions. We don't need to rush to remove the reconciliation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not specific to this field, but we need to consider the case of an update from an old client that drops the new spec field. I think for this (and for other immutable fields), we should probably preserve the existing field value if an update attempts to clear it (since that could be coming from a client unaware it needs to round-trip this new field)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed kubernetes/kubernetes#87301 to track this. It's not clear to me what the right way of handling this is. One argument for special-casing it for seccomp is that the field may be set by something the client does know about, the annotation. So there could be a situation where:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider how cluster operators will know they have things that need updating. a metric that fires on any podtemplate object with seccomp annotations, and (once we drop pod field -> annotation defaulting) any pod with a seccomp annotation might be helpful for tracking this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. It could be any combination of:
A metric seems too hidden, if you don't know to look for it. I like the event, except it will disappear if you don't notice it right away. I'm leaning towards an annotation on both the controller object and the pod:
We may also want to consider forbidding setting the seccomp annotation after we drop support, so that pods aren't being run
unconfined
unexpectedly. That would be more likely to break users, but follows the secure failure principle.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my biggest concern, but I think the arguments you've laid out are correct.
This is a necessary step to get to the good stuff :)
Thanks so much Tim!