Certificates SANs FQDN - #713
Conversation
lhotari
left a comment
There was a problem hiding this comment.
Thanks for this. To set the bar I'm reviewing against, from my reply on #712:
wildcard certs are required for broker, bookie & zk TLS when hostname verification is enabled on the client side (it should be enabled for security reason). The "client" can be a pulsar proxy or broker (towards bk, zk).
an alternative for wildcard certs would be to list all possible hostnames for SAN.
fqdn mode is exactly that alternative, and it's the right shape for the issuer-forbids-wildcards case in #712 — where wildcard isn't a fallback the user can keep. So the question this review has to answer is narrow: does fqdn mode actually list all possible hostnames?
Today it doesn't, in three cases, and two of them hit broker and bookie — two of the three components that need it.
| Component | Headless svc? | Per-pod FQDN resolves? | SAN list complete? |
|---|---|---|---|
| zookeeper | yes | yes | yes — replicaCount is authoritative, no HPA |
| bookie | yes | yes | no — breaks on kubectl scale |
| broker | yes | yes | no — breaks under its own HPA |
| proxy | no (ClusterIP) | no | n/a — client, not a verified server |
What's solid
wildcard mode renders byte-identical to master across proxy/broker/bookie/recovery/toolset/zookeeper — I diffed it, so existing users are genuinely unaffected. An invalid sanMode fails the render with a clear message rather than silently emitting a certificate with no SANs, which is the right call. clusterDomain is respected throughout rather than hardcoding cluster.local.
Render recipe I used:
helm dependency build charts/pulsar
helm template test charts/pulsar --set "certs.internal_issuer.enabled=true,\
components.proxy=true,components.toolset=true,tls.enabled=true,tls.broker.enabled=true,\
tls.proxy.enabled=true,tls.zookeeper.enabled=true,tls.bookie.enabled=true,\
tls.autorecovery.enabled=true,tls.toolset.enabled=true" \
--set tls.common.sanMode=fqdn -s templates/tls-certs-internal.yaml
One more thing, not a blocker
Switching wildcard → fqdn changes Certificate.spec.dnsNames (so cert-manager reissues) but doesn't change any pod-template checksum — I rendered broker/ZK StatefulSets under both modes and got identical hashes. So Helm performs no restart and the tightened SANs may not take effect until processes reload. Not an outage on transition, since the old wildcard still covers existing ordinals while issuance completes, but worth a docs note that a rolling restart is needed — consistent with the existing TLS upgrade guidance.
Unrelated pre-existing bug I noticed while checking this
standalone-deployment.yaml sets subdomain: <fullname>-standalone-headless and advertises $(hostname -f), so its real name is <pod>.<fullname>-standalone-headless.<ns>.svc.<domain>. No SAN mode on master includes that — not even wildcard, which emits *.<fullname>-standalone…, a different subdomain. With hostname verification on, lookup-directed standalone connections would fail today. I'll file that separately; nothing for you to do here.
Reviewed with Codex gpt-5.6-sol and Claude Opus 5; every finding reproduced locally by rendering the chart.
| {{/* | ||
| Return the StatefulSet replica count for a certificate component. | ||
| */}} | ||
| {{- define "pulsar.certs.statefulset.replicaCount" -}} |
There was a problem hiding this comment.
This is the one that matters most, because completeness is the whole point of the mode.
This helper reads .Values.<component>.replicaCount — but for broker and proxy that value isn't the live replica count. Both StatefulSets omit spec.replicas entirely when autoscaling is on:
# broker-statefulset.yaml:34-36
{{- if not .Values.broker.autoscaling.enabled }}
replicas: {{ .Values.broker.replicaCount }}
{{- end }}
So with broker.autoscaling.enabled: true, maxReplicas: 6, the certificate covers ordinals 0-2 only and replicaCount isn't even consulted by the workload. When the HPA creates broker-3, a proxy verifying the broker's hostname fails — which is precisely the failure fqdn mode exists to prevent, on exactly the client→server hop from #712.
Bookies have no HPA, but kubectl scale breaks them the same way, and scaling bookies is routine.
Even the Helm path isn't fully safe: changing replicaCount does re-render the Certificate, but cert-manager reissues asynchronously, so the StatefulSet can create the new ordinal before the new Secret lands. For ZooKeeper the stale certificate can persist until restart, since we enable certReload for ZK clients but not the ZK server.
Options, roughly in order of preference:
- Enumerate up to
autoscaling.maxReplicaswhen autoscaling is enabled. Cheap, covers the HPA case completely, and SANs for pods that don't exist yet are harmless. - Add a
tls.<component>.extraDnsNamesescape hatch and document thatfqdncovers only the rendered replica count. - At minimum,
failwhensanMode: fqdnis combined withautoscaling.enabled: true, rather than silently issuing an incomplete certificate.
| {{- if not (has $sanMode (list "wildcard" "fqdn" "none")) -}} | ||
| {{- fail (printf "tls.common.sanMode must be one of: wildcard, fqdn, none (got %q)" $sanMode) -}} | ||
| {{- end -}} | ||
| {{- $statefulSetComponents := list "zookeeper" "bookie" "broker" "proxy" "toolset" "recovery" "function-worker" -}} |
There was a problem hiding this comment.
$component comes from the user-configurable .componentConfig.component (e.g. bookkeeper.component, broker.component), but this list — and both new helpers above — compare it against hard-coded literals.
Set bookkeeper.component: pulsar-bookie with sanMode: fqdn and pulsar-bookie isn't in this list, so no ordinal SANs are emitted at all. And since fqdn also drops the wildcard, the resulting certificate covers none of the per-pod names. Silent — no warning, no render failure.
For a user who moved to fqdn precisely because their issuer forbids wildcards (#712), that's a hostname-verification outage with no fallback.
Either derive the set from the component values rather than literals, or fail when a renamed component is used with sanMode: fqdn.
| {{- $replicaCount := (include "pulsar.certs.statefulset.replicaCount" (dict "root" $root "component" $component) | int) -}} | ||
| {{- if gt $replicaCount 0 }} | ||
| {{- range $i := until $replicaCount }} | ||
| - {{ printf "%s-%d.%s.%s.svc.%s" (printf "%s-%s" (include "pulsar.fullname" $root) $component) $i $serviceName (include "pulsar.namespace" $root) $root.Values.clusterDomain | quote }} |
There was a problem hiding this comment.
For proxy, the service name resolved above falls through to <fullname>-proxy, which does match the StatefulSet's serviceName — but proxy-service.yaml renders type: {{ .Values.proxy.service.type }} (default ClusterIP) with no clusterIP: None. Per-pod <pod>.<svc>.<ns>.svc.<domain> records only exist for headless services, so <release>-pulsar-proxy-0.<release>-pulsar-proxy… never resolves.
(bookkeeper, autorecovery and toolset services all set clusterIP: None, so theirs do resolve — this is proxy-specific.)
Lower severity than the two above: per #712 the proxy is a client toward the broker, not one of the servers whose hostname gets verified, and its service FQDN and short name remain valid. But fqdn mode does swap proxy's wildcard for three names that never resolve, which is misleading in a certificate.
Suggest either dropping proxy from $statefulSetComponents — it isn't a component this mode is for — or emitting per-pod names only when the proxy service is actually headless.
| # - wildcard (default): use wildcard SANs (existing behavior) | ||
| # - fqdn: for StatefulSet components, generate explicit per-pod FQDN SANs | ||
| # from replica counts | ||
| # - none: do not add automatic SAN entries (use tls.<component>.dnsNames and/or ipAddresses) |
There was a problem hiding this comment.
Small accuracy fix: none doesn't remove all automatic SAN entries. The service FQDN and short name are unconditional, and broker/zookeeper additionally keep their headless name, because that block sits outside the sanMode conditional. Rendered none output for broker is three entries, not the two the PR description lists.
Suggest wording it as what it actually does — skip the wildcard and per-pod names, keep the service names:
| # - none: do not add automatic SAN entries (use tls.<component>.dnsNames and/or ipAddresses) | |
| # - none: no wildcard or per-pod SANs; only the service FQDN and short name are added | |
| # (plus the headless name for broker/zookeeper). Use tls.<component>.dnsNames | |
| # and/or ipAddresses to add your own. |
Fixes #712
Motivation
Currently certificates are generated with wildcard SANs.
This PR allow to generate SANs with either :
tls.<component>.dnsNames)Modifications
Add
tls.common.sanMode:wildcardmode :fqdnmode :nonemode :Verifying this change