Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Watch CR in multiple namespaces with namespaced RBAC resources #1106

Merged
merged 9 commits into from
Jun 5, 2023
Merged

[Feature] Watch CR in multiple namespaces with namespaced RBAC resources #1106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5fd1446
watch multiple namespaces
kevin85421 May 23, 2023
6e27ff4
update
kevin85421 May 23, 2023
853026b
update
kevin85421 May 23, 2023
b04ce3d
rbac consistency
kevin85421 May 23, 2023
41fedd0
update
kevin85421 May 23, 2023
28cb9b9
update
kevin85421 May 23, 2023
d4f183c
update
kevin85421 May 23, 2023
ecba427
update
kevin85421 May 23, 2023
f209400
add comment
kevin85421 Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions helm-chart/kuberay-operator/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ spec:
{{- $argList = append $argList "--enable-batch-scheduler" -}}
{{- end -}}
{{- $watchNamespace := "" -}}
{{- if .Values.singleNamespaceInstall -}}
{{- if and .Values.singleNamespaceInstall (not .Values.watchNamespace) -}}
{{- $watchNamespace = .Release.Namespace -}}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case 1: If singleNamespaceInstall is set to true and watchNamespace is not specified, the KubeRay operator will only watch the namespace where the operator is deployed.

Case 2: If watchNamespace is specified, the KubeRay operator will watch all namespaces listed in the variable. In this case, we concatenate all namespaces into a string separated by commas. Then, the string will be parsed by strings.Split(watchNamespace, ",") in main.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this, it looks likesingleNamespaceInstall and watchNamespace are existing fields in KubeRay. The new behavior for both is clearly explained by Cases 1-4. The old behavior for singleNamespaceInstall is also described. But I think the old behavior of watchNamespace is missing from the PR description, do you mind adding it? (Maybe the only change is that it now supports multiple namespaces?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check, are commas , forbidden characters in namespaces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think the old behavior of watchNamespace is missing from the PR description, do you mind adding it?

The behavior is described by the comments in values.yaml. The comments are deleted by this PR.

# kuberay operator will only watch the resource events from the "watchNamespace" namespace.
# this option has no effect if singleNamespaceInstall is true, because we assume there are no
# permissions outside of the current namespace
# watchNamespace: ray-user-namespace

Just to double check, are commas , forbidden characters in namespaces?

The flag --watch-namespace is a list of namespaces, separated by commas. It will be parsed by watchNamespaces := strings.Split(watchNamespace, ",") in main.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but if a namespace is allowed to have a comma in it then breaking by commas won't work. Anyways I checked and it looks like namespaces can only have alphanumeric characters, so it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but if a namespace is allowed to have a comma in it then breaking by commas won't work.

Oh, I understand now. I missed that part. Thank you for pointing it out!

{{- else if .Values.watchNamespace -}}
{{- $watchNamespace = .Values.watchNamespace -}}
{{- $watchNamespace = join "," .Values.watchNamespace -}}
{{- end -}}
{{- if $watchNamespace -}}
{{- $argList = append $argList "--watch-namespace" -}}
Expand Down
12 changes: 10 additions & 2 deletions helm-chart/kuberay-operator/templates/leader_election_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
{{ include "kuberay-operator.labels" . | indent 4 }}
labels: {{ include "kuberay-operator.labels" . | nindent 4 }}
name: {{ include "kuberay-operator.fullname" . }}-leader-election
rules:
- apiGroups:
Expand Down Expand Up @@ -32,4 +31,13 @@ rules:
- events
verbs:
- create
- apiGroups:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary for leader election.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education, how did you notice that this was necessary? (Did some test fail?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to skip the installation of role.yaml, but it resulted in the failure of the leader election due to permission issues. Then, I found leader_election_role.yaml.

- coordination.k8s.io
resources:
- leases
verbs:
- create
- get
- list
- update
{{- end }}
3 changes: 1 addition & 2 deletions helm-chart/kuberay-operator/templates/leader_election_role_binding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
{{ include "kuberay-operator.labels" . | indent 4 }}
labels: {{ include "kuberay-operator.labels" . | nindent 4 }}
name: {{ include "kuberay-operator.fullname" . }}-leader-election
subjects:
- kind: ServiceAccount
Expand Down
240 changes: 240 additions & 0 deletions helm-chart/kuberay-operator/templates/multiple_namespaces_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,240 @@
# Install Role for namespaces listed in watchNamespace.
# This should be consistent with `role.yaml`, except for the `kind` field.
{{- if and .Values.rbacEnable .Values.singleNamespaceInstall }}
{{- $watchNamespaces := default (list .Release.Namespace) .Values.watchNamespace }}
{{- range $namespace := $watchNamespaces }}
---
kind: Role
Copy link
Contributor

@Yicheng-Lu-llll Yicheng-Lu-llll Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could add thescenario: singleNamespaceInstall is set to false or not set here. So that there is no need to synchronize multiple_namespaces_role.yaml and role.yaml (same as rolebinding)

More specifically, when singleNamespaceInstall is false:

  • set $watchNamespaces to a placeholder value (so that only iterates once)
  • create a ClusterRole instead of a Role.
  • omit the metadata.namespace field

Copy link
Member Author

@kevin85421 kevin85421 Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I intended to achieve this by updating role.yaml and rolebinding.yaml instead of creating new files (multi_namespaces_*.yaml). However, it can be extremely challenging or even impossible (?) to achieve that. The Helm parser is not smart enough.

apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels: {{ include "kuberay-operator.labels" $ | nindent 4 }}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a range block, . no longer refers to the top level scope, so use $ to access the top-level variables.

name: {{ include "kuberay-operator.fullname" $ }}
namespace: {{ $namespace }}
rules:
- apiGroups:
- coordination.k8s.io
resources:
- leases
verbs:
- create
- get
- list
- update
- apiGroups:
- ""
resources:
- events
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- pods
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- pods/status
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- serviceaccounts
verbs:
- create
- delete
- get
- list
- watch
- apiGroups:
- ""
resources:
- services
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- services/status
verbs:
- get
- patch
- update
- apiGroups:
- extensions
resources:
- ingresses
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- networking.k8s.io
resources:
- ingressclasses
verbs:
- get
- list
- watch
- apiGroups:
- networking.k8s.io
resources:
- ingresses
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ray.io
resources:
- rayclusters
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ray.io
resources:
- rayclusters/finalizers
verbs:
- update
- apiGroups:
- ray.io
resources:
- rayclusters/status
verbs:
- get
- patch
- update
- apiGroups:
- ray.io
resources:
- rayjobs
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ray.io
resources:
- rayjobs/finalizers
verbs:
- update
- apiGroups:
- ray.io
resources:
- rayjobs/status
verbs:
- get
- patch
- update
- apiGroups:
- ray.io
resources:
- rayservices
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ray.io
resources:
- rayservices/finalizers
verbs:
- update
- apiGroups:
- ray.io
resources:
- rayservices/status
verbs:
- get
- patch
- update
- apiGroups:
- rbac.authorization.k8s.io
resources:
- rolebindings
verbs:
- create
- delete
- get
- list
- watch
- apiGroups:
- rbac.authorization.k8s.io
resources:
- roles
verbs:
- create
- delete
- get
- list
- update
- watch
{{- if $.Values.batchScheduler.enabled }}
- apiGroups:
- scheduling.volcano.sh
resources:
- podgroups
verbs:
- create
- delete
- get
- list
- update
- watch
- apiGroups:
- apiextensions.k8s.io
resources:
- customresourcedefinitions
verbs:
- get
{{- end }}
{{- end }}
{{- end }}
22 changes: 22 additions & 0 deletions helm-chart/kuberay-operator/templates/multiple_namespaces_rolebinding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Install RoleBinding for namespaces listed in watchNamespace.
# This should be consistent with `rolebinding.yaml`, except for the `kind` field.
{{- if and .Values.rbacEnable .Values.singleNamespaceInstall }}
{{- $watchNamespaces := default (list .Release.Namespace) .Values.watchNamespace }}
{{- range $namespace := $watchNamespaces }}
---
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels: {{ include "kuberay-operator.labels" $ | nindent 4 }}
name: {{ include "kuberay-operator.fullname" $ }}
namespace: {{ $namespace }}
subjects:
- kind: ServiceAccount
name: {{ $.Values.serviceAccount.name }}
namespace: {{ $.Release.Namespace }}
roleRef:
kind: Role
name: {{ include "kuberay-operator.fullname" $ }}
apiGroup: rbac.authorization.k8s.io
{{- end }}
{{- end }}
9 changes: 2 additions & 7 deletions helm-chart/kuberay-operator/templates/ray_rayjob_editor_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
# permissions for end users to edit rayjobs.
{{- if .Values.rbacEnable }}
{{- if and .Values.rbacEnable (not .Values.singleNamespaceInstall) }}

{{- if .Values.singleNamespaceInstall }}
kind: Role
{{- else }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the effect of this change? It looks like now we don't create these roles if singleNamespaceInstall is false, why don't we need them anymore in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Is it because we're replacing these roles with the new multiple_namespaces_role?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the effect of this change? It looks like now we don't create these roles if singleNamespaceInstall is false, why don't we need them anymore in this case?

The permissions of this file are a subset of those in role.yaml. Therefore, KubeRay operator does not need it before/after this PR. In my understanding, there is no RoleBinding to link these Roles with any ServiceAccount. These RBAC YAML files are generated by kubebuilder. I guess they are used to define users' access.

kind: ClusterRole
{{- end }}
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
{{ include "kuberay-operator.labels" . | indent 4 }}
labels: {{ include "kuberay-operator.labels" . | nindent 4 }}
name: rayjob-editor-role
rules:
- apiGroups:
Expand Down
9 changes: 2 additions & 7 deletions helm-chart/kuberay-operator/templates/ray_rayjob_viewer_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
# permissions for end users to view rayjobs.
{{- if .Values.rbacEnable }}
{{- if and .Values.rbacEnable (not .Values.singleNamespaceInstall) }}

{{- if .Values.singleNamespaceInstall }}
kind: Role
{{- else }}
kind: ClusterRole
{{- end }}
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
{{ include "kuberay-operator.labels" . | indent 4 }}
labels: {{ include "kuberay-operator.labels" . | nindent 4 }}
name: rayjob-viewer-role
rules:
- apiGroups:
Expand Down
7 changes: 1 addition & 6 deletions helm-chart/kuberay-operator/templates/ray_rayservice_editor_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
# permissions for end users to edit rayservices.
{{- if .Values.rbacEnable }}
{{- if and .Values.rbacEnable (not .Values.singleNamespaceInstall) }}
apiVersion: rbac.authorization.k8s.io/v1

{{- if .Values.singleNamespaceInstall }}
kind: Role
{{- else }}
kind: ClusterRole
{{- end }}
metadata:
name: rayservice-editor-role
rules:
Expand Down
7 changes: 1 addition & 6 deletions helm-chart/kuberay-operator/templates/ray_rayservice_viewer_role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
# permissions for end users to view rayservices.
{{- if .Values.rbacEnable }}
{{- if and .Values.rbacEnable (not .Values.singleNamespaceInstall) }}
apiVersion: rbac.authorization.k8s.io/v1

{{- if .Values.singleNamespaceInstall }}
kind: Role
{{- else }}
kind: ClusterRole
{{- end }}
metadata:
name: rayservice-viewer-role
rules:
Expand Down
6 changes: 1 addition & 5 deletions helm-chart/kuberay-operator/templates/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
{{- if .Values.rbacEnable }}
{{- if and .Values.rbacEnable (not .Values.singleNamespaceInstall) }}

{{- if .Values.singleNamespaceInstall }}
kind: Role
{{- else }}
kind: ClusterRole
{{- end }}
apiVersion: rbac.authorization.k8s.io/v1
metadata:
labels:
Expand Down
Loading