[FLINK-39791][helm] Honor defaultConfiguration.config.yaml and always mount config.yaml

[gerkElznik]

What is the purpose of the change

The Helm chart advertises defaultConfiguration.config.yaml (added in FLINK-38409), but a downstream consumer who sets it never gets it mounted — the operator silently runs on flink-conf.yaml built from chart defaults, while the user's config.yaml content lands in the flink-operator-config ConfigMap but is never read by the operator JVM.

Three interacting causes:

1. values.yaml hardcodes a non-empty defaultConfiguration.flink-conf.yaml. Helm's additive merge means a downstream -f my-values.yaml cannot unset it, so hasKey "flink-conf.yaml" is always true.
2. templates/controller/configmap.yaml unconditionally emits both config.yaml and flink-conf.yaml keys.
3. templates/controller/deployment.yaml selects the mounted file with an independent hasKey "flink-conf.yaml" — always true — so the config.yaml branch is unreachable.

This PR resolves the file choice in the chart template and emits/mounts a single, statically named config.yaml. The approach was discussed and agreed on the JIRA ticket.

Why one always-config.yaml file, appended from a flat seed

The chart seed carries the mandatory Java-17 --add-opens/--add-exports opts, which must apply no matter which format the user supplies. So the seed is prepended to the resolved override — the same "append" model log4j-*.properties already uses — rather than replaced by it (the model logback-*.xml uses, since two XML documents can't be concatenated). It cannot use the replace model, because that would drop the Java-17 opts for anyone who sets their own config.yaml.

Because the mounted result is parsed by Flink's strict config.yaml parser (snakeyaml-engine, allowDuplicateKeys=false), an appended flat user override of a key that is also in the seed would be a duplicate-key crash. Two consequences, both reflected here:

• the seed is kept flat (a nested seed would instead collide with a nested user override, which is the recommended form); and
• the two redundant seed keys taskmanager.numberOfTaskSlots: 1 / parallelism.default: 1 are dropped — they only restate Flink's compiled default of 1, so removing them is behavior-neutral while shrinking the collision surface.

Brief change log

• configmap.yaml — emit a single config-file key, always named config.yaml, resolved with a nested {{ with (index … "config.yaml") }} … {{ else }}{{ with (index … "flink-conf.yaml") }} … {{ end }}{{ end }} (plain with/else, so it renders on every Helm version): the user's config.yaml wins, else their flink-conf.yaml, else the values.yaml default. Keys off value truthiness rather than hasKey (which downstream can't unset), consistent with the chart's existing log4j/logback blocks.
• deployment.yaml — mount the key statically at path: config.yaml. Emit and mount are both literally config.yaml, so they can't drift; config.yaml is also the only filename Flink 2.x reads, so default installs keep booting after the operator's eventual 2.x rebase.
• conf/flink-conf.yaml → conf/config.yaml (renamed) — the always-prepended seed, named to match what it is mounted as; kept flat; the two redundant : 1 keys removed (see above).
• values.yaml — restructured the showcase comment into two mutually-exclusive format examples (modern config.yaml / legacy flink-conf.yaml) with precedence + mount notes; opinionated defaults stay here (not relocated into the always-applied seed).
• docs (operations/configuration.md, operations/helm.md, development/guide.md) — document the config.yaml format and precedence, and fix the now-stale /opt/flink/conf/flink-conf.yaml inspect path.

Verifying this change

This change added tests and can be verified as follows:

• helm-unittest matrix (tests/controller/{configmap,deployment}_test.yaml): exactly one config-file key, always config.yaml (never flink-conf.yaml), carrying today's effective defaults with the two dropped seed keys absent; a user flink-conf.yaml still flows in when config.yaml is unset; config.yaml wins when both are set; and a flat override of a former seed key no longer duplicates it. The Deployment mounts the key statically as config.yaml. (Values are injected via structured set:; a dotted-string set: no-ops on keys containing dots.)
• Parser load test: each matrix row's rendered config.yaml was loaded through a strict, no-duplicate-key YAML parser equivalent to Flink's config.yaml loader, with a positive control confirming the check rejects a genuine duplicate. All rows parse clean.
• In a kind cluster: installed the chart and confirmed the operator loads /opt/flink/conf/config.yaml via Flink's standard YAML parser — with scalar, nested-map, and list values — that the ConfigMap carries a single config.yaml key, and that the operator reconciles FlinkDeployments with both flat and nested flinkConfiguration. ct lint against the chart-testing image (Helm 3.16.4) confirms the resolver renders on older Helm too.
• helm lint and a full helm template render pass.
• Effective-config equivalence for a default install: the mounted payload is unchanged except the two dropped keys, which resolve to Flink's compiled default of 1 either way.

Does this pull request potentially affect one of the following parts:

• Dependencies (add or upgrade a dependency): no
• The public API (CustomResourceDescriptors): no
• Core observer or reconciler logic that is regularly executed: no (Helm chart + docs only)

Documentation

• Does this introduce a new feature? No — it makes the existing defaultConfiguration.config.yaml work as advertised.
• How is it documented? docs — operations/configuration.md and operations/helm.md.

---

Notes for reviewers (call-outs / open questions)

1. Precedence flip. When both keys are set, config.yaml now wins; the old hasKey behavior preferred flink-conf.yaml. Worth a release note.
2. ConfigMap shape change. flink-operator-config now carries one config-file key instead of two — visible to anyone reading that ConfigMap directly.
3. Coordination with FLINK-39571. That ticket's "known limitation" note on the configuration page isn't on main yet; if it lands first, I'll rebase and remove it.
4. Residual collision surface. A flat config.yaml override of the two remaining kubernetes.operator.default-configuration.flink-version.* Java-17 seed keys could still duplicate-key. It's rare (they're rarely flat-overridden, and the recommended nested form is collision-free). The clean long-term fix is shipping framework defaults as compiled ConfigOption defaults rather than a text-prepended seed — proposed as a separate follow-up.
5. Chinese docs. Synced with the same English content (per review feedback).
6. Backports? Which active release-* branch(es) should this target?

[gyfora]

Why are we removing these?

[gerkElznik]

Good question — there are three separate edits in that hunk:

1. The watchNamespaces / operatorHealth injections aren't actually dropped.
   The old template emitted two config-file keys (config.yaml and
   flink-conf.yaml) and appended both blocks to each, so they rendered twice.
   Collapsing to a single config.yaml key removes the duplicate — one copy still
   renders just below the resolver (lines 37–43); helm template confirms
   kubernetes.operator.health.probe.* and the watched.namespaces line still
   emit, once.

2. The hasKey "config.yaml" block is replaced by the with-based resolver
   above it. That's the core of the ticket: hasKey keys off presence, and since
   the chart hardcodes a non-empty defaultConfiguration.flink-conf.yaml that a
   downstream -f values.yaml can't unset, the config.yaml branch was
   unreachable. with keys off value-truthiness (like the existing log4j/
   logback blocks), so the user's config.yaml actually wins.

3. The flink-conf.yaml: key is removed so the ConfigMap emits a single,
   always-config.yaml file (the only name Flink 2.x reads), mounted statically.

The one behavioral consequence of (3) I'd like your read on before acting: runtime
patching of the operator ConfigMap now has to target the config.yaml key. In
particular e2e-tests/test_dynamic_flink_conf.sh patches the flink-conf.yaml
key and would no longer be picked up — its twin test_dynamic_config.sh already
covers the same dynamic-reload path via config.yaml. I'm inclined to drop
test_dynamic_flink_conf.sh (and its two ci.yml matrix rows) plus add a short
migration note, but since that removes legacy coverage I'd rather confirm with you
first.

(Aside, unrelated to this hunk: I've rebased onto latest main — the earlier
test_application_operations.sh smoke failure was a pipeline.jars/Flink 1.20.4
app-mode error on a stale base.)

[gyfora]

@gerkElznik the e2es seem to be failing

[gerkElznik]

@gerkElznik the e2es seem to be failing

Heads-up before going further: this regresses Flink 1.x application deployments

While tracking down the failing test_application_operations.sh smoke test, I found that switching the operator to mount config.yaml breaks Flink 1.x application-mode deployments. I think it reshapes this PR, so I wanted to lay it out.

Symptom

A v1_xx application deployment's JobManager crashloops at startup:

ERROR ClusterEntrypoint - Could not create application program.
java.net.URISyntaxException: Illegal character in scheme name at index 0:
    ['local:///opt/flink/usrlib/myjob.jar']
  at KubernetesApplicationClusterEntrypoint.fetchArtifacts(…) [flink-dist-1.20.4.jar]

Isolated with an A/B

Same operator image (built from this branch), same kind cluster, same v1_20
StateMachineExample — the only difference is the chart's mounted config file:

	mounts config.yaml (this PR)	mounts flink-conf.yaml (main)
operator config mode	standard-YAML	legacy
operator writes to the JM ConfigMap	pipeline.jars: ['local:///…/myjob.jar']	pipeline.jars: local:///…/myjob.jar
v1_20 JobManager	URISyntaxException → CrashLoopBackOff	READY / RUNNING

Root cause

FlinkConfMountDecorator#getClusterSideConfData (the operator's copy, from FLINK-37236):

// … otherwise it would simply inherit it from the base config (which would always be false currently).
Configuration clusterSideConfig = new Configuration(useStandardYamlConfig()); // false for v1_x → flink-conf.yaml ✓
clusterSideConfig.addAll(flinkConfig);                                          // copies the operator's global-config values
return ConfigurationUtils.convertConfigToWritableLines(clusterSideConfig, false);

useStandardYamlConfig() correctly keys off the deployment's FlinkVersion, but addAll(flinkConfig) copies values from the operator's global config — and the comment assumes that global config is "always false" (legacy). Once the operator reads config.yaml, its global config is standardYaml=true, so list options (pipeline.jars, pipeline.classpaths, …) carry standard-YAML form and get written into the v1_x flink-conf.yaml as ['local://…'], which the legacy parser rejects. So it's a pre-existing operator assumption that this PR is simply the first to expose — and it affects any list-valued option on any Flink-1.x deployment, not only pipeline.jars.

How would you like to sequence this @gyfora? Tagging @Dennis-Mircea as well since he's mentioned planning for the overall 2.x upgrade.

1. Fix FlinkConfMountDecorator to serialize per the target deployment's YAML mode (downgrade list values when useStandardYamlConfig() is false) as a prerequisite, then land this chart change; or
2. Hold the config.yaml mount (this change) until the operator no longer manages Flink-1.x deployments.

Happy to help with either, including the operator-side code change.

Two smaller notes, same root (single config.yaml key):
- Re your inline question on the removed lines: the watchNamespaces/operatorHealth injections aren't dropped, they were emitted once per config-file key (so twice), and the dedup keeps one copy (still just below the resolver; helm template shows them rendering once). The hasKey → with swap is the actual fix, and the flink-conf.yaml key is removed to emit a single file.
- e2e-tests/test_dynamic_flink_conf.sh patches the flink-conf.yaml ConfigMap key, which is no longer mounted; its twin test_dynamic_config.sh already covers the same path via config.yaml, so it'd need updating/removing — but that's moot until the above is settled.

[gerkElznik]

@gyfora Following up on this:
I've filed the operator-side bug as FLINK-40055 with the full root cause. It runs one level deeper than my comment above (the YAML dialect is process-global in flink-core, and it leaks at a toMap() round-trip in removeOperatorConfigs, not in the decorator itself).

I have a fix ready on a branch, it keeps typed values intact through removeOperatorConfigs and serializes at the write boundaries per target Flink version, so in-process parse semantics don't change. Verified beyond CI; unit test pinning the process-global flag, all four operator-format × deployment-version combinations in a kind cluster, and an in-place operator chart migration under running v1/v2 jobs with JobManager kills after each flip (no disruption, no churn, byte-identical generated configs).

Since the scope of this original change has expanded into more complexity I took the liberty to open the fix PR for FLINK-40055 so it's reviewable but happy to pivot on your direction... if it's accepted I'll rebase this PR once it lands.

[gerkElznik]

The PR to land before this for the fix: #1152

[Dennis-Mircea]

Thanks for opening this PR! Good investigation on the process-global YAML dialect leak, and thanks for opening #1152 with the root cause and the verification matrix.

On sequencing, I'd support landing #1152 first and then reshaping this PR around a broader fix rather than the flat-seed workaround. The root cause of the collision surface (defaults duplicated across values.yaml + conf/config.yaml, plus text-written Helm-value injections) is the same issue Note 4 gestures at, and I think it deserves being fixed properly rather than mitigated.

Concretely, the cleanup I have in mind:

• Chart drops the SLF4J reporter class+interval, reconcile.interval, and observer.progress-check.interval overrides from values.yaml:
  • Reconcile and observer overrides fall back to their compiled ConfigOption defaults (60 s and 10 s today, worth confirming those are the right values) which currently are not used at all. Maybe it makes sense to change those defaults with the overrided ones.
  • The SLF4J reporter class+interval can be dropped entirely. It is the user choice if it should be configured or not.
• Chart's conf/config.yaml (the Java 17 opts under kubernetes.operator.default-configuration.flink-version.*) migrates to compiled defaults on the operator side. conf/config.yaml becomes empty, or a commented example that documents structure without shipping active values.
• watchNamespaces and operatorHealth stop being text-written into the operator ConfigMap. They become env vars the operator reads at startup, mirroring how WATCH_NAMESPACES already works today (Helm sets the env var, FlinkOperatorConfiguration.fromConfiguration reads it and overrides the runtime Configuration). Extending the same pattern to KUBERNETES_OPERATOR_HEALTH_PROBE_ENABLED and KUBERNETES_OPERATOR_HEALTH_PROBE_PORT is symmetry, not new machinery.

Net result: config.yaml on disk contains only user-supplied content. No seed to collide against, no mixed-form aesthetic concern, no ordering-per-format debate. Note 4's residual surface goes away because there is no shipped-in-text seed left for a user override to duplicate.

Are you ok with this approach, @gyfora?

[Dennis-Mircea]

Not necessary to specify this property here.

[gerkElznik]

done

[Dennis-Mircea]

Comment not needed.

[gerkElznik]

done

[Dennis-Mircea]

Comment not needed.

[gerkElznik]

done

[Dennis-Mircea]

The chinese docs can be synced with the same english content.

[gerkElznik]

done

[gerkElznik]

Thanks @Dennis-Mircea! Inline comments are all addressed in the latest commits on both PRs (including the Chinese docs sync).

Before responding to the cleanup proposal I tried each piece in a kind cluster and traced the code paths, to check my own understanding. Taking your three items in order:

1. Dropping the SLF4J reporter and interval overrides from values.yaml

Confirmed the compiled defaults are 60s (kubernetes.operator.reconcile.interval) and 10s (kubernetes.operator.observer.progress-check.interval), and that both are currently dead in Helm installs since the chart always overrides them. Dropping the overrides without touching the compiled defaults does change default-install behavior though: I measured the reconcile cadence going from 15.0s to 60.1s, and the progress check would go from 5s to 10s (slower observation of in-progress deployments and savepoints). So the choice is between changing the compiled defaults to 15s/5s (which also affects non-Helm deployments, the generated docs, and idle load on the API server and job REST endpoints) or letting default installs slow down with a release note. I don't have a strong opinion on which, but it feels like a decision that deserves its own visibility.

One consistency note: dropping the SLF4J reporter is also a behavior change on default installs, since operator metrics stop being logged every 5 minutes and anyone scraping or alerting on those lines would notice. I agree it should be user choice going forward; I'd just put it in the same release note.

2. Migrating the Java-17 opts to compiled defaults

I think this is the right destination, and it's the same end-state note 4 above was gesturing at. Two design questions the follow-up would need to answer, since the per-version keys are dynamic string prefixes resolved at runtime (FlinkConfigManager.getRelevantVersionPrefixes) and can't be plain ConfigOption defaults:

• Where the compiled seed gets injected decides precedence; user config needs to keep winning over it.
• Today defaultConfiguration.append: false produces a config without the Java-17 opts entirely. Compiled defaults can't be shed by omitting text, so the migration probably needs an explicit off-switch to preserve that escape hatch.

3. watchNamespaces and operatorHealth as env vars

WATCH_NAMESPACES works on the operator side exactly as you said: I set it on the operator container in kind and the watch scoped correctly with no code change. Two wrinkles I found while tracing the full path:

• The webhook consumes the watched namespaces too (FlinkOperatorWebhook scopes its informers through the same config path), and it runs as a separate container with its own env block. The chart would need to set the var on both containers; otherwise the webhook informers stay cluster-scoped, which fails under the namespaced RBAC that setting watchNamespaces switches on.
• The env var takes precedence over the config file, so shipping namespaces via env silently disables kubernetes.operator.dynamic.namespaces.enabled (no-restart namespace changes via ConfigMap edit). That might be acceptable, since an env change through helm upgrade forces a rolling restart, which is arguably more correct than today's silently stale config. But it seems worth a deliberate call in the follow-up.

For the health probe: nothing reads KUBERNETES_OPERATOR_HEALTH_PROBE_* today. Those names don't exist in the tree (I checked EnvUtils and OperatorHealthService), so that half is a small amount of new operator code mirroring ENV_WATCH_NAMESPACES. Notably it's only needed to keep operatorHealth.port overrides and disablement working: for a default install the compiled defaults (true/8085) already match what the chart writes, so dropping those two ConfigMap lines is behavior-neutral there. The Helm value itself stays either way, since it also wires the containerPort and the liveness/startup probes.

On sequencing

Landing #1152 first makes sense to me regardless. For this PR, my thinking is that the cleanup and the fix here are complementary rather than alternatives: the cleanup removes the collision surface, but even with an empty seed and env vars the chart still needs this PR's template changes to emit and mount the user's config.yaml (the always-true hasKey is what makes that branch unreachable today). That makes me lean toward landing this PR at its current scope and doing the cleanup as a dedicated follow-up ticket, where the compiled-defaults question can get proper visibility. Happy to fold it into this PR instead if you and @gyfora prefer; if it goes the follow-up route, I'm glad to file the ticket and take it on.

[gerkElznik]

Quick follow-up: two points in my comment above were from code tracing only, so I runtime-tested both in the same kind setup (webhook enabled via cert-manager, namespaced RBAC via watchNamespaces).

• Webhook + env var: the webhook container does honor WATCH_NAMESPACES through the same config path. With the var on the operator container only (and no ConfigMap line), the webhook informers stayed cluster-scoped and the first FlinkSessionJob apply failed admission with InternalError ... cannot list flinkdeployments at the cluster scope. The informers are created lazily, so nothing fails at startup; it surfaces on the first session job. With the var on both containers, admission worked normally. So the follow-up would just need to set it on both.
• Dynamic namespaces: with the env var unset and kubernetes.operator.dynamic.namespaces.enabled: true, a live ConfigMap edit rescoped all three controllers and the webhook informers with no restart. With the env var set, the same edit reloads the config (the log even prints the new namespace list) but the watch never changes and nothing warns about it; a FlinkDeployment created in the newly added namespace was never reconciled. So I'd want the precedence behavior to be a deliberate, documented call in the follow-up.

Neither changes the options above; just confirming the details for the follow-up work.