Repository navigation
feat(cozy-lib): add TLS certificate helper - #2485
Arsolitt (Arsolitt) wants to merge 18 commits into
Conversation
📝 WalkthroughWalkthroughAdds two TLS helper templates to the cozy-lib Helm library (certificate and secretName), updates the library README, and introduces a comprehensive suite of Helm unit tests and test templates validating certificate rendering and input validation. ChangesTLS Certificate Management Module
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
area/platform
Incorporate the latest cozy-lib extraDnsNames changes from the upstream branch so that PR #2485 can be merged before PR #2540 without conflicts. Signed-off-by: Arsolitt <[email protected]>
Add helm-unittest tests for the upcoming cozy-lib.tls.certificate helper. Tests cover: required fields, dnsNames, issuerRef, labels, default and custom duration/renewBefore, default and custom usages, secretName defaulting, and both ClusterIssuer and Issuer kinds. Signed-off-by: Arsolitt <[email protected]> Signed-off-by: Arsolitt <[email protected]>
Add cozy-lib.tls.certificate template that renders a cert-manager Certificate CR for services needing TLS. This provides a shared foundation so each chart does not duplicate cert-manager boilerplate. Also adds cozy-lib.tls.secretName helper for resolving a TLS secret name from user input or generating one from the release name. Signed-off-by: Arsolitt <[email protected]> Signed-off-by: Arsolitt <[email protected]>
…faults and add missing tests Signed-off-by: Arsolitt <[email protected]>
…lidation Signed-off-by: Arsolitt <[email protected]>
…name Signed-off-by: Arsolitt <[email protected]>
… and improve docs Signed-off-by: Arsolitt <[email protected]>
… validation Add missing space in renewBefore parameter description and add fail guards for missing Release and suffix in the cozy-lib.tls.secretName helper. Signed-off-by: Arsolitt <[email protected]>
…ML safety Add optional extraDnsNames parameter to the TLS certificate helper so consumers can append user-supplied custom domains to the SAN list. Pipe all dnsNames (and extraDnsNames) through | quote to ensure wildcard entries like *.example.com are properly YAML-quoted. Assisted-By: Claude <[email protected]> Signed-off-by: Arsolitt <[email protected]>
Add missing test cases for name and issuerRef validation guards. Quote usages list items for consistency with dnsNames quoting. Fix doc comment referencing test-internal tls.yaml wrapper. Signed-off-by: Arsolitt <[email protected]>
Quote the hardcoded "server auth" default for consistency with the range path that uses `| quote`. Rename test chart from "quota" to "cozy-lib-tests" to reflect its actual scope. Signed-off-by: Arsolitt <[email protected]>
Move the suffix validation inside the else branch of secretName helper so callers providing an explicit secretName are not forced to also supply an unused suffix parameter. Signed-off-by: Arsolitt <[email protected]>
…mplates Set metadata.namespace from Release.Namespace so Certificate CRs land in the correct namespace. Add privateKey.rotationPolicy: Always for forward secrecy on renewal. Remove duplicate test template by reusing the existing suffix-missing fixture for both test scenarios. Signed-off-by: Arsolitt <[email protected]>
…group field Clarify in doc comments that empty usages list falls back to default, duration values are passed through without validation, and issuerRef.group is always hardcoded to cert-manager.io. Add test for empty usages list. Signed-off-by: Arsolitt <[email protected]>
Add | quote to all string scalar fields in Certificate CR output for defensive YAML safety. Document hardcoded privateKey parameters and extraDnsNames nil behavior. Replace empty README stub with library description. Signed-off-by: Arsolitt <[email protected]>
Quote duration, renewBefore, and label values for full consistency. Add kindIs slice check to reject string dnsNames that would silently iterate over individual characters via range. Signed-off-by: Arsolitt <[email protected]>
Add kindIs slice check for extraDnsNames to match dnsNames guard. Quote the hardcoded group field for full output consistency. Update README to enumerate all helper categories in the library. Signed-off-by: Arsolitt <[email protected]>
3304087 to
6b633b1
Compare
github-actions
Bot
added
the
size/XL
Arsolitt (Arsolitt)
added
the
do-not-merge/work-in-progress
dosubot
Bot
added
size:L
and removed
size/XL
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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. 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 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 counter productive. You can react with 👍 and 👎 on Gemini (@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
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/library/cozy-lib/templates/_tls.tpl (1)
78-85: ⚡ Quick winAdd a
kindIs "slice"type guard forusages, consistent withdnsNames/extraDnsNames.Both
dnsNames(line 44) andextraDnsNames(line 47) have explicit guards that surface a clear error when a string is passed instead of a list.usageshas no such guard — a caller passingusages: "server auth"(string) passes the truthiness check on line 79, then hitsrange .usageswhich fails with a cryptic Go template error ("can't iterate over …") rather than an actionable message.🛡️ Proposed fix
+{{- if and .usages (not (kindIs "slice" .usages)) -}} +{{- fail "ERROR: \"usages\" must be a list for cozy-lib.tls.certificate. Got a string — wrap it in a list." -}} +{{- end -}} usages: {{- if .usages }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/cozy-lib/templates/_tls.tpl` around lines 78 - 85, Add the same type-guard used for dnsNames and extraDnsNames to usages: ensure you check kindIs "slice" before ranging over .usages and surface a clear error when a string is provided; update the template logic around usages (the usages block that currently does "if .usages" then "range .usages") to mirror the dnsNames/extraDnsNames pattern so callers passing usages: "server auth" get a descriptive error instead of the Go template iteration failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/library/cozy-lib/templates/_tls.tpl`:
- Around line 78-85: Add the same type-guard used for dnsNames and extraDnsNames
to usages: ensure you check kindIs "slice" before ranging over .usages and
surface a clear error when a string is provided; update the template logic
around usages (the usages block that currently does "if .usages" then "range
.usages") to mirror the dnsNames/extraDnsNames pattern so callers passing
usages: "server auth" get a descriptive error instead of the Go template
iteration failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5905dad6-0a31-42b4-9d2d-35ffebad3875
📒 Files selected for processing (17)
packages/library/cozy-lib/README.mdpackages/library/cozy-lib/templates/_tls.tplpackages/tests/cozy-lib-tests/Chart.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-missing-release.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-missing-secretname.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname-missing-release.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname-missing-suffix.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname.yamlpackages/tests/cozy-lib-tests/templates/tests/tls.yamlpackages/tests/cozy-lib-tests/tests/tls_missing_release_test.yamlpackages/tests/cozy-lib-tests/tests/tls_missing_secretname_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_missing_release_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_missing_suffix_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_no_suffix_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_test.yamlpackages/tests/cozy-lib-tests/tests/tls_test.yamlpackages/tests/cozy-lib-tests/tests/tls_values.yaml
|
/gemini review |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Observations on the helper extraction — not gating, leaving approval for another reviewer.
Substantive helper extraction with thorough test coverage — helm unittest from packages/tests/cozy-lib-tests passes 34/34 across 8 suites locally, covering both the certificate-render happy path and every validation branch (missing Release, missing secretName, slice guards on dnsNames, missing suffix when secretName not provided). The 16-commit history reflects honest iteration on review feedback rather than a single take-it-or-leave-it patch — quoted scalars across all string outputs, enum-validation on issuerRef.kind, the "suffix is only required when secretName is not provided" carve-out in cozy-lib.tls.secretName, and the explicit kindIs "slice" guards on dnsNames / extraDnsNames are all the right shapes.
Design choices that look correct:
- Hardcoding
privateKey: { algorithm: RSA, size: 2048, rotationPolicy: Always }and exposing nothing about it. Documented as deliberate. Anyone who needs a different algo or rotation behavior should write their own Certificate CR — keeping this helper opinionated on a single sane default avoids the Helm-template combinatorial explosion that would otherwise creep in. - Defaulting
usages: ["server auth"]instead of cert-manager's["digital signature", "key encipherment"]. Explicitly documented as a deviation. For the cozystack use case (TLS server certs for managed services),server authis what reviewers actually want; cert-manager's default is for general-purpose certs. Right call. group: "cert-manager.io"hardcoded on the issuerRef. There is exactly one valid group, so exposing it as a parameter would be cosmetic surface area without a use case.cozy-lib.tls.secretNamereturning the user-provided name verbatim or generating<release>-<suffix>— this is the right primitive for the conditional render pattern (mount existing secret if user-provided, otherwise emit a Certificate CR that will create one).
One outstanding finding from the latest CodeRabbit pass that's worth folding in either here or in a follow-up:
usagesis the only list-typed parameter without akindIs "slice"guard (comparednsNamesat line 44-46 andextraDnsNamesat line 47-49). A user who passesusages: "server auth"(string instead of single-element list) currently hits Helm's generic "range can't iterate over string" error, which is much harder to action than the bespoke "must be a list ... wrap it in a list" message the other two parameters produce. Trivial consistency fix:
{{- if and .usages (not (kindIs "slice" .usages)) -}}
{{- fail "ERROR: \"usages\" must be a list for cozy-lib.tls.certificate. Got a string — wrap it in a list." -}}
{{- end -}}Two non-blocking observations for future iterations:
privateKey.encodingis unset, so cert-manager defaults it to PKCS#1. Most modern consumers expect PKCS#8, and cert-manager itself recommends PKCS#8 for new deployments. If a consumer chart hits this, it's worth promotingencoding: PKCS8to the helper (still hardcoded, no parameter). Out of scope here.- The Certificate CR's
metadata.namespacecomes from.Release.Namespace. If a chart ever needs to render a Certificate into a namespace different from the release namespace (cross-tenant cert sharing, control-plane cert mirrored into a tenant ns, etc.), the helper currently has no escape hatch. Today no consumer needs this — all current managed-service charts render into their own release namespace — so worth flagging as a known constraint rather than a fix.
![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 certificate and secret name helpers to the cozy-lib library chart, featuring robust input validation and a comprehensive test suite. The changes also include a README for the library and a rename of the testing chart to cozy-lib-tests. Feedback was provided to incorporate the trim function into the secret name helpers to prevent potential issues caused by accidental whitespace.
| {{- if not (empty .secretName) -}} | ||
| {{- .secretName -}} |
There was a problem hiding this comment.
The helper returns the secretName directly without quoting. While the caller might quote it, it's safer to ensure the output of this helper is consistent with other string-returning helpers in the library. However, since this is often used in assignments like {{- $tlsSecret := include ... }}, returning the raw string is generally preferred for flexibility.
One minor improvement: using trim to ensure no accidental whitespace is included in the returned secret name.
{{- if not (empty .secretName) -}}
{{- .secretName | trim -}}
{{- else -}}
There was a problem hiding this comment.
The {{- ... -}} delimiters already strip surrounding template whitespace. For value-level whitespace in secretName, Kubernetes name validation ([a-z0-9]([-a-z0-9]*[a-z0-9])?) catches it at apply time with a clear error. Adding | trim would silently fix a misconfigured value instead of surfacing the error — we'd rather let the API server reject invalid names so the user knows their values.yaml needs correcting. Both Release.Name (Helm-managed DNS subdomain) and user-provided secretName (Kubernetes resource name) cannot legitimately contain whitespace.
| {{- if not .suffix -}} | ||
| {{- fail "ERROR: \"suffix\" is required for cozy-lib.tls.secretName when secretName is not provided. Provide a suffix for the generated secret name (e.g. \"tls\", \"server-tls\")." -}} | ||
| {{- end -}} | ||
| {{- printf "%s-%s" .Release.Name .suffix -}} |
There was a problem hiding this comment.
Similar to the user-provided secretName, the generated name should be trimmed to avoid issues if Release.Name or suffix contain leading/trailing spaces (though unlikely for Release.Name).
{{- printf "%s-%s" .Release.Name .suffix | trim -}}
There was a problem hiding this comment.
Same reasoning as above — Release.Name is always a valid DNS subdomain provided by Helm, and suffix is a string literal from the calling template. The {{- ... -}} delimiters handle template whitespace, and Kubernetes name validation covers the (theoretical) value-level case. Silently trimming would mask configuration errors.
Address review feedback from lexfrei on _tls.tpl: Add type guard for usages to match dnsNames/extraDnsNames pattern, producing an actionable error instead of Helm's generic range error. Signed-off-by: Arsolitt <[email protected]>
github-actions
Bot
added
size/XL
|
Good catch — added a On the non-blocking notes: |
|
CodeRabbit (@coderabbitai) full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/library/cozy-lib/templates/_tls.tpl (2)
67-72: ⚡ Quick winConsider adding an optional
labelsparameter for consumer-injected labelsThe PR description lists "configurable labels" as a feature, but the template's
Parametersblock has nolabelsentry and the metadata section only auto-generatesapp.kubernetes.io/instanceandapp.kubernetes.io/managed-byfromRelease. Callers cannot attach custom labels (e.g., forNetworkPolicyselectors or monitoring alerts) to the Certificate CR without wrapping the helper.♻️ Suggested approach
metadata: name: {{ .name | quote }} namespace: {{ .Release.Namespace | quote }} labels: app.kubernetes.io/instance: {{ .Release.Name | quote }} app.kubernetes.io/managed-by: {{ .Release.Service | quote }} + {{- if .labels }} + {{- if not (kindIs "map" .labels) }} + {{- fail "ERROR: \"labels\" must be a map for cozy-lib.tls.certificate." }} + {{- end }} + {{- range $k, $v := .labels }} + {{ $k }}: {{ $v | quote }} + {{- end }} + {{- end }}And document it in the Parameters block:
+ - labels (optional) - map of additional labels to add to the Certificate CR metadata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/cozy-lib/templates/_tls.tpl` around lines 67 - 72, Add an optional consumer-provided labels parameter so callers can inject arbitrary labels into the generated Certificate metadata: add a new parameter (e.g., "labels" or ".Values.labels") to the template's Parameters block and merge it into the metadata.labels map alongside the existing app.kubernetes.io/instance and app.kubernetes.io/managed-by entries (ensuring nil/empty checks so no extra keys when not provided). Update the template to iterate/merge the provided labels into metadata and document the new parameter in the Parameters block so consumers can pass custom labels for NetworkPolicy/monitoring selectors.
115-127: ⚡ Quick win
Releaseis required even whensecretNameis suppliedThe
Releaseguard at line 116 fires unconditionally before thesecretNamecheck at line 119. A caller who provides a staticsecretNameand omitsReleasewill receive an error even thoughRelease.Nameis only needed in the generated-name branch (line 125). Reordering makesReleasetruly optional when a staticsecretNameis given.♻️ Suggested reorder
{{- define "cozy-lib.tls.secretName" -}} -{{- if not .Release -}} -{{- fail "ERROR: \"Release\" is required for cozy-lib.tls.secretName. Pass the Helm release object via the context dict." -}} -{{- end -}} {{- if not (empty .secretName) -}} {{- .secretName -}} {{- else -}} +{{- if not .Release -}} +{{- fail "ERROR: \"Release\" is required for cozy-lib.tls.secretName. Pass the Helm release object via the context dict." -}} +{{- end -}} {{- if not .suffix -}} {{- fail "ERROR: \"suffix\" is required for cozy-lib.tls.secretName when secretName is not provided. ..." -}} {{- end -}} {{- printf "%s-%s" .Release.Name .suffix -}} {{- end -}} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/cozy-lib/templates/_tls.tpl` around lines 115 - 127, The guard for .Release in the template cozy-lib.tls.secretName runs too early and blocks callers who provided a static .secretName; change the logic to first check if .secretName is non-empty and return it, and only when .secretName is empty validate .suffix and require .Release (so you can safely use .Release.Name to construct the generated name). Ensure you still fail with the existing messages when .secretName is empty and .suffix or .Release is missing.packages/tests/cozy-lib-tests/tests/tls_test.yaml (1)
1-521: 💤 Low valueLGTM — comprehensive coverage across all configurable parameters and validation guards.
One minor asymmetry:
dnsNames,extraDnsNames, andusagesall have dedicated "errors when X is a string instead of a list" tests (lines 464–520), but there is no equivalent test for passingissuerRefas a string (e.g.,issuerRef: "my-issuer"). In practice, a stringissuerRefwould likely produce the "issuerRef.kind is required" error rather than a type-specific message, so the failure is still actionable; this is a nice-to-have for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tests/cozy-lib-tests/tests/tls_test.yaml` around lines 1 - 521, Add a test case to tls_test.yaml that checks behavior when issuerRef is provided as a string (e.g., issuerRef: "my-issuer"); create a new "it: errors when issuerRef is a string instead of a dict" test similar to the existing string-type tests for dnsNames/extraDnsNames/usages, set tls with name/dnsNames and issuerRef as a string, and assert a failedTemplate with the expected validation error (either the existing 'ERROR: "issuerRef.kind" is required...' message or a clearer 'ERROR: "issuerRef" must be a dict...' if you prefer to make the validation stricter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/library/cozy-lib/templates/_tls.tpl`:
- Around line 67-72: Add an optional consumer-provided labels parameter so
callers can inject arbitrary labels into the generated Certificate metadata: add
a new parameter (e.g., "labels" or ".Values.labels") to the template's
Parameters block and merge it into the metadata.labels map alongside the
existing app.kubernetes.io/instance and app.kubernetes.io/managed-by entries
(ensuring nil/empty checks so no extra keys when not provided). Update the
template to iterate/merge the provided labels into metadata and document the new
parameter in the Parameters block so consumers can pass custom labels for
NetworkPolicy/monitoring selectors.
- Around line 115-127: The guard for .Release in the template
cozy-lib.tls.secretName runs too early and blocks callers who provided a static
.secretName; change the logic to first check if .secretName is non-empty and
return it, and only when .secretName is empty validate .suffix and require
.Release (so you can safely use .Release.Name to construct the generated name).
Ensure you still fail with the existing messages when .secretName is empty and
.suffix or .Release is missing.
In `@packages/tests/cozy-lib-tests/tests/tls_test.yaml`:
- Around line 1-521: Add a test case to tls_test.yaml that checks behavior when
issuerRef is provided as a string (e.g., issuerRef: "my-issuer"); create a new
"it: errors when issuerRef is a string instead of a dict" test similar to the
existing string-type tests for dnsNames/extraDnsNames/usages, set tls with
name/dnsNames and issuerRef as a string, and assert a failedTemplate with the
expected validation error (either the existing 'ERROR: "issuerRef.kind" is
required...' message or a clearer 'ERROR: "issuerRef" must be a dict...' if you
prefer to make the validation stricter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fe9da0f-727b-426b-b3d0-49b7a7e6c1eb
📒 Files selected for processing (17)
packages/library/cozy-lib/README.mdpackages/library/cozy-lib/templates/_tls.tplpackages/tests/cozy-lib-tests/Chart.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-missing-release.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-missing-secretname.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname-missing-release.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname-missing-suffix.yamlpackages/tests/cozy-lib-tests/templates/tests/tls-secretname.yamlpackages/tests/cozy-lib-tests/templates/tests/tls.yamlpackages/tests/cozy-lib-tests/tests/tls_missing_release_test.yamlpackages/tests/cozy-lib-tests/tests/tls_missing_secretname_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_missing_release_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_missing_suffix_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_no_suffix_test.yamlpackages/tests/cozy-lib-tests/tests/tls_secretname_test.yamlpackages/tests/cozy-lib-tests/tests/tls_test.yamlpackages/tests/cozy-lib-tests/tests/tls_values.yaml
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Verified the usages kindIs "slice" guard in 4a4215956 — the new branch in _tls.tpl:50-52 mirrors the existing dnsNames / extraDnsNames guards, the tls_test.yaml test case ("errors when usages is a string") pins the contract, and helm unittest is now 35/35 passing across 8 suites locally. Thanks for that.
One note before the substance: in #2485 (comment) you stated that privateKey.encoding (PKCS#1 vs PKCS#8) and the Release.Namespace constraint are "documented in the helper's doc comment." I re-read _tls.tpl:1-30 after 4a4215956 and neither is mentioned — line 28 calls out RSA-2048 and rotationPolicy: Always as the hardcoded private-key constraints, but says nothing about encoding, and the docstring's parameter list says nothing about the namespace coming verbatim from Release.Namespace with no override path. The notes exist in the review thread but not in the file. Both items are below as blockers/observations rather than as accepted limitations.
Blockers
B1: helper hardcodes privateKey.encoding to cert-manager's default (PKCS#1) for a permanent cross-service TLS primitive
File: packages/library/cozy-lib/templates/_tls.tpl:74-77
Issue: The helper sets privateKey: { algorithm: RSA, size: 2048, rotationPolicy: Always } and leaves encoding unset. cert-manager defaults unset privateKey.encoding to PKCS1. Cert-manager's own documentation (https://cert-manager.io/docs/reference/api-docs/#cert-manager.io/v1.CertificatePrivateKey) lists PKCS8 as the recommended encoding for modern deployments, and several Kubernetes-ecosystem TLS clients (Go's crypto/tls since 1.18 expects either; many Java/Python TLS libraries built against modern OpenSSL prefer PKCS#8) read PKCS#8 keys without an extra conversion step, while PKCS#1 needs openssl pkcs8 -topk8 for compatibility.
For a one-off chart this would be cosmetic. For a shared library helper that the PR description bills as the anchor for all future managed-service TLS in cozystack (cozy-lib.tls.certificate), the PKCS#1 default ossifies into the contract for every consumer that follows. By the time the second or third managed service plugs into this helper and someone hits a TLS-handshake-fails-against-Go-1.22-client bug, the helper has shipped the implicit promise.
This is materially the same shape as the kindIs "slice" guards already added to dnsNames / extraDnsNames / usages — defaults that bake in an opinion the helper should hold strongly because callers can't easily override it.
Fix: Add encoding: PKCS8 alongside algorithm: RSA and size: 2048 in the privateKey block (keeping it hardcoded — this is opinionation by design, not a parameter to expose). Update the docstring at _tls.tpl:28 to read Private key is hardcoded to RSA-2048 PKCS#8 with rotationPolicy: Always; these are not configurable via this helper. Add a test that asserts spec.privateKey.encoding == "PKCS8" so the contract is pinned.
B2: helper's scope (leaf TLS certs only) is not documented and will mislead consumers attempting CA certs
File: packages/library/cozy-lib/templates/_tls.tpl:1-30 (docstring) and packages/library/cozy-lib/README.md
Issue: The helper is built around defaults that are correct only for leaf TLS certs: usages: ["server auth"], rotationPolicy: Always, RSA-2048, 1-year duration, no commonName, no isCA, no subject parameter. The repo already contains six hand-rolled CA Certificate CRs (packages/system/cozystack-api/templates/certmanager.yaml, packages/system/kubeovn-webhook/templates/certmanager.yaml, packages/system/lineage-controller-webhook/templates/certmanager.yaml, packages/system/linstor/templates/linstor-internal-tls.yaml, packages/extra/etcd/templates/etcd-cluster.yaml — two CAs there) using the opposite shape: isCA: true, commonName, usages: [signing, key encipherment, cert sign], rotationPolicy: Never, RSA-4096, 5–10 year duration, subject.organizations for etcd. None of those can migrate to this helper, and that's intentional.
Nothing in _tls.tpl's docstring or cozy-lib/README.md says so. A future contributor reading "shared TLS certificate helpers for managed services" will reasonably try cozy-lib.tls.certificate for a CA cert, hit the missing-isCA / wrong-usages / wrong-rotationPolicy / wrong-key-size wall, and either fork the helper or land a half-broken Certificate CR.
Fix: Add an explicit "Scope" line near the top of the docstring (around _tls.tpl:5) and the corresponding README section:
Scope: leaf (end-entity) TLS certificates only. For CA certificates that
need `isCA: true`, `commonName`, signing/key-encipherment/cert-sign
usages, `rotationPolicy: Never`, RSA-4096, or `subject` fields, write the
Certificate CR by hand — see `packages/system/{cozystack-api,kubeovn-webhook,
lineage-controller-webhook,linstor}/templates/` for the established pattern.
No test required — this is documentation-only.
Non-blocking observations
-
Release.Namespaceis hardcoded with no override path._tls.tpl:66rendersmetadata.namespace: {{ .Release.Namespace | quote }}with no parameter to override. Today no consumer needs cross-namespace rendering, but a one-line entry in the docstring ("metadata.namespaceisRelease.Namespace; cross-namespace rendering is not supported — caller must use a separate Helm release") would save the next maintainer the same investigation. Trivial. -
secretNameis required, no default-to-name. Forces every caller to repeat(default .name .secretName)—templates/tests/tls.yaml:2itself demonstrates this boilerplate. The docstring acknowledges "a caller-level wrapper can default this to name" as deliberate design, but defaultingsecretNametonameinside the helper would remove the repeated boilerplate without losing any expressiveness. Optional design call, not a defect. -
Hardcoded labels on the Certificate CR.
_tls.tpl:67-69emits onlyapp.kubernetes.io/instanceandapp.kubernetes.io/managed-bylabels. Consumers cannot injectapp.kubernetes.io/name,app.kubernetes.io/component, monitoring/team labels, or NetworkPolicy selectors without wrapping the helper. CodeRabbit flagged the same on #2485 (comment) (coderabbitai 2026-05-04T13:51:22Z). For a shared helper this is the kind of extensibility-tax that bites the third or fourth consumer; an optionallabelsdict parameter (withkindIs "map"guard, mirroring the slice guards) would future-proof it. Not blocking — current behavior is consistent and tested. -
README does not mention the cert-manager CRDs prerequisite.
cozy-lib/README.mddocuments the helper categories but does not state that consumers' clusters must already have cert-manager installed for any ofcozy-lib.tls.*to work. One sentence ("cert-manager CRDs must be present in the cluster — these helpers do not install or vendor cert-manager itself") would prevent consumer charts from shipping into clusters that 404 onCertificate.cert-manager.io. Trivial. -
Em-dash
—in error messages._tls.tpl:46,:49,:52use the U+2014 em-dash; every other tpl inpackages/library/cozy-lib/templates/is ASCII-only. Cosmetic; switch to--for consistency. Won't fail any test. -
duration: 8760h(1 year) is on the long side. The doc points out this is a deliberate departure from cert-manager's 90-day default. WithrenewBefore: 720h(30 days) the rotation cadence is quarterly-ish, fine for most operators. Compliance postures requiring shorter (e.g., 90-day commercial) would need to overridedurationper consumer; the override path is exposed and tested, so this is just worth a doc nudge alongside the scope clarification in B2.
Verification
helm unittest packages/tests/cozy-lib-tests→ 35/35 pass after4a4215956(was 34 prior; the newusagesslice-guard test brought it to 35).- Validation paths fully covered:
not Release/not name/not secretName/not dnsNames/dnsNames not slice/extraDnsNames not slice/usages not slice/not issuerRef/not issuerRef.name/not issuerRef.kind/ invalidissuerRef.kindenum /secretNamehelpernot Release/secretNamehelpernot suffix(only when secretName empty). Every branch has a correspondingfailedTemplateassertion. cozy-lib-testschart symlink atpackages/tests/cozy-lib-tests/charts/cozy-libresolves topackages/library/cozy-lib; tests pick up the latest helper without a vendoring step.- Chart rename
quota→cozy-lib-testshas no external consumers —grepis clean across packages, scripts, hack, .github.
…ssages Address review feedback from lexfrei: - Set privateKey.encoding to PKCS8 for modern TLS compatibility - Document helper scope as leaf certs only (not CA certificates) - Add namespace and cert-manager prerequisite documentation - Normalize em-dashes to double-dashes in error messages for ASCII consistency Signed-off-by: Arsolitt <[email protected]>
dosubot
Bot
added
size:L
and removed
size/XL
|
Addressed both blockers in B1: Added B2: Added scope documentation at the top of Also folded in the non-blocking items: namespace doc note ("metadata.namespace is always Release.Namespace; cross-namespace rendering is not supported"), cert-manager prerequisite in README, and normalized em-dashes to Apologies for the inaccuracy in my earlier reply about encoding/namespace being documented — they weren't at that point. They are now. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tests/cozy-lib-tests/tests/tls_test.yaml (1)
44-56: 💤 Low valueConsider adding
hasDocuments: count: 1to standalone positive test cases.The first test case (lines 15–16) asserts
hasDocuments: count: 1, but the remaining positive test cases that render a document don't repeat that assertion. If the template were ever modified to conditionally emit multiple documents, those tests would silently pass on the first document while the regression goes unnoticed. Adding the assertion to each positive case makes them independently self-validating.Compare with the "uses default duration when duration is an empty string" test (line 402–403) which does include
hasDocuments: count: 1— making it the more robust pattern.♻️ Example for the "sets dnsNames correctly" test (same pattern for all affected cases)
asserts: + - hasDocuments: + count: 1 - equal: path: spec.dnsNamesAlso applies to: 58-74, 76-89, 91-103, 132-143, 171-191, 194-211, 214-234, 335-358, 361-382, 446-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tests/cozy-lib-tests/tests/tls_test.yaml` around lines 44 - 56, Add an explicit document-count assertion to each standalone positive test case that renders a document (e.g., the "sets dnsNames correctly" test and the other listed tests) by inserting a top-level hasDocuments: count: 1 entry in the test YAML alongside values/release before the asserts block; ensure every positive case that currently asserts on spec.* also includes hasDocuments: count: 1 so each test independently verifies exactly one rendered document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tests/cozy-lib-tests/tests/tls_test.yaml`:
- Around line 44-56: Add an explicit document-count assertion to each standalone
positive test case that renders a document (e.g., the "sets dnsNames correctly"
test and the other listed tests) by inserting a top-level hasDocuments: count: 1
entry in the test YAML alongside values/release before the asserts block; ensure
every positive case that currently asserts on spec.* also includes hasDocuments:
count: 1 so each test independently verifies exactly one rendered document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bfdaf59-1868-49b3-b1ab-06ff93dfb6ef
📒 Files selected for processing (3)
packages/library/cozy-lib/README.mdpackages/library/cozy-lib/templates/_tls.tplpackages/tests/cozy-lib-tests/tests/tls_test.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/library/cozy-lib/templates/_tls.tpl
- packages/library/cozy-lib/README.md
| privateKey: | ||
| algorithm: RSA | ||
| encoding: PKCS8 | ||
| size: 2048 |
There was a problem hiding this comment.
I would rather default to a more modern algorithm. RSA support can be left as an opt-in for legacy applications.
|
Holding pending rework: this should be promoted from a helper file in Marking |
Aleksei Sviridkin (lexfrei)
added
the
do-not-merge/hold
github-actions
Bot
added
size/L
3 participants
What this PR does
Add shared TLS certificate helpers to cozy-lib for reuse across managed service packages.
Two new template helpers in
_tls.tpl:cozy-lib.tls.certificate— renders a cert-managerCertificateCR with configurable issuer,DNS SANs, duration, key usages, and sensible defaults (RSA-2048, rotationPolicy: Always,
1-year duration, server auth usage)
cozy-lib.tls.secretName— resolves a user-provided TLS secret name or generates a<release>-<suffix>default; suffix is only required when secretName is not providedKey design decisions:
["server auth"]instead of cert-manager's defaultAlso updates the library README to enumerate all helper categories and renames the test chart
from
quotatocozy-lib-tests.Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit
New Features
Documentation
Tests