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

feat: support CRI-O #697

Conversation

DelusionalOptimist
Copy link
Member

@DelusionalOptimist DelusionalOptimist commented May 16, 2022

Signed-off-by: Rudraksh Pareek [email protected]

Fixes #666

  • This PR makes KubeArmor compatible with CRI-O container runtime.
  • The approach used is similar to that of containerd with some changes here and there.

Tasks:

  • Implement monitoring
  • Tests
  • Setup scripts

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #697 (72993b5) into main (7e1810d) will decrease coverage by 1.61%.
The diff coverage is 9.56%.

@@            Coverage Diff             @@
##             main     #697      +/-   ##
==========================================
- Coverage   39.30%   37.69%   -1.62%     
==========================================
  Files          24       25       +1     
  Lines        8702     8997     +295     
==========================================
- Hits         3420     3391      -29     
- Misses       4828     5158     +330     
+ Partials      454      448       -6     
Impacted Files Coverage Δ
KubeArmor/core/containerdHandler.go 3.35% <0.00%> (-12.47%) ⬇️
KubeArmor/core/crioHandler.go 0.00% <0.00%> (ø)
KubeArmor/core/kubeUpdate.go 48.30% <0.00%> (-0.16%) ⬇️
KubeArmor/core/kubeArmor.go 49.04% <20.83%> (-5.39%) ⬇️
KubeArmor/config/config.go 87.12% <75.00%> (-1.22%) ⬇️
KubeArmor/common/common.go 41.66% <83.33%> (+1.06%) ⬆️
KubeArmor/core/dockerHandler.go 47.95% <100.00%> (-4.09%) ⬇️
KubeArmor/enforcer/appArmorEnforcer.go 48.29% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e1810d...72993b5. Read the comment docs.

@DelusionalOptimist DelusionalOptimist force-pushed the DelusionalOptimist/feat/crio-support branch 3 times, most recently from 394d2ea to b72e40a Compare May 20, 2022 20:56
@DelusionalOptimist DelusionalOptimist changed the title WIP: feat: support CRI-O feat: support CRI-O May 20, 2022
@DelusionalOptimist DelusionalOptimist marked this pull request as ready for review May 20, 2022 21:32
@DelusionalOptimist
Copy link
Member Author

@nam-jaehyun @nyrahul @daemon1024 Please give this a review. (I'm unable to request reviews 😅 ).
I've temporarily modified the gh workflow so that k3s runs with crio and tests can happen. Github tests 6, 8 & 13 are failing and I'll take a look into them in the meantime.
Also, please terminate https://github.com/kubearmor/KubeArmor/actions/runs/2360471345

@daemon1024 daemon1024 requested review from nam-jaehyun, nyrahul and daemon1024 and removed request for nam-jaehyun and nyrahul May 21, 2022 02:14
@DelusionalOptimist
Copy link
Member Author

Github test 06 and 08 passed eventually.
However, github test 13 didn't pass because crio by default doesn't permit the capability CAP_NET_RAW and it has to be first enabled by editing default capabilities in /etc/crio/crio.conf. What should we do here? 🤔

Also, in local tests, multiubuntu test 02 and 07 are failing. 07 fails due to the same reason as above but I'm not sure about 02 as audit action works fine when tested manually. Here are logs from karmor when the test conditions are created -

== Alert / 2022-05-22 15:42:57.985910 ==
Cluster Name: default
Host Name: kubearmor-dev
Namespace Name: multiubuntu
Pod Name: ubuntu-4-deployment-566bf47cd7-25dx6
Container ID: 72dbd9ab9ec9e2b997ac809087e228479f6f8e8df035b52e1ff251e8f5bb414f
Container Name: ubuntu-4-container
Policy Name: ksp-group-2-proc-path-audit
Severity: 4
Type: MatchedPolicy
Source: /bin/bash
Operation: Process
Resource: /bin/sleep 1
Data: syscall=SYS_EXECVE
Action: Audit
Result: Passed

For ref: the default capabilties applied by CRI-O:

default_capabilities = [
        "CHOWN",
        "DAC_OVERRIDE",
        "FSETID",
        "FOWNER",
        "SETGID",
        "SETUID",
        "SETPCAP",
        "NET_BIND_SERVICE",
        "KILL",
]

@DelusionalOptimist DelusionalOptimist force-pushed the DelusionalOptimist/feat/crio-support branch from b72e40a to c3fcffa Compare May 27, 2022 14:55
@DelusionalOptimist
Copy link
Member Author

@nam-jaehyun @nyrahul as discussed, I've updated the github workflows to test for all the runtimes and skip the tests that have been specifically failing only for crio.

@nyrahul
Copy link
Contributor

nyrahul commented May 27, 2022

@nam-jaehyun @nyrahul as discussed, I've updated the github workflows to test for all the runtimes and skip the tests that have been specifically failing only for crio.

Hey, I see that tests are still failing! Also can you please squash the commits?

@DelusionalOptimist
Copy link
Member Author

Hey, I see that tests are still failing! Also can you please squash the commits?

Oof yes. Taking a look into why the tests with containerd are failing 👀

@nyrahul nyrahul added this to the v0.5 milestone Jun 2, 2022
Copy link
Collaborator

@nam-jaehyun nam-jaehyun left a comment

Choose a reason for hiding this comment

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

can you also update contribution/vagrant/Vagrantfile?

cri-o needs to work under the following commands.
KubeArmor/KubeArmor$ make vagrant-up RUNTIME=crio
KubeArmor/KubeArmor$ make vagrant-up RUNTIME=crio NETNEXT=1
if possible, KubeArmor/KubeArmor$ make vagrant-up RUNTIME=crio OS=centos

contribution/k3s/install_k3s.sh Outdated Show resolved Hide resolved
.github/workflows/ci-test.yml Outdated Show resolved Hide resolved
contribution/k3s/install_k3s.sh Show resolved Hide resolved
tests/test-scenarios-github.sh Outdated Show resolved Hide resolved
KubeArmor/core/kubeArmor.go Outdated Show resolved Hide resolved
@DelusionalOptimist DelusionalOptimist force-pushed the DelusionalOptimist/feat/crio-support branch from 9372567 to c10756d Compare June 14, 2022 19:00
@DelusionalOptimist
Copy link
Member Author

@nam-jaehyun @nyrahul

I and @daemon1024 were thinking about extending the use of cri-api and using it for docker and containerd as well. The current code for handling the two runtimes would be deprecated and the code used here would be used to communicate with the runtimes' socket.
It is possible as kubernetes mandates a cri friendly interface for every runtime. Containerd implements the cri plugin which it needs to communicate with kubelet and docker has dockershim (or cri-dockerd now). So since both of these are compatible with cri we can save the effort of maintaining code for their respective APIs.

Does this sound good?

@nyrahul
Copy link
Contributor

nyrahul commented Jun 16, 2022

@nam-jaehyun @nyrahul

I and @daemon1024 were thinking about extending the use of cri-api and using it for docker and containerd as well. The current code for handling the two runtimes would be deprecated and the code used here would be used to communicate with the runtimes' socket. It is possible as kubernetes mandates a cri friendly interface for every runtime. Containerd implements the cri plugin which it needs to communicate with kubelet and docker has dockershim (or cri-dockerd now). So since both of these are compatible with cri we can save the effort of maintaining code for their respective APIs.

Does this sound good?

Imo, it sounds very good. But my understanding was that we are using some containerd or docker specific apis which are not part of cri-api spec. If this understanding is wrong, what you saying is the best way to handle things i.e. just use cri-api for everything ... This will simplify lot of our program logic. But I am keen on what @nam-jaehyun has to say.

Copy link
Member

@daemon1024 daemon1024 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nyrahul nyrahul left a comment

Choose a reason for hiding this comment

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

Great PR!! Please check my comments inline.

KubeArmor/core/crioHandler.go Outdated Show resolved Hide resolved
KubeArmor/core/crioHandler.go Outdated Show resolved Hide resolved
KubeArmor/core/crioHandler.go Show resolved Hide resolved
KubeArmor/core/crioHandler.go Show resolved Hide resolved
KubeArmor/core/kubeArmor.go Outdated Show resolved Hide resolved
@nyrahul
Copy link
Contributor

nyrahul commented Jun 23, 2022

@DelusionalOptimist , can you squash the commits?

Signed-off-by: Rudraksh Pareek <[email protected]>
@DelusionalOptimist DelusionalOptimist force-pushed the DelusionalOptimist/feat/crio-support branch from 72993b5 to 2c56af2 Compare June 23, 2022 11:27
@DelusionalOptimist
Copy link
Member Author

@nyrahul done 👍

Copy link
Contributor

@nyrahul nyrahul left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@nyrahul nyrahul merged commit 24dab96 into kubearmor:main Jun 23, 2022
@DelusionalOptimist DelusionalOptimist deleted the DelusionalOptimist/feat/crio-support branch June 13, 2023 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support for CRIO container runtime
5 participants