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.

Sign up for GitHub

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

Jump to bottom

Add kubelet SeccompDefault alpha feature #101943

Merged
merged 1 commit into from
Jun 24, 2021
Merged

Add kubelet SeccompDefault alpha feature #101943

merged 1 commit into from
Jun 24, 2021

Conversation

saschagrunert
Copy link
Member

@saschagrunert saschagrunert commented May 12, 2021 •
edited
Loading

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 requested review from dchen1107 and mrunalp May 12, 2021 09:29
@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 mentioned this pull request May 12, 2021
Closed
12 tasks
@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
@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
@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 •
edited
Loading

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 •
edited
Loading

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 saschagrunert mentioned this pull request Jun 2, 2021
Merged
@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
Copy link
Member

liggitt commented Jun 23, 2021

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

@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

@saschagrunert @pjbgf
Add SeccompDefault feature
8b7003a 
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
@cynepco3hahue cynepco3hahue mentioned this pull request Jun 24, 2021
Closed
@liggitt
Copy link
Member

liggitt commented Jun 24, 2021 •
edited
Loading

/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
@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
@cji cji mentioned this pull request Jun 25, 2021
Open
@MMerzinger MMerzinger mentioned this pull request Aug 24, 2021
Closed
@negz negz mentioned this pull request Apr 30, 2022
Merged
3 tasks
@liggitt liggitt mentioned this pull request Feb 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@johnbelamaric johnbelamaric johnbelamaric left review comments

@pjbgf pjbgf pjbgf left review comments

@liggitt liggitt liggitt left review comments

@mrunalp mrunalp mrunalp approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@rphillips rphillips

@mrunalp mrunalp

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
API Reviews
Status: API review completed, 1.22
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.