From b4c66f2b55ef447ebdcdf7ae1fc3dfca65e649f7 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Sun, 19 Jul 2026 15:14:41 +0300 Subject: [PATCH] docs(rabbitmq): record the metrics port change when TLS is enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enabling TLS swaps the published metrics port: the operator exposes either prometheus:15692 or prometheus-tls:15691 and never both, deliberately, so a scrape config selecting every port does not count each node twice. That choice keys off TLS alone rather than off disableNonTLSListeners, so a ServiceMonitor selecting the port by the name prometheus stops matching once TLS is on, even though the broker keeps listening on 15692. Since tls.enabled inherits external, this reaches existing instances on upgrade with no user action. Re-adding 15692 through spec.override.service looks like the fix and is not. updatePorts keeps only port NAMES the operator owns and carries their NodePort across, so every reconcile drops the re-added entry; the override then reinstates it with nodePort unset, which the apiserver reads as "allocate" on a LoadBalancer. The resulting write retriggers the operator's own Service watch and the NodePort is reallocated each round. ClusterIP escapes it only because Update zeroes every NodePort there, leaving the patched spec equal to the live one — and external:true, the configuration that turns TLS on by default, is precisely the one that would loop. Trading a renamed metrics port for continuous LoadBalancer and kube-proxy reprogramming is not a trade worth making. So the chart renders no service override at all. The README says what changed and how to retarget a scrape config, including that 15691 is HTTPS under the release CA and so needs the trust anchor to verify, and the template comment records why neither direction of override is used. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- packages/apps/rabbitmq/README.md | 8 +++- .../apps/rabbitmq/templates/rabbitmq.yaml | 39 ++++++++++++------- .../rabbitmq/tests/rabbitmq_tls_test.yaml | 24 ++++++------ 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/packages/apps/rabbitmq/README.md b/packages/apps/rabbitmq/README.md index 769886b91..66bcfaf68 100644 --- a/packages/apps/rabbitmq/README.md +++ b/packages/apps/rabbitmq/README.md @@ -48,7 +48,13 @@ This chart adds TLS listeners for AMQP (port 5671) and the management HTTPS inte **The plaintext listeners stay open.** AMQP on 5672 and management on 15672 keep listening whenever TLS is on, and when `external` is `true` they are published on the LoadBalancer alongside the TLS ports. The operator's `spec.tls.disableNonTLSListeners` is the only switch that closes them, and it closes them at the broker level — it writes `listeners.tcp = none` into `rabbitmq.conf`, so the socket is never opened on any interface and in-cluster clients lose plaintext access at the same instant as external ones. There is no operator-supported way to drop a port from the published Service alone: `spec.override.service.spec.ports` is applied as a strategic merge patch keyed on `port`, which can add or mutate entries but never remove them, and the operator regenerates its default port set on every reconcile. This chart therefore does not set the flag, and connecting over TLS is currently the client's choice rather than something the endpoint enforces. Treat an `external: true` RabbitMQ as reachable in plaintext from outside the cluster and restrict it at the network layer if that is not acceptable. -**Upgrading an existing `external: true` instance turns TLS on.** `tls.enabled` defaults to the value of `external`, so an instance that predates this feature gets a certificate chain without any change to its own configuration. Two consequences worth planning for: the broker pods roll once, because the operator mounts the new TLS Secret into the pod template and that is a StatefulSet template change; and the instance gains a dependency on cert-manager, since the operator will not finish reconciling until the Secret has been issued. No client breaks — the plaintext listeners stay open, as above — but a stateful broker does restart. Set `tls.enabled: false` explicitly to keep the previous behaviour. +**Upgrading an existing `external: true` instance turns TLS on.** `tls.enabled` defaults to the value of `external`, so an instance that predates this feature gets a certificate chain without any change to its own configuration. Three consequences worth planning for. + +The broker pods roll once, because the operator mounts the new TLS Secret into the pod template and that is a StatefulSet template change. The instance gains a dependency on cert-manager, since the operator will not finish reconciling until the Secret has been issued. + +And **the published metrics port changes**. The operator exposes *either* `prometheus` (15692) *or* `prometheus-tls` (15691) on its Service, never both — deliberately, so that a scrape config selecting every port does not count each node twice — and that choice keys off TLS alone. Enabling TLS therefore withdraws 15692 from the Service and publishes 15691 in its place. The broker itself keeps listening on 15692, but a `ServiceMonitor` or `VMServiceScrape` selecting the port by the name `prometheus` stops matching. Retarget it to `prometheus-tls`, which is HTTPS under the release CA and so needs a `tlsConfig` carrying the trust anchor described below. The chart does not paper over this by re-adding 15692 through `spec.override.service`: the operator drops any port name it does not own on each reconcile and the override would reinstate it without a NodePort, which on a LoadBalancer means a fresh allocation and another reconcile every round. + +Messaging clients are unaffected — the plaintext AMQP and management listeners stay open, as above. Set `tls.enabled: false` explicitly to keep the previous behaviour entirely. Intra-cluster Erlang distribution (port 25672) also remains plaintext, and `disableNonTLSListeners` would not change that either — the operator never applies it to the headless Service that carries the distribution port. Inter-node mTLS via `spec.tls.caSecretName` is not wired by this chart. diff --git a/packages/apps/rabbitmq/templates/rabbitmq.yaml b/packages/apps/rabbitmq/templates/rabbitmq.yaml index 32d0a875b..2181b69c4 100644 --- a/packages/apps/rabbitmq/templates/rabbitmq.yaml +++ b/packages/apps/rabbitmq/templates/rabbitmq.yaml @@ -31,22 +31,31 @@ spec: secretName: {{ .Release.Name }}-tls {{- end }} override: - {{- /* No spec.override.service here, deliberately. Listing only the TLS ports - would look like it removes the plaintext ones from the published Service, - and it does not: the operator rebuilds its full default port set from - generateServicePortsMap() on every reconcile and only then applies the - override as a strategic merge patch keyed on `port` (corev1.ServiceSpec - tags Ports with patchMergeKey:"port"), which can add or mutate entries but - never remove them. A partial port list is therefore a no-op — and with TLS - on, amqps:5671 and management-tls:15671 are byte-identical to what the - operator already generates. Plaintext amqp:5672 and management:15672 stay - published, including on the LoadBalancer when external is true. + {{- /* No spec.override.service here, and both directions are deliberate. - The only switch that closes them is spec.tls.disableNonTLSListeners, which - writes `listeners.tcp = none` into rabbitmq.conf — a broker-level change - that closes the socket on every interface at once, dropping in-cluster - plaintext clients along with external ones. It is not set here, so - enabling TLS never breaks a working connection. See README "TLS scope". */}} + Removing a port is impossible: the operator rebuilds its default port set + from generateServicePortsMap() on every reconcile and only then applies the + override as a strategic merge patch keyed on `port` (corev1.ServiceSpec + tags Ports with patchMergeKey:"port"), which can add or mutate an entry but + never delete one. A TLS-only port list would be a no-op that reads like a + guarantee. So plaintext amqp:5672 and management:15672 stay published, + including on the LoadBalancer when external is true; only + spec.tls.disableNonTLSListeners closes them, and it writes + `listeners.tcp = none` into rabbitmq.conf — broker level, so it drops + in-cluster clients along with external ones. It is not set here. + + Adding a port is possible but not safe here. Enabling TLS swaps the + published metrics port from prometheus:15692 to prometheus-tls:15691 + (service.go: "We expose either 15692 or 15691 in the Service, but not + both"), and adding 15692 back through this override would churn: on each + reconcile updatePorts drops it, because it keeps only operator-known port + NAMES and carries their NodePort across, then the override re-adds it with + nodePort unset. For a LoadBalancer the apiserver reads that as "allocate", + assigns a fresh NodePort, and the resulting write retriggers the operator's + own Service watch. ClusterIP escapes it only because Update() zeroes every + NodePort there, making the patched spec equal to the live one. The default + external configuration is exactly the one that would loop, so the port + change is documented in README "TLS scope" instead. */}} statefulSet: spec: template: diff --git a/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml b/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml index c1afb16d6..1979e9810 100644 --- a/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml +++ b/packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml @@ -154,18 +154,18 @@ tests: # ----------------------------------------------------------------------- # B1: no spec.override.service is rendered, in any combination. # - # Listing only the TLS ports there would READ as "plaintext is not published", - # but it cannot do that. The operator rebuilds its default port set from - # generateServicePortsMap() on every reconcile and only then applies the - # override as a strategic merge patch keyed on `port` (corev1.ServiceSpec tags - # Ports patchMergeKey:"port"), so a partial list can add or mutate entries but - # never remove them. With TLS on, amqps:5671 and management-tls:15671 are also - # byte-identical to what the operator already generates, making such an - # override a pure no-op that credits itself with a guarantee it does not - # provide. These tests pin its absence so it cannot come back as decoration. + # Not a TLS-only port list: the merge is a strategic merge patch keyed on + # `port`, which can add or mutate an entry but never delete one, so omitting + # amqp:5672 would be a no-op that reads like a guarantee to close plaintext. # - # Closing plaintext requires spec.tls.disableNonTLSListeners, which is a - # broker-level switch affecting in-cluster clients too — see the block below. + # And not a re-added prometheus:15692 either, even though enabling TLS swaps the + # published metrics port to prometheus-tls:15691. Adding it back would churn: + # updatePorts keeps only operator-known port NAMES and carries their NodePort + # across, so each reconcile drops the re-added entry and the override reinstates + # it with nodePort unset; on a LoadBalancer the apiserver reallocates, and the + # write retriggers the operator's Service watch. ClusterIP escapes only because + # NodePorts are zeroed there. external:true is the default TLS path, so that is + # the configuration that would loop. # ----------------------------------------------------------------------- - it: no service override is rendered when TLS is on and external is false @@ -195,6 +195,8 @@ tests: path: kind value: RabbitmqCluster asserts: + # The LoadBalancer path specifically: a port entry here would be + # reallocated on every reconcile. - notExists: path: spec.override.service