Remove unused DisableAuth from kubeops. Add decorator for user perms.

[absoludity]

Description of the change

Removes an unused field from the kubeops handler tests and renamed to clarify the same in tiller-proxy.

EDIT: second commit reverted, so following is not included.
I then got thinking, from an earlier review (see #1450 ) about how we could decorate the handler functions with the permission check, enabling the handler tested separately from the user permission checks etc. rather than the way it is included individually to each handler in tiller-proxy. So I've sketched that out here.

It is landable (if you agree with the direction) as the decorator isn't yet applied, but may not be worth landing unless someone wants to continue it today. I can do the remaining work tomorrow if no one is interested :)

Benefits

Sets up the UserPermissionChecks decorator so that we can have the same detail in the UI as per Andres' "Drawbacks" screenshots at #1450 .

Once generating the manifests via helm3 is added (see the TODO), the decorator should be able to be applied like:

--- a/cmd/kubeops/main.go
+++ b/cmd/kubeops/main.go
@@ -71,7 +71,7 @@ func main() {
        addRoute := handler.AddRouteWith(r.PathPrefix("/v1").Subrouter(), withHandlerConfig)
        addRoute("GET", "/releases", handler.ListAllReleases)
        addRoute("GET", "/namespaces/{namespace}/releases", handler.ListReleases)
-       addRoute("POST", "/namespaces/{namespace}/releases", handler.CreateRelease)
+       addRoute("POST", "/namespaces/{namespace}/releases", handler.UserPermissionChecks(handler.CreateRelease, "create"))
        addRoute("GET", "/namespaces/{namespace}/releases/{releaseName}", handler.GetRelease)
        addRoute("PUT", "/namespaces/{namespace}/releases/{releaseName}", handler.OperateRelease)
        addRoute("DELETE", "/namespaces/{namespace}/releases/{releaseName}", handler.DeleteRelease)

Applicable issues

Related to comment at #1450 (review)

cc @latiif @SimonAlling

[andresmgot]

You fell in the trap of adding different things to the same PR :P

The changes for removing the DisableAuth setting for kubeops LGTM. Approving this PR in case you want to merge that part.

I have some concerns though about the decorator. Let me elaborate:

• The auth checker is something that was mandatory when working with Helm 2. Now that with Helm 3 we are using the user serviceaccount it should not be necessary. That obviously means less code for us to maintain (even though the code is already written, we would still need to apply fixes).
• We may mask the real error. For example, when installing something without write permissions, Helm returns the error that the user cannot create secrets in the namespace (because the release info is stored as a secret) but we would return the list of elements related to the chart. We could fix that and also check if the user can create secrets, but then it's also possible to use other storage mechanisms like ConfigMaps or in memory, that would complicate our code more. If Helm changes the behavior we would need to adapt to them. As a summary, it would be more prone to errors.
• In addition to the second point, I haven't checked yet but maybe if the user has permissions to create secrets, Helm 3 already returns all the permissions that the user need to deploy the chart, I will investigate that. If that is the case, it's just a matter from us to prettify the result from Helm.

[absoludity]

You fell in the trap of adding different things to the same PR :P

I did... I thought I'd just try investigating bit further, then invested enough time for the sunk cost fallacy to kick in :)

The changes for removing the DisableAuth setting for kubeops LGTM. Approving this PR in case you want to merge that part.

Great, will do. Reverting the extra commit, but comments below.

I have some concerns though about the decorator. Let me elaborate:

* The auth checker is something that was mandatory when working with Helm 2. Now that with Helm 3 we are using the user serviceaccount it _should_ not be necessary.

Correct, it's not necessary, but...

That obviously means less code for us to maintain (even though the code is already written, we would still need to apply fixes).

the question is whether the benefit of the clean feedback to the user, which we already have, is worth keeping.

* We may mask the real error. For example, when installing something without write permissions, Helm returns the error that the user cannot create `secrets` in the namespace (because the release info is stored as a `secret`) but we would return the list of elements related to the chart.

If the list of elements related to that chart require write access, that would not be masking the real error, but rather not showing all the errors, I think? Once RBAC is updated with correct perms for those other resources, you'd still see the helm error about being unable to create the secret (if we didn't also check that, which I think we would as you've mentioned).

We could fix that and also check if the user can create secrets, but then it's also possible to use other storage mechanisms like ConfigMaps or in memory, that would complicate our code more.

I don't think it would. It is Kubeapps that needs to configure the driver when interacting with Helm, so it's trivial to add that resource (ie. Secret or ConfigMap currently).

If Helm changes the behavior we would need to adapt to them. As a summary, it would be more prone to errors.

The only behaviour Helm could change which would affect us is if they changed the default driver and we were using the default (which we do currently), which is both an unlikely decision for helm and easy to change at our end (we'd just be explicit with the driver, and updated when we decide).

So yes, it's a little more code (which is already written for the helm2 work) to maintain, I honestly think it's just whether the detailed required RBAC perms is a benefit to keep.

* In addition to the second point, I haven't checked yet but maybe if the user has permissions to create secrets, Helm 3 already returns all the permissions that the user need to deploy the chart, I will investigate that. If that is the case, it's just a matter from us to prettify the result from Helm.

Nice - that would be better, letting helm do the work.

[andresmgot]

Let me investigate the error message and check if Helm already does the work for us. I will open an issue/PR with more info.