KIALI-2034 Missing create change on clusterrole.yaml #810
Conversation
|
Note, we are not splitting create/update/patch from UI perspective, canUpdate will give grant to all three verbs. If we need additional granularity we can add it in next iterations. |
business/istio_config.go
Outdated
| @@ -471,5 +473,5 @@ func getUpdateDeletePermissions(k8s kubernetes.IstioClientInterface, namespace, | |||
| log.Errorf("Error getting permissions [namespace: %s, api: %s, resourceType: %s]: %v", namespace, api, resourceType, permErr) | |||
| } | |||
| } | |||
| return canUpdate || canPatch, canDelete | |||
| return canCreate || canUpdate || canPatch, canDelete | |||
There was a problem hiding this comment.
it seems weird to "merge" create permission with update/patch boolean.
How is it used on the frontend? a "create" button is displayed only if we have "create" permission, or is it something different?
There was a problem hiding this comment.
Create/Update are being using in similar scenarios, so it makes sense merge them at the moment. IMO
There was a problem hiding this comment.
ok, but then maybe it should be ANDed rather than ORed, because if you have update rights but not create rights, you should not be able to reach the create screen.
| @@ -458,6 +458,8 @@ func getUpdateDeletePermissions(k8s kubernetes.IstioClientInterface, namespace, | |||
| for _, ssar := range ssars { | |||
| if ssar.Spec.ResourceAttributes != nil { | |||
| switch ssar.Spec.ResourceAttributes.Verb { | |||
| case "create": | |||
There was a problem hiding this comment.
line 456 is missing the "create" argument to k8s.GetSelfSubjectAccessReview.
There was a problem hiding this comment.
ups, true, good catch !
| @@ -78,6 +80,7 @@ rules: | |||
| resources: | |||
| - policies | |||
| verbs: | |||
| - create | |||
There was a problem hiding this comment.
I added these three new "create" perms to my istio branch that I will use when I submit our PR for upgrading istio helm charts when we release kiali v0.14. See https://github.com/istio/istio/compare/master...jmazzitelli:kiali-next-version-changes?expand=1
|
This PR will require some updates on Clusterole comparison (Currently, I am comparing Kiali master to Istio master) but it has a minor priority. Since Istio 1.1 branch is closed, probably we would need send this PR to Istio Master. |
|
@jotak done, I've reviewed the verbs and we query the "update" and "patch" but we only use patch for modifications. |
|
@jotak Finally I splitted the canCreate / canUpdate. It was a minor change after all and perhaps it's more clear now. |
There was a problem hiding this comment.
LGTM
@lucasponce the reason we had that weird stuff with update/patch was that kube's documentation wasn't on page with its actual behaviour. We're really using "patch" but kube's doc, iirc, does only mention the "update" verb not "patch", so we decided to accept both.
|
True, thanks @jotak |
We have added a new endpoint for create operations but the changes on clusterrole.yaml were missed.
This is blocker for the feature.
cc @jmazzitelli @gbaufake as these are another changes in the pipeline for Maistra and Istio upstream.