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

Allow setting securityContext in ephemeral containers #99023

Merged
merged 3 commits into from Jul 10, 2021

Conversation

verb
Copy link
Contributor

@verb verb commented Feb 12, 2021

What type of PR is this?

/kind feature
/sig node
/priority important-soon

What this PR does / why we need it: This adds securityContext to the whitelist of fields allowed in ephemeral containers (kubernetes/enhancements#277).

Which issue(s) this PR fixes:

Fixes #53188

Special notes for your reviewer:

KEP-277 was amended to allow securityContext in kubernetes/enhancements#1690.

Does this PR introduce a user-facing change?

Ephemeral containers are now allowed to configure a securityContext that differs from that of the Pod.

action required: Cluster administrators should ensure that security policy controllers support EphemeralContainers before enabling this feature in clusters.

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

- [KEP]: https://git.k8s.io/enhancements/keps/sig-node/277-ephemeral-containers

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 12, 2021
@k8s-ci-robot
Copy link
Contributor

@verb: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 12, 2021
@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Feb 12, 2021
@verb
Copy link
Contributor Author

verb commented Feb 12, 2021

/test all

@verb verb marked this pull request as ready for review February 12, 2021 12:06
@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 Feb 12, 2021
@verb
Copy link
Contributor Author

verb commented Feb 12, 2021

/assign @tallclair

@verb
Copy link
Contributor Author

verb commented Feb 12, 2021

/retest

@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@ehashman ehashman added this to Triage in SIG Node PR Triage Feb 12, 2021
@kikisdeliveryservice kikisdeliveryservice added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 12, 2021
@kikisdeliveryservice kikisdeliveryservice moved this from Triage to Needs Reviewer in SIG Node PR Triage Feb 12, 2021
@verb
Copy link
Contributor Author

verb commented Feb 16, 2021

/retest

Copy link
Member

@tallclair tallclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

pkg/apis/core/validation/validation_test.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 17, 2021
@verb
Copy link
Contributor Author

verb commented Jun 25, 2021

@tallclair now that we've got the /ephemeralcontainers API changes, I think we can pick this up again. I've added the tests you requested. PTAL. Thanks!

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2021
@akifkhan01
Copy link

Which release version are we planning to add this support for securityContext ?
@verb
this would be a critical support to enable ptrace and other tools in ephemeral containers via kubectl debug

@verb
Copy link
Contributor Author

verb commented Jul 6, 2021

@akifkhan01 Ideally this will merge in time for 1.22. This PR is all that's required for configurable security context.

@enj enj added this to Needs Triage in SIG Auth Old Jul 6, 2021
@tallclair
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2021
@verb
Copy link
Contributor Author

verb commented Jul 7, 2021

/assign @liggitt

@@ -85,6 +85,7 @@ var allowedEphemeralContainerFields = map[string]bool{
"TerminationMessagePath": true,
"TerminationMessagePolicy": true,
"ImagePullPolicy": true,
"SecurityContext": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any coverage of this field for PodSecurity and PodSecurityPolicy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For PodSecurityPolicy I added tests to podsecuritypolicy/provider_test.go to run the Container tests on EphemeralContainer as well.

I wasn't following the PSP replacement and didn't know about PodSecurity until you mentioned it. I'm happy to add test coverage there as well once I bring myself up to speed. If the PSP coverage looks sufficient, how do you feel about addressing PodSecurity in a follow-up PR since it's still in alpha?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as long as there's double opt-in and we have test coverage for the interaction before either this feature or PodSecurity graduates from alpha, that's ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including test coverage for ephemeral containers is in the PodSecurity KEP for alpha → beta graduation

fwiw, running with PodSecurity enabled, I just verified the PodSecurity enforce level already applies to ephemeral containers as well:

FEATURE_GATES=PodSecurity=true,EphemeralContainers=true hack/local-up-cluster.sh 

kubectl label ns default pod-security.kubernetes.io/enforce=restricted

kubectl apply -f ~/snippets/pods/restricted_pod.json 

kubectl debug restricted -it --image=busybox

Defaulting debug container name to debugger-86nkb.
Error from server (Forbidden): allowPrivilegeEscalation != false (container "debugger-86nkb" must set securityContext.allowPrivilegeEscalation=false), unrestricted capabilities (container "debugger-86nkb" must set securityContext.capabilities.drop=["ALL"]), runAsNonRoot != true (pod or container "debugger-86nkb" must set securityContext.runAsNonRoot=true), seccompProfile (pod or container "debugger-86nkb" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")

@ehashman ehashman moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Jul 7, 2021
@liggitt liggitt added this to the v1.22 milestone Jul 8, 2021
@liggitt liggitt added this to In progress in API Reviews Jul 8, 2021
@liggitt
Copy link
Member

liggitt commented Jul 8, 2021

/lgtm
/approve

@liggitt liggitt moved this from In progress to API review completed, 1.22 in API Reviews Jul 8, 2021
@liggitt liggitt moved this from Needs Triage to Closed / Done in SIG Auth Old Jul 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tallclair, verb

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 Jul 8, 2021
@ehashman ehashman moved this from Needs Approver to Done in SIG Node PR Triage Jul 9, 2021
@JamesLaverack
Copy link
Member

Hi v1.22 Enhancements Lead here. Enhancement 277 was not opted into v1.22. If you would still like to land this change in this release, please file an exception request.

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.22 milestone Jul 9, 2021
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@enj
Copy link
Member

enj commented Jul 9, 2021

/milestone v1.22

Exception was approved.

@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jul 9, 2021
@verb
Copy link
Contributor Author

verb commented Jul 9, 2021

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit e799d7b into kubernetes:master Jul 10, 2021
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. 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-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 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
Archived in project
SIG Auth Old
Closed / Done
Development

Successfully merging this pull request may close these issues.

Ephemeral Containers: Allow setting security context
10 participants