feat(tls): support multiple server certificates#158
Conversation
GatewayJ
force-pushed
the
fix/multi-cert-tls-sni
branch
from
08f9642 to
7819263
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78192632a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if config.require_san_match | ||
| && let Err(failure) = | ||
| validate_tls_secret_san_match(&secret_name, &cert_bytes, &san_dns_names) |
There was a problem hiding this comment.
When enableInternodeHttps is false, legacy/single-certificate configs that omit includeGeneratedDnsNames still make san_validation_dns_names include the generated service/pod DNS names, so this now blocks user-provided TLS Secrets whose cert only covers the public endpoint. Previously the SAN check only ran for internode HTTPS, and the docs still frame generated peer DNS coverage as required only in that mode; keep that guard or avoid adding generated names for this case.
Useful? React with 👍 / 👎.
| let certificate_name = certificate_name(tenant, &entry); | ||
| desired_managed_certificate_names.insert(certificate_name.clone()); |
There was a problem hiding this comment.
If two spec.tls.certificates entries set the same certManager.certificateName, this insert silently collapses them while the loop still applies the same cert-manager Certificate twice with different secretName/DNS data; the second apply wins, leaving the earlier entry to mount or report a Secret that is no longer reconciled by its Certificate. Resolve and reject duplicate managed Certificate names before applying them.
Useful? React with 👍 / 👎.
| cert_manager, | ||
| &entry.hosts, | ||
| include_generated_dns_names(&entry), | ||
| &secret_name, |
There was a problem hiding this comment.
For manageCertificate=true entries, this secretName becomes Certificate.spec.secretName, but the multi-certificate validation never rejects two managed entries sharing the same Secret. In that configuration cert-manager has two Certificate resources racing to write different keypairs into one Secret, so one SNI mount can end up serving the other certificate; require managed certificate Secret names to be unique, or require a single Certificate entry to cover all of those hosts.
Useful? React with 👍 / 👎.
1 participant
Type of Change
Related Issues
Fixes #156
Summary of Changes
This PR extends Tenant TLS configuration to support multiple server certificates for public and internal RustFS hosts.
It adds
spec.tls.certificates[]while keeping the existing single-certificatespec.tls.certManagerpath backward compatible. Each certificate entry can reconcile its own cert-managerCertificateand Secret, and the operator now projects certificates into the RustFS TLS directory layout used for SNI selection:rustfs_cert.pemandrustfs_key.pem<host>/rustfs_cert.pemand<host>/rustfs_key.pemcaTrustor the default certificate trust settingsThe change also updates Tenant TLS status, generated CRDs, docs, and examples.
Checklist
make pre-commit(fmt-check + clippy + test + console-lint + console-fmt-check)[Unreleased](if user-visible change; N/A because this repository does not currently include CHANGELOG.md)Impact
Verification
Additional Notes
The CRD manifests under
deploy/rustfs-operator/crds/were regenerated from the Rust types so chart installs accept the newspec.tls.certificatesand top-levelspec.tls.caTrustfields.Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.