Skip to content

[release/1.5] seutil: Fix setting the "container_kvm_t" label#6381

Merged
crosbymichael merged 1 commit intocontainerd:release/1.5from
fidencio:wip/backport-seutil-fix-container_kvm_t-type-detection-to-release-1.5
Dec 15, 2021
Merged

[release/1.5] seutil: Fix setting the "container_kvm_t" label#6381
crosbymichael merged 1 commit intocontainerd:release/1.5from
fidencio:wip/backport-seutil-fix-container_kvm_t-type-detection-to-release-1.5

Conversation

@fidencio
Copy link
Copy Markdown
Contributor

The ability to handle KVM based runtimes with SELinux has been added as
part of d715d00.

However, that commit introduced some logic to check whether the
"container_kvm_t" label would or not be present in the system, and while
the intentions were good, there's two major issues with the approach:

  1. Inspecting "/etc/selinux/targeted/contexts/customizable_types" is not
    the way to go, as it doesn't list the "container_kvm_t" at all.
  2. There's no need to check for the label, as if the label is invalid an
    "Invalid Label" error will be returned and that's it.

With those two in mind, let's simplify the logic behind setting the
"container_kvm_t" label, removing all the unnecessary code.

Here's an output of VMM process running, considering:

  • The state before this patch:

    $ containerd --version
    containerd github.com/containerd/containerd v1.6.0-beta.3-88-g7fa44fc98 7fa44fc98f4bde30fa154c361e11609f742aaccb
    $ kubectl apply -f ~/simple-pod.yaml
    pod/nginx created
    $ ps -auxZ | grep cloud-hypervisor
    system_u:system_r:container_runtime_t:s0 root 609717 4.0  0.5 2987512 83588 ?    Sl   08:32   0:00 /usr/bin/cloud-hypervisor --api-socket /run/vc/vm/be9d5cbabf440510d58d89fc8a8e77c27e96ddc99709ecaf5ab94c6b6b0d4c89/clh-api.sock
    
  • The state after this patch:

    $ containerd --version
    containerd github.com/containerd/containerd v1.6.0-beta.3-89-ga5f2113c9 a5f2113c9fc15b19b2c364caaedb99c22de4eb32
    $ kubectl apply -f ~/simple-pod.yaml
    pod/nginx created
    $ ps -auxZ | grep cloud-hypervisor
    system_u:system_r:container_kvm_t:s0:c638,c999 root 614842 14.0  0.5 2987512 83228 ? Sl 08:40   0:00 /usr/bin/cloud-hypervisor --api-socket /run/vc/vm/f8ff838afdbe0a546f6995fe9b08e0956d0d0cdfe749705d7ce4618695baa68c/clh-api.sock
    

Note, the tests were performed using the following configuration snippet:

[plugins]
  [plugins.cri]
    enable_selinux = true
    [plugins.cri.containerd]
      [plugins.cri.containerd.runtimes]
        [plugins.cri.containerd.runtimes.kata]
           runtime_type = "io.containerd.kata.v2"
           privileged_without_host_devices = true

And using the following pod yaml:

apiVersion: v1
kind: Pod
metadata:
  name: nginx
spec:
  runtimeClassName: kata
  containers:
  - name: nginx
    image: nginx:1.14.2
    ports:
    - containerPort: 80

Fixes: #6371
Backports: #6372

Signed-off-by: Fabiano Fidêncio [email protected]
(cherry picked from commit f1c7993)

The ability to handle KVM based runtimes with SELinux has been added as
part of d715d00.

However, that commit introduced some logic to check whether the
"container_kvm_t" label would or not be present in the system, and while
the intentions were good, there's two major issues with the approach:
1. Inspecting "/etc/selinux/targeted/contexts/customizable_types" is not
   the way to go, as it doesn't list the "container_kvm_t" at all.
2. There's no need to check for the label, as if the label is invalid an
   "Invalid Label" error will be returned and that's it.

With those two in mind, let's simplify the logic behind setting the
"container_kvm_t" label, removing all the unnecessary code.

Here's an output of VMM process running, considering:
* The state before this patch:
  ```
  $ containerd --version
  containerd github.com/containerd/containerd v1.6.0-beta.3-88-g7fa44fc98 7fa44fc
  $ kubectl apply -f ~/simple-pod.yaml
  pod/nginx created
  $ ps -auxZ | grep cloud-hypervisor
  system_u:system_r:container_runtime_t:s0 root 609717 4.0  0.5 2987512 83588 ?    Sl   08:32   0:00 /usr/bin/cloud-hypervisor --api-socket /run/vc/vm/be9d5cbabf440510d58d89fc8a8e77c27e96ddc99709ecaf5ab94c6b6b0d4c89/clh-api.sock
  ```

* The state after this patch:
  ```
  $ containerd --version
  containerd github.com/containerd/containerd v1.6.0-beta.3-89-ga5f2113c9 a5f2113c9fc15b19b2c364caaedb99c22de4eb32
  $ kubectl apply -f ~/simple-pod.yaml
  pod/nginx created
  $ ps -auxZ | grep cloud-hypervisor
  system_u:system_r:container_kvm_t:s0:c638,c999 root 614842 14.0  0.5 2987512 83228 ? Sl 08:40   0:00 /usr/bin/cloud-hypervisor --api-socket /run/vc/vm/f8ff838afdbe0a546f6995fe9b08e0956d0d0cdfe749705d7ce4618695baa68c/clh-api.sock
  ```

Note, the tests were performed using the following configuration snippet:
```
[plugins]
  [plugins.cri]
    enable_selinux = true
    [plugins.cri.containerd]
      [plugins.cri.containerd.runtimes]
        [plugins.cri.containerd.runtimes.kata]
           runtime_type = "io.containerd.kata.v2"
           privileged_without_host_devices = true
```

And using the following pod yaml:
```
apiVersion: v1
kind: Pod
metadata:
  name: nginx
spec:
  runtimeClassName: kata
  containers:
  - name: nginx
    image: nginx:1.14.2
    ports:
    - containerPort: 80
```

Fixes: containerd#6371
Backports: containerd#6372

Signed-off-by: Fabiano Fidêncio <[email protected]>
(cherry picked from commit f1c7993)
@k8s-ci-robot
Copy link
Copy Markdown

Hi @fidencio. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@fidencio
Copy link
Copy Markdown
Contributor Author

/cc @dmcgowan @crosbymichael

@crosbymichael
Copy link
Copy Markdown
Member

/ok-to-test

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 15, 2021

Build succeeded.

@fidencio fidencio changed the title seutil: Fix setting the "container_kvm_t" label [release/1.5] seutil: Fix setting the "container_kvm_t" label Dec 15, 2021
@crosbymichael crosbymichael merged commit 167c3e0 into containerd:release/1.5 Dec 15, 2021
@crosbymichael
Copy link
Copy Markdown
Member

Thanks again @fidencio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants