Repository navigation
[SPARK-59519] Add opt-in Kueue RBAC rules to Helm chart - #827
dongjoon-hyun wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
The four rules match Kueue's external-framework marker set and the gate is off by default. I rendered the chart with the default values and the output is byte-identical to main, so the "no user-facing change" claim holds. Two things I'd fix before merge. Nothing in CI ever renders or runs the new helm test assertions. And the grant is a subset of the doc it cites, missing scheduling.k8s.io/priorityclasses, which the already-merged WorkloadSpec.priority field will need. Two smaller points below, on where the cluster-scoped rules land and on the schema's required list.
Blocking
- 1. New
helm testassertions never run in CI: Both gates in front of them stay shut, so the rules ship with no automated coverage. Thehelm-testsmatrix at.github/workflows/build_and_test.yml:239is the ready-made place to open them. [inline:build-tools/helm/spark-kubernetes-operator/templates/tests/test-rbac.yaml:84] - 2. Grant is five of the seven rules in the cited Kueue doc:
scheduling.k8s.io/priorityclassesandevents.k8s.io/eventsare missing. Either add them or say in the description why they are excluded. [inline:build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:161]
Non-blocking
- 3. Cluster-scoped rules also land in the per-namespace
Role:ResourceFlavorandWorkloadPriorityClassare cluster-scoped, so those two entries are inert there. A ClusterRole-only wrapper keeps the renderedRolehonest. [inline:build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:155] - 4.
kueueis missing fromoperatorRbac.required: Both templates dereference.Values.operatorRbac.kueue.enabledunconditionally. A null block gives a Go template nil-pointer instead of the clean schema error every sibling block produces. [inline:build-tools/helm/spark-kubernetes-operator/values.schema.json:566]
Minor
- 5.
operations.mdrow omitsSparkClusterand overstates theclusterRole.createdependency: Theworkloadshalf works through a namespacedRoletoo. [inline:docs/operations.md:111]
|
|
||
| # 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 |
There was a problem hiding this 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.iocheck 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:
# tests/e2e/helm/helm-test-values/kueue/values.yaml
operatorRbac:
kueue:
enabled: trueplus - 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:
- 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.yamlOne 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.
There was a problem hiding this 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 upgrades 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.
| - get | ||
| - list | ||
| - watch | ||
| {{- end }} |
There was a problem hiding this 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/priorityclasseswithget;list;watch. This is the one that matters.WorkloadSpecalready carriespriorityandpriorityClassRef(spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/v1beta2/WorkloadSpec.java:47-48). As soon as the runtime resolves a pod'spriorityClassNameinto a Workload priority it will 403 here.events.k8s.io/eventswithcreate;watch;update;patch. RBAC matches API groups literally, so theapiGroups: [""]rule at:21-36does 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.
| {{- end }} | |
| - apiGroups: | |
| - "scheduling.k8s.io" | |
| resources: | |
| - priorityclasses | |
| verbs: | |
| - get | |
| - list | |
| - watch | |
| {{- end }} |
There was a problem hiding this 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.
| - apiGroups: | ||
| - "kueue.x-k8s.io" | ||
| resources: | ||
| - resourceflavors |
There was a problem hiding this 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:
{{/*
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.
There was a problem hiding this 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.
| } | ||
| } | ||
| }, | ||
| "kueue": { |
There was a problem hiding this 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
There was a problem hiding this comment.
Done in 8225336. kueue is now in operatorRbac.required, and --set operatorRbac.kueue=null fails with missing property 'kueue' like the sibling blocks.
| | 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 | |
There was a problem hiding this 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.
| | operatorRbac.kueue.enabled | Grant the operator access to Kueue `workloads`, `resourceflavors` and `workloadpriorityclasses`. Needs `clusterRole.create`. Also register `SparkApplication` in Kueue. | false | | |
| | 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 | |
There was a problem hiding this 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.
|
Thank you for the review, @peter-toth. All five findings are addressed in 8225336 and the PR description is updated. The new |
There was a problem hiding this comment.
Re-checked through 7f5aeac — findings 1-5 resolved, nothing regressed. The new helm-tests / kueue job passed on this head, the per-namespace Role renders with only the three workloads rules, and --set operatorRbac.kueue=null now gives the clean schema error. Four new points, none blocking.
Non-blocking
- 6. Description's
events.k8s.io/eventssentence is wrong (new): The Kueue doc's seventh marker isgroups="",resources=events— the core group, whichoperator-rbac.yaml:21-36already grants with a superset ofcreate;watch;update;patch. Nothing from that list is left out. My round-1 finding 2 had the group wrong and the sentence inherited it. - 7. The default-denial check passes on any command failure (new):
kubectl auth can-iexits non-zero for a denial and for an error alike, so a wrong impersonated subject keeps the assertion green. One--ascall before the upgrade pins it. [inline:.github/workflows/build_and_test.yml:293] - 8. Kueue is not in
Optional Prerequisites(late catch):docs/operations.md:31-39is the section for an optional feature whose CRDs the chart does not bundle, and Gateway API already has an entry there. Kueue is the same shape but appears only as a values-table cell. [inline:docs/operations.md:111]
Minor
- 9. Unguarded
priorityclassesassertion reads as a mistake (new): It sits after thefibecausescheduling.k8s.iois built in, and this file explains every other placement in a comment. [inline:build-tools/helm/spark-kubernetes-operator/templates/tests/test-rbac.yaml:92]
| 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 |
There was a problem hiding this 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:
| 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 | |
| # 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 |
There was a problem hiding this 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.
| | 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 | |
There was a problem hiding this 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:
- **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 |:
| | 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 | | |
| | 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 | |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this 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.
| kubectl auth can-i watch priorityclasses.scheduling.k8s.io | |
| # PriorityClass is built in, so this one needs no CRD-presence guard. | |
| kubectl auth can-i watch priorityclasses.scheduling.k8s.io |
There was a problem hiding this comment.
Done in e53b287. Added the comment explaining that PriorityClass is built in and so needs no CRD-presence guard.
|
For Finding 6: I re-checked the doc source rather than the rendered page. The seventh marker in integrate_a_custom_job.md is |
|
Merged to main |
|
Thank you again~ |
2 participants
What changes were proposed in this pull request?
This PR adds an opt-in Helm value
operatorRbac.kueue.enabled(defaultfalse). When enabled, the operator is granted the RBAC rules that Kueue requires from an external framework integration:kueue.x-k8s.io/workloadskueue.x-k8s.io/workloads/statuskueue.x-k8s.io/workloads/finalizerskueue.x-k8s.io/resourceflavors,workloadpriorityclassesscheduling.k8s.io/priorityclassesThe namespaced
workloadsrules live in the sharedoperatorRbacRulesblock under{{- if }}, like the existingleasesrule. The cluster-scoped resources go into a newoperatorClusterRbacRuleswrapper used only by the ClusterRole, since a namespaced Role cannot grant them.events.k8s.io/eventsfrom the Kueue doc is intentionally left out: the operator only emits coreevents, which are already granted.Also included:
helm testassertions for the new grants, ahelm-testsCI groupkueuethat installs Kueue v0.19.4 and runs them, a check thatworkloadscreate is denied with the default values, and adocs/operations.mdentry.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
integrations.externalFrameworksconfiguration are out of scope.Does this PR introduce any user-facing change?
Yes, a new Helm value
operatorRbac.kueue.enabled(defaultfalse). With the default, the rendered manifests are identical to the current chart.How was this patch tested?
helm lint --strictpasses.helm templateoutput with the default values is identical tomain; withoperatorRbac.kueue.enabled=truethe ClusterRole gets all five rules and, whenoperatorRbac.role.create=true, each workload-namespace Role gets only the threeworkloadsrules.--set operatorRbac.kueue=nullis now rejected by the values schema like every otheroperatorRbacblock.helm-tests / kueueCI job: installs Kueue, runshelm testwith the value enabled, then upgrades to the default values and asserts the operator service account is deniedcreateonworkloads.kueue.x-k8s.io.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5.1