peter-toth commented on code in PR #825:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/825#discussion_r4013109468
##########
build-tools/helm/spark-kubernetes-operator/templates/_helpers.tpl:
##########
@@ -106,14 +106,34 @@ List of Spark workload namespaces. If not provied in
values, use the same namesp
{{- end }}
{{- end }}
+{{/*
+Whether the operator pod NetworkPolicy is enabled. The legacy key
+{operatorDeployment.networkPolicy.enable} is deprecated but still honored: the
feature is
+enabled when either key is true.
+*/}}
+{{- define "spark-operator.networkPolicy.enabled" -}}
+{{- $np := .Values.operatorDeployment.networkPolicy -}}
+{{- if or $np.enabled $np.enable }}true{{ else }}false{{ end -}}
Review Comment:
**Finding 1.** `or` makes the legacy key a one-way latch. A user whose base
values file still carries `enable: true` cannot turn the feature off through
the new key:
```
$ helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorDeployment.networkPolicy.enable=true \
--set operatorDeployment.networkPolicy.enabled=false | grep -c 'kind:
NetworkPolicy'
1
$ helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorConfiguration.dynamicConfig.enable=true \
--set operatorConfiguration.dynamicConfig.enabled=false | grep
'dynamicConfig.enabled='
spark.kubernetes.operator.dynamicConfig.enabled=true
```
That is the layered-values shape people actually use, a checked-in base
values file plus `--set` overrides in a pipeline. The override silently does
nothing, and no output names the key that won.
The rule is stated in `values.yaml`, so this is documented behaviour rather
than a bug. It is not in `docs/operations.md` though, which is where someone
debugging a stuck toggle will look. At minimum add a sentence there for both
toggles saying the legacy key wins while it is `true`. Finding 4 sketches a
shape that removes the asymmetry instead.
##########
build-tools/helm/spark-kubernetes-operator/values.yaml:
##########
@@ -94,7 +94,9 @@ operatorDeployment:
# operator pod is denied. Note that this requires a CNI plugin with
NetworkPolicy support,
# and that egress traffic is not restricted.
networkPolicy:
- enable: false
+ # The legacy key `enable` is deprecated and will be removed in chart
2.0.0. It is still
Review Comment:
**Finding 5.** Chart `1.8.0` shipped with operator `1.0.0` (`git show
1.0.0:build-tools/helm/spark-kubernetes-operator/Chart.yaml`) and the chart is
now at `1.9.0-dev`, so the chart minor tracks the operator minor. On that
cadence `2.0.0` is roughly ten releases out, and nothing tracks the removal. A
follow-up JIRA linked from these comments would keep it from being forgotten.
##########
docs/operations.md:
##########
@@ -154,7 +155,7 @@ for the operator pod:
```yaml
operatorDeployment:
networkPolicy:
- enable: true
+ enabled: true
Review Comment:
**Finding 2.** The deprecation of `operatorDeployment.networkPolicy.enable`
is documented in `values.yaml` and `values.schema.json`, but nowhere under
`docs/`. The values table above has no `operatorDeployment.networkPolicy.*`
rows at all, so unlike `dynamicConfig.enable` (which this PR does add a row
for, at line 137) there is nothing telling a 1.0.0 user that their `enable:
true` still works.
One sentence after this block covers it:
```markdown
The legacy key `operatorDeployment.networkPolicy.enable` is deprecated in
favour of
`enabled` and will be removed in chart `2.0.0`. It is still honored: the
NetworkPolicy
is created when either key is `true`.
```
##########
build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:
##########
@@ -173,7 +173,7 @@ metadata:
{{- template "spark-operator.operatorRbacRules" $ }}
---
{{- end }}
-{{- if and .Values.operatorConfiguration.dynamicConfig.enable (eq
.Values.operatorConfiguration.dynamicConfig.source "configMap") }}
+{{- if and (eq (include "spark-operator.dynamicConfig.enabled" .) "true") (eq
.Values.operatorConfiguration.dynamicConfig.source "configMap") }}
Review Comment:
**Finding 3.** Pre-existing, and a follow-up rather than something for this
PR. `operatorRbac.configManagement.create` is documented at
`docs/operations.md:108` as a toggle defaulting to `true`, but `grep -rn
configManagement` finds it only in `values.yaml`, `values.schema.json`, and the
`roleName` / `roleBindingName` references at lines 180, 200 and 206. No
template reads `create`, so setting it to `false` does not suppress the Role
and RoleBinding gated on this line.
The doc entry also credits it with covering leader election. That is
actually handled by the `coordination.k8s.io/leases` rule the ClusterRole adds
when `replicas > 1` (lines 111-115 of this file).
Since this PR is auditing `values.yaml` toggles, worth a separate JIRA to
either wire `create` in or drop it.
##########
build-tools/helm/spark-kubernetes-operator/templates/_helpers.tpl:
##########
@@ -106,14 +106,34 @@ List of Spark workload namespaces. If not provied in
values, use the same namesp
{{- end }}
{{- end }}
+{{/*
+Whether the operator pod NetworkPolicy is enabled. The legacy key
+{operatorDeployment.networkPolicy.enable} is deprecated but still honored: the
feature is
+enabled when either key is true.
+*/}}
+{{- define "spark-operator.networkPolicy.enabled" -}}
Review Comment:
**Finding 4.** An alternative that keeps the legacy key working and still
lets `enabled` win when it is set explicitly. Helm strips null values during
coalescing, so defaulting `enabled` to null in `values.yaml` makes
`.Values...enabled` absent unless the user sets it. That is the presence signal
the `or` shape lacks.
```gotemplate
{{- define "spark-operator.networkPolicy.enabled" -}}
{{- $np := .Values.operatorDeployment.networkPolicy -}}
{{- if not (kindIs "invalid" $np.enabled) }}{{ $np.enabled }}
{{- else if not (kindIs "invalid" $np.enable) }}{{ $np.enable }}
{{- else }}false{{ end -}}
{{- end }}
```
with `values.yaml`:
```yaml
networkPolicy:
# Default: false. The legacy key `enable` is deprecated and will be
removed in chart
# 2.0.0. It is honored when `enabled` is not set.
enabled:
```
I rendered this variant on all six key combinations, and `helm lint
--strict` passes:
| values | `or` (this PR) | presence check |
|---|---|---|
| defaults | off | off |
| `enabled=true` | on | on |
| `enable=true` | on | on |
| `enable=false` | off | off |
| `enable=true`, `enabled=false` | **on** | off |
| `enable=false`, `enabled=true` | on | on |
The cost is that `enabled` has to leave `networkPolicy.required` in
`values.schema.json`, because Helm drops the null key before schema validation
runs. Without that, `helm lint --strict` fails with `at
'/operatorDeployment/networkPolicy': missing property 'enabled'`.
Honest counter-argument: `enabled:` reading as null in `values.yaml` is less
obvious than `enabled: false`, and dropping the `required` entry weakens
validation for the length of the deprecation window. If you would rather keep
the documented default, `or` is the right call and finding 1's doc sentence is
enough on its own.
--
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]
