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 |
j4ckstraw
force-pushed
the
add-aggregate-labels-to-cluster-role
branch
from
857255b to
96eaed5
Compare
MortalHappiness
left a comment
MortalHappiness
left a comment
There was a problem hiding this comment.
Also, please fix the CI errors.
| 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.
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
| - 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.
Future-Outlier
left a comment
Future-Outlier
left a comment
There was a problem hiding this comment.
Hi, @j4ckstraw sorry for the late review, do you mind solve the merge conflict and ping me to do a final pass, thank you!
j4ckstraw
force-pushed
the
add-aggregate-labels-to-cluster-role
branch
from
1e64622 to
fb4f98f
Compare
kubernetes design user-facing roles: view/edit/admin. add aggregated clusterrole to support multi-tenant scenario NOTE: editor imply viewer 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
fb4f98f to
e185cdd
Compare
Signed-off-by: Future-Outlier <[email protected]>
Future-Outlier
left a comment
Future-Outlier
left a comment
There was a problem hiding this comment.
cc @machichima @win5923 to do final pass, thank you!!
helm-chart/kuberay-operator/templates/ray_raycluster_editor_role.yaml
Outdated
Show resolved
Hide resolved
helm-chart/kuberay-operator/templates/ray_rayjob_editor_role.yaml
Outdated
Show resolved
Hide resolved
helm-chart/kuberay-operator/templates/ray_rayservice_editor_role.yaml
Outdated
Show resolved
Hide resolved
…le.yaml Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
…le.yaml Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
…le.yaml Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
Co-authored-by: Nary Yeh <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
Co-authored-by: 江家瑋 <[email protected]> Signed-off-by: j4ckstraw <[email protected]>
|
@machichima @win5923 thank you very much. Every line matters 🫡 |
9 participants
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