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