Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes shift certificate provisioning from HTTP01 ingress-based validation to DNS01 DigitalOcean DNS provider validation. A new wildcard-certificate Helm chart is introduced to manage certificate generation, external-dns is removed, the reflector Helm chart is added, and the DigitalOcean DNS secret configuration is reorganized. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc63e211-8917-4eb8-8b10-bfc4594dd49d
📒 Files selected for processing (12)
helm/cluster-issuer/chart/templates/cluster-issuer.yamlhelm/cluster-issuer/chart/values.yamlhelm/gitlab/values.yamlhelm/wildcard-certificate/chart/.helmignorehelm/wildcard-certificate/chart/Chart.yamlhelm/wildcard-certificate/chart/templates/_helpers.tplhelm/wildcard-certificate/chart/templates/wildcard-certificate.yamlhelm/wildcard-certificate/chart/values.yamlhelm/wildcard-certificate/values.yamlterraform/dns.tfterraform/helm.tfterraform/kubernetes.tf
📜 Review details
🧰 Additional context used
🪛 YAMLlint (1.38.0)
helm/cluster-issuer/chart/templates/cluster-issuer.yaml
[error] 15-15: too many spaces inside braces
(braces)
[error] 15-15: too many spaces inside braces
(braces)
[error] 16-16: too many spaces inside braces
(braces)
[error] 16-16: too many spaces inside braces
(braces)
helm/wildcard-certificate/chart/templates/wildcard-certificate.yaml
[error] 4-4: too many spaces inside braces
(braces)
[error] 4-4: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 5-5: too many spaces inside braces
(braces)
[error] 7-7: too many spaces inside braces
(braces)
[error] 7-7: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
[error] 9-9: too many spaces inside braces
(braces)
[error] 11-11: too many spaces inside braces
(braces)
[error] 11-11: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 13-13: too many spaces inside braces
(braces)
[error] 18-18: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (1)
helm/cluster-issuer/chart/templates/cluster-issuer.yaml (1)
12-16: DNS-01 solver wiring looks correct; yamllint brace warnings are false positives for Helm templates.Cross-checked against
terraform/kubernetes.tf(kubernetes_secret_v1.do_dns_secretin the cert-manager namespace with keypassword) andhelm/cluster-issuer/chart/values.yaml— thetokenSecretRefresolves to the correct secret/key. cert-manager requires the DO token secret to live in the same namespace as cert-manager itself (not the ClusterIssuer), and Terraform places it there.Two small considerations worth confirming:
- No
selectoris set on the solver, so this DNS-01 config applies to all certificates using this issuer. That's fine for a single-tenant cluster but will need scoping if you later add HTTP-01 for other domains.- The yamllint "too many spaces inside braces" errors on lines 15-16 are noise from yamllint not understanding Go template syntax — no action needed.
| privateKeySecretName: letsencrypt-account-key | ||
| ingressClass: nginx No newline at end of file | ||
| secretName: do-dns-secret | ||
| secretKey: password No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
secretKey: password is misleading for a DigitalOcean API token.
The value being stored is a DigitalOcean PAT (var.do_dns_token, marked sensitive in terraform/variables.tf), not a password. Using password as the key works functionally but is confusing during on-call debugging and in logs. Consider access-token (cert-manager convention for the DO webhook) to match typical examples:
Proposed rename
-secretName: do-dns-secret
-secretKey: password
+secretName: do-dns-secret
+secretKey: access-tokenCorresponding change in terraform/kubernetes.tf:
data = {
- password = var.do_dns_token
+ access-token = var.do_dns_token
}| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| appVersion: "1.16.0" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
appVersion: "1.16.0" looks like it was left over from helm create.
This chart packages a cert-manager Certificate resource — there is no "application" version 1.16.0 being deployed. Consider aligning appVersion with version (0.1.0) or with the cert-manager API version this chart targets, to avoid misleading the app.kubernetes.io/version label should you ever wire up the helpers in _helpers.tpl.
| # resource "kubernetes_namespace_v1" "external_dns" { | ||
| # metadata { | ||
| # name = "external-dns" | ||
| # } | ||
|
|
||
| depends_on = [ digitalocean_kubernetes_cluster.main ] | ||
| } | ||
| # depends_on = [ digitalocean_kubernetes_cluster.main ] | ||
| # } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the commented-out external_dns namespace block.
With helm_release.external_dns and this namespace both commented out — and helm/external-dns/values.yaml still referencing the old external-dns-do-secret name — the external-dns surface is fully dead. Prefer deleting rather than leaving it commented; git history preserves it if you need to restore. Otherwise the next person has to reason about stale wiring (e.g., the secret-name drift) that no longer exists.
No description provided.