Add Helm subchart for cert-manager to manage TLS certificates#325
Add Helm subchart for cert-manager to manage TLS certificates#325maneeshaxyz wants to merge 1 commit intoLSFLK:mainfrom
Conversation
|
/gemini review |
![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 implements TLS support for Silver services via cert-manager and Let's Encrypt, adding a bootstrap script for Cloudflare DNS-01 challenges and updating Helm templates to manage Certificate resources. Review feedback suggests removing restrictive domain selectors in the ClusterIssuers to support multiple domains, utilizing the provided Helm helper for issuer references, and replacing a hardcoded sleep with a kubectl wait command for better reliability.
| solvers: | ||
| - dns01: | ||
| cloudflare: | ||
| apiTokenSecretRef: | ||
| name: cloudflare-api-token | ||
| key: api-token | ||
| selector: | ||
| dnsZones: | ||
| - "${DOMAIN}" |
There was a problem hiding this comment.
The dnsZones selector restricts this ClusterIssuer to only the domain provided during the bootstrap process. This will prevent the issuer from working with any other domains added to the Helm chart later. Removing this selector allows the issuer to handle any domain authorized by the Cloudflare API token.
| solvers: | |
| - dns01: | |
| cloudflare: | |
| apiTokenSecretRef: | |
| name: cloudflare-api-token | |
| key: api-token | |
| selector: | |
| dnsZones: | |
| - "${DOMAIN}" | |
| solvers: | |
| - dns01: | |
| cloudflare: | |
| apiTokenSecretRef: | |
| name: cloudflare-api-token | |
| key: api-token |
| solvers: | ||
| - dns01: | ||
| cloudflare: | ||
| apiTokenSecretRef: | ||
| name: cloudflare-api-token | ||
| key: api-token | ||
| selector: | ||
| dnsZones: | ||
| - "${DOMAIN}" |
There was a problem hiding this comment.
Similar to the staging issuer, this dnsZones selector limits the production issuer to a single domain. Removing it provides better support for the multi-domain configuration allowed by the Helm chart.
| solvers: | |
| - dns01: | |
| cloudflare: | |
| apiTokenSecretRef: | |
| name: cloudflare-api-token | |
| key: api-token | |
| selector: | |
| dnsZones: | |
| - "${DOMAIN}" | |
| solvers: | |
| - dns01: | |
| cloudflare: | |
| apiTokenSecretRef: | |
| name: cloudflare-api-token | |
| key: api-token |
| name: {{ $.Values.tls.issuer }} | ||
| kind: ClusterIssuer |
There was a problem hiding this comment.
The silver.issuerRef helper defined in _helpers.tpl should be used here to maintain consistency and reduce duplication.
{{- include "silver.issuerRef" $ | nindent 4 }}|
|
||
| # --- Verify ------------------------------------------------------------------- | ||
| info "Waiting for ClusterIssuers to become ready (up to 60s)..." | ||
| sleep 5 |
There was a problem hiding this comment.
Using a hardcoded sleep is unreliable. It is better to use kubectl wait to ensure the ClusterIssuer resources are actually ready before proceeding or finishing the script.
| sleep 5 | |
| kubectl wait --for=condition=Ready clusterissuer/le-staging clusterissuer/le-prod --timeout=60s |
There was a problem hiding this comment.
Code Review
This pull request introduces TLS support for the Silver services using cert-manager and Let's Encrypt. Key additions include a bootstrap script for cluster infrastructure setup, comprehensive documentation in the README, and Helm templates for automated certificate generation. The review feedback suggests enhancing the bootstrap script's idempotency for namespace creation and improving the sanitization logic for Kubernetes resource names to handle a wider range of domain characters.
| kubectl get namespace cert-manager >/dev/null 2>&1 \ | ||
| || kubectl create namespace cert-manager |
There was a problem hiding this comment.
Using kubectl get namespace to check for existence is prone to race conditions. It is safer to use kubectl create namespace cert-manager --dry-run=client -o yaml | kubectl apply -f - to ensure idempotency.
| kubectl get namespace cert-manager >/dev/null 2>&1 \ | |
| || kubectl create namespace cert-manager | |
| kubectl create namespace cert-manager --dry-run=client -o yaml | kubectl apply -f - |
| apiVersion: cert-manager.io/v1 | ||
| kind: Certificate | ||
| metadata: | ||
| name: {{ . | replace "." "-" }}-tls |
There was a problem hiding this comment.
The replace function might produce invalid resource names if the domain contains characters other than dots or hyphens. Consider using a more robust sanitization method or ensuring the input is validated.
1 participant
📌 Description
Implemented automated TLS certificate management with cert-manager and Let’s Encrypt in the existing Silver Helm deployment flow. Certificates are generated per configured domain, renewed automatically, and stored as Kubernetes Secrets for downstream services.
🔍 Changes Made
1. Certificate templating in Helm
2. Issuer integration
3. ACME + DNS challenge bootstrap
4. Operational documentation
Design Decisions
✅ Checklist (Email System)
🧪 Testing Instructions