dongjoon-hyun commented on code in PR #827:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4016692279
##########
build-tools/helm/spark-kubernetes-operator/templates/tests/test-rbac.yaml:
##########
@@ -77,6 +77,19 @@ spec:
kubectl auth can-i watch grpcroutes.gateway.networking.k8s.io
--all-namespaces
kubectl auth can-i create grpcroutes.gateway.networking.k8s.io
--all-namespaces
fi
+ {{- if .Values.operatorRbac.kueue.enabled }}
+
+ # The Kueue grant is opt-in and, like Gateway API, can only be
asserted
+ # where the Kueue CRDs are installed.
+ if kubectl api-resources --api-group=kueue.x-k8s.io --no-headers -o
name | grep -q workloads; then
Review Comment:
Done in 8225336. Added `tests/e2e/helm/helm-test-values/kueue/values.yaml`
and a `kueue` group in the `helm-tests` matrix. The group installs Kueue
v0.19.4 before `helm install`, runs `helm test` with the value enabled, then
`helm upgrade`s back to the default values and asserts that the operator
service account is denied `create` on `workloads.kueue.x-k8s.io` via `kubectl
auth can-i --as`. I put the denial check in the workflow step rather than the
`helm test` hook since it needs a cluster that serves the CRDs and the value
turned off.
##########
build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:
##########
@@ -122,6 +122,43 @@ rules:
- patch
- delete
{{- end }}
+{{- if .Values.operatorRbac.kueue.enabled }}
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads
+ verbs:
+ - get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads/status
+ verbs:
+ - get
+ - update
+ - patch
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads/finalizers
+ verbs:
+ - update
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - resourceflavors
+ - workloadpriorityclasses
+ verbs:
+ - get
+ - list
+ - watch
+{{- end }}
Review Comment:
Added `scheduling.k8s.io/priorityclasses` (get, list, watch) in 8225336.
Since `PriorityClass` is cluster-scoped too, I placed it in the new
ClusterRole-only block from Finding 3 instead of the shared one.
`events.k8s.io/events` stays out, and the PR description now says why: the
operator only emits core `events`, which are already granted.
##########
build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:
##########
@@ -122,6 +122,43 @@ rules:
- patch
- delete
{{- end }}
+{{- if .Values.operatorRbac.kueue.enabled }}
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads
+ verbs:
+ - get
+ - list
+ - watch
+ - create
+ - update
+ - patch
+ - delete
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads/status
+ verbs:
+ - get
+ - update
+ - patch
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - workloads/finalizers
+ verbs:
+ - update
+ - apiGroups:
+ - "kueue.x-k8s.io"
+ resources:
+ - resourceflavors
Review Comment:
Done in 8225336. Added `spark-operator.operatorClusterRbacRules` as you
sketched it, used by the ClusterRole only. It carries `resourceflavors`,
`workloadpriorityclasses` and the new `priorityclasses` rule; the per-namespace
Role keeps only the three `workloads` rules. Re-rendered with
`role.create=true` to confirm the split, and the default render is still
identical to `main`.
##########
build-tools/helm/spark-kubernetes-operator/values.schema.json:
##########
@@ -563,6 +563,19 @@
}
}
},
+ "kueue": {
Review Comment:
Done in 8225336. `kueue` is now in `operatorRbac.required`, and `--set
operatorRbac.kueue=null` fails with `missing property 'kueue'` like the sibling
blocks.
##########
docs/operations.md:
##########
@@ -108,6 +108,7 @@ following table:
| operatorRbac.configManagement.create | Enable
this to create a Role for operator configuration management (hot property
loading and leader election).
| true
|
| operatorRbac.configManagement.roleName | Role name
for operator configuration management.
| `spark-operator-config-role`
|
| operatorRbac.configManagement.roleBinding |
RoleBinding name for operator configuration management.
| `"spark-operator-config-monitor-role-binding"`
|
+| operatorRbac.kueue.enabled | Grant the
operator access to Kueue `workloads`, `resourceflavors` and
`workloadpriorityclasses`. Needs `clusterRole.create`. Also register
`SparkApplication` in Kueue. | false
|
Review Comment:
Done in 8225336. The row now names both
`SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org`,
limits the `clusterRole.create` note to the cluster-scoped resources, and also
lists `priorityclasses`. The `values.yaml` comment was updated to match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
