-
Notifications
You must be signed in to change notification settings - Fork 85
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
Enabled operator installation for karmor #402
Conversation
dd165d5
to
cd2035a
Compare
Snyk issues remediation:-
There are still some pkgs whose fixes are pushed to the main branch but not published yet.
|
Fixes kubearmor/KubeArmor#1256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall code LGTM. Great Work.
I have some enhancement suggestions inline.
Let's fix the conflicts as well.
Can you include screenshots/asciinema video of how the installation and uninstallation process looks like?
cd2035a
to
5b01f5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registry flag is not working
Expected - config images and operator image to be prepended with registry info |
Not working as expected Expected - config images and operator image to have their tags changed |
I couldn't help but notice, even if I am just using save. It takes a lot of time to generate things. Is it a drawback of using helm client? |
I tried printing manifests without running the client and it returns a null pointer. So, the install/upgrade client had to dryrun in order to get the manifests. |
`rootxrishabh@rootxrishabh:~/kubearmor-client$ ./karmor install --legacy=true 🔧 Verifying KubeArmor functionality (this may take upto a minute)...
rootxrishabh@rootxrishabh:~/kubearmor-client$ ./karmor uninstall |
Installation Process in general is smooooth. Great Work. But this does not look cohesive.
|
I think we should handle logs ourselves and suppress helm logs. We could manually output logs for deployments etc. |
Yup. We can list packages and go through that ig. |
I think it will automatically be handled when we have the entire logging figured out for helm. |
We currently dont have --save watching tag and registry flags. I will do it 👍 |
That's concerning. Ideal case it should be not be 2 different approaches. We finalise on the config then decide whether to save it or install it. |
Got it. I will make sure we are handling all flags first. |
Update: I fixed --save not running for clusterless environments and now the process is running client side only, executes without helm logs and is quick. |
The configmap kubearmor-config does get deleted but the operator CR kubearmorconfig-default does not and so for every new installation after this, the CR does not get updated and thus we don't see posture settings supplied through CLI. Inorder to remove the CR(kubearmorconfig-default), we must do |
We can notice it outputs KubeArmorConfig created in a fresh installation. |
We should switch to doing err:= Resource.Create()
if !strings.Contains(err.Error(), "already exists") {
err:= Resource.Update()
return err/nil
} |
I remember us discussing the idea of |
This is a different issue. People won't realise that CR is not updated. the update command was meant to patch things as well patch the helm release if needed. We cannot ask people to run uninstall with force just so that they have a fresh installation. It's a 2 liner change here so I believe it's worth it to handle here. Wdyt? |
Agreed. |
807b29a
to
eb4b432
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One question inline and one nit.
eb4b432
to
fb64253
Compare
48117cc
to
dee6e5f
Compare
dee6e5f
to
4f0c9a0
Compare
Signed-off-by: rootxrishabh <[email protected]>
4f0c9a0
to
53aba13
Compare
Signed-off-by: daemon1024 <[email protected]>
661198e
to
e3df2ed
Compare
Purpose:
This PR shifts karmor install and karmor uninstall to use kubearmor operator using helm.
Does this PR introduce a breaking change?
Yes