-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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-24: Promote Apparmor to GA #3298
Conversation
Signed-off-by: Sascha Grunert <sgrunert@suse.com> Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
Welcome @jan0ski! |
Hi @jan0ski. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
A couple of general comments, but since this is based on a kep draft from Sacha from a year ago, some changes have occurred in the KEP template. I've marked the two sections that definitely need to pull in the new template (PRR and Test Plan), but suggest that you go through and ensure you're using the current version for all sections :)
/assign @tallclair |
Before we graduate this, can we check whether https://kubernetes.io/docs/tutorials/security/apparmor/ is current? That page talks a lot about requiring Kubernetes v1.4 or later, so my guess is that it does need an update. |
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 path keps/sig-node/24-apparmor-ga
looks wrong. We usually don't put -ga
suffixes on short names for KEPs; the intention to stabilize is implicit.
@tallclair can you point these concerns out for me? I might be missing context, and had assumed PRR questions were addressed already. Happy to try and answer whatever I can. |
Back in october the PRR was approved #3298 (comment) There was a new question added to scalability: https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md#scalability , but that should be easy to update. |
##### e2e tests | ||
|
||
AppArmor already has [e2e tests][https://github.com/kubernetes/kubernetes/blob/2f6c4f5eab85d3f15cd80d21f4a0c353a8ceb10b/test/e2e_node/apparmor_test.go], | ||
but the tests are guarded by the `[Feature:AppArmor]` tag and not run in the |
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.
it is running in some jobs: https://storage.googleapis.com/k8s-triage/index.html?test=AppArmor
The AppArmor fields on a pod are immutable, which also applies to the | ||
[annotation](https://github.com/kubernetes/kubernetes/blob/b46612a74224b0871a97dae819f5fb3a1763d0b9/pkg/apis/core/validation/validation.go#L177-L182). | ||
|
||
When an [Ephemeral Container](20190212-ephemeral-containers.md) is added, 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.
ephemeral containers are GA now: #277
The tests section doesn't mention e2e tests validating ephemeral containers behavior. Should this be added?
All the feedback left on this KEP can be addressed by adding action items into the Graduation Criteria for GA. There is no absolutely blocking comments, mostly work items |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deads2k, jan0ski, tallclair The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
available to Localhost profiles. This prevents profiles meant for other system | ||
daemons to be utilized by Kubernetes and will be configurable by the kubelet. | ||
|
||
###### Updating AppArmor profiles |
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 be nice to cover this behavior as well: kubernetes/kubernetes#116003
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
@mccormickt I'd like to drive this forward in v1.30. Are you still open to working on it, or should I take over this PR? |
| 2) Using custom or `runtime/default` profile that restricts actions a container is trying to make. | Pod created | The outcome is workload and AppArmor dependent. In this scenario containers may 1) fail to start, 2) misbehave or 3) log violations. | | ||
| 3) Using custom profile that does not exist on the node. | Pod created | Containers fail to start. Retry respecting RestartPolicy and back-off delay. | | ||
| 4) Using an unsupported runtime profile (i.e. `runtime/default-audit`). | Pod **not** created. | N/A | | ||
| 5) Using localhost profile with invalid name | Pod **not** created. | N/A | |
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.
There is not currently any validation of the localhost profile name. We should add field validation, but need to handle annotation validation carefully.
object, events, or a warning as described in [KEP | ||
#1693](/keps/sig-api-machinery/1693-warnings). | ||
|
||
#### PodSecurityPolicy Support |
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.
Needs to be updated for PodSecurity admission
| -------------------------------------------------------------------------------------------------- | -------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| 1) Using localhost or `runtime/default` profile when container runtime does not support AppArmor. | Pod created | The outcome is container runtime dependent. In this scenario containers may 1) fail to start or 2) run normally without having its policies enforced. | | ||
| 2) Using custom or `runtime/default` profile that restricts actions a container is trying to make. | Pod created | The outcome is workload and AppArmor dependent. In this scenario containers may 1) fail to start, 2) misbehave or 3) log violations. | | ||
| 3) Using custom profile that does not exist on the node. | Pod created | Containers fail to start. Retry respecting RestartPolicy and back-off delay. | |
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.
| 3) Using custom profile that does not exist on the node. | Pod created | Containers fail to start. Retry respecting RestartPolicy and back-off delay. | | |
| 3) Using localhost profile that does not exist on the node. | Pod created | Containers fail to start. Retry respecting RestartPolicy and back-off delay. | |
| -------------------------------------------------------------------------------------------------- | -------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| 1) Using localhost or `runtime/default` profile when container runtime does not support AppArmor. | Pod created | The outcome is container runtime dependent. In this scenario containers may 1) fail to start or 2) run normally without having its policies enforced. | | ||
| 2) Using custom or `runtime/default` profile that restricts actions a container is trying to make. | Pod created | The outcome is workload and AppArmor dependent. In this scenario containers may 1) fail to start, 2) misbehave or 3) log violations. | | ||
| 3) Using custom profile that does not exist on the node. | Pod created | Containers fail to start. Retry respecting RestartPolicy and back-off delay. | |
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.
Is this outcome managed by the Kubelet or the container 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.
How is the error surfaced to the user?
If a Pod with a container specifies an AppArmor profile by field/annotation, | ||
then the container will only apply the Pod level field/annotation if none | ||
are set on the container level. |
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.
If a Pod with a container specifies an AppArmor profile by field/annotation, | |
then the container will only apply the Pod level field/annotation if none | |
are set on the container level. | |
Container-level AppArmor profiles override anything set at the pod-level. |
To raise awareness of annotation usage (in case of old automation), a warning | ||
mechanism will be used to highlight that support will be dropped in v1.30. | ||
The mechanisms being considered are audit annotations, annotations on the | ||
object, events, or a warning as described in [KEP | ||
#1693](/keps/sig-api-machinery/1693-warnings). |
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.
Needs to be updated.
To raise awareness of existing controllers using the AppArmor annotations that | ||
need to be migrated, a warning mechanism will be used to highlight that support | ||
will be dropped in v1.30. |
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.
Needs update.
#### PodTemplates | ||
|
||
PodTemplates (e.g. ReplaceSets, Deployments, StatefulSets, etc.) will be | ||
ignored. The field/annotation resolution will happen on template instantiation. |
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 are the implications for PodSecurity admission?
Since | ||
[we support](https://kubernetes.io/docs/setup/release/version-skew-policy/) up | ||
to 2 minor releases of version skew between the master and node, annotations | ||
must continue to be supported and backfilled for at least 2 versions passed the | ||
initial implementation. If this feature is implemented in v1.27, I propose v1.30 as a | ||
target for removal of the old behavior. Specifically, annotation support will be removed | ||
in the kubelet after this period, and fields will no longer be copied to annotations for older kubelet | ||
versions. However, annotations submitted to the API server will continue to be copied to fields at the | ||
kubelet indefinitely, as was done with Seccomp. |
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.
Timeline needs update
Thanks @tallclair, please feel free to take this over. Apologies for the lack of recent communication, life has gotten in the way of progress here as of late. Let me know if I can help clarify any gaps. |
Closing in favor of #4417. |
KEP-24: Promote Apparmor to GA
New updated KEP outlining requirement for Apparmor to move to GA.
Supersedes #1444