Skip to content

[SPARK-59504] Use enabled key convention in Helm values.yaml while honoring legacy enable #825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dongjoon-hyun wants to merge 3 commits into from
Closed

[SPARK-59504] Use enabled key convention in Helm values.yaml while honoring legacy enable #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9748021
[SPARK-59504] Use `enabled` key convention in Helm `values.yaml` whil…
dongjoon-hyun Sep 14, 2026
8859867
Address comments
dongjoon-hyun Sep 15, 2026
4541e30
Address comments
dongjoon-hyun Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/build_and_test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,3 +304,27 @@ jobs:
- name: Validate helm chart linting
run: |
helm lint --strict build-tools/helm/spark-kubernetes-operator
- name: Validate deprecated helm values are still honored
run: |
helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorDeployment.networkPolicy.enable=true \
| 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'

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
| 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.

- 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
if helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorConfiguration.dynamicConfig.enable=false \
| grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true'; then
echo "dynamicConfig enabled with enable=false"; exit 1
fi
22 changes: 21 additions & 1 deletion build-tools/helm/spark-kubernetes-operator/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" -}}

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- $np := .Values.operatorDeployment.networkPolicy -}}
{{- if or $np.enabled $np.enable }}true{{ else }}false{{ end -}}

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- end }}

{{/*
Whether dynamic config (hot properties loading) is enabled. The legacy key
{operatorConfiguration.dynamicConfig.enable} is deprecated but still honored: the feature is
enabled when either key is true.
*/}}
{{- define "spark-operator.dynamicConfig.enabled" -}}
{{- $dc := .Values.operatorConfiguration.dynamicConfig -}}
{{- if or $dc.enabled $dc.enable }}true{{ else }}false{{ end -}}
{{- end }}

{{/*
Default property overrides
*/}}
{{- define "spark-operator.defaultPropertyOverrides" -}}
# Runtime resolved properties
spark.kubernetes.operator.namespace={{ .Release.Namespace }}
spark.kubernetes.operator.name={{- include "spark-operator.name" . }}
spark.kubernetes.operator.dynamicConfig.enabled={{ .Values.operatorConfiguration.dynamicConfig.enable }}
spark.kubernetes.operator.dynamicConfig.enabled={{ include "spark-operator.dynamicConfig.enabled" . }}
spark.kubernetes.operator.dynamicConfig.source={{ .Values.operatorConfiguration.dynamicConfig.source }}
spark.kubernetes.operator.metrics.port={{ include "spark-operator.metricsPort" . }}
spark.kubernetes.operator.health.probePort={{ include "spark-operator.probePort" . }}
Expand Down
2 changes: 1 addition & 1 deletion build-tools/helm/spark-kubernetes-operator/templates/network-policy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

{{- if .Values.operatorDeployment.networkPolicy.enable }}
{{- if eq (include "spark-operator.networkPolicy.enabled" .) "true" }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
Expand Down
2 changes: 1 addition & 1 deletion build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") }}

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
Expand Down
4 changes: 2 additions & 2 deletions build-tools/helm/spark-kubernetes-operator/templates/spark-operator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ spec:
mountPath: /opt/spark-operator/logs
- name: tmp-volume
mountPath: /tmp
{{- if and .Values.operatorConfiguration.dynamicConfig.enable (eq .Values.operatorConfiguration.dynamicConfig.source "file") }}
{{- if and (eq (include "spark-operator.dynamicConfig.enabled" .) "true") (eq .Values.operatorConfiguration.dynamicConfig.source "file") }}
- name: spark-operator-dynamic-config-volume
mountPath: /opt/spark-operator/dynamic-conf
readOnly: true
Expand Down Expand Up @@ -191,7 +191,7 @@ spec:
emptyDir: { }
- name: tmp-volume
emptyDir: { }
{{- if and .Values.operatorConfiguration.dynamicConfig.enable (eq .Values.operatorConfiguration.dynamicConfig.source "file") }}
{{- if and (eq (include "spark-operator.dynamicConfig.enabled" .) "true") (eq .Values.operatorConfiguration.dynamicConfig.source "file") }}
- name: spark-operator-dynamic-config-volume
configMap:
name: spark-kubernetes-operator-dynamic-configuration
Expand Down
2 changes: 1 addition & 1 deletion build-tools/helm/spark-kubernetes-operator/templates/tests/test-network-policy.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

{{- if .Values.operatorDeployment.networkPolicy.enable }}
{{- if eq (include "spark-operator.networkPolicy.enabled" .) "true" }}
apiVersion: v1
kind: Pod
metadata:
Expand Down
16 changes: 12 additions & 4 deletions build-tools/helm/spark-kubernetes-operator/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,13 +396,17 @@
"type": "object",
"description": "NetworkPolicy for the operator pod",
"required": [
"enable"
"enabled"
],
"properties": {
"enable": {
"enabled": {
"type": "boolean",
"description": "Whether to create a NetworkPolicy for the operator pod"
},
"enable": {
"type": "boolean",
"description": "Deprecated, use 'enabled' instead; still honored (the NetworkPolicy is created when either key is true) and will be removed in chart 2.0.0 (SPARK-59533)"
},
"metricsIngress": {
"type": ["array", "null"],
"description": "List of NetworkPolicyPeer(s) allowed to reach the metrics port; when empty, ingress to the metrics port is denied",
Expand Down Expand Up @@ -818,16 +822,20 @@
"type": "object",
"description": "Dynamic configuration",
"required": [
"enable",
"enabled",
"create",
"annotations",
"data"
],
"properties": {
"enable": {
"enabled": {
"type": "boolean",
"description": "Enable dynamic configuration"
},
"enable": {
"type": "boolean",
"description": "Deprecated, use 'enabled' instead; still honored (dynamic configuration is enabled when either key is true) and will be removed in chart 2.0.0 (SPARK-59533)"
},
"create": {
"type": "boolean",
"description": "Create ConfigMap for dynamic configuration"
Expand Down
10 changes: 7 additions & 3 deletions build-tools/helm/spark-kubernetes-operator/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (SPARK-59533).

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# It is still honored: the NetworkPolicy is created when either `enabled` or `enable` is true.
enabled: false
# List of NetworkPolicyPeer(s) allowed to reach the metrics port, e.g. the Prometheus
# scraper. When empty, ingress to the metrics port is denied.
metricsIngress: [ ]
Expand Down Expand Up @@ -218,8 +220,10 @@ operatorConfiguration:
metrics.properties: |+
# Metrics Properties Overrides
dynamicConfig:
# Enable this for hot properties loading.
enable: false
# Enable this for hot properties loading. The legacy key `enable` is deprecated and will be
# removed in chart 2.0.0 (SPARK-59533). It is still honored: hot properties loading is
# enabled when either `enabled` or `enable` is true.
enabled: false
# Source of the dynamic config overrides when enabled. Supported values:
# configMap - (default) watch a ConfigMap via a Kubernetes informer. Requires the operator
# to have RBAC to read ConfigMaps (created by this chart).
Expand Down
11 changes: 9 additions & 2 deletions docs/operations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ following table:
| operatorConfiguration.spark-operator.properties | The default operator configuration. | |
| operatorConfiguration.metrics.properties | The default operator metrics (sink) configuration. | |
| operatorConfiguration.dynamicConfig.create | If set to true, a config map would be created & watched by operator as source of truth for hot properties loading. | false |
| operatorConfiguration.dynamicConfig.enable | If set to true, operator would honor the created config map as source of truth for hot properties loading. | false |
| operatorConfiguration.dynamicConfig.enabled | If set to true, operator would honor the created config map as source of truth for hot properties loading. | false |
| operatorConfiguration.dynamicConfig.enable | Deprecated, use `operatorConfiguration.dynamicConfig.enabled`. Still honored: `enable: true` wins over `enabled: false`. Removed in chart 2.0.0 (SPARK-59533). | |
| operatorConfiguration.dynamicConfig.annotations | Annotations to be applied for the dynamicConfig resources. | `"helm.sh/resource-policy": keep` |
| operatorConfiguration.dynamicConfig.data | Data field (key-value pairs) that acts as hot properties in the config map. | `spark.kubernetes.operator.reconciler.intervalSeconds: "60"` |

Expand All @@ -154,7 +155,7 @@ for the operator pod:
```yaml
operatorDeployment:
networkPolicy:
enable: true
enabled: true

@peter-toth peter-toth Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

metricsIngress:
- namespaceSelector:
matchLabels:
Expand All @@ -174,6 +175,12 @@ Note that this requires a CNI plugin that enforces NetworkPolicy; on clusters wi
a plugin the policy is silently ignored. Egress traffic of the operator (Kubernetes API
server, DNS) is not restricted by this policy.

The legacy key `operatorDeployment.networkPolicy.enable` is deprecated in favor of `enabled`
and will be removed in chart `2.0.0` ([SPARK-59533](https://issues.apache.org/jira/browse/SPARK-59533)).
It is still honored: the NetworkPolicy is created when either key is `true`, so a stale
`enable: true` in a base values file wins over `enabled: false` and must be removed to turn
the feature off. The same rule applies to `operatorConfiguration.dynamicConfig.enable`.

## Operator Health(Liveness) Probe with Sentinel Resource

Learning
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/helm/dynamic-config-values-2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ workloadResources:

operatorConfiguration:
dynamicConfig:
enable: true
enabled: true
create: true
data:
spark.kubernetes.operator.watchedNamespaces: "spark-3"
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/helm/dynamic-config-values-file.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ operatorDeployment:

operatorConfiguration:
dynamicConfig:
enable: true
enabled: true
source: file
create: true
data:
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/helm/dynamic-config-values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ operatorDeployment:

operatorConfiguration:
dynamicConfig:
enable: true
enabled: true
create: true
data:
spark.kubernetes.operator.watchedNamespaces: "default"
2 changes: 1 addition & 1 deletion tests/e2e/helm/helm-test-values/network-policy/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

operatorDeployment:
networkPolicy:
enable: true
enabled: true
metricsIngress:
- namespaceSelector:
matchLabels:
Expand Down