Skip to content
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

Add kubelet SeccompDefault alpha feature #101943

Merged
merged 1 commit into from Jun 24, 2021

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented May 12, 2021

What type of PR is this?

/kind feature

What this PR does / why we need it:

This adds the gate SeccompDefault as new alpha feature. Seccomp path and field fallbacks are now passed to the helper functions, whereas unit tests covering those code paths have been added as well.

Which issue(s) this PR fixes:

Refers to kubernetes/enhancements#2413

Special notes for your reviewer:

The functions fieldProfile and fieldSeccompProfile usually never receive scmp smp *v1.SeccompProfile == nil. I decided to not cleanup those paths for security reasons.

Beside enabling the feature gate, the feature has to be enabled by the SeccompDefault kubelet configuration or its corresponding --seccomp-default CLI flag.

Another thing to mention is that the feature does not change the API at all. This means that people using the feature will not get a modified SecurityContext if they do not specify anything within it.

Does this PR introduce a user-facing change?

Added new kubelet alpha feature `SeccompDefault`. This feature enables falling back to
the `RuntimeDefault` (former `runtime/default`) seccomp profile if nothing else is specified
in the pod/container `SecurityContext` or the pod annotation level. To use the feature, enable
the feature gate as well as set the kubelet configuration option `SeccompDefault`
(`--seccomp-default`) to `true`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- KEP: https://github.com/kubernetes/enhancements/tree/34fa3dd/keps/sig-node/2413-seccomp-by-default

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 12, 2021
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 12, 2021
@saschagrunert saschagrunert changed the title Add SeccompDefault feature WIP: Add SeccompDefault feature May 12, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2021
@saschagrunert saschagrunert changed the title WIP: Add SeccompDefault feature Add SeccompDefault feature May 12, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 12, 2021
@saschagrunert saschagrunert changed the title Add SeccompDefault feature Add kubelet SeccompDefault alpha feature May 12, 2021
@ehashman ehashman added this to Triage in SIG Node PR Triage May 12, 2021
@SergeyKanzhelev
Copy link
Member

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 12, 2021
@SergeyKanzhelev SergeyKanzhelev moved this from Triage to Needs Reviewer in SIG Node PR Triage May 12, 2021
@saschagrunert
Copy link
Member Author

@mrunalp @pjbgf @liggitt PTAL

@liggitt
Copy link
Member

liggitt commented May 20, 2021

looking at the PRR, it is unclear to me how an operator would detect that enabling this caused an issue

How can a rollout fail? Can it impact already running workloads?

  • Workloads on a node may starting to fail when (re)scheduled on the node where the feature is enabled. Required specific syscalls may be blocked by the default seccomp profile, which will cause the application to get terminated.

What specific metrics should inform a rollback?

  • If a workload is starting to fail because of blocked syscalls (audit logs), then a temporarily rollback would be appropriate in production.

can you clarify how a cluster operator would know if this was causing problems?

@saschagrunert
Copy link
Member Author

saschagrunert commented May 20, 2021

can you clarify how a cluster operator would know if this was causing problems?

Pods which previously run successfully would now get some syscalls blocked they either need for startup of the workload or a single feature. This means that in the worst case the pod gets stuck in a crash loop or encounter degraded functionality. So a cluster operator would see restarting containers, while the logs would indicate something like "operation not permitted".

A good pre-check would be to run an e2e test suite for the application with the runtime default profile if the application was running unconfined before.

@liggitt
Copy link
Member

liggitt commented May 20, 2021

Pods which previously run successfully would now get some syscalls blocked they either need for startup of the workload or a single feature. This means that in the worst case the pod gets stuck in a crash loop or encounter degraded functionality. So a cluster operator would see restarting containers, while the logs would indicate something like "operation not permitted".

Detecting when a crash is due to this requires application-specific knowledge, right? I don't see a way for a cluster operator to detect this generically.

A good pre-check would be to run an e2e test suite for the application with the runtime default profile if the application was running unconfined before.

That is a good idea for the application owner, but the application owner and cluster operator are often two different entities.

@saschagrunert
Copy link
Member Author

saschagrunert commented May 20, 2021

Pods which previously run successfully would now get some syscalls blocked they either need for startup of the workload or a single feature. This means that in the worst case the pod gets stuck in a crash loop or encounter degraded functionality. So a cluster operator would see restarting containers, while the logs would indicate something like "operation not permitted".

Detecting when a crash is due to this requires application-specific knowledge, right? I don't see a way for a cluster operator to detect this generically.

The only way to get a hint to it is when toggling the feature toggles crashing pods on a node.

A good pre-check would be to run an e2e test suite for the application with the runtime default profile if the application was running unconfined before.

That is a good idea for the application owner, but the application owner and cluster operator are often two different entities.

Agreed, mixed up the roles in my mind.

@saschagrunert
Copy link
Member Author

Ah, but I see @johnbelamaric already covered this in a comment:

I don't think it's a good idea to release a feature that will break workloads on upgrade. We should require a flag to enable this even in GA.

Does that reflect the current plan, and is it expected the KEP will be updated?

We definitely have to update the KEP reflecting the flag. I think together with that I can also add a rollout plan. 🙂

@saschagrunert
Copy link
Member Author

KEP update PR in kubernetes/enhancements#2773

@saschagrunert
Copy link
Member Author

The KEP update PR got merged, which means this is now ready for review. @liggitt @kubernetes/sig-node-pr-reviews please take a look again

@liggitt liggitt added this to Assigned in API Reviews Jun 23, 2021
@liggitt
Copy link
Member

liggitt commented Jun 23, 2021

config API and cli bits lgtm (one nit on documenting the feature gate requirement)

@liggitt liggitt moved this from Assigned to API review completed, 1.22 in API Reviews Jun 23, 2021
@liggitt
Copy link
Member

liggitt commented Jun 23, 2021

tag me once the functional bits of this have approval and I can add approve for the config API

This adds the gate `SeccompDefault` as new alpha feature. Seccomp path
and field fallbacks are now passed to the helper functions, whereas unit
tests covering those code paths have been added as well.

Beside enabling the feature gate, the feature has to be enabled by the
`SeccompDefault` kubelet configuration or its corresponding
`--seccomp-default` CLI flag.

Signed-off-by: Sascha Grunert <sgrunert@redhat.com>

Apply suggestions from code review

Co-authored-by: Paulo Gomes <pjbgf@linux.com>
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
@saschagrunert
Copy link
Member Author

Pinging for approval:

@saschagrunert
Copy link
Member Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@mrunalp
Copy link
Contributor

mrunalp commented Jun 23, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021
@liggitt
Copy link
Member

liggitt commented Jun 24, 2021

/approve
for API/CLI changes
based on @mrunalp lgtm (feature gate approved by @derekwaynecarr in the KEP)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mrunalp, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2021
@saschagrunert
Copy link
Member Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@k8s-ci-robot k8s-ci-robot merged commit 2e93b39 into kubernetes:master Jun 24, 2021
SIG Node PR Triage automation moved this from Needs Reviewer to Done Jun 24, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 24, 2021
@saschagrunert saschagrunert deleted the seccomp-default branch June 24, 2021 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Status: API review completed, 1.22
Development

Successfully merging this pull request may close these issues.

None yet