RoadieHQ / kubewise

Get Helm notifications in your team chat
Apache License 2.0
59 stars 6 forks source link

Fixes #47 Make clusterRoleBinding optional #48

Closed TJM closed 4 years ago

TJM commented 4 years ago

This simply allows deployment into environments where we are unable to create a clusterRole or clusterRoleBinding due to access restrictions (we are limited to a single namespace).

dtuite commented 4 years ago

Hey @TJM.

Pardon my ignorance, but what results would you expect when you install kubewise with either clusterRole.create=false or rbac.create=false. I tried it just now, and if I set either property to false then I get no notifications at all, even if I'm installing charts into the same namespace as kubewise is installed. Are you seeing different results?

TJM commented 4 years ago

@dtuite I probably submitted the PR a bit early ;) ... we got it to install and thought we were good to go. It should be reporting only on its current namespace. Needs more science, perhaps?

dtuite commented 4 years ago

Ok. Play around with it a bit more and report back please. If you get it working for yourselves I'm happy to merge the patch because it's unlikely to break anything. It's just that, based on my small bit of research, it doesn't do anything useful either 😂

I'm not exactly an expert on kubernetes rbac and clusterRoleBindings so I look forward to being corrected.

TJM commented 4 years ago

Actually, it does work as designed, as per the screenshots in #49 (I have some more notifications, but they are "internal" things)

The problem we are trying to solve is that we have multiple "customers" (dev, qa, stage, uat, etc) each separated by namespace. They are limited to only their namespace and cannot create global resources (clusterRole, ClusterRoleBinding) as we don't want them creating resources or accessing data outside their namespace. In order to get kubewise notifications, we deploy kubewise directly into their namespace with this change. Kubewise is only able to see changes within its own namespace due to the rolebinding being namespace bound.

Additionally, we want to have the notifications in one namespace (dev) going to one channel, while notifications from another namespace (qa) going to their channel, so the best way we could come up with to do that was embedding kubewise directly in their namespace. That way each of the "customers" can direct notifications to their own channel.

~tommy

TJM commented 4 years ago

In order to "test" this, you have to deploy an update to kubewise itself or another chart in the same namespace as kubewise.

dtuite commented 4 years ago

Ok. Thanks for following up Tommy. I must have messed something up when testing it. I was rushing. I'll take another look and merge it.

dtuite commented 4 years ago

Right, I tested this again today at a more relaxed pace. My findings are the same though. I wonder what is different about your cluster that you get different results.

I'm using Slack, but that shouldn't make a difference. It's the same code which detects the installs and upgrades.

My test is basically like this:

kubectl create namespace my-namespace
helm install kubewise -n my-namespace ./helm_chart --set slack.token="<token>" --set image.tag="0.11.1" --set clusterRole.create=falsew --set rbac.create=true
helm install flux fluxcd/flux --set git.url=git@github.com:fluxcd/flux-get-started --namespace my-namespace --wait --timeout 60
helm upgrade flux fluxcd/flux -n flux --set git.url=git@github.com:fluxcd/flux-get-started9 --set myValue=red --wait --timeout 60s -n my-namespace
helm uninstall flux -n my-namespace
helm uninstall kubewise -n my-namespace

Here are the results:

--set rbac.create= --set clusterRole.create= Install result Upgrade result
false true No notification No notification
true false No notification No notification
false false No notification No notification
true true Notification Notification
TJM commented 4 years ago

Interesting... perhaps this is where test driven development would have helped ;)

Here are the vague test descriptions:

dtuite commented 4 years ago

We also set namespaceToWatch: my-namespace. Perhaps that changes the detection code?

This is it. I just tested this and you can set clusterRole.create=false and still get notifications for one namespace as long as you set namespaceToWatch to some value.

dtuite commented 4 years ago

Thanks for the pull request @TJM and team.

I will let you know when I release a new version with this change.

TJM commented 4 years ago

We also set namespaceToWatch: my-namespace. Perhaps that changes the detection code?

This is it. I just tested this and you can set clusterRole.create=false and still get notifications for one namespace as long as you set namespaceToWatch to some value.

Thanks, I guess we better note that in the readme, since you already merged, I will leave that part up to you :p

dtuite commented 4 years ago

@TJM this is released in 0.11.8.

Thanks for the help.

TJM commented 4 years ago

Thanks @dtuite ... side note, as this is a "new feature" and not a "patch" it should have probably been released with 0.12.0, strictly speaking. Although I think you can play fast and loose with SymVer prior to 1.0.0 ;)

dtuite commented 4 years ago

Yeah. I agree. Releasing is a bit of a mess at the moment and I opted to get the release out under a non-optimal version number just so it was done. It's going to take a bit more work to come up with a nice release flow. It will settle down in time.