You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/15 05:51:10 UTC
[PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827
### What changes were proposed in this pull request?
This PR adds an opt-in Helm value `operatorRbac.kueue.enabled` (default `false`). When enabled, the operator (Cluster)Role is granted the RBAC rules that [Kueue](https://kueue.sigs.k8s.io/docs/tasks/dev/integrate_a_custom_job/) requires from an external framework integration:
| Resource (`kueue.x-k8s.io`) | Verbs |
|---|---|
| `workloads` | get, list, watch, create, update, patch, delete |
| `workloads/status` | get, update, patch |
| `workloads/finalizers` | update |
| `resourceflavors`, `workloadpriorityclasses` | get, list, watch |
The rules are added to the shared `operatorRbacRules` block under `{{- if }}`, like the existing `leases` rule, so both the ClusterRole and the per-namespace Role paths pick them up. A guarded `helm test` assertion and a `docs/operations.md` entry are included.
### Why are the changes needed?
This is the deployment-side preparation for the Kueue integration (SPARK-59486, SPARK-59490, SPARK-59503). Keeping the grant opt-in avoids widening the operator ClusterRole for users who do not run Kueue. The operator runtime changes and the Kueue-side `externalFrameworks` configuration are out of scope.
### Does this PR introduce _any_ user-facing change?
No because the default value of new Helm value `operatorRbac.kueue.enabled` is `false`. With the default, the rendered manifests are identical to the current chart.
### How was this patch tested?
- `helm lint --strict` passes.
- `helm template` output with the default values is identical to `main`; with `operatorRbac.kueue.enabled=true` the four rules are added to the ClusterRole and, when `operatorRbac.role.create=true`, to each workload-namespace Role.
- The new `helm test` assertions are skipped on clusters without the Kueue CRDs, so existing CI jobs are unaffected.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5.1
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4017062080
##########
.github/workflows/build_and_test.yml:
##########
@@ -277,6 +285,12 @@ jobs:
build-tools/helm/spark-kubernetes-operator/
minikube docker-env --unset
helm test spark
+ - name: Verify Kueue RBAC is denied by default
+ if: matrix.test-group == 'kueue'
+ run: |
+ helm upgrade spark -f build-tools/helm/spark-kubernetes-operator/values.yaml \
+ build-tools/helm/spark-kubernetes-operator/
+ if kubectl auth can-i create workloads.kueue.x-k8s.io --as=system:serviceaccount:default:spark-operator; then exit 1; fi
Review Comment:
**Finding 7.** `kubectl auth can-i` exits non-zero for a denial and for an error alike. So this assertion is satisfied by anything that makes the command fail: a typo in the service account name, a release namespace other than `default`, an API error.
Nothing else in the job pins that subject string. `helm test` does prove the grant works, but it runs inside the pod as the service account itself, never through `--as`, so the two halves share no identity.
Running the same impersonated check once before the upgrade fixes it. It must answer "yes" while the value is still on:
```suggestion
# The same impersonated check must answer "yes" first, otherwise a wrong subject
# would make the denial below vacuous.
kubectl auth can-i create workloads.kueue.x-k8s.io --as=system:serviceaccount:default:spark-operator
helm upgrade spark -f build-tools/helm/spark-kubernetes-operator/values.yaml \
build-tools/helm/spark-kubernetes-operator/
if kubectl auth can-i create workloads.kueue.x-k8s.io --as=system:serviceaccount:default:spark-operator; then exit 1; fi
```
##########
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`, `workloadpriorityclasses` and to `priorityclasses`. The cluster-scoped ones need `clusterRole.create`. Also register `SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org` in Kueue's `integrations.externalFrameworks`.| false |
Review Comment:
**Finding 8.** `Optional Prerequisites` at `:31-39` is the section for exactly this: an optional feature whose CRDs the chart does not bundle. Gateway API has an entry there saying what to install and what breaks without it. Kueue is the same shape and gets only a values-table cell, which is also where the `integrations.externalFrameworks` registration ended up — three sentences in a table column next to a one-word `false`.
Suggested bullet after the Gateway API one:
```markdown
- **Kueue** (`workloads.kueue.x-k8s.io`, `resourceflavors.kueue.x-k8s.io`,
`workloadpriorityclasses.kueue.x-k8s.io`) — required only when `operatorRbac.kueue.enabled` is
set. Kueue is not bundled with the operator; install it from
[kueue.sigs.k8s.io](https://kueue.sigs.k8s.io/docs/installation/), and register
`SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org` in Kueue's
`integrations.externalFrameworks`.
```
The row can then shrink to the grant itself. It is also missing the space before its closing `|`:
```suggestion
| operatorRbac.kueue.enabled | Grant the operator access to Kueue `workloads`, `resourceflavors`, `workloadpriorityclasses` and to `priorityclasses`. The cluster-scoped ones need `clusterRole.create`. See [Optional Prerequisites](#optional-prerequisites). | false |
```
##########
build-tools/helm/spark-kubernetes-operator/templates/tests/test-rbac.yaml:
##########
@@ -77,6 +77,20 @@ 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
+ kubectl auth can-i watch workloads.kueue.x-k8s.io --all-namespaces
+ kubectl auth can-i create workloads.kueue.x-k8s.io --all-namespaces
+ kubectl auth can-i patch workloads.kueue.x-k8s.io --subresource=status --all-namespaces
+ kubectl auth can-i update workloads.kueue.x-k8s.io --subresource=finalizers --all-namespaces
+ kubectl auth can-i watch resourceflavors.kueue.x-k8s.io
+ kubectl auth can-i watch workloadpriorityclasses.kueue.x-k8s.io
+ fi
+ kubectl auth can-i watch priorityclasses.scheduling.k8s.io
Review Comment:
**Finding 9.** This one sits after the `fi` because `PriorityClass` is a built-in API and needs no CRD-presence guard. Every other placement in this file carries a comment saying why, and without one it reads as a bracket that slipped.
```suggestion
# PriorityClass is built in, so this one needs no CRD-presence guard.
kubectl auth can-i watch priorityclasses.scheduling.k8s.io
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#issuecomment-5681932335
Thank you for the review, @peter-toth. All five findings are addressed in 8225336 and the PR description is updated. The new `helm-tests / kueue` job is the first one to actually run the Kueue assertions, so I'll watch that CI result.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #827: [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart
URL: https://github.com/apache/spark-kubernetes-operator/pull/827
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4017167685
##########
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:
On Finding 6: I re-checked the doc source rather than the rendered page. The seventh marker in [integrate_a_custom_job.md](https://github.com/kubernetes-sigs/kueue/blob/main/site/content/en/docs/tasks/dev/integrate_a_custom_job.md?plain=1#L140) is `groups=events.k8s.io,resources=events,verbs=create;watch;update;patch`, and the built-in JobSet integration carries the same group at [jobset_controller.go#L59](https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobs/jobset/jobset_controller.go#L59). So the group in the description is right: the chart grants core `events` only, which does not cover `events.k8s.io`, and the operator does not use that API. I'll keep the sentence as is.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4017177655
##########
.github/workflows/build_and_test.yml:
##########
@@ -277,6 +285,12 @@ jobs:
build-tools/helm/spark-kubernetes-operator/
minikube docker-env --unset
helm test spark
+ - name: Verify Kueue RBAC is denied by default
+ if: matrix.test-group == 'kueue'
+ run: |
+ helm upgrade spark -f build-tools/helm/spark-kubernetes-operator/values.yaml \
+ build-tools/helm/spark-kubernetes-operator/
+ if kubectl auth can-i create workloads.kueue.x-k8s.io --as=system:serviceaccount:default:spark-operator; then exit 1; fi
Review Comment:
Done in e53b287. The same impersonated `kubectl auth can-i create workloads.kueue.x-k8s.io --as=...` now runs before the `helm upgrade` and must answer "yes", so a wrong subject fails there instead of making the denial vacuous.
##########
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`, `workloadpriorityclasses` and to `priorityclasses`. The cluster-scoped ones need `clusterRole.create`. Also register `SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org` in Kueue's `integrations.externalFrameworks`.| false |
Review Comment:
Done in e53b287. Added the Kueue bullet to `Optional Prerequisites` right after the Gateway API one, moved the `integrations.externalFrameworks` registration there, and shrank the table row to the grant itself with a link to that section. The missing space before the closing pipe is fixed too.
##########
build-tools/helm/spark-kubernetes-operator/templates/tests/test-rbac.yaml:
##########
@@ -77,6 +77,20 @@ 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
+ kubectl auth can-i watch workloads.kueue.x-k8s.io --all-namespaces
+ kubectl auth can-i create workloads.kueue.x-k8s.io --all-namespaces
+ kubectl auth can-i patch workloads.kueue.x-k8s.io --subresource=status --all-namespaces
+ kubectl auth can-i update workloads.kueue.x-k8s.io --subresource=finalizers --all-namespaces
+ kubectl auth can-i watch resourceflavors.kueue.x-k8s.io
+ kubectl auth can-i watch workloadpriorityclasses.kueue.x-k8s.io
+ fi
+ kubectl auth can-i watch priorityclasses.scheduling.k8s.io
Review Comment:
Done in e53b287. Added the comment explaining that `PriorityClass` is built in and so needs no CRD-presence guard.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#issuecomment-5682793327
For Finding 6: I re-checked the doc source rather than the rendered page. The seventh marker in [integrate_a_custom_job.md](https://github.com/kubernetes-sigs/kueue/blob/main/site/content/en/docs/tasks/dev/integrate_a_custom_job.md?plain=1#L140) is `groups=events.k8s.io,resources=events,verbs=create;watch;update;patch`, and the built-in JobSet integration carries the same group at [jobset_controller.go#L59](https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobs/jobset/jobset_controller.go#L59). So the group in the description is right: the chart grants core `events` only, which does not cover `events.k8s.io`, and the operator does not use that API. I'll keep the sentence as is.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4013705790
##########
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:
**Finding 1.** These assertions cannot run in any CI job as things stand, so the new rules ship with no automated coverage.
Two gates have to open and neither does:
- The Helm `{{- if .Values.operatorRbac.kueue.enabled }}` at `:80`. No workflow or values file sets it, `grep -rni kueue .github/ tests/` returns nothing.
- The `kubectl api-resources --api-group=kueue.x-k8s.io` check on this line. No CI cluster installs the Kueue CRDs.
The repo already has the mechanism. `.github/workflows/build_and_test.yml:270-279` runs a `helm-tests` matrix that installs the chart with `tests/e2e/helm/helm-test-values/<group>/values.yaml` and then `helm test spark`. A new group opens the first gate:
```yaml
# tests/e2e/helm/helm-test-values/kueue/values.yaml
operatorRbac:
kueue:
enabled: true
```
plus `- kueue` in the `test-group` list at `:239-242`. That alone proves the chart renders and installs with the flag on. To open the second gate, the job needs the Kueue CRDs, which is one guarded step before `helm install`:
```yaml
- name: Install Kueue CRDs
if: matrix.test-group == 'kueue'
run: |
kubectl apply --server-side -f https://github.com/kubernetes-sigs/kueue/releases/download/v0.19.4/manifests.yaml
```
One more assertion worth adding in that job: with the default `enabled: false` the operator should be *denied* `create` on `workloads.kueue.x-k8s.io`. Nothing currently proves the opt-in gate actually gates. That check is only meaningful on a cluster that serves the CRDs, so it belongs here rather than in the default `helm test`.
##########
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:
**Finding 2.** The description says this grants "the RBAC rules that Kueue requires from an external framework integration" and links the custom-job doc. That doc lists seven `+kubebuilder:rbac` markers under "Extend your existing RBAC Authorizations". This block ships five.
Missing:
- `scheduling.k8s.io/priorityclasses` with `get;list;watch`. This is the one that matters. `WorkloadSpec` already carries `priority` and `priorityClassRef` (`spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/v1beta2/WorkloadSpec.java:47-48`). As soon as the runtime resolves a pod's `priorityClassName` into a Workload priority it will 403 here.
- `events.k8s.io/events` with `create;watch;update;patch`. RBAC matches API groups literally, so the `apiGroups: [""]` rule at `:21-36` does not cover this group. The operator emits no Events today, so leaving it out is defensible. Then it is worth saying so, rather than letting the table read as the complete set.
The suggestion below adds the first one. I rendered it with `--set operatorRbac.kueue.enabled=true` and ran `helm lint --strict`, both pass.
```suggestion
- apiGroups:
- "scheduling.k8s.io"
resources:
- priorityclasses
verbs:
- get
- list
- watch
{{- end }}
```
##########
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:
**Finding 3.** Both of these are cluster-scoped. Kueue marks them `+kubebuilder:resource:scope=Cluster` in `apis/kueue/v1beta2/resourceflavor_types.go:28` and `workloadpriorityclass_types.go:27`. This define also backs the per-workload-namespace `Role` at `:263`, and a rule for a cluster-scoped resource in a namespaced `Role` is silently inert.
Rendering with `--set operatorRbac.kueue.enabled=true --set operatorRbac.role.create=true --set workloadResources.namespaces.data={spark-1}` puts both into the `spark-1` `Role`, where they grant nothing. Every other rule in this block is for a namespaced resource, so this is the first one that splits.
The `values.yaml` comment already says `clusterRole.create` is required for these two. The template can say it instead. Moving the pair into a ClusterRole-only wrapper works:
```gotemplate
{{/*
Rules used only by the operator ClusterRole, for cluster-scoped resources
*/}}
{{- define "spark-operator.operatorClusterRbacRules" }}
{{- include "spark-operator.operatorRbacRules" . }}
{{- if .Values.operatorRbac.kueue.enabled }}
- apiGroups:
- "kueue.x-k8s.io"
resources:
- resourceflavors
- workloadpriorityclasses
verbs:
- get
- list
- watch
{{- end }}
{{- end }}
```
with `:210` calling the wrapper and `:263` keeping the plain define. The `workloads`, `workloads/status` and `workloads/finalizers` rules stay shared, since `Workload` is namespaced. I applied this and re-rendered: the operator ClusterRole keeps both resources, the `spark-1` `Role` keeps `workloads` and drops them, and `helm lint --strict` passes.
##########
build-tools/helm/spark-kubernetes-operator/values.schema.json:
##########
@@ -563,6 +563,19 @@
}
}
},
+ "kueue": {
Review Comment:
**Finding 4.** `kueue` should also join the `operatorRbac.required` list at `:427`. Both `operator-rbac.yaml:125` and `tests/test-rbac.yaml:80` dereference `.Values.operatorRbac.kueue.enabled` unconditionally, and every other sub-block they dereference is already required.
The difference shows up when the block goes missing:
```
$ helm template t . --set 'operatorRbac.clusterRole=null'
Error: values don't meet the specifications of the schema(s) in the following chart(s):
spark-kubernetes-operator:
- at '/operatorRbac': missing property 'clusterRole'
$ helm template t . --set 'operatorRbac.kueue=null'
Error: template: spark-kubernetes-operator/templates/tests/test-rbac.yaml:80:24:
executing "..." at <.Values.operatorRbac.kueue.enabled>: nil pointer evaluating interface {}.enabled
```
##########
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:
**Finding 5.** Two things in this cell.
`KueueWorkloadFactory.buildWorkload` has a `SparkCluster` overload (`spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadFactory.java:124`), so the registration note should name both kinds. Kueue takes them in `integrations.externalFrameworks` as `Kind.version.group`, which is worth spelling out because it is not guessable from the cell.
"Needs `clusterRole.create`" is true only for `resourceflavors` and `workloadpriorityclasses`. `Workload` is namespaced, so the `workloads` half works through the per-namespace `Role` too.
```suggestion
| operatorRbac.kueue.enabled | Grant the operator access to Kueue `workloads`, `resourceflavors` and `workloadpriorityclasses`. The two cluster-scoped ones need `clusterRole.create`. Also register `SparkApplication.v1.spark.apache.org` and `SparkCluster.v1.spark.apache.org` in Kueue's `integrations.externalFrameworks`. | false |
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#discussion_r4017167685
##########
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:
On Finding 6: I re-checked the doc source rather than the rendered page. The seventh marker in [integrate_a_custom_job.md](https://github.com/kubernetes-sigs/kueue/blob/main/site/content/en/docs/tasks/dev/integrate_a_custom_job.md?plain=1#L140) is `groups=events.k8s.io,resources=events,verbs=create;watch;update;patch`, and the built-in JobSet integration carries the same group at [jobset_controller.go#L59](https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/jobs/jobset/jobset_controller.go#L59). So the group in the description is right: the chart grants core `events` only, which does not cover `events.k8s.io`, and the operator does not use that API. I'll keep the sentence as is.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#issuecomment-5683155186
Thank you again~
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59519] Add opt-in `Kueue` RBAC rules to Helm chart [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #827:
URL: https://github.com/apache/spark-kubernetes-operator/pull/827#issuecomment-5683150854
Merged to main
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org