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 in cluster tests + fix synk vuln SNYK-CC-K8S-8 #762

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add in cluster tests + fix synk vuln SNYK-CC-K8S-8 #762

wants to merge 1 commit into from

Conversation

achrefbensaad
Copy link
Member

@achrefbensaad achrefbensaad commented Jul 8, 2022 •
edited
Loading

What does this PR do:

  • fixes synk vuln SNYK-CC-K8S-8 (enable readonly file system) mentioned here snyk test found some issues in kubearmor deployment #733 point 11
  • Impliments new ci tests inside a kubernetes cluster
  • updates ci to include cmctl to avoid random ci cashe due to delayed cert manager CA cert injection

Why the new in cluster tests:

already existant tests test kubearmor functionalities but not its related yaml files. In order for us to properly determine that our hardening actions wont impact kubearmor inside kubernetes environment we need to run tests in kubernetes cluster rather than as binary at the host level.

Signed-off-by: Achref ben saad [email protected]

@achrefbensaad achrefbensaad force-pushed the add-incluster-tests branch 2 times, most recently from 1977832 to 9eefd3a Compare July 8, 2022 05:01
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022 •
edited
Loading

Codecov Report

Merging #762 (d003c93) into main (d6fef36) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #762   +/-   ##
=======================================
  Coverage   40.33%   40.33%           
=======================================
  Files          29       29           
  Lines        9318     9318           
=======================================
  Hits         3758     3758           
  Misses       5073     5073           
  Partials      487      487

@achrefbensaad achrefbensaad mentioned this pull request Jul 8, 2022
Open
@achrefbensaad achrefbensaad force-pushed the add-incluster-tests branch 5 times, most recently from f4ff064 to a43dfe2 Compare July 11, 2022 09:10
daemon1024
daemon1024 requested changes Jul 11, 2022
View reviewed changes
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.

Since the tests seem to be passing in general, We should be good to go ahead with this after a few modifications 😄

Thanks a lot.

.github/workflows/ci-test-incluster.yml Show resolved Hide resolved
deployments/generic/kubearmor.yaml Outdated Show resolved Hide resolved
@achrefbensaad achrefbensaad requested a review from daemon1024 July 18, 2022 05:23
@nyrahul
Copy link
Contributor

nyrahul commented Jul 18, 2022

Hey @achrefbensaad , Thanks for taking this up.
This morning there was another issue reported by Synk. Can you handle this as well as part of this PR? Thanks

@achrefbensaad
Copy link
Member Author

@nyrahul we cannot bump gopkg.in/yaml.v2 version to gopkg.in/yaml.v3 as it is an indirect dependency of sigs.k8s.io/yaml.

The latest version of sigs.k8s.io/yaml still uses the v2. From what I can see their repo is not active and I dont think that they will update it.
too many rotten PR's and issues.
kubernetes-sigs/yaml#26
kubernetes-sigs/yaml#18
kubernetes-sigs/yaml#61

daemon1024
daemon1024 reviewed Jul 18, 2022
View reviewed changes
.github/workflows/ci-test-incluster.yml Show resolved Hide resolved
KubeArmor/build/kubearmor-test-crio.yaml
@@ -57,6 +57,7 @@ spec:
- containerPort: 32767
securityContext:
privileged: true
readOnlyRootFilesystem: true
terminationMessagePath: /dev/termination-log
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what is the termination log for, but would readonlyfilesystem impact that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked, it does not impact it.

you can check in the pod by :
$ echo "random_string" >> /dev/termination-log
$ cat/dev/termination-log should output random_string

@achrefbensaad achrefbensaad force-pushed the add-incluster-tests branch 3 times, most recently from bcb9271 to 9db4a5e Compare July 20, 2022 20:53
@achrefbensaad
Copy link
Member Author

@daemon1024 can you please re-review this as if it is a new PR. I had to redo it again due to many absurd merge conflicts

@achrefbensaad
add readonly fs
d003c93 
Signed-off-by: achref ben saad <[email protected]>
@nyrahul nyrahul removed their request for review January 4, 2023 05:30
@DelusionalOptimist
Copy link
Member

Hey @achrefbensaad what parts of this PR are still relevant?

  • Our test suite runs in cluster currently.
  • Cert manager is no longer used.
  • Is the snyk vuln. still a concern?

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

@daemon1024 daemon1024 daemon1024 requested changes

@nam-jaehyun nam-jaehyun Awaiting requested review from nam-jaehyun

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@achrefbensaad @codecov-commenter @nyrahul @DelusionalOptimist @daemon1024