fix(controller): own the metrics bind address, keep the proxy out of readiness

[duckhawk]

Description

Three follow-ups to #210, found while reviewing the sixteen storage-module MRs that consume it.

helm_lib_module_controller_manifests emits METRICS_BIND_ADDRESS itself. 127.0.0.1:<controllerMetricsPort> when controllerMetricsProxyPort is set, :<controllerMetricsPort> otherwise. The parameter documentation now also states that a consuming chart must not pass this variable through additionalControllerEnvs.

The kube-rbac-proxy container loses its readinessProbe. The livenessProbe stays.

nodeMetricsPort is removed from helm_lib_csi_node_manifests, together with its two test cases.

Chart version bumped to 1.72.18.

Why do we need it, and what problem does it solve?

The loopback contract was documented and not enforced

#210 says "the controller is expected to bind it on 127.0.0.1" and backs that up by not rendering a containerPort for the controller's own metrics port. But containerPort is metadata — it restricts no bind, and dropping it does not keep a process off the pod IP. What actually held the contract was one identical line copied into all sixteen consuming charts:

(dict "name" "METRICS_BIND_ADDRESS" "value" (printf "127.0.0.1:%d" (int .Values.<key>.internal.controllerMetricsPort)))

plus sixteen copies of a const defaultMetricsBindAddress = "127.0.0.1:8080" in the Go configs, each with the same five-line comment. Classic Shotgun Surgery: one rule, sixteen places to get it wrong.

And getting it wrong is cheap, because controllerMetricsPort and controllerMetricsProxyPort default to the same number (8080) in fourteen of those modules. It works today only because the two sockets differ in the host part: 127.0.0.1:8080 and <podIP>:8080. Write ":8080" in any one of the sixteen charts — or drop the line, reasoning that the library handles it — and you get one of:

• an unauthenticated /metrics answering on the pod IP, which is exactly what the proxy was added to prevent;
• listen tcp :8080: bind: address already in use in the shared network namespace, killing whichever container starts second.

Neither is caught by a linter or a template test. Both surface in production.

A metrics sidecar could take cluster-wide admission down

The proxy carried a readinessProbe. This template renders the webhook Service next to the Deployment, that Service selects app: controller, and an EndpointSlice contains only Ready pods. helm_lib_module_validating_webhook_configuration emits no failurePolicy, and the admissionregistration.k8s.io/v1 default is Fail.

So a kube-rbac-proxy that fails to become ready — image pull, OOM at the 25Mi VPA floor, a port typo in a future chart edit — removes the pod from the webhook Service's endpoints and every guarded API write starts failing. Twelve pods in the fleet are shaped this way. Concretely:

• csi-nfs guards storage.k8s.io/v1/storageclasses with operations: ["*"], cluster scope. A dead metrics sidecar stops every StorageClass create and update in the cluster, including other modules'.
• sds-elastic guards cephclusters, cephobjectstores and sixteen more Rook kinds on CREATE/UPDATE/DELETE. The same failure halts Rook reconciliation.

It gets worse for two modules where that same Service also backs an aggregated API:

• storage-foundation — v1.subresources.storage-foundation.deckhouse.io and v1.subresources.snapshot.storage.k8s.io
• state-snapshotter — v1alpha1.subresources.state-snapshotter.deckhouse.io

An APIService with no ready endpoints takes its whole API group out of discovery.

Before #210 the pod's readiness was decided by the controller and the webhook — the two containers the webhook actually needs. Adding a purely observability sidecar to that gate is a blast-radius increase, and a wedged proxy is still restarted by the liveness probe without it.

The removal does have a cost, stated plainly: with no readinessProbe the pod is Ready before the proxy listens, so during a rollout a scrape or two can fail (the readiness filter the monitors carry — __meta_kubernetes_endpointslice_endpoint_conditions_ready / __meta_kubernetes_pod_ready — still excludes terminating pods and pods whose controller is not ready, but no longer "proxy not up yet"). That is the trade: a few gaps in a metric against an admission or discovery outage.

nodeMetricsPort has no user and a sharp edge

No module passes it — verified across all sixteen storage modules plus the sds-local-volume pilot the sweep started from. It shipped into a shared library with zero consumers.

Worse than dead: _csi_node.tpl defaults csiNodeHostNetwork to "true", so the first module to use the parameter "as intended" publishes the CSI node's /metrics on every interface of the node, unauthenticated. That is precisely the defect the same sweep just fixed in the sds-replicated-volume agent, which had been serving :4270 node-wide past the kube-rbac-proxy that exists to gate it. A second effect is that the port number silently becomes a node-level resource: two modules picking the same nodeMetricsPort cannot both schedule on one node.

Better to delete it now and reintroduce it, if ever, with the loopback + proxy shape spelled out in the helper.

What is the expected result?

METRICS_BIND_ADDRESS appears on the controller container whenever controllerMetricsPort is set:

# controllerMetricsPort: 8080, controllerMetricsProxyPort: 4207
- name: METRICS_BIND_ADDRESS
  value: "127.0.0.1:8080"

# controllerMetricsPort: 8080, no proxy
- name: METRICS_BIND_ADDRESS
  value: ":8080"

A chart that sets neither port renders exactly as before.

The kube-rbac-proxy container renders without readinessProbe and with livenessProbe unchanged. helm_lib_csi_node_manifests no longer accepts nodeMetricsPort; since nothing passes it, no consumer output changes.

Backward compatibility

Checked against every consumer I can see — all ourmodules/*, flant/deckhouse (full checkout, vendoring 1.72.14), flant/deckhouse-cse (modules/990-* and modules-ext/*), plus a GitHub-wide code search:

Change	Consumers affected
nodeMetricsPort removed	None. Nothing passes it anywhere — not the nine cloud-provider modules that call helm_lib_csi_node_manifests, not the storage fleet, and no public hit. A chart that did pass it would not fail to render either: an unknown $config key is simply nil.
METRICS_BIND_ADDRESS emitted	Two consumers set controllerMetricsPort without passing the variable: sds-local-volume (8080, no proxy) and the sds-node-configurator snapshots in deckhouse-cse (8080, no proxy). Both get ":8080" — identical to sds-local-volume's own DefaultMetricsBindAddress, and ignored outright by the cse snapshot, whose binary reads METRICS_PORT. No rendered behaviour change.
	The sixteen storage modules of the in-flight sweep pass the variable themselves with the same value, so between step 2 and step 3 below the container carries it twice. Duplicate env names are valid in the API and both entries are byte-identical, so behaviour is unchanged; dmt's env-variables-duplicates reports it, which is the forcing function for step 3.
	The variable lands on the controller container only. The webhooks container is untouched, so a chart passing METRICS_BIND_ADDRESS through additionalWebhooksEnvs (the example #210 used) does not collide. Verified across all sixteen rendered charts: the webhooks container's env is ["LOG_LEVEL"] everywhere.
Env ordering	The new entry sits between CONTROLLER_NAMESPACE and additionalControllerEnvs. No downstream repository asserts an exact env list for this container — the only such assertion was this library's own additionalWebhooksEnvs case, updated here.
readinessProbe removed	Behaviour change, no API change: pods reach and keep Ready in cases where they previously did not. Rollout semantics return to what they were before #210.

Rollout order for the consuming modules — this matters, because a module on 1.72.17 that vendors 1.72.18 without the second step gets a duplicate env entry:

1. merge and release 1.72.18;
2. re-vendor charts/deckhouse_lib_helm-1.72.18.tgz in the sixteen modules;
3. drop METRICS_BIND_ADDRESS from each module's additionalControllerEnvs.

Until step 2 those modules keep their own copy of the variable; the value is byte-identical, so behaviour does not change in the meantime. After step 2 and before step 3, dmt's env-variables-duplicates rule reports the duplicate — which is the intended forcing function rather than a silent override.

Covered by three new cases in helm_lib_module_controller_manifests_test.yaml: loopback bind behind a proxy together with the absent readinessProbe and the present livenessProbe, wildcard bind with no proxy, and no variable at all when the module declares no metrics port. The additionalWebhooksEnvs ordering case from #210 stops using METRICS_BIND_ADDRESS as its example, since the library now owns that name. helm unittest ./tests/ — 373 passed, 84 suites. go run tools/build-doc.go --diff clean.

Checklist

• The code is covered by unit tests.
• e2e tests passed.
• Documentation updated according to the changes.
• Changes were tested in the Kubernetes cluster manually.