feat(rabbitmq): add TLS support for AMQPS and management HTTPS#2683
feat(rabbitmq): add TLS support for AMQPS and management HTTPS#2683Arsolitt wants to merge 6 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds tri-state TLS support to the RabbitMQ application. TLS configuration is controlled by an optional ChangesRabbitMQ TLS Configuration Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
area/uncategorized
Arsolitt
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei,
lllamnyp,
myasnikovdaniil and
sircthulhu
as code owners
dosubot
Bot
added
the
lifecycle/active
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces TLS support for the RabbitMQ managed service, allowing secure communication for AMQPS and the management interface. It leverages cert-manager to handle certificate generation and rotation, while providing a flexible configuration schema that defaults to secure settings when external access is enabled. The changes ensure that existing plaintext traffic remains unaffected while enabling encrypted alternatives. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml (2)
136-169: ⚡ Quick winAdd tri-state coverage for LoadBalancer port overrides.
The unset-
tls.enabledpath is validated forspec.tls.secretNameand management SSL, but not forspec.override.service.spec.ports. A regression where LB overrides require explicittls.enabled: truewould slip through.Suggested additional test
+ - it: renders LoadBalancer override ports when tls.enabled is unset (default) and external is true + release: + name: myrabbitmq + namespace: tenant-test + values: + - fixtures/tls_default_values.yaml + set: + external: true + _namespace.host: example.com + documentSelector: + path: kind + value: RabbitmqCluster + asserts: + - contains: + path: spec.override.service.spec.ports + content: + name: management-tls + port: 15671 + targetPort: 15671 + protocol: TCPAlso applies to: 253-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml` around lines 136 - 169, Add tri-state coverage for LoadBalancer port overrides: extend the existing test blocks (the ones using fixtures/tls_default_values.yaml, release name myrabbitmq, documentSelector kind: RabbitmqCluster) to also assert spec.override.service.spec.ports when tls.enabled is unset (default) and external: true; specifically add an asserts block that checks spec.override.service.spec.ports contains the expected LB port entries (the same check you have for the other two tests that validate spec.tls.secretName and management.ssl.port) and mirror the same additional test for the other range (lines 253-293) so the unset/true/false matrix is fully covered.
320-330: ⚡ Quick winAvoid index-coupled baseline assertion.
Line 330 uses
documentIndex: 0, which is brittle if another manifest is rendered first. Select by kind like the other tests to keep this baseline stable.Stable selector-based variant
- it: always renders RabbitmqCluster release: name: myrabbitmq namespace: tenant-test set: tls.enabled: false external: false + documentSelector: + path: kind + value: RabbitmqCluster asserts: - isKind: of: RabbitmqCluster - documentIndex: 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml` around lines 320 - 330, The baseline test currently asserts the rendered RabbitmqCluster using a documentIndex (documentIndex: 0), which is brittle; update the assertion to select the RabbitmqCluster by kind instead of by index so the test doesn't break if other manifests are rendered first — replace the assertion that includes "documentIndex: 0" with a selector-style isKind check that targets "RabbitmqCluster" (matching how other tests select by kind) so the test locates the resource by its kind rather than its position.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/rabbitmq/templates/certmanager.yaml`:
- Around line 54-57: The SAN list creation in the $dnsNames template hardcodes
"cluster.local" which breaks clusters with custom DNS; change the third entry to
use a configurable cluster domain (e.g. .Values.clusterDomain) instead of the
literal "cluster.local" so the SAN becomes (printf "%s.%s.svc.%s" .Release.Name
.Release.Namespace .Values.clusterDomain), and add a default clusterDomain:
"cluster.local" to values.yaml so .Values.clusterDomain is available.
- Around line 5-10: The current logic only checks hasKey and then assigns
$tlsEnabled = .Values.tls.enabled which treats a null value as false and
prevents falling back to .Values.external; change the branch to explicitly treat
null as "inherit" by checking for nil (e.g., use hasKey and ne
.Values.tls.enabled nil) or use Helm's default semantics to set $tlsEnabled to
.Values.tls.enabled when it is non-null, otherwise to .Values.external; update
the assignment that sets $tlsEnabled (referencing $tlsEnabled,
.Values.tls.enabled, .Values.external, and hasKey) so that a null
.Values.tls.enabled falls back to .Values.external.
In `@packages/apps/rabbitmq/templates/rabbitmq.yaml`:
- Around line 4-9: The template treats a present-but-null .Values.tls.enabled as
an explicit value because it uses hasKey, which causes the external fallback
(.Values.external) to be skipped; replace the tri-state check with a
default-based expression (e.g. set $tlsEnabled using default .Values.external
.Values.tls.enabled or the Helm "or" idiom) so that nil/absent tls.enabled falls
back to .Values.external and downstream logic that emits spec.tls.secretName
uses the correct value; update the block that defines $tlsEnabled (currently
using hasKey .Values.tls "enabled" and .Values.tls.enabled) to the single
default/or expression.
---
Nitpick comments:
In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml`:
- Around line 136-169: Add tri-state coverage for LoadBalancer port overrides:
extend the existing test blocks (the ones using
fixtures/tls_default_values.yaml, release name myrabbitmq, documentSelector
kind: RabbitmqCluster) to also assert spec.override.service.spec.ports when
tls.enabled is unset (default) and external: true; specifically add an asserts
block that checks spec.override.service.spec.ports contains the expected LB port
entries (the same check you have for the other two tests that validate
spec.tls.secretName and management.ssl.port) and mirror the same additional test
for the other range (lines 253-293) so the unset/true/false matrix is fully
covered.
- Around line 320-330: The baseline test currently asserts the rendered
RabbitmqCluster using a documentIndex (documentIndex: 0), which is brittle;
update the assertion to select the RabbitmqCluster by kind instead of by index
so the test doesn't break if other manifests are rendered first — replace the
assertion that includes "documentIndex: 0" with a selector-style isKind check
that targets "RabbitmqCluster" (matching how other tests select by kind) so the
test locates the resource by its kind rather than its position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3980558a-e1ed-4ebc-9242-c75c97984d18
📒 Files selected for processing (12)
api/apps/v1alpha1/rabbitmq/types.goapi/apps/v1alpha1/rabbitmq/zz_generated.deepcopy.gopackages/apps/rabbitmq/Makefilepackages/apps/rabbitmq/README.mdpackages/apps/rabbitmq/templates/certmanager.yamlpackages/apps/rabbitmq/templates/rabbitmq.yamlpackages/apps/rabbitmq/tests/certmanager_test.yamlpackages/apps/rabbitmq/tests/fixtures/tls_default_values.yamlpackages/apps/rabbitmq/tests/rabbitmq_tls_test.yamlpackages/apps/rabbitmq/values.schema.jsonpackages/apps/rabbitmq/values.yamlpackages/system/rabbitmq-rd/cozyrds/rabbitmq.yaml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces TLS support for the RabbitMQ application, including the necessary API changes, Helm templates for cert-manager integration, and updated schema definitions. It also adds comprehensive unit tests to verify the TLS configuration and service port overrides. Regarding the implementation, consider parameterizing the hardcoded .cluster.local domain in the cert-manager templates to better support environments with custom cluster domains.
| {{- $dnsNames := list | ||
| .Release.Name | ||
| (printf "%s.%s.svc" .Release.Name .Release.Namespace) | ||
| (printf "%s.%s.svc.cluster.local" .Release.Name .Release.Namespace) |
There was a problem hiding this comment.
The internal cluster domain is hardcoded to .cluster.local. While this is the Kubernetes default, some environments might use a custom domain. Consider if this should be parameterized or if it's acceptable for this PaaS environment.
Extend RabbitMQ values schema with TLS configuration options, update Go API types, regenerate CRD artifacts, and add TLS fields to the rabbitmq-rd operator config. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Add cert-manager Certificate and Issuer resources for RabbitMQ TLS, and wire TLS secret references into the RabbitMQ cluster manifest. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Add Helm unittest cases for cert-manager resource generation and RabbitMQ TLS configuration, with a fixture file for default TLS values. Signed-off-by: Arsolitt <arsolitt@gmail.com>
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Arsolitt
force-pushed
the
feat/tls-rabbitmq
branch
from
676576e to
4f857f1
Compare
…e cluster domain for cert SANs The previous hasKey-only check treated an explicit null tls.enabled as a real value (false), skipping the external fallback. Add a nil guard so null is handled the same as absent — TLS inherits from .Values.external. Replace the hardcoded cluster.local suffix in cert SANs with the platform-injected _cluster.cluster-domain value (defaulting to cozy.local), consistent with how clickhouse, foundationdb, mongodb, and opensearch resolve the cluster DNS domain. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Arsolitt <arsolitt@gmail.com>
… LB port tri-state coverage Update certmanager_test.yaml SAN assertions to match the new default cluster domain (cozy.local) and add a test that verifies a custom _cluster.cluster-domain propagates into the FQDN SAN. Add a tri-state LoadBalancer port override test (tls.enabled unset + external=true) to cover the case that was untested in the existing matrix. Replace documentIndex: 0 in the baseline RabbitmqCluster test with a documentSelector so the test is stable regardless of render order. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Arsolitt <arsolitt@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/apps/rabbitmq/templates/certmanager.yaml (2)
5-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
nullas “inherit”, not as disabled.A present-but-null
tls.enabledstill takes thehasKeybranch, sotls.enabled: nullresolves to falsey here instead of falling back to.Values.external. That breaks the advertised tri-state inheritance for external releases.Proposed fix
-{{- $tlsEnabled := false }} -{{- if hasKey .Values.tls "enabled" }} - {{- $tlsEnabled = .Values.tls.enabled }} -{{- else }} - {{- $tlsEnabled = .Values.external }} -{{- end }} +{{- $tlsEnabled := .Values.external }} +{{- $tlsExplicit := dig "tls" "enabled" nil .Values }} +{{- if ne $tlsExplicit nil }} + {{- $tlsEnabled = $tlsExplicit }} +{{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/templates/certmanager.yaml` around lines 5 - 10, The template treats a present-but-null .Values.tls.enabled as false because hasKey alone is used; change the logic so null means "inherit" by checking for presence AND non-null. Replace the if that sets $tlsEnabled to use: if hasKey .Values.tls "enabled" and not (eq .Values.tls.enabled nil) then set $tlsEnabled = .Values.tls.enabled else set $tlsEnabled = .Values.external; reference the variables $tlsEnabled, .Values.tls.enabled and .Values.external and the hasKey check when making the change.
54-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hardcode
cluster.localinto the certificate SANs.The RabbitMQ TLS contract here is
svc.<cluster-domain>, not a fixed suffix. On clusters with a custom domain, the fully qualified service name will no longer match the certificate. (rabbitmq.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/templates/certmanager.yaml` around lines 54 - 57, The template currently hardcodes "cluster.local" in the SANs (inside the $dnsNames list), which breaks clusters with custom cluster domains; update the $dnsNames construction to use a configurable cluster domain instead (e.g., use default "cluster.local" for backward compatibility via a chart value like .Values.clusterDomain) and build the FQDN with printf "%s.svc.%s" .Release.Name (default .Values.clusterDomain) so the SANs reflect the actual cluster domain; adjust any values.yaml to document .Values.clusterDomain default.packages/apps/rabbitmq/templates/rabbitmq.yaml (1)
4-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
nullas “inherit” here too.This has the same tri-state bug as the cert-manager template:
tls.enabled: nullis treated as an explicit value and skips the.Values.externalfallback, so external deployments can silently lose TLS wiring.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/templates/rabbitmq.yaml` around lines 4 - 9, The template treats tls.enabled=null as an explicit value and skips the external fallback; change the logic that computes $tlsEnabled so null is treated like "not set" — i.e., if .Values.tls.enabled is missing or nil use .Values.external, otherwise use .Values.tls.enabled; you can implement this by replacing the hasKey branch with a check that .Values.tls.enabled is not nil (or by using Helm's default helper: default .Values.external .Values.tls.enabled) to assign $tlsEnabled.
🧹 Nitpick comments (1)
packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml (1)
25-38: ⚡ Quick winScope TLS-off assertions to management SSL keys, not full
additionalConfigabsence.These checks can fail if unrelated RabbitMQ config is added later. Prefer asserting that
management.ssl.*entries are absent instead of assertingspec.rabbitmq.additionalConfigis entirely missing.Proposed test change
- - notExists: - path: spec.rabbitmq.additionalConfig + - notMatchRegex: + path: spec.rabbitmq.additionalConfig + pattern: "management\\.ssl\\." @@ - - notExists: - path: spec.rabbitmq.additionalConfig + - notMatchRegex: + path: spec.rabbitmq.additionalConfig + pattern: "management\\.ssl\\."Also applies to: 174-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml` around lines 25 - 38, The test currently asserts that spec.rabbitmq.additionalConfig does not exist when tls.enabled is false; instead, change the assertions to only scope absence to the management SSL keys (e.g., assert notExists for paths like spec.rabbitmq.additionalConfig.management.ssl and/or spec.rabbitmq.additionalConfig.management.ssl.*) so unrelated additionalConfig entries won't break the test; update the two test cases (the one shown and the one at lines 174-190) to replace the top-level notExists on spec.rabbitmq.additionalConfig with notExists checks targeting spec.rabbitmq.additionalConfig.management.ssl (or the specific keys under management.ssl) while leaving other parts of additionalConfig allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/apps/rabbitmq/templates/certmanager.yaml`:
- Around line 54-58: The DNS SAN list variable $dnsNames in certmanager.yaml
currently only includes the client service names; update it to also include the
headless nodes service SANs required by RabbitMQ Cluster Operator by adding
entries for the "-nodes" service variants (e.g., add (printf "%s-nodes"
.Release.Name), (printf "%s-nodes.%s.svc" .Release.Name .Release.Namespace) and
(printf "%s-nodes.%s.svc.cluster.local" .Release.Name .Release.Namespace) to the
list) so node-to-node TLS hostname verification succeeds.
---
Duplicate comments:
In `@packages/apps/rabbitmq/templates/certmanager.yaml`:
- Around line 5-10: The template treats a present-but-null .Values.tls.enabled
as false because hasKey alone is used; change the logic so null means "inherit"
by checking for presence AND non-null. Replace the if that sets $tlsEnabled to
use: if hasKey .Values.tls "enabled" and not (eq .Values.tls.enabled nil) then
set $tlsEnabled = .Values.tls.enabled else set $tlsEnabled = .Values.external;
reference the variables $tlsEnabled, .Values.tls.enabled and .Values.external
and the hasKey check when making the change.
- Around line 54-57: The template currently hardcodes "cluster.local" in the
SANs (inside the $dnsNames list), which breaks clusters with custom cluster
domains; update the $dnsNames construction to use a configurable cluster domain
instead (e.g., use default "cluster.local" for backward compatibility via a
chart value like .Values.clusterDomain) and build the FQDN with printf
"%s.svc.%s" .Release.Name (default .Values.clusterDomain) so the SANs reflect
the actual cluster domain; adjust any values.yaml to document
.Values.clusterDomain default.
In `@packages/apps/rabbitmq/templates/rabbitmq.yaml`:
- Around line 4-9: The template treats tls.enabled=null as an explicit value and
skips the external fallback; change the logic that computes $tlsEnabled so null
is treated like "not set" — i.e., if .Values.tls.enabled is missing or nil use
.Values.external, otherwise use .Values.tls.enabled; you can implement this by
replacing the hasKey branch with a check that .Values.tls.enabled is not nil (or
by using Helm's default helper: default .Values.external .Values.tls.enabled) to
assign $tlsEnabled.
---
Nitpick comments:
In `@packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml`:
- Around line 25-38: The test currently asserts that
spec.rabbitmq.additionalConfig does not exist when tls.enabled is false;
instead, change the assertions to only scope absence to the management SSL keys
(e.g., assert notExists for paths like
spec.rabbitmq.additionalConfig.management.ssl and/or
spec.rabbitmq.additionalConfig.management.ssl.*) so unrelated additionalConfig
entries won't break the test; update the two test cases (the one shown and the
one at lines 174-190) to replace the top-level notExists on
spec.rabbitmq.additionalConfig with notExists checks targeting
spec.rabbitmq.additionalConfig.management.ssl (or the specific keys under
management.ssl) while leaving other parts of additionalConfig allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1514db0b-2ffb-49d0-9cc6-8db8e053645d
📒 Files selected for processing (12)
api/apps/v1alpha1/rabbitmq/types.goapi/apps/v1alpha1/rabbitmq/zz_generated.deepcopy.gopackages/apps/rabbitmq/Makefilepackages/apps/rabbitmq/README.mdpackages/apps/rabbitmq/templates/certmanager.yamlpackages/apps/rabbitmq/templates/rabbitmq.yamlpackages/apps/rabbitmq/tests/certmanager_test.yamlpackages/apps/rabbitmq/tests/fixtures/tls_default_values.yamlpackages/apps/rabbitmq/tests/rabbitmq_tls_test.yamlpackages/apps/rabbitmq/values.schema.jsonpackages/apps/rabbitmq/values.yamlpackages/system/rabbitmq-rd/cozyrds/rabbitmq.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/apps/rabbitmq/README.md
| {{- $dnsNames := list | ||
| .Release.Name | ||
| (printf "%s.%s.svc" .Release.Name .Release.Namespace) | ||
| (printf "%s.%s.svc.cluster.local" .Release.Name .Release.Namespace) | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -path "*rabbitmq*" -name "*.yaml" | head -20Repository: cozystack/cozystack
Length of output: 234
🏁 Script executed:
cat -n packages/apps/rabbitmq/templates/certmanager.yamlRepository: cozystack/cozystack
Length of output: 2815
🏁 Script executed:
# Check for headless service definition in RabbitMQ chart
find packages/apps/rabbitmq -name "*.yaml" -o -name "*.yml" | xargs grep -l "headless\|-nodes" 2>/dev/nullRepository: cozystack/cozystack
Length of output: 47
🏁 Script executed:
find packages/apps/rabbitmq -type f \( -name "*.yaml" -o -name "*.yml" \)Repository: cozystack/cozystack
Length of output: 551
🏁 Script executed:
grep -r "kind: Service" packages/apps/rabbitmq/templates/Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
cat packages/apps/rabbitmq/templates/service.yaml 2>/dev/null || echo "File not found"Repository: cozystack/cozystack
Length of output: 78
🏁 Script executed:
# Search for any reference to nodes, headless, or clusterIP settings
rg -i "headless|clusterIP|serviceName" packages/apps/rabbitmq/Repository: cozystack/cozystack
Length of output: 191
🏁 Script executed:
cat -n packages/apps/rabbitmq/templates/rabbitmq.yamlRepository: cozystack/cozystack
Length of output: 5047
🏁 Script executed:
cat -n packages/apps/rabbitmq/tests/rabbitmq_tls_test.yaml | head -100Repository: cozystack/cozystack
Length of output: 3482
🌐 Web query:
RabbitMQ Cluster Operator TLS certificate -nodes SAN requirements
💡 Result:
When configuring TLS for a RabbitMQ cluster deployed with the RabbitMQ Cluster Operator, the server certificate must include specific Subject Alternative Name (SAN) attributes to ensure proper identification and trust for both internal node-to-node communication and client access [1][2]. The certificate's SAN field must contain at least the following attributes: 1. Wildcard entry for internal node communication: *.<cluster-name>-nodes.<namespace>.svc.<cluster-domain> [1][2] 2. Client service DNS name: <cluster-name>.<namespace>.svc.<cluster-domain> [1][2] If your environment does not permit the use of wildcards, you must instead provide a separate SAN attribute for every individual RabbitMQ node in the cluster [1][2]. For a cluster named myrabbit in the mynamespace namespace using the default cluster.local domain, the required SANs would be: * myrabbit-server-0.myrabbit-nodes.mynamespace.svc.cluster.local * myrabbit-server-1.myrabbit-nodes.mynamespace.svc.cluster.local * myrabbit-server-2.myrabbit-nodes.mynamespace.svc.cluster.local * myrabbit.mynamespace.svc.cluster.local Additionally, depending on the service type defined in the RabbitmqCluster resource, further SAN attributes may be required [1][2]. For example, if you are using the NodePort service type, you must include the external IP address of each Kubernetes node in the SAN list [1][2].
Citations:
- 1: https://github.com/rabbitmq/cluster-operator/tree/main/docs/examples/tls
- 2: https://www.rabbitmq.com/kubernetes/operator/using-operator
🏁 Script executed:
# Check test fixtures to see if they mention nodes SAN
cat packages/apps/rabbitmq/tests/fixtures/tls_default_values.yamlRepository: cozystack/cozystack
Length of output: 343
🏁 Script executed:
# Search for any existing references to -nodes in the chart
rg -i "nodes" packages/apps/rabbitmq/ --type yamlRepository: cozystack/cozystack
Length of output: 45
Add the -nodes headless service SAN to the leaf certificate.
RabbitMQ Cluster Operator requires that certificates cover both the client service DNS and *.<cluster-name>-nodes.<namespace>.svc.cluster.local (or equivalent per-node SANs) for node-to-node TLS communication. The current list only includes client service names, which can cause hostname verification failures during inter-node communication. (rabbitmq.com)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/apps/rabbitmq/templates/certmanager.yaml` around lines 54 - 58, The
DNS SAN list variable $dnsNames in certmanager.yaml currently only includes the
client service names; update it to also include the headless nodes service SANs
required by RabbitMQ Cluster Operator by adding entries for the "-nodes" service
variants (e.g., add (printf "%s-nodes" .Release.Name), (printf "%s-nodes.%s.svc"
.Release.Name .Release.Namespace) and (printf "%s-nodes.%s.svc.cluster.local"
.Release.Name .Release.Namespace) to the list) so node-to-node TLS hostname
verification succeeds.
1 participant
What this PR does
Adds TLS support to the RabbitMQ managed service via cert-manager.
tls.enabledfield with tri-state semantics: when unset, defaults to the value ofexternal(auto-on for externally published services, off for cluster-internal). Explicittls.enabledalways wins..ns.svc,.ns.svc.<cluster-domain>) plus the external hostname whenexternal: true.spec.tls.secretName, enabling AMQPS on port 5671 and Management HTTPS on port 15671.Intra-cluster Erlang distribution (port 25672) stays plaintext even when TLS is enabled. RabbitMQ operator supports
spec.tls.disableNonTLSListenersand inter-node mTLS viaspec.tls.caSecretName; enabling Erlang distribution TLS is tracked as a follow-up and is not wired by this chart yet.Verified end-to-end on a sandbox cluster: cert chain reaches Ready, AMQPS handshake succeeds against the tenant service.
Release note
Summary by CodeRabbit
New Features
tls.enabled: when unset, TLS follows the existingexternalsetting; explicit value overrides it.Documentation
Tests
Chores