docs(unified-tls-pki): fix the canonical trust-anchor name and its owner - #36
docs(unified-tls-pki): fix the canonical trust-anchor name and its owner#36Aleksei Sviridkin (lexfrei) wants to merge 4 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe proposals standardize the tenant trust-anchor Secret as ChangesTenant trust-anchor contract
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![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 updates the design proposals for external database exposure and unified TLS PKI. It renames the canonical trust-anchor Secret from <release>-ca-cert to <release>-tenant-ca to prevent naming collisions with existing operator-created Secrets (such as Percona PSMDB). Additionally, it updates the extraction controller's design to owner-reference the HelmRelease instead of the virtual application instance CR to ensure proper garbage collection. The reviewer feedback points out that the external database exposure proposal still contains outdated text claiming redis self-publishes its CA directly, whereas under the updated design, all engines (including redis and kafka) utilize the extraction controller to project their CAs to the canonical <release>-tenant-ca Secret.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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
🤖 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 `@design-proposals/external-database-exposure/README.md`:
- Line 139: Update the diagram’s trust-anchor Secret reference near the external
database exposure flow to use <release>.tenant-ca instead of <release>-ca-cert,
matching the trust-anchor name defined in the surrounding proposal and keeping
the ca.crt-only contract unchanged.
In `@design-proposals/unified-tls-pki/README.md`:
- Around line 156-167: Define one canonical delivery path: require every
engine-specific CA Secret, including self-publishing Redis and similar sources,
to be projected into <release>.tenant-ca by the extraction controller rather
than writing that canonical object directly. Update
design-proposals/unified-tls-pki/README.md at lines 156-167 to make this
mandatory and consistent, and update
design-proposals/external-database-exposure/README.md at line 19 to describe
Redis’s <release>-ca-cert strictly as an internal source.
- Around line 156-167: Clarify the design so every engine has one mandatory path
for producing <release>.tenant-ca. Either require the extraction controller to
project the canonical Secret for all engines, or explicitly document each
self-publishing engine's equivalent canonical-object creation and remove any
conflicting opt-out language. Update the descriptions in the
extraction-controller and reuse sections consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff0f6c24-ea7c-4bf2-a5a6-98138745a2e1
📒 Files selected for processing (2)
design-proposals/external-database-exposure/README.mddesign-proposals/unified-tls-pki/README.md
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed as a docs pass. The rename and the ownership correction are applied only in the definitional sections, so the document currently contradicts itself on both points it sets out to fix. Suggest holding until the references are reconciled.
Stale/contradictory references (design-proposals/unified-tls-pki/README.md unless noted):
- L175 still says the one canonical trust-anchor object a tenant sees is
<release>-ca-cert, contradicting the new definition at L137 (<release>.tenant-ca). - L197 re-asserts both errors this PR fixes: the stale name, and garbage-collection via an owner reference to the application instance, contradicting rewritten section (b) which shows the owner-ref must be the HelmRelease.
- L99 (interior-tier diagram node, ca.crt-only) and external-database-exposure/README.md L110 (diagram node projected via tenantsecrets) still use the stale canonical name.
- L171, L179, L203, L228 still reference the projected/canonical object by the old name.
Note: occurrences at L41, L43, L121-124, L156, L160, L167, L214, L226 correctly keep -ca-cert because they name engine-specific source secrets or describe the old-name collision; those should not change.
Minor: external-database-exposure/README.md L19 says redis self-publishes while unified-tls-pki/README.md L167 says redis still needs projection; the two docs disagree.
bbbbfce to
1366c24
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@design-proposals/unified-tls-pki/README.md`:
- Line 160: Update the source-value validation contract to strictly parse every
PEM value with x509.ParseCertificate, rejecting non-certificate PEM blocks,
malformed input, and any trailing data instead of relying on PRIVATE KEY header
detection. Extend the controller tests covering the source selection and
certificate validation flow with malformed and trailing-data cases.
- Line 215: The proposal inconsistently treats publish-ca-cert and
publish-ca-cert-release as both required contract fields and unresolved names.
Update the documentation around the label/annotation convention and the
definitions at the corresponding contract section to either finalize these exact
names consistently or mark them as placeholders throughout, ensuring source
producers and the controller use the same contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 680ecd39-8923-4eb8-8ab7-9bba409a51ad
📒 Files selected for processing (2)
design-proposals/external-database-exposure/README.mddesign-proposals/unified-tls-pki/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- design-proposals/external-database-exposure/README.md
|
Good catch, and it was wrong in a stronger sense than the comment says. The forked redis-operator does not self-publish a key-free CA at all on |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
NOT LGTM (request changes). The titled change (renaming the trust anchor and moving its owner to the HelmRelease) is done cleanly and consistently across both proposals, with no orphaned old names and the mermaid diagrams updated.
One blocker: the description says it corrects four errors, but the document implements three. The fourth (the guard must parse the certificate with x509.ParseCertificate rather than pattern-match the PEM header) is absent: grep finds no x509/ParseCertificate/pem.Decode, and section 5(b) still specifies the header check the PR body itself calls insufficient. Please either add the x509-parse correction to the contract, or drop the fourth item from the description, so an implementer does not build the weaker header guard on a security-load-bearing boundary.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Requesting changes: the CHANGES_REQUESTED from 2026-08-18 is not addressed.
- [MAJOR] §5(b) still specifies the guard as "carries no
-----BEGIN … PRIVATE KEY-----header", while the PR body claims the guard was moved tox509.ParseCertificate. The diff does not touch that text (grep forx509/ParseCertificate/pem.Decodeis empty on the branch head), and there have been no commits since 2026-07-19. Worse, the already-merged controller (#3407) actually implementsx509.ParseCertificatein apem.Decodeloop, so the ADR now lags both its own PR body and the deployed code. Please align §5(b) with the merged implementation. - [MINOR] The §3 redis row and external-database-exposure/README.md still assert in the present tense that redis self-publishes a key-free
<release>-ca-certthrough its forked operator. That was confirmed inaccurate in-thread: on main the operator is stock and the fork lives in the open #2729. The fix currently exists only in the stacked #41, so #36 on its own carries a false statement. - [NIT] Open questions still list the
publish-ca-certlabel names as unresolved although §5(a) treats them as an accepted contract (#41 removes the label mechanism entirely, so this is minor).
Implementation surfaced two errors in the accepted design. The canonical projection cannot be named <release>-ca-cert. Percona PSMDB creates a Secret of exactly that name and puts a private key in it, so source and target collide on the very engine that most needs the projection; Strimzi publishes its key-free CA under a different name again. The document itself warned three times that this name means opposite things on different engines, used that to reject name-based source selection, and then picked it for the target anyway. The canonical object is now <release>-tenant-ca, which no shipped operator claims. The projection also cannot be owner-referenced to the application CR. The apps.cozystack.io kinds are virtual: an application is stored as a HelmRelease, so there is no object in etcd to reference. The HelmRelease is the only real owner, and deleting the application deletes it, which garbage-collects the projection. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
Implementation showed the source-selection label is not enough on its own. The platform runs cert-manager with enableCertificateOwnerRef set to false, so a cert-manager-issued Secret carries no ownerReference back to its application and the lineage graph cannot tell which release it belongs to. The consume contract therefore requires a second label, internal.cozystack.io/publish-ca-cert-release, carrying the release name, stamped alongside the opt-in label in the same secretTemplate block. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
1366c24 to
a30f354
Compare
…truction The trust anchor was named <release>-tenant-ca on the grounds that no operator the platform ships claims it. That is false. It collides across releases: for an application foo the projection is postgres-foo-tenant-ca, and for a sibling application foo-tenant CloudNativePG's own CA secret is postgres-foo-tenant plus -ca, which is the same string. Both names are legal and several Postgres instances in one namespace is ordinary. The collision has a direction no guard can refuse. When the projection is written first, CloudNativePG rejects the key-free secret with 'missing ca.key secret data' and the sibling's PKI never completes, so that application never starts, blaming a secret its owner never created. The controller that wrote first was within its rights, so there is nothing to refuse; and before this contract existed both applications worked. The name is now <release>.tenant-ca, which is disjoint by character class rather than by survey: application names are DNS-1035 labels and cannot contain a dot, release prefixes are dot-free, and secret names are DNS-1123 subdomains where a dot is legal. No prefix, application name and operator suffix can therefore produce a dotted name, for any operator, now or later. This is the second time the survey argument failed — the previous name was rejected because Percona claims it. Recording the reasoning so the third name is not chosen the same way. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
a30f354 to
f8cc231
Compare
The sentinel-based §5 rework predates the extraction controller's actual merge and drifted from it on three points: the redis row and its §5(a) framing still claimed a key-free self-published CA in present tense, although the forked operator that would emit it is still an open pull request; the content guard described a private-key-header check where the merged controller instead decodes each PEM block and rebuilds the chain only from blocks that parse as an x509 certificate; and three places promised a ValidatingAdmissionPolicy pinning sentinel writes to helm-controller, which was never built — RBAC denying tenants any verb on internal.cozystack.io is the actual control. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
1. What was wrong, what it now says, and what the code says
The claim checks out on both counts. Old, new, and ground truth:
| Before this PR | This PR | cozystack/cozystack main today |
|
|---|---|---|---|
| Canonical trust anchor | <release>-ca-cert |
<release>.tenant-ca |
<release>.tenant-ca |
| Owner of the projected Secret | the application-instance CR, resolved from the app.kubernetes.io/instance label on the source |
the TenantProjection sentinel |
the TenantProjection sentinel |
| How the source is selected | a label on the CA Secret, internal.cozystack.io/publish-ca-cert |
a chart-rendered TenantProjection naming sourceSecretName |
TenantProjection.spec.projections[].sourceSecretName |
Evidence, all present on cozystack/cozystack main (merged in 37a91f98b feat(cozystack-controller): extract application CA into a key-free tenant Secret):
internal/controller/cacert/reconciler.go:194—projectionSuffix = ".tenant-ca", and:353buildstarget := release + projectionSuffix. The comment there is explicit that the dot is load-bearing ("Do not 'tidy' the dot into a dash"), for exactly the DNS-1035-vs-DNS-1123 reason this PR's new §4 paragraph gives.internal/controller/cacert/reconciler.go:1103sentinelOwnerRef()— the projection's singleOwnerReferenceisinternal.cozystack.io/v1alpha1, Kind=TenantProjection,Controller: true,BlockOwnerDeletion: false. §5(b) states all three, includingblockOwnerDeletionfalse and the reason (a trust anchor must not delay app teardown).internal/controller/cacert/reconciler.go:115—TenantCALabel = "internal.cozystack.io/tenant-ca", matching §4/§5(c).api/internalapi/v1alpha1/tenantprojection_types.go— groupinternal.cozystack.io,v1alpha1, kindTenantProjection,spec.projections[].{type,sourceSecretName,sourceKey},typeenumCACert,status.conditionswithReady/SourceNotFound. Every field name in the PR's YAML block is correct.packages/apps/postgres/templates/tenant-projection.yaml— the first real consumer, rendering exactly this shape.
So the old owner claim was not merely different, it was unimplementable as written: the doc pointed the owner-reference at the application-instance CR, and the new text explains why that cannot work (apps.cozystack.io kinds are virtual, served by the aggregated apiserver over a stored HelmRelease, so there is no UID to reference). That reasoning is correct and is the most valuable part of this change.
The name history in §4 is also accurate, with one nuance worth calling out: <release>-ca-cert → <release>-tenant-ca → <release>.tenant-ca describes iterations within this PR's own commit series (a294a97 introduced -tenant-ca, f8cc231 replaced it with .tenant-ca); only -ca-cert was ever on main. The middle step is reasoning worth keeping — it is what produced the dot — but a reader may assume -tenant-ca was once published, and it was not.
2. Is amending in place the right vehicle?
Better than the pattern's usual shape, but it uses none of this repo's existing conventions.
The why is not lost — this PR deliberately preserves it in three places: §4's paragraph on the name revisions, §5's paragraph recording that the label design was replaced after the API-owner review in cozystack/cozystack#3299, and an "Alternatives considered" entry keeping the superseded label mechanism with its flaws.
But the metadata block is untouched: Date: 2026-06-24, Status: Accepted, no revision note. This repo already has three conventions that would fit — Date: … revised <date> (used by database-horizontal-autoscaling and distributed-tracing), a Supersedes: line (compute-plane), and a decisions/ record (compute-plane/decisions/0001-*.md).
Worth flagging: design-proposals/decision-records/README.md, currently in Review, names this PR by number — "#40, #41 and #36 are the same shape: an implementation finding rewriting an accepted proposal, with the why left in the pull-request body." That is only half true of this one, and adding a revised 2026-07-16 to the Date: line plus a one-line pointer to cozystack/cozystack#3299 would make it not true at all, cheaply.
3. Coherence and completeness
No dangling <release>-ca-cert or <release>-tenant-ca references to the projection remain anywhere in this repository. Every surviving -ca-cert string names a different object on purpose — PSMDB's key-bearing <release>-ca-cert, Kafka's <release>-clients-ca-cert, the redis fork's output. Both mermaid diagrams were updated. Prose, the engine table, §6 ordering, User-facing changes, Upgrade/rollback, Security, Failure cases, Testing, Rollout and Alternatives were all swept consistently. external-database-exposure/README.md changes because it is a downstream consumer that named the same anchor in three spots (dependency bullet, diagram node, WS5 hook list); all three now say <release>.tenant-ca. That is the right reason and it is consistent.
The head commit says "reconcile the proposal with the merged controller," and four things did not get reconciled:
(a) cozystack/cozystack#2990 is merged, and the doc still says it is open — in three places. It merged 2026-07-15, and internal/controller/wildcardsecret/reconciler.go is on main. The doc still carries "Edge, open: cozystack/cozystack#2990", the caveat "paths attributed to an open PR (for example the wildcard-secret reconciler in PR cozystack/cozystack#2990) are not yet on main", and Rollout step 1's "still open". The §5(b) reference to that reconciler as a skeleton is now a reference to merged code, which is strictly better — worth saying so.
(b) The RBAC claim in §5(d) understates the merged grant. The doc says the controller "holds only get/list/watch on tenantprojections". The merged packages/system/cozystack-controller/templates/rbac.yaml also grants tenantprojections/status: get, update, patch — which §5(e) requires, since the controller writes Ready. The point being made (no create/delete; the chart owns creation) is right; the word "only" is what is wrong.
(c) Open questions is stale. It still calls TenantProjection a "working name" and says "the group, kind, and the spec.projections[].type vocabulary are not yet frozen", and asks about the namespace convention. All four are settled and shipped: group internal.cozystack.io, v1alpha1, kind TenantProjection (shortName tproj), type enum pinned to CACert, namespaced, CRD at packages/system/cozystack-controller/definitions/internal.cozystack.io_tenantprojections.yaml. In a change whose purpose is reconciling with merged code, leaving "not yet frozen" over a shipped CRD is the most visible remaining drift.
(d) The sentinel YAML example's metadata.name does not match the merged chart, and implies the name matters. The doc shows metadata.name: <release>; the merged postgres chart renders name: {{ .Release.Name }}-ca. More importantly the sentinel's own name is irrelevant to the output — the controller derives the release from the helm.toolkit.fluxcd.io/name label Flux stamps (reconciler.go:332), and a sentinel without that label gets Ready=False, Reason=NoRelease. Chart authors will copy this block, so either match the shipped example or add a half-sentence that the name is free-form and the Flux label is the load-bearing part.
Two smaller factual points:
- The redis table row overstates what exists.
| redis (open PR) | chart + cert-manager | <release>-ca-tls (chart cert-manager) | yes | …. Onmain,packages/apps/rediscontains no TLS at all — a case-insensitive search fortlsacross that package returns nothing, and the templates directory has no cert-managerCertificate. So the "CA-bearing Secret today" and "PKI owner: chart + cert-manager" cells describecozystack/cozystack#2729, notmain. This PR fixed the row's conclusion (redis is not at the target) but carried the premise over unchanged. The rest of the row and the correctednot yetverdict are right, and the mongodb row's "the chart renders no TLS objects" is correct formain. external-database-exposurenow says "copying verbatim", which contradicts the other half of this PR. Its dependency bullet reads "copying verbatim from redis and kafka's own key-free CA object". Butunified-tls-pki§5(b), rewritten here, says the controller "rebuild[s] the projected value only from blocks that validate as a certificate — never copying the input verbatim", and the mergedcertificateChainPEM()does exactly that re-encode. "Copying with nothing to strip" is the phrase used elsewhere and is the accurate one.
One optional completeness note. The merged controller carries design-level behaviour the proposal does not record: the SelectorsDigestAnnotation mechanism that forces re-admission when an ApplicationDefinition's spec.secrets change, and the revocation path it exists for — when a definition stops selecting the anchor, the tenant's access must be taken away, and a frozen tenantresource=true verdict would leave a tenant reading a withdrawn trust anchor. §5(c) glosses this as "(and to false should the Secret ever stop matching)", which is the conclusion without the hard part. Also unrecorded: the source-sits-at-the-canonical-name path (key-free → silent success; key-bearing → refuse loudly, CanonicalNameOccupied), and the MultipleCACertProjections / ReleaseContested guards. None of this is required of a design doc, but revocation is a design decision rather than an implementation detail.
Line-level
- The prose is strong. §4's "collision-free by character class rather than by survey… That is a proof. 'No operator claims it today' is a guess with an expiry date, and it expired twice" is the best paragraph in the document, and it is the argument the code comment at
reconciler.go:186arrives at independently. SHOUTINGfor emphasis (ENGINES,RELEASES,CHARACTER CLASS) appears four times in one paragraph. The surrounding document uses bold; one convention would read better.- Markdown and mermaid are clean — table column count unchanged, the dotted name inside the quoted mermaid node label is fine, no broken links, the YAML block is well-formed and fenced.
- The §5 heading rename to "Delivery: a declared sentinel and one engine-agnostic controller" is good; the sub-part lettering runs (a)–(e) with no gaps.
Bottom line
The correction is accurate — the substance has my approval. <release>.tenant-ca and the sentinel owner-reference both match internal/controller/cacert/reconciler.go on cozystack/cozystack main exactly, as do the label, the CRD field names, the SourceNotFound reason, and blockOwnerDeletion: false. The reasoning added here (the dot as a structural collision proof; why the application-instance CR cannot be an owner) is correct and is the part worth having.
Requesting changes for one more pass on the four items above that the reconciliation commit missed — #2990 described as open when it merged 2026-07-15, the status subresource missing from the RBAC claim, Open questions calling a shipped CRD "not yet frozen", and the sentinel example's name — plus the two one-line fixes ("copying verbatim" in external-database-exposure, and the redis row's -ca-tls premise). All are mechanical; none touches the argument.
Reviewed with assistance from Claude Code; findings verified by hand against cozystack/cozystack main.
3 participants
Implementing the accepted design surfaced four errors in it. All are corrected here, across both proposals. Every one was found by building the thing, not by reading it.
The canonical trust anchor is now
<release>.tenant-ca, and the reason it took three names is the point.<release>-ca-certwas rejected because Percona PSMDB creates a Secret of exactly that name and puts a private key in it — a collision across engines. Its replacement<release>-tenant-cawas justified as "claimed by no operator this platform ships". That was also false, one level up: it collides across releases. For an applicationfoothe projection ispostgres-foo-tenant-ca; for a sibling applicationfoo-tenant, CloudNativePG's own CA ispostgres-foo-tenant+-ca— the same string. Both names are legal and several Postgres instances per namespace is ordinary. One direction of that collision cannot be guarded at all: if the projection is written first, CloudNativePG rejects the key-free Secret (missing ca.key secret data), the sibling's PKI never completes, and that application never starts — silently, blaming a Secret its owner never created. It is a regression, and there is nothing to refuse, because the controller that wrote first was legitimate.<release>.tenant-cais disjoint by character class instead: application names are DNS-1035 labels and structurally cannot contain a dot, release prefixes are dot-free, and Secret names are DNS-1123 subdomains where a dot is legal — so no prefix + application + operator suffix can ever produce a dotted name, for any operator, now or later. That is a proof rather than a survey, which is what the two previous names lacked.The projection cannot be owner-referenced to the application CR. The
apps.cozystack.iokinds are virtual: they are served by the aggregated apiserver, and an application is stored as a HelmRelease whosespec.valuesis the application spec verbatim. There is no application object in etcd and therefore no UID to reference. The HelmRelease is the only real object in the chain — and it is sufficient, because deleting the application through the aggregated API deletes the HelmRelease, which garbage-collects the projection with it.The source-selection label is not enough on its own. The platform runs cert-manager with
enableCertificateOwnerRef: false, so a cert-manager-issued Secret carries no ownerReference back to its application and the lineage graph cannot resolve which release it belongs to. The consume contract therefore requires a second label,internal.cozystack.io/publish-ca-cert-release, carrying the release name, stamped alongside the opt-in label in the samesecretTemplateblock.The trust-anchor guard must parse, not pattern-match. The proposal's fail-closed boundary was specified as a header check — reject anything carrying a private-key header, accept anything carrying a certificate header. That is not sufficient: PEM armour around arbitrary bytes satisfies it, and
pem.Decodedoes not close the gap either, since it validates the armour and the base64 rather than the contents. Onlyx509.ParseCertificateon every decoded block, with no trailing remainder, actually asserts "this is a certificate".All four were found by the implementation in cozystack/cozystack#3299.
Summary by CodeRabbit
<release>.tenant-caSecret name (replacing the prior<release>-ca-cert).ca.crtand uses the updated tenant-visible labeling.