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

Enabled operator installation for karmor #402

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

rootxrishabh
Copy link
Member

Purpose:
This PR shifts karmor install and karmor uninstall to use kubearmor operator using helm.

Does this PR introduce a breaking change?
Yes

@rootxrishabh rootxrishabh force-pushed the karmorOperator branch 2 times, most recently from dd165d5 to cd2035a Compare February 8, 2024 20:53
@Aryan-sharma11
Copy link
Member

Snyk issues remediation:-

  • Upgrade github.com/opencontainers/runc/libcontainer to version 1.1.12 or higher. ( and all pkgs related to it)
  • Upgrade github.com/lestrrat-go/jwx/v2/jws to version 2.0.19 or higher.
  • Upgrade go.etcd.io/etcd/v3/server/lease to version 3.4.29, 3.5.11 or higher
  • Upgrade go.etcd.io/etcd/v3/server/etcdserver to version 3.4.29, 3.5.11 or higher.

There are still some pkgs whose fixes are pushed to the main branch but not published yet.

@daemon1024
Copy link
Member

Fixes kubearmor/KubeArmor#1256

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.

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?

cmd/uninstall.go Outdated Show resolved Hide resolved
cmd/uninstall.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
cmd/uninstall.go Show resolved Hide resolved
cmd/uninstall.go Outdated Show resolved Hide resolved
@daemon1024
Copy link
Member

karmor install --save failing if no kubernetes cluster is configured
We do not mandate K8s cluster if we only need to save

image

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.

I don't think the first log is looking nice, is it something we need to keep?

image

@daemon1024
Copy link
Member

daemon1024 commented Feb 13, 2024

Registry flag is not working

./karmor install -r "ttl.sh" --save

Expected - config images and operator image to be prepended with registry info
Actual - No change in config

@daemon1024
Copy link
Member

./karmor install --tag=v1.2.1 --save

Not working as expected

Expected - config images and operator image to have their tags changed
Actual - No change in config

@daemon1024
Copy link
Member

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?

@rootxrishabh
Copy link
Member Author

rootxrishabh commented Feb 13, 2024

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.
Taking a look at other points 👀

@rootxrishabh
Copy link
Member Author

rootxrishabh commented Feb 13, 2024

Have we verified this behaviour with a legacy karmor install? Please include screenshots

`rootxrishabh@rootxrishabh:~/kubearmor-client$ ./karmor install --legacy=true
😄 Environment : k3s
🔥 CRD kubearmorpolicies.security.kubearmor.com
ℹ️ CRD kubearmorpolicies.security.kubearmor.com already exists
🔥 CRD kubearmorhostpolicies.security.kubearmor.com
ℹ️ CRD kubearmorhostpolicies.security.kubearmor.com already exists
💫 Service Account
💫 Service Account
⚙️ Cluster Role
ℹ️ Cluster Role already exists
⚙️ Cluster Role Bindings
ℹ️ Cluster Role Bindings already exists
⚙️ KubeArmor Relay Roles
🛡 KubeArmor Relay Service
🛰 KubeArmor Relay Deployment
🛡 KubeArmor DaemonSet - Init kubearmor/kubearmor-init:stable, Container kubearmor/kubearmor:stable -gRPC=32767
ℹ️ KubeArmor DaemonSet already exists
🛡 KubeArmor Controller TLS certificates
💫 KubeArmor Controller Service Account
⚙️ KubeArmor Controller Roles
🚀 KubeArmor Controller Deployment
🚀 KubeArmor Controller Metrics Service
🚀 KubeArmor Controller Webhook Service
🥳 Done Installing KubeArmor
🤩 KubeArmor Controller Mutation Admission Registration
ℹ️ KubeArmor Controller Mutation Admission Registration already exists 88%
🚀 KubeArmor ConfigMap Creation 88%
😋 Checking if KubeArmor pods are running...▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇] 111.76%
🥳 Done Checking , ALL Services are running!
⌚️ Execution Time : 13.858858021s

🔧 Verifying KubeArmor functionality (this may take upto a minute)...

    🛡️       Your Cluster is Armored Up! 

rootxrishabh@rootxrishabh:~/kubearmor-client$ ./karmor uninstall
❌ KubeArmor is either not installed, or the specified namespace is incorrect.
ℹ️ Please ensure you have installed KubeArmor, and check that you are specifying the correct namespace.
ℹ️ Attempting legacy uninstallation.
🗑️ Mutation Admission Registration
🗑️ KubeArmor Services
❌ Service: kubearmor removed
❌ Service: kubearmor-controller-metrics-service removed
💨 Service Accounts
❌ ServiceAccount kubearmor removed
❌ ServiceAccount kubearmor-relay removed
❌ ServiceAccount kubearmor-controller removed
💨 Cluster Roles
❌ ClusterRole kubearmor-snitch removed
❌ ClusterRole kubearmor-clusterrole removed
❌ ClusterRole kubearmor-relay-clusterrole removed
❌ ClusterRole kubearmor-controller-proxy-role removed
❌ ClusterRole kubearmor-controller-clusterrole removed
💨 Cluster Role Bindings
❌ ClusterRoleBinding kubearmor-clusterrolebinding removed
❌ ClusterRoleBinding kubearmor-relay-clusterrolebinding removed
❌ ClusterRoleBinding kubearmor-controller-clusterrolebinding removed
❌ ClusterRoleBinding kubearmor-controller-proxy-rolebinding removed
❌ ClusterRoleBinding kubearmor-snitch-binding removed
🧹 Roles
❌ Role kubearmor-controller-leader-election-role removed
🧹 RoleBindings
❌ RoleBinding kubearmor-controller-leader-election-rolebinding removed
👻 KubeArmor Controller TLS certificates
❌ KubeArmor Controller TLS certificate kubearmor-controller-webhook-server-cert removed
👻 KubeArmor ConfigMap
❌ ConfigMap kubearmor-config removed
👻 KubeArmor DaemonSet
❌ KubeArmor DaemonSet kubearmor removed
👻 KubeArmor Deployments
❌ KubeArmor Deployment kubearmor-relay removed
❌ KubeArmor Deployment kubearmor-controller removed
🔄 Checking if KubeArmor pods are stopped...
🔴 Done Checking; all services are stopped!
⌚️ Termination Time: 9.787902842s `
Yes

@daemon1024
Copy link
Member

image

Installation Process in general is smooooth. Great Work.

But this does not look cohesive.

  • the helm logs and emoji logs are not streamlines, We should suppress helm logs and manually log based on the output maybe. Not sure what's best here.
  • We are spamming Deployment is not ready messages
  • We need to type something when KubeArmor Daemonset is deploying, Because it takes a while, including messaging around expected time, inform that snitch is running

@daemon1024
Copy link
Member

karmor uninstall

This is not working as expected
I did the standard helm install
But it is saying it cannot detect it
image
It fellback to legacy uninstallation, but my helm package still exists

@rootxrishabh
Copy link
Member Author

image

Installation Process in general is smooooth. Great Work.

But this does not look cohesive.

* the helm logs and emoji logs are not streamlines, We should suppress helm logs and manually log based on the output maybe. Not sure what's best here.

* We are spamming `Deployment is not ready` messages

* We need to type something when KubeArmor Daemonset is deploying, Because it takes a while, including messaging around expected time, inform that snitch is running

I think we should handle logs ourselves and suppress helm logs. We could manually output logs for deployments etc.
I will add deployment message for snitch and include an expected time logs.

@rootxrishabh
Copy link
Member Author

karmor uninstall

This is not working as expected I did the standard helm install But it is saying it cannot detect it image It fellback to legacy uninstallation, but my helm package still exists

Yes, I was thinking of discussing this earlier. If we remove the default namespace in uninstall and don't specify namespace for example "kubearmor" then our current helm implementation doesn't automatically detect the installed namespace. I guess implementing auto detection of namespace is required?

@daemon1024
Copy link
Member

I guess implementing auto detection of namespace is required?

Yup. We can list packages and go through that ig.

@rootxrishabh
Copy link
Member Author

rootxrishabh commented Feb 13, 2024

I don't think the first log is looking nice, is it something we need to keep?

image

It is not necessary, just denoting the initiation of helm client.
Should we suppress logs for save flag?

@daemon1024
Copy link
Member

I think it will automatically be handled when we have the entire logging figured out for helm.

@rootxrishabh
Copy link
Member Author

./karmor install --tag=v1.2.1 --save

Not working as expected

Expected - config images and operator image to have their tags changed Actual - No change in config

We currently dont have --save watching tag and registry flags. I will do it 👍

@daemon1024
Copy link
Member

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.

@rootxrishabh
Copy link
Member Author

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.

@rootxrishabh
Copy link
Member Author

rootxrishabh commented Feb 13, 2024

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?

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.

@rootxrishabh
Copy link
Member Author

Here's a demo of the functionality -
kubearmorgif

@daemon1024
Copy link
Member

We should be updating the operator image as well when we use tag/flags/registry
image

operator image is not changing with any flags. We might need to impement a new flag to update operator image. I believe the helm chart accepts updates to operator image as well

@rootxrishabh
Copy link
Member Author

I see that the kubearmorconfig is not being recreated. You need to do karmor uninstall --force to remove the CR and then reinstall with the posture settings.

Is there any way to get the configmap recreated, imo it's a better way. Maybe remove the configmap after uninstalling

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 karmor uninstall --force and do a fresh installation.
Ref

@rootxrishabh
Copy link
Member Author

I see that the kubearmorconfig is not being recreated. You need to do karmor uninstall --force to remove the CR and then reinstall with the posture settings.

Is there any way to get the configmap recreated, imo it's a better way. Maybe remove the configmap after uninstalling

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 karmor uninstall --force and do a fresh installation. Ref

We can notice it outputs KubeArmorConfig created in a fresh installation.
image

@daemon1024
Copy link
Member

If I do a kubectl apply -f with the update config, it does update existing config if there are changes. I believe the same behaviour should translate here

image

@daemon1024
Copy link
Member

daemon1024 commented Mar 6, 2024

We should switch to doing

err:= Resource.Create()
if !strings.Contains(err.Error(), "already exists") {
		err:= Resource.Update()
		return err/nil
}

@rootxrishabh
Copy link
Member Author

If I do a kubectl apply -f with the update config, it does update existing config if there are changes. I believe the same behaviour should translate here

image

I remember us discussing the idea of karmor update. I think we discussed that we would update all operator CR options separately using the update command, it can be handled in a separate issue. Wdyt?

@daemon1024
Copy link
Member

I remember us discussing the idea of karmor update. I think we discussed that we would update all operator CR options separately using the update command, it can be handled in a separate issue. Wdyt?

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?

@rootxrishabh
Copy link
Member Author

rootxrishabh commented Mar 6, 2024

I remember us discussing the idea of karmor update. I think we discussed that we would update all operator CR options separately using the update command, it can be handled in a separate issue. Wdyt?

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. --force uninstallation shouldn't be an intermediatory to update the CR. I will implement the CR updation here.

@daemon1024 daemon1024 requested a review from PrimalPimmy March 7, 2024 15:15
install/install.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
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. One question inline and one nit.

install/install.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
cmd/install.go Outdated Show resolved Hide resolved
cmd/install.go Show resolved Hide resolved
@rootxrishabh rootxrishabh force-pushed the karmorOperator branch 3 times, most recently from 48117cc to dee6e5f Compare March 13, 2024 16:56
go.mod Outdated Show resolved Hide resolved
@daemon1024 daemon1024 merged commit 8cc3935 into kubearmor:main Mar 14, 2024
9 of 10 checks passed
@rootxrishabh rootxrishabh deleted the karmorOperator branch March 14, 2024 20:24
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.

5 participants