feat: add aggregated clusterrole #3193
Conversation
| - ray.io | ||
| resources: | ||
| - rayjobs | ||
| - rayjobs/status |
There was a problem hiding this comment.
only operator can update/patch status subresource. so viewer can get/list/watch xx/status, while editor not
| metadata: | ||
| name: raycluster-viewer-role | ||
| labels: | ||
| rbac.authorization.k8s.io/aggregate-to-view: "true" |
There was a problem hiding this comment.
according to https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go#L290C1-L291C1, and https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go#L280 view clusterrole will aggregated to edit clusterrole, and edit clusterrole will aggregated to admin clusterrole, so only rbac.authorization.k8s.io/aggregate-to-view is enough.
|
cc @andrewsykim @MortalHappiness @rueian for review |
kubernetes design user-facing roles: view/edit/admin. add aggregated clusterrole to support multi-tenant scenario see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#default-roles-and-role-bindings Signed-off-by: j4ckstraw <[email protected]>
j4ckstraw
force-pushed
the
add-aggregate-labels-to-cluster-role
branch
from
857255b to
96eaed5
Compare
| @@ -1,6 +1,6 @@ | |||
| apiVersion: v2 | |||
| description: A Helm chart for Kubernetes | |||
| name: kuberay-operator | |||
| version: 1.1.0 | |||
| version: 1.1.1 | |||
There was a problem hiding this comment.
Why this line needs to be changed?
There was a problem hiding this comment.
chart has changed, if version not update, users cannot distinguish between new and old versions.
There was a problem hiding this comment.
cc @kevin85421 Do you consider this only a patch version change, or do you think it is better to update the version to 1.2.0 or 2.0.0?
There was a problem hiding this comment.
Oops, I wasn't aware that it's 1.1.0. We should change it to nightly instead. Typically, I only update version in the release branch. I don't know why I updated it before.
Signed-off-by: j4ckstraw <[email protected]>
Signed-off-by: j4ckstraw <[email protected]>
fixed. PTAL @MortalHappiness |
helm-chart/kuberay-operator/templates/ray_rayservice_editor_role.yaml
Outdated
Show resolved
Hide resolved
helm-chart/kuberay-operator/templates/ray_rayjob_viewer_role.yaml
Outdated
Show resolved
Hide resolved
| rules: | ||
| - apiGroups: | ||
| - ray.io | ||
| resources: | ||
| - rayjobs | ||
| - rayjobs/status |
There was a problem hiding this comment.
Why grant additional list and watch permissions for rayjobs/status?
There was a problem hiding this comment.
I just refer to kubernetes default clusterrole system:aggregate-to-view https://github.com/kubernetes/kubernetes/blob/b4c6895d0b0a913e3461bdc78358aa9514604b8f/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go#L111, it grant */status to view clusterrole. If rayxxx/status is not appropriate to be here, I can remove it.
There was a problem hiding this comment.
cc @kevin85421 Do you think it is okay to grant those additional permissions to status? Personally, I think it is fine to grant them.
There was a problem hiding this comment.
It's fine with me. I plan to revisit all RBAC permissions soon and can decide whether to remove it at that time. We can leave it as is for this PR.
Signed-off-by: j4ckstraw <[email protected]>
kubernetes design user-facing roles: view/edit/admin. add aggregated clusterrole to support multi-tenant scenario
see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#aggregated-clusterroles see https://kubernetes.io/docs/reference/access-authn-authz/rbac/#default-roles-and-role-bindings
Why are these changes needed?
Related issue number
ray-project/kuberay-helm#48
ray-project/kuberay-helm#50
Checks