kubelet: Use CRI SecurityProfile for Seccomp #96281
Conversation
k8s-ci-robot
added
release-note-none
|
@mrunalp: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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
added
needs-priority
| @@ -34,8 +34,11 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Deprected, remove after we switch to Seccomp field | |||
| func fieldSeccompProfile(scmp *v1.SeccompProfile, profileRootPath string) *runtimeapi.SecurityProfile { | ||
| if scmp == nil { | ||
| return &runtimeapi.SecurityProfile{ | ||
| ProfileType: runtimeapi.SecurityProfile_Unconfined, |
There was a problem hiding this comment.
Now that I see the code assigning enum 1 unconfined to the ProfileType on nil seccomp...should unconfined be the first ordered value in the enum list?
There was a problem hiding this comment.
Intention is to move to runtime default as the default in the future.
There was a problem hiding this comment.
SGTM secure by default .. unconfined should only be the default for privileged mode... if seccomp security profile is set and not privileged the default should be runtime default... makes sense
| ProfileType: runtimeapi.SecurityProfile_Unconfined, | ||
| } | ||
| } | ||
| if scmp.Type == v1.SeccompProfileTypeRuntimeDefault { |
There was a problem hiding this comment.
suggest switch and maybe log err for unexpected type..
| ProfileType: runtimeapi.SecurityProfile_RuntimeDefault, | ||
| } | ||
| } | ||
| if scmp.Type == v1.SeccompProfileTypeLocalhost && scmp.LocalhostProfile != nil && len(*scmp.LocalhostProfile) > 0 { |
There was a problem hiding this comment.
should we use unconfined if local host is set and nil profile in this new mode.. or make this an error.. or keep the type and empty ref.. letting the caller decide how to handle the situation.
There was a problem hiding this comment.
This is mirroring the existing behavior. Added a TODO for moving to runtime default as a follow up.
There was a problem hiding this comment.
Added a TODO for moving to runtime default as a follow up.
Aren't there compatibility considerations for that?
There was a problem hiding this comment.
Aren't there compatibility considerations for that?
Yes, existing workloads could potentially break. Hence, we need to come up with a plan for defaulting
to runtime default instead of unconfined. We could potentially add a feature gate. I think it is best
tackled separately (from this PR) with wider discussion in sig-node first.
There was a problem hiding this comment.
Agreed. If you add a TODO, make sure it points to an issue with sufficient context to ensure no one tries to resolve the TODO without addressing those issues.
There was a problem hiding this comment.
Sure, I will open an issue for it 👍
| return fieldSeccompProfile(containerSecContext.SeccompProfile, m.seccompProfileRoot) | ||
| } | ||
|
|
||
| // when container seccomp is not defined, try to apply from pod field |
There was a problem hiding this comment.
would it be fair to pick up the ref from the pod if nil as well.. hmm
There was a problem hiding this comment.
This is mirroring the existing behavior.
We set both the old and the new fields for now and will remove the old field in the next release. Signed-off-by: Mrunal Patel <mpatel@redhat.com>
mrunalp
force-pushed
the
kubelet_cri_seccomp_profile
branch
from
9b5912e
to
32b9ac7
Compare
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-e2e-kind |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikebrow, 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 |
The seccomp field is the new default since a couple of releases, means we can stop using `SeccompProfilePath`. Follow-up on kubernetes#96281 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
The seccomp field is the new default since a couple of releases, means we can stop using `SeccompProfilePath`. Follow-up on kubernetes#96281 Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
We set both the old and the new fields for now and will
remove the old field in the next release.
@saschagrunert @mikebrow ptal
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
We add support to the kubelet to use the new CRI SecurityProfile introduced in
#95876 for Seccomp
This is a part of moving CRI to beta
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
-[KEP] kubernetes/enhancements#2040