-
Notifications
You must be signed in to change notification settings - Fork 402
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.
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
[Feature] Watch CR in multiple namespaces with namespaced RBAC resources #1106
[Feature] Watch CR in multiple namespaces with namespaced RBAC resources #1106
Conversation
ray-operator/main.go
Outdated
}) | ||
} | ||
|
||
if len(watchNamespaces) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not possible for len(watchNamespaces) == 0
to be true. The length of strings.Split("", ",")
is still 1. See this link for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems worth including as a code comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added f209400.
|
||
if len(watchNamespaces) == 1 { | ||
options.Namespace = watchNamespaces[0] | ||
if watchNamespaces[0] == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this doc, if Namespace is specified, restricts the manager's cache to watch objects in the desired namespace. If the field is not specified, it defaults to all namespaces.
setupLog.Info(fmt.Sprintf("Only watch custom resources in the namespace: %s", watchNamespaces[0])) | ||
} | ||
} else { | ||
options.NewCache = cache.MultiNamespacedCacheBuilder(watchNamespaces) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To watch multiple namespaces, we can use MultiNamespacedCache. See this example and this doc for more details.
@@ -30,3 +30,12 @@ rules: | |||
- events | |||
verbs: | |||
- create | |||
- apiGroups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary for Leader Election. We can refer to leader_election_role.yaml in the kubebuilder repo for further.
@@ -32,4 +31,13 @@ rules: | |||
- events | |||
verbs: | |||
- create | |||
- apiGroups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary for leader election.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
@@ -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 -}} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
kind: Role | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
metadata: | ||
labels: {{ include "kuberay-operator.labels" $ | nindent 4 }} |
There was a problem hiding this comment.
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.
cc @msumitjain @anshulomar @Yicheng-Lu-llll would you mind reviewing this PR? Thanks! |
Follow up:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have prior experience with this area so I don't really have substantial critical feedback, just comments/questions.
As a general note, the PR comments are really helpful, and I think most of them could be included directly as code comments to improve readability with little or no downside.
@@ -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 -}} |
There was a problem hiding this comment.
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?)
@@ -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 -}} |
There was a problem hiding this comment.
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?
@@ -32,4 +31,13 @@ rules: | |||
- events | |||
verbs: | |||
- create | |||
- apiGroups: |
There was a problem hiding this comment.
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?)
|
||
{{- if .Values.singleNamespaceInstall }} | ||
kind: Role | ||
{{- else }} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?)
There was a problem hiding this comment.
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.
# permissions outside of the current namespace | ||
# watchNamespace: ray-user-namespace | ||
# The KubeRay operator will watch the custom resources in the namespaces listed in the "watchNamespace" parameter. | ||
# watchNamespace: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the source of truth for documentation, or is there a corresponding doc page that also needs to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is the source of truth. There is no document related to it.
ray-operator/main.go
Outdated
}) | ||
} | ||
|
||
if len(watchNamespaces) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems worth including as a code comment.
Also, is this a breaking change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding comments in Helm chart template YAML files may cause issues with the Helm parser.
Nope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kevin85421 for putting up this PR! It looks good to me but I have few comments:
- (Functional) While the changes are fine for a manual deploy, for automated deploys there could be challenges. For example, an Argo CD project can deploy to only a single namespace. In such a case, the Argo CD won't be able to apply
multiple_namespaces_role
andmultiple_namespaces_rolebinding
to any namespace other than the one it is configured with - where it deploys the operator. So, Argo CD projects for respective namespaces (where RayServices run) will have to apply the role and rolebinding to allow the operator manage RayService in their namespaces. Hence, to facilitate such use-cases, we might want to introduce a new config attribute, saymultipleNamespacesRbacEnabled
. If its value is true, only thenmultiple_namespaces_role/rolebinding
should be applied. What do you think? - (Config Readability) I think the field name
singleNamespaceInstall
becomes confusing after this change. This is because it could mean installing KubeRay and its roles/rolebindings either in a single namespace or in multiple namespaces (defined in watchNamespaces list). One way to resolve this confusion could be to replace this field with another field, say clusterScopedInstall. WhenclusterScopedInstall: true
is set, then ClusterRole and ClusterRoleBinding should be created. WhenclusterScopedInstall: false
is set, then roles and rolebindings should be created in the namespaces listed inwatchNamespaces
. That said, I do understand thatsingleNamespaceInstall
field may be required for backward-compatibility. If that is the case, please ignore this point.
Thank @anshulomar for the insight!
Wow, I wasn't aware that ArgoCD has this restriction.
This is a bit confusing for me. Could you please provide more details or an example to help clarify? Thanks! Q: Which RBAC resources (Role / RoleBinding / ClusterRole / ClusterRoleBinding / ServiceAccount) should be created by the KubeRay operator Helm chart when Current situation: (1) Users can set (2) Users can use the (3) If Based on (1)(2)(3), users seem to have enough power? I do not have any experience with ArgoCD. I may miss some points. |
I agree that this can be confusing. Breaking backward compatibility can be a challenging decision. For example, even if there is a typo like
|
{{- $watchNamespaces := default (list .Release.Namespace) .Values.watchNamespace }} | ||
{{- range $namespace := $watchNamespaces }} | ||
--- | ||
kind: Role |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Thanks @kevin85421 !
Sure. Given that an ArgoCD project can deploy only to one namespace, ArgoCD for kuberay operator won't be able to apply
By configuring
When
Yes, users have enough power but for automated deployment tools like Argo, there could be permission restrictions for applying manifests to multiple namespaces as explained in this comment above. Does it help? Or is it still confusing? |
@anshulomar Thank you for your reply! I understand your point clearly, and I will proceed to draft a pull request. |
…ces (ray-project#1106) Watch CR in multiple namespaces with namespaced RBAC resources
Why are these changes needed?
Currently, KubeRay supports the following options:
singleNamespaceInstall
to true, which installs namespaced RBAC resources (i.e., Role and RoleBinding).However, for users who only have namespaced access, they would need to deploy one KubeRay operator for each namespace. This can increase the maintenance overhead, such as upgrading the version of KubeRay for each deployed instance.
This PR implements a feature that supports users to deploy a KubeRay operator to watch multiple namespaces without installing ClusterRole and ClusterRoleBinding as proposal in #1084.
There are two fields available to configure the KubeRay operator to achieve this:
singleNamespaceInstall
:watchNamespace
is set, it will create Role and RoleBinding for all namespaces listed inwatchNamespace
.watchNamespace
is not set, it will create Role and RoleBinding for the namespace where the operator is deployed.singleNamespaceInstall
is not set, the KubeRay operator will create ClusterRole and ClusterRoleBinding.watchNamespace
: A list of namespaces that the KubeRay operator will watch. In addition, it will be passed to KubeRay binary as the--watch-namespace
flag.Related issue number
Closes #1084
Closes #1083
#1094
Checks
Case 1: If
singleNamespaceInstall
is set to false, the KubeRay operator will create ClusterRole and ClusterRoleBinding.Case 2: If
singleNamespaceInstall
is set to true and watchNamespace is set, it will create Role and RoleBinding for all namespaces listed in watchNamespace.watchNamespace
:n1
,n2
Case 3: If
singleNamespaceInstall
is set to true andwatchNamespace
is not set, it will create Role and RoleBinding for the namespace where the operator is deployed.Case 4: If
singleNamespaceInstall
is not set, the KubeRay operator will create ClusterRole and ClusterRoleBinding.