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: [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]
