Repository navigation
[SPARK-59504] Use enabled key convention in Helm values.yaml while honoring legacy enable - #825
dongjoon-hyun wants to merge 3 commits into
Conversation
…e honoring legacy `enable`
There was a problem hiding this comment.
Thanks for the PR, @dongjoon-hyun!
The rename lands cleanly. Both toggles now resolve through new helpers in _helpers.tpl, and all five callsites use them. I rendered the chart across all six combinations of the two keys, and the four cases in the description hold. The new CI step is an effective guard, since it fails once I strip the or out of the helpers. The one gap is that or makes the legacy key a one-way latch, so --set operatorDeployment.networkPolicy.enabled=false cannot turn off a stale enable: true.
Non-blocking
- 1. Legacy key is a one-way latch: a stale
enable: truein a base values file wins over an explicitenabled: false, and nothing in the output names the key that won. Document the precedence indocs/operations.md, or see finding 4 for a shape that removes the asymmetry. [inline:_helpers.tpl:116] - 2.
networkPolicy.enabledeprecation missing fromdocs/: the PR adds a values-table row for the deprecateddynamicConfig.enablebut says nothing aboutnetworkPolicy.enable, which has no table row at all. A 1.0.0 user readingdocs/operations.mdsees onlyenabled: trueand cannot tell that theirenable: truestill works. [inline:docs/operations.md:158] - 3. Pre-existing:
operatorRbac.configManagement.createis never read: it is documented as a toggle defaulting totrue, but no template references it. Worth a separate JIRA rather than this PR. [inline:operator-rbac.yaml:176]
Alternatives
- 4. Presence detection gives exact precedence: defaulting
enabledto null lets the helper tell "user set it" from "chart default", so the new key can win when set while the legacy key is still honored otherwise. I verified this variant on all six key combinations andhelm lint --strictpasses. [inline:_helpers.tpl:114]
Minor
- 5.
chart 2.0.0removal target is untracked: chart1.8.0shipped with operator1.0.0and the chart is now1.9.0-dev, so2.0.0is roughly ten releases out with nothing tracking the removal. [inline:values.yaml:97]
| */}} | ||
| {{- define "spark-operator.networkPolicy.enabled" -}} | ||
| {{- $np := .Values.operatorDeployment.networkPolicy -}} | ||
| {{- if or $np.enabled $np.enable }}true{{ else }}false{{ end -}} |
There was a problem hiding this 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.
| operatorDeployment: | ||
| networkPolicy: | ||
| enable: true | ||
| enabled: true |
There was a problem hiding this 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:
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`.| --- | ||
| {{- 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") }} |
There was a problem hiding this 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.
| {operatorDeployment.networkPolicy.enable} is deprecated but still honored: the feature is | ||
| enabled when either key is true. | ||
| */}} | ||
| {{- define "spark-operator.networkPolicy.enabled" -}} |
There was a problem hiding this 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.
{{- 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:
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.
| # 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 |
There was a problem hiding this 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.
|
Thank you for the review, @peter-toth. Addressed in 8859867.
|
There was a problem hiding this comment.
Re-checked through 8859867 — findings 1, 2 and 5 resolved: the precedence rule is now stated in docs/operations.md for both toggles, the NetworkPolicy section carries the deprecation paragraph, and SPARK-59533 is filed and linked from all four places. I re-ran the six-combination render matrix and helm lint --strict on the new head; nothing regressed. Not pursuing finding 4 further. Finding 3 stays deferred, and I don't see a JIRA for operatorRbac.configManagement.create yet.
Non-blocking
- 6. CI pins only the ON direction (late catch): the new step asserts each legacy key switches its feature on, but nothing anywhere asserts the toggles stay off. Two assertions in the same job cover it, and they fail on a stuck-on helper. [inline:
.github/workflows/build_and_test.yml:314] - 7. No install-time deprecation signal (late catch): a user on
enable: truelearns about the rename only from the docs, and at 2.0.0 their NetworkPolicy stops being created with no error.hasKeyis an exact presence signal now thatenableis out of the chart defaults. [inline:values.yaml:97]
Minor
- 8. Deprecation paragraph splits the NetworkPolicy explanation (new): it sits between the example and the "When enabled, all ingress traffic ..." list that explains that example. [inline:
docs/operations.md:165]
| | grep -q 'kind: NetworkPolicy' | ||
| helm template spark build-tools/helm/spark-kubernetes-operator \ | ||
| --set operatorConfiguration.dynamicConfig.enable=true \ | ||
| | grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true' |
There was a problem hiding this comment.
Finding 6. This step pins the ON direction for both legacy keys. Nothing pins the OFF direction, here or in the Helm Tests job — the network-policy group installs with enabled: true, and no test asserts the NetworkPolicy is absent.
That gap matters because the helpers return the strings "true" / "false", so every callsite has to spell out eq (include "...") "true". A future gated resource written as {{- if include "spark-operator.networkPolicy.enabled" . }} renders unconditionally, since "false" is a non-empty string and therefore truthy. I checked that on this head with a probe template: the naive if takes the truthy branch on default values.
The four callsites today all get it right. But with the helper mutated to {{- if or $np.enabled $np.enable }}true{{ else }}true{{ end -}} — a stuck-on toggle — helm lint --strict and both of the new assertions still pass. These two fail on that mutation and pass on this head:
| | grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true' | |
| | grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true' | |
| - name: Validate helm values resolve to disabled | |
| run: | | |
| if helm template spark build-tools/helm/spark-kubernetes-operator \ | |
| | grep -q 'kind: NetworkPolicy'; then | |
| echo "NetworkPolicy rendered with the default values"; exit 1 | |
| fi | |
| if helm template spark build-tools/helm/spark-kubernetes-operator \ | |
| --set operatorDeployment.networkPolicy.enable=false \ | |
| | grep -q 'kind: NetworkPolicy'; then | |
| echo "NetworkPolicy rendered with enable=false"; exit 1 | |
| fi |
Worth using if ... then exit 1; fi rather than ! ... | grep -q. The step has no shell: key, so it runs under bash -e without pipefail, and bash exempts a !-inverted command from -e. I confirmed that ! helm template ... | grep -q 'kind: NetworkPolicy' exits 0 and the script keeps going even when the NetworkPolicy is rendered.
| # 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 (SPARK-59533). |
There was a problem hiding this comment.
Finding 7. The deprecation is announced in four files, but helm install / helm upgrade prints nothing for a values file that still says enable: true. Docs only reach people who go looking. When SPARK-59533 lands, that user's NetworkPolicy quietly stops being created, and a lost ingress restriction is not something you want to discover later.
Now that enable is out of the chart defaults, hasKey is an exact presence signal. I verified it on this head: hasKey .Values.operatorDeployment.networkPolicy "enable" renders false on defaults and true under --set operatorDeployment.networkPolicy.enable=false, and the same holds for operatorConfiguration.dynamicConfig. So a templates/NOTES.txt covers both toggles:
{{- if hasKey .Values.operatorDeployment.networkPolicy "enable" }}
WARNING: `operatorDeployment.networkPolicy.enable` is deprecated, use `enabled` instead.
It is still honored and will be removed in chart 2.0.0 (SPARK-59533).
{{- end }}
The chart has no NOTES.txt today and .github/.licenserc.yaml does not exempt one, so it needs the ASF header. Putting the header inside a {{/* ... */}} comment keeps it out of the install output while leaving the text in the file for skywalking-eyes. helm lint --strict passes with that file added.
A follow-up PR is fine if you would rather keep this one to the rename.
| kubernetes.io/metadata.name: "monitoring" | ||
| ``` | ||
|
|
||
| The legacy key `operatorDeployment.networkPolicy.enable` is deprecated in favor of `enabled` |
There was a problem hiding this comment.
Finding 8. The content is right, the position splits the section. This paragraph lands between the enabled: true example and the list that explains it, so "When enabled, all ingress traffic to the operator pod is denied except:" now reads as a continuation of the deprecation note rather than of the example above it.
Moving the paragraph to the end of the section, after the CNI note at docs/operations.md:181-183, keeps the example next to its explanation and still puts the legacy key in front of anyone reading the section.
|
Thank you for the second pass, @peter-toth. Addressed in 4541e30.
|
|
Thank you always, @peter-toth. BTW, as you know, #44 introduced the bug you found, F3. And, you are able to file a JIRA issue for your finding instead of asking me . 
This repository is owned by the Apache Spark community including you. So, it's a little weird for me because you tried to enforce me again and again to do something independent from this context. Do you want to avoid creating a JIRA issue for this repository, @peter-toth ? 
Anyway, as I wrote in the previous comment, I filed SPARK-59537 for it. |
|
Yeah, this the current behaviour of my general review skill to offer opening follow-ups to the PR author, not specific to this repo at all. Let me try to fine tune it to be more pro-active next time. Thanks for the feedback. |
peter-toth
left a comment
peter-toth
left a comment
There was a problem hiding this comment.
Re-checked through 4541e30. Findings 6 and 8 are resolved, and nothing new came up. The new Validate helm values resolve to disabled step pins the OFF direction for both toggles, and the deprecation paragraph now follows the CNI note. I re-ran the six-combination render matrix, helm lint --strict, and both new CI steps on this head.
Thanks for working through all of these, @dongjoon-hyun. Nothing left open from my side.
|
Thank you always for helping this, @peter-toth ! |
|
Merged to main |
2 participants
What changes were proposed in this pull request?
This PR aims to use the
enabledkey convention in the Helm chartvalues.yamlwhile still honoring the legacyenablekeys.operatorDeployment.networkPolicy.enable→operatorDeployment.networkPolicy.enabledoperatorConfiguration.dynamicConfig.enable→operatorConfiguration.dynamicConfig.enabledTwo helpers in
_helpers.tplresolve each toggle totruewhen either the new or the legacy key istrue, and all templates use them. The legacy keys stay valid invalues.schema.json, marked as deprecated, and will be removed in chart2.0.0.Why are the changes needed?
<feature>.enabledis the common Helm convention, and the operator's runtime property is alreadyspark.kubernetes.operator.dynamicConfig.enabled. Resolving withorkeeps existingenable: trueusers working because the chart defaultenabled: falseis always merged and cannot be distinguished from a user-supplied value.Does this PR introduce any user-facing change?
Yes, compared to v1.0.0 (2026-07-23). New users should use
enabled. Existing values files withenablekeep working without modification.How was this patch tested?
helm lint --strictpasses.helm templatefor default,enabled=true, legacyenable=true, and legacyenable=falseand confirmed the expected resources and properties.lintjob that renders the chart with the legacyenablekeys and asserts they are still honored.enabledkeys.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5.1