docs(unified-tls-pki): fix the canonical trust-anchor name and its owner - #36
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description gives detailed context, motivation, implementation changes, and decision-record rationale. It does not include the required ✨ 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).
a30f354 to
f8cc231
Compare
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.
e1b9a82 to
b76b162
Compare
|
Done in one more commit on the branch, all six points and the smaller ones. #2990 reads as merged everywhere it appears. §5(d) states the grant the chart actually makes: get/list/watch on tenantprojections plus get/update/patch on the status subresource, with the point about the chart owning creation kept. The "working name / not yet frozen" bullet is gone; the settled shape sits next to the sentinel example, which now uses the name postgres ships and says the name is free-form because the controller reads the Flux release label. Every row of the engine table describes what main ships, so redis, mariadb, rabbitmq and opensearch no longer carry their open PRs as the present tense, and the takeaway under the table is scoped the same way. "copying verbatim" is out of both documents; Date carries the revision and a pointer to #3299; the name history says One thing the check turned up beyond the review: redis in cozystack/cozystack#2729 does not fork the operator any more, it carries the TLS work as a downstream patch and sources its sentinel from the chart CA, so the fork-based wording is gone as well. |
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Approving.
Every item raised earlier on this pull request is resolved, and I checked each against the merged controller rather than against the diff: the cozystack/cozystack#2990 status, the tenantprojections/status grant in the controller's RBAC, the settled group/version/kind and the CACert enum, the free-form sentinel name with the release derived from helm.toolkit.fluxcd.io/name (internal/controller/cacert/reconciler.go:332), and the "copying with nothing to strip" wording in external-database-exposure. The new §5(f) on revocation and the canonical-name guards holds up in every particular — the selectors digest, the managed-by-cozystack drop that hands the projection back to the lineage webhook, and the CanonicalNameOccupied / MultipleCACertProjections / ReleaseContested conditions.
What was left was small enough that I pushed it as 85bd068 rather than asking for another round:
- The engine table described
mainin the present tense, so it went stale every time the contract landed somewhere new. It is now anchored to a single dated moment, with "today" dropped from the two column headers and from the takeaway. The table is a record of the gap this contract closes, and it should not need an edit each time an engine converges. - Three cells were stale under that anchor. postgres, nats and qdrant all render a
TenantProjectionsentinel onmainand deliver<release>.tenant-ca. Section 6 and rollout step 4 still listed nats and qdrant as convergence yet to come. - redis's CA Secret is
<release>.ca-tls, with a dot (two places). Worth more than the correction: the redis chart's own comment gives the same collision-across-releases reason §4 gives for<release>.tenant-ca— releasefoo's CA Secret and releasefoo-ca's leaf Secret would otherwise both beredis-foo-ca-tls. A second chart reaching for the dot independently is corroboration §4 could cite, and I left that as prose for you rather than writing it in. mariadb's<release>-ca-tlsis genuinely hyphenated and is untouched. - The carried operator patch is cut from freshworks-oss/redis-operator#85 commit
2ad2ed8d1; the row said38b9e3732. The v3.3.5 pin and the patch path were both right.
external-database-exposure/README.md needed nothing further. Revert or force-push over my commit freely if you would rather word any of it differently — the approval stands either way.
Reviewed with assistance from Claude Code; findings verified by hand against cozystack/cozystack main and the head of cozystack/cozystack#2729.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
85bd0686 fixes three of the four engine-table staleness arms and corrects the redis patch reference. Two blockers remain: mongodb converged on main two days ago and four places still say it has not, and the redis row this commit added is a counterexample to the dot-disjointness proof two sections below it.
Reviewed at 85bd0686 against merge-base ebbd9d4. Every upstream claim below was checked against cozystack/cozystack at 3a70f91 (main, 2026-09-02).
Previous review
My CHANGES_REQUESTED of 2026-08-21 is addressed, and I re-checked all three points rather than taking the summary for it. §5(b) at line 177 now specifies the pem.Decode loop with x509.ParseCertificate; the -----BEGIN ... PRIVATE KEY----- header phrasing that review objected to is gone from the document entirely. The redis fork claim is retired: the §3 row now says the stock freshworks-oss operator has no TLS support and #2729 carries a downstream patch over v3.3.5, and line 246 records the retirement in Alternatives. The open-questions NIT went with the label mechanism.
One half of that redis point survives, in external-database-exposure, and it is listed below. Nothing else in this review re-raises an earlier one.
Findings
[MAJOR] design-proposals/unified-tls-pki/README.md:132, mongodb converged on main on 2026-09-01 and four places still say it has not
The previous head was wrong about four engines; this commit fixed three. The mongodb arm survives intact. Line 132 now reads that postgres, nats and qdrant "have since closed it by declaring a sentinel, leaving mongodb". Line 123's ca.crt to tenant? cell still reads no (gap; ...) and its second cell still says "the chart renders no TLS objects". Line 194 still groups mongodb with rabbitmq, mariadb and opensearch as engines that "converge by declaring a TenantProjection sentinel" in the future tense. Line 235 lists only postgres, nats and qdrant as merged.
All four are false on main, verified independently three times: packages/apps/mongodb/templates/tenant-projection.yaml exists and is explicitly ungated ("the PSMDB operator issues the certificate chain unconditionally, so the source Secret exists for every release"); packages/system/mongodb-rd/cozyrds/mongodb.yaml:67-68 places matchLabels: {internal.cozystack.io/tenant-ca: "true"} under secrets.include: at line 61, which is the selector §5(c) of this document names as the thing that makes the projection tenant-readable; and gh pr view 2692 returns MERGED, 2026-09-01T10:16:24Z. The chart also renders TLS objects, at packages/apps/mongodb/templates/mongodb.yaml:48-61 (secrets.ssl: {{ .Release.Name }}-ssl, tls.mode: preferTLS).
On the new anchor sentence at line 116, "The table describes main as of 2026-09-02": it does not discharge the objection, it sharpens it. mongodb merged the day before that date, so the anchor converts an undated staleness into a dated claim that is checkably false on its own terms. An anchor is worth keeping, but only over rows that were true on the date it names.
[MAJOR] design-proposals/unified-tls-pki/README.md:24, #2692 is still listed as still-open, which leaves the Security section naming a merged PR as a live private-key leak
Same root cause as the finding above, different consequence, and this commit did not touch either line. Line 24 claims every cited path is on main "except those attributed to the still-open per-app TLS series", and lists cozystack/cozystack#2692 among them; it is MERGED. Line 208 therefore still carries "mongodb (cozystack/cozystack#2692) projects the key-bearing <release>-ca-cert" in the inventory of in-flight PRs that "currently propose labelling a key-bearing Secret to tenants".
On main mongodb does no such thing: packages/system/mongodb-rd/cozyrds/mongodb.yaml exposes only mongodb-{{ .name }}-credentials plus the tenant-ca label selector, and the merged sentinel sources {{ .Release.Name }}-ssl with sourceKey: ca.crt. A security document whose live-risk list names a shipped and already-corrected PR as an open leak is worse than one that omits it. Since this and the previous finding are one missed merge, both clear in a single edit pass.
[MAJOR] design-proposals/unified-tls-pki/README.md:140, the dot-disjointness proof is falsified by the redis row this commit added
Line 140 states the argument as a universal: "So <prefix><application><operator-suffix> is dot-free by construction and can never equal a dotted name — for any application name, any operator, and any suffix an operator may add in future. That is a proof." Line 121, added by 85bd0686, records that the redis chart's CA Secret is <release>.ca-tls. That is <prefix><application> followed by a dotted suffix, which is exactly the composition line 140 says cannot produce a dotted name. I verified the row is correct, so the defect is in the proof, not the row: cozystack/cozystack#2729 at head 3cf9431 renders secretName: {{ .Release.Name }}.ca-tls, and its own template comment gives the reason ("A dot, not a hyphen: the Secret suffix set holds both -ca-tls and -tls, so ... are both redis-foo-ca-tls"). The same PR renders <release>.tls, and <release>.ca-cert is written by the redis operator itself (caCertSecretName: {{ .Release.Name }}.ca-cert), so one redis release contributes three dotted per-release Secret names to its namespace and one of them comes from an operator, which is precisely the quantifier line 140 ranges over.
Two of the proof's three premises are genuinely machine-enforced, and I checked both: application names go through IsDNS1035Label at pkg/apis/apps/validation/validation.go:70, and ApplicationDefinitionRelease.Prefix carries +kubebuilder:validation:Pattern=^[a-z0-9-]*$ at api/v1alpha1/applicationdefinitions_types.go:87. The third premise, that the appended suffix is dot-free, is the one nothing enforces, and it is now contradicted inside this document. The conclusion may well still hold, since .ca-tls is not .tenant-ca; what has gone is the derivation. What remains is "no dotted suffix in use equals .tenant-ca", which is a survey over a set cozystack is itself growing, and a survey is the epistemic category this very paragraph disparages two sentences earlier. Rewrite line 140 to say the composition is dot-free whenever the appended suffix is, name that as the one assumption the platform does not enforce, and point at the CanonicalNameOccupied guard in §5(f) as what covers the residue. What would change my mind: a validating rule constraining suffixes on per-release Secrets, at which point all three conjuncts are enforced and "proof" is earned.
[MAJOR] no decision record for a revision that reverses an accepted proposal's design
.github/pull_request_template.md opens its "Before review" section with a required item: if the PR revises a merged design proposal it adds a decision record under that proposal's decisions/ directory, or says in the body why none is needed. design-proposals/README.md:61 repeats it, and :93 names the exact trigger, "Implementation contradicted an accepted proposal and the design changed course". That is this PR: an accepted proposal's §5 loses its source-selection label and gains a namespaced CRD after an API-owner review, and the canonical object name changes.
At 85bd0686 there is still no design-proposals/unified-tls-pki/decisions/, no ## Decisions section in either README, and no sentence in the PR body explaining the omission. Two proposals already follow the convention (design-proposals/compute-plane/decisions/0001-computeplane-ships-as-an-operator-owned-module.md linked from compute-plane/README.md:39, and decision-records/decisions/0001-decision-records-live-with-their-proposals.md from decision-records/README.md:18), so the shape is settled. The Alternatives bullet at line 249 is good raw material, but it lives inside the document being revised, which is the loss design-proposals/README.md:61 describes.
[MINOR] design-proposals/unified-tls-pki/README.md:177, the normative bullet omits the failure behaviour and reads as filter semantics
Line 177 specifies the guard as "decode it in a pem.Decode loop, parse each block with x509.ParseCertificate, and rebuild the projected value only from blocks that validate as a certificate". Two of those three clauses match the shipped code exactly, including the rebuild: certificateChainPEM at internal/controller/cacert/reconciler.go:1042-1075 re-encodes only the parsed DER rather than copying the input through. What line 177 never says is what happens to a block that does not validate, and "rebuild only from blocks that validate" is naturally read as filtering: drop the bad blocks, publish the rest. The code does the opposite. It returns on the first block whose Type != "CERTIFICATE", on the first x509.ParseCertificate failure, and on any trailing bytes that are not a PEM block, so the whole value is rejected and nothing is published:
for len(rest) > 0 {
var block *pem.Block
block, rest = pem.Decode(rest)
if block == nil {
return nil, fmt.Errorf("%w: value carries %d bytes that are not a PEM block", errNotCertificate, len(rest))
}
if block.Type != certificatePEMType {
return nil, fmt.Errorf("%w: PEM block is of type %q, want %q", errNotCertificate, block.Type, certificatePEMType)
}
if _, err := x509.ParseCertificate(block.Bytes); err != nil {
return nil, fmt.Errorf("%w: %v", errNotCertificate, err)
}internal/controller/cacert/reconciler_test.go:763 (TestReconcile_RefusesNonCertificate) pins it: no Secret is published and the sentinel goes Ready=False, Reason=SourceRejected. The document does state the rejection, but only at line 215, in the failure-modes list, and that sentence undercuts itself by giving "the guard rebuilds the projection only from validated blocks" as the reason the controller refuses to write. So the contract is recoverable from the document as a whole and is not misstated; the defect is that §5's normative bullet, read on its own, licenses a filtering implementation of a security boundary. Add the rejection and the no-trailing-remainder rule to line 177, and drop the rebuild clause from line 215's parenthetical, where it explains the wrong thing.
[MINOR] design-proposals/external-database-exposure/README.md:19, this PR still classifies redis two different ways across the two files
85bd0686 did not touch this file. Line 19 says the controller copies "with nothing to strip from redis and kafka's own key-free CA object". design-proposals/unified-tls-pki/README.md:150 says the opposite, "redis is not in this group, despite appearances", and line 194 there calls redis "a stripping case rather than a straight copy". Both statements arrived in this PR. The unified-tls-pki version is the one the evidence supports, so redis belongs on the stripping side here too.
[MINOR] design-proposals/unified-tls-pki/README.md:121, the contract is silent on source-Secret naming while its two in-review consumers diverge
The table now records redis's CA Secret as dotted <release>.ca-tls and mariadb's as undotted <release>-ca-tls, and I confirmed both are accurate: cozystack/cozystack#2729 renders secretName: {{ .Release.Name }}.ca-tls, cozystack/cozystack#2680 at head 9ece9dc renders secretName: {{ .Release.Name }}-ca-tls. So this is not an incomplete rename. It is two sibling PRs under one contract solving the same suffix-set problem two different ways, with the contract saying nothing. §3 calls this table "the artifact the per-app pull requests must conform to", and §4 argues at length that the dot is load-bearing, so the document is the right place to say whether a dotted separator is now the convention for chart-rendered per-release Secrets or a redis-local choice. Leaving it unstated means the next per-engine PR guesses.
[MINOR] design-proposals/unified-tls-pki/README.md:194, mongodb's merged sentinel names the leaf Secret, not the CA Secret
This line says mongodb and the remaining engines converge "by declaring a TenantProjection sentinel that names their CA Secret", and line 151 says "The projection sources from the key-bearing CA Secret". The merged mongodb chart names {{ .Release.Name }}-ssl, and its template comment gives three reasons against <release>-ca-cert: it is the name the chart sets in spec.secrets.ssl, the operator's self-signed fallback writes it when cert-manager is absent, and during a CA rotation the operator merges old and new CA into its ca.crt. As written, the text points the next per-engine author at the source the merged chart deliberately rejected.
[MINOR] design-proposals/unified-tls-pki/README.md:188, one name is used for two different labels
§5(f) says the drift write "drops the internal.cozystack.io/managed-by-cozystack marker", and line 219 says a foreign Secret at the target name is left untouched by "the management-label guard". In the merged controller those are separate labels with separate owners. reconciler.go:121 defines ManagedLabel = "internal.cozystack.io/ca-cert-copy" as the anti-clobber guard, written by the reconciler. reconciler.go:169 defines managedByCozystackLabel as the webhook's marker, and its comment is explicit that the reconciler "never writes it; it only takes care not to strip it" (stamped at internal/lineagecontrollerwebhook/webhook.go:36,161). Reading the document alone, an implementer would use one label for both, and the selector-digest write would then make the controller treat its own projection as a foreign Secret on the next pass. Naming internal.cozystack.io/ca-cert-copy at line 219 removes the trap.
[MINOR] design-proposals/unified-tls-pki/README.md:215, the failure taxonomy is a strict subset of the shipped one
The section enumerates SourceNotFound, NoRelease, CanonicalNameOccupied, MultipleCACertProjections and ReleaseContested. The merged reconciler carries more Ready reasons an operator will actually meet: SourceRejected (content fails the certificate gate), SourceNotReady (source exists, the named key is absent or empty), SourceInvalid, UnsupportedProjectionType, ProjectionCollision and ProjectionTerminating, defined at reconciler.go:229-240 and set at :437,476,503,512,517,520,523. The omission compounds the line-177 gap, because SourceRejected is precisely the outcome that bullet leaves unstated.
[MINOR] design-proposals/unified-tls-pki/README.md:226, the Testing section describes a suite that exists and is a different shape
The bullet reads "The extraction controller gets an envtest/Ginkgo suite (its skeleton mirrors internal/controller/wildcardsecret/reconciler_test.go)". The suite is merged, and neither it nor the cited skeleton is Ginkgo or envtest: internal/controller/cacert/reconciler_test.go holds 42 plain func Test... cases over a fake client, and internal/controller/wildcardsecret/reconciler_test.go holds 24 of the same shape (grep -n 'ginkgo\|Ginkgo\|envtest' returns nothing in either). Since the section is being rewritten anyway, list what is missing rather than what exists. There is no case for a source carrying a valid certificate followed by trailing junk, which is the boundary the line-177 finding turns on.
Claim mismatches
[MISSING] "The consume contract therefore requires a second label, internal.cozystack.io/publish-ca-cert-release, carrying the release name". grep -rn 'publish-ca-cert-release' over the tree at 85bd0686 returns nothing. The release-attribution problem is real and the diff does address it, but by the sentinel carrying Flux's helm.toolkit.fluxcd.io/name label, not by a second label on the Secret. The PR description is describing a revision the branch has moved past.
[MISSING] "The HelmRelease is the only real object in the chain, and it is sufficient". Line 180 says the opposite: the projection owner-refs the TenantProjection, "not to the application-instance CR, and not to the HelmRelease directly". The premise behind the claim holds (no application object in etcd, so no UID), and the merged code agrees (the reconciler.go package comment has a section headed "Why the owner is the sentinel, not the HelmRelease"), but the stated conclusion is a revision behind the diff.
[PARTIAL] "the trust-anchor guard must call x509.ParseCertificate on every decoded PEM block with no trailing remainder". Accurate as a description of the merged code, and it is the fix the document should record. The document does not say it; see the line-177 finding.
[PARTIAL] "All four were found by the implementation in cozystack/cozystack#3299". #3299 is CLOSED with mergedAt: null. The implementation landed as #3407 (merged 2026-07-23, and it shipped projectionSuffix = ".tenant-ca" in its first revision) and #3408 (merged 2026-07-24). Citing #3299 for the review that prompted the redesign is fine; a reader looking for the code will not find it there.
Caveats
- Upgrade and fresh-install impact: this PR touches no
packages/, so there is no chart, migration, RBAC or image surface. The one runtime question worth closing was whether the rename in this document corresponds to a rename in a shipped cluster, and it does not.gh pr diff 3407showsprojectionSuffix = ".tenant-ca"present from that PR's initial merge, so no release ever wrote a projected<release>-ca-cert, and the document's claim on that point at line 138 holds. Nothing here changes an upgrade or an install path. - There are no charts here to run a configuration matrix over, so I ran the design's own matrix instead and re-checked all twelve per-engine rows at
85bd0686. postgres, nats and qdrant now matchmain. mongodb does not. - Two things I checked and found correct, so nobody needs to re-open them: §5(d)'s RBAC claim is exact (
packages/system/cozystack-controller/templates/rbac.yaml:29-34grantsget,list,watchontenantprojectionsandget,update,patchon the status subresource, and no tenant ClusterRole inpackages/system/cozystack-basics/templates/clusterroles.yamlmentionsinternal.cozystack.io; the wildcards at lines 34, 86 and 296 are scoped toapps.cozystack.io,sdn.cozystack.ioandkubevirt.io). TheenableCertificateOwnerRef: falsepremise is real, atpackages/system/cert-manager/charts/cert-manager/values.yaml:268, with no override. And both new facts in85bd0686are correct: redis's dotted<release>.ca-tlsand the patch reference2ad2ed8d1, which I read in#2729's ownpatches/tls.diffheader. - I derived a tenant-side attack on the canonical name (a tenant pre-creating a Secret at
<release>.tenant-cato suppress the anchor permanently) and refuted it by execution: tenants hold no verb on rawsecretsat any tier, onlyget,list,watchoncore.cozystack.io/tenantsecrets(clusterroles.yaml:44-51,186-191), so there is no path to create one. - One leg I did not execute, and reasoned from source comments instead. I read the marker's re-admission semantics in the reconciler's own commentary and confirmed the webhook constant and its write site, but did not trace the
objectSelectoron the shippedMutatingWebhookConfiguration. That leg of the label-conflation finding rests onwebhook.go:36,161plus the reconciler comment, not on the registration manifest. - No cluster was touched at any point: no kubectl, no
--dry-run=server. Upstream evidence came from a--filter=blob:noneclone ofcozystack/cozystackat3a70f91and fromghreads of#2692,#2729,#2680,#3299,#3407,#3408.
Recommended follow-ups
packages/apps/mongodb/templates/tenant-projection.yamlcarries a three-reason argument for sourcing the leaf rather than the CA Secret, and that reasoning exists nowhere else. It is a decision record waiting to be written, and it would slot under the samedecisions/directory this PR needs anyway.- The upstream implementation stamps
internal.cozystack.io/ca-cert-copyand the webhook stampsinternal.cozystack.io/managed-by-cozystack, and neither name appears in the proposal. A short table of the labels and their writers in §5 would stop the conflation recurring in the next revision.
85bd068 to
f8ea55a
Compare
|
All ten are in at b71cca3, together with a reshape that answers the staleness from the other side. The engine table and the prose around it no longer describe main at all: the table keeps the structural facts per engine (who owns the PKI, which Secret carries the CA and whether it holds a key, how the anchor is delivered under this contract), and cozystack/cozystack#2814 is the one place that tracks who has converged. A dated anchor would have been false again on the next merge, so the date is gone rather than moved, and the mongodb arms and the #2692 references went with it. Line 140 is conditional now: the two enforced legs are cited where they live ( One point I checked and did not take: the suite does have a case for a valid certificate followed by trailing junk, Three corrections you did not ask for, all verified against the tree. kafka's trust anchor was named as The reviewer-pushed commit is kept with its authorship; only its trailers moved to the repository's form. Known and not fixed here, for your read: §5(g)'s label table credits |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Every finding from the previous round is closed, and I verified each against the tree and against cozystack/cozystack main rather than against the commit subjects. What is left is small in edit size: a red DCO check, and three factual items. One of them lands in prose this repository freezes at merge, which is why it is marked MAJOR despite being a single sentence.
Reviewed at b71cca3 against merge-base ebbd9d4.
Where the previous round stands
All eleven findings and all three description mismatches are closed. That is worth saying plainly, because several commits in this push exist to repair earlier commits in the same push, so I re-derived each one instead of trusting the subjects.
The checks that carried the most weight: the twelve Ready reasons at README.md:250-261 are exactly the twelve constants at internal/controller/cacert/reconciler.go:230-241; the §5(b) content gate now says "all-or-nothing, and not a filter" and enumerates the three refusal conditions, which is what certificateChainPEM (reconciler.go:1042-1074) actually does; the two labels at README.md:207 and :209 map to reconciler.go:121 and :169, and the controller only ever deletes the second (reconciler.go:889); the suite description matches 42 and 24 plain func Test cases with no ginkgo, envtest or gomega imports; and §4 now calls the third leg of the dot argument an assumption rather than a proof, naming the dotted suffixes already in use.
Findings
[MAJOR] design-proposals/unified-tls-pki/decisions/0001-the-trust-anchor-is-declared-by-a-namespaced-sentinel.md:39, the residual assumption is stated as something this same PR shows is already false
The Consequences bullet says the dot "rests on one assumption the platform does not enforce, that per-release Secret suffixes are dot-free", and Revisit if at :43 repeats it. README.md:146 in this same PR says the opposite: dotted suffixes are already in use, since under #2729 the redis chart names its CA Secret <release>.ca-tls and its leaf <release>.tls, and the patched operator writes <release>.ca-cert. I confirmed all three on that branch's head 3cf9431.
Read on its own, the record therefore tells a future contributor the design rests on a premise that was already falsified when it was written. The assumption it actually rests on is the narrower one README.md:146 lands on: no suffix in use spells .tenant-ca. The bullet's second half already describes the right mitigation, so this is a sentence, not a rethink.
The edit is one sentence, and I am still calling it MAJOR, because the severity here follows the consequence rather than the size of the fix. design-proposals/README.md:154 freezes record prose at merge: "the body of an accepted record is not edited. Fix typos and broken links; change nothing else." So this is a one-way door. After merge, a record whose stated rationale does not hold can be corrected only by superseding it, and the wrong reasoning stays in the history the record exists to be.
[MINOR] two commits carry no DCO sign-off, and the check is red
b71cca3 (head) and 606a4d0 carry Assisted-by: LLM and no Signed-off-by. The other 31 commits on the branch have it. gh pr checks 36 reports DCO fail. The repository asks for sign-off twice in its own docs (design-proposals/README.md:38-42 and :122-126) and the PR template carries it as a checkbox (.github/pull_request_template.md:21). Severity aside, this is the one item that mechanically stops the merge button from working.
[MINOR] design-proposals/unified-tls-pki/README.md:133, the clickhouse row's stated reason is not what the chart does
The row gives "supplied cert mount" as the CA-bearing Secret and "mounts a supplied cert; no native issuance" as the operator capability. The chart mounts no server certificate. packages/apps/clickhouse/templates/ contains no cert-manager objects, and grepping the chart for certificateFile, privateKeyFile, caConfig, https_port, tcp_port_secure, 9440, 8443 and verificationMode returns nothing. The only certificate input is backup.endpointCA (values.yaml:140, :162-164, mounted at templates/clickhouse.yaml:370-382), an outbound trust bundle the backup sidecar uses to verify an S3 endpoint. That points the other way from a server certificate a tenant would verify.
The verdict cell is right and the reason cells are not. clickhouse is out of the series because the chart serves no TLS at all, not because it serves TLS from a certificate someone supplied. This matters more than it would have last round, since the PR's own goal is that the table now carries only structural facts per engine.
[MINOR] design-proposals/unified-tls-pki/README.md:45, an absolute quantifier about postgres that main contradicts
"The only tenant-facing object is <release>-credentials". On main the postgres tenant also receives the trust anchor: packages/system/postgres-rd/cozyrds/postgres.yaml:45-46 carries a second spec.secrets.include entry with matchLabels: {internal.cozystack.io/tenant-ca: "true"}, fed by packages/apps/postgres/templates/tenant-projection.yaml, and hack/e2e-chainsaw/postgres/chainsaw-test.yaml:232 reads postgres-test.tenant-ca back through core.cozystack.io/tenantsecrets.
The adjacent clause this push rewrote, "nothing in the engine hands it ca.crt", is engine-scoped and stands, and the citation to the dashboard resource map is accurate for name grants. It is the quantifier that fails, along with the present-tense "postgres carries a trust-anchor delivery gap" in the same sentence, and both sit in tension with :183 and :267, which treat postgres as the engine that has converged. Someone reading §Context in order is told postgres has no anchor delivery, then told two sections later that it is the reference implementation. What would change my mind: a one-line frame saying §Context describes engine-native capability rather than current tenant-visible state. With that, the sentence reads correctly and this drops.
Claim mismatches
[PARTIAL] "The engine table ... now carries only structural facts per engine (who owns the PKI, which Secret carries the CA and whether it holds a key, how the trust anchor is delivered under this contract)". True for ten of the eleven rows. The clickhouse row's CA-bearing-Secret and operator-capability cells describe a BYO server certificate the chart does not have.
The other load-bearing claims in the description check out: #3407 merged 2026-07-23 and #3408 merged 2026-07-24 while #3299 is closed unmerged; #3407 as merged already carried projectionSuffix = ".tenant-ca"; release prefixes are dot-free by validation at api/v1alpha1/applicationdefinitions_types.go:87; the Ready reasons match the reconciler one for one; and both records are linked from Decisions.
Caveats
- The eleven-row engine table was re-checked row by row against main and against the four open series branches, not only the rows flagged last round. Everything except clickhouse holds. Kafka's key-freeness is correct upstream Strimzi behaviour, but the cited file (
packages/apps/kafka/templates/dashboard-resourcemap.yaml:22-23) evidences the names and the tenant grant rather than the absence of a key. The claim is right; the citation carries less than it looks like it does. decisions/0002hedges its second and third reasons as claims about PSMDB behaviour "that the comment asserts and this record has not independently verified against upstream". Both verify at the pinned v1.22.0 (packages/system/mongodb-operator/charts/psmdb-operator/Chart.yaml): the self-signed fallback creates only the leaf and internal Secrets and never<cluster>-ca-cert(pkg/controller/perconaservermongodb/ssl.go:444-509, with-ca-certreachable only fromapplyCertManagerCertificatesatssl.go:396-398), andmergeNewCA(ssl.go:309-374) merges old and new CA into the leaf'sca.crtviatls.MergePEM(pkg/psmdb/tls/pem.go:43-60) while never touching-ca-cert. Since record prose freezes at merge, the hedge is worth tightening now rather than carrying it permanently.README.md:19marks#2812and#2400as closed but leaves#2816and#2977unannotated. Both are CLOSED as COMPLETED, on 2026-07-14 and 2026-07-27. The line is untouched by this diff, so it is adjacent rather than introduced, but it is the same staleness class the rest of the PR is cleaning up.- Verified negatives, recorded so nobody re-opens them: all five relative links in the four changed files resolve; a repo-wide grep for
<release>-ca-cert,<release>-tenant-caandpublish-ca-certfinds only deliberate historical mentions; the only other proposal referencing this one (distributed-tracing/README.md:179) carries no naming dependency; no chart'sdashboard-resourcemap.yamlgrants.tenant-caby name, which matchesREADME.md:222;packages/apps/*/tests/tenant_projection_test.yamlexists for all four converged engines, matchingREADME.md:268; and the Testing section's claims about what the e2e fixtures do and do not assert are accurate, including that no case mutates a source between two reconciles. - There are no charts, values or templates in this diff, so there is no configuration matrix to run. The analogue of an upgrade check here is whether downstream consumers of the accepted contract break, and they do not: the document is moving toward what shipped, not away from it.
- No cluster was touched at any point: no kubectl, no
--dry-run=server. Upstream evidence came from reads ofcozystack/cozystackmain and of the#2729,#2680,#2682and#2683branches.
Recommended follow-ups
README.md:5describes this revision as "the §5 delivery mechanism, revised after the API-owner review incozystack/cozystack#3299", which undersells it. This push also rewrote §3's table, §4's naming and separator rule, §6, Testing and Alternatives.decisions/0001:9lists#3411underImplemented in, while the PR description names only#3407and#3408. Worth aligning the two.
b71cca3 to
e8558bc
Compare
|
Pushed 13606b8, DCO is green now. The record's assumption is split in two: no suffix in use ends with |
Implementation surfaced errors in the accepted design that the text now corrects. The canonical projection cannot be named <release>-ca-cert or <release>-tenant-ca: Percona PSMDB claims the first with a key-bearing Secret, and the second collides across releases, because for an application foo-tenant CloudNativePG's own CA Secret is postgres-foo-tenant plus -ca, the same string as the projection of application foo. The name is now <release>.tenant-ca, disjoint by character class rather than by survey: application names are DNS-1035 labels and release prefixes are dot-free, while Secret names are DNS-1123 subdomains where a dot is legal. The projection cannot be owner-referenced to the application CR. The apps.cozystack.io kinds are virtual, the HelmRelease is the only real owner, and deleting the application deletes it. Record what has merged: the edge tier, the per-tenant wildcard propagation, the wildcard-secret reconciler, and the TenantProjection API with its group, version, short name, namespace scope and single CACert projection type. State the controller's RBAC as the chart grants it, and the fact that the release is derived from the Flux release label, with NoRelease reported when it is absent. Drop the promised ValidatingAdmissionPolicy on sentinel writes, which was never built; RBAC denying tenants any verb on internal.cozystack.io is the control. Describe the content guard as the merged controller has it: each PEM block is decoded and the chain rebuilt only from blocks that parse as an x509 certificate. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
The engine table described `main` in the present tense, so it goes stale every time the contract lands somewhere new. Anchor it to one dated moment and drop "today" from the two column headers and the takeaway; the table is a record of the gap this contract closes, not a tracker of `main`. Three claims were stale under that anchor. postgres, nats and qdrant all render a TenantProjection sentinel on `main` and deliver `<release>.tenant-ca`, so their "ca.crt to tenant?" cells and the takeaway now say so, and section 6 and the rollout list no longer carry nats and qdrant as convergence still to come. Two identifiers were wrong outright: redis's CA Secret is `<release>.ca-tls`, with a dot for the same collision-across-releases reason section 4 gives for the canonical name, and the carried operator patch is cut from freshworks-oss/redis-operator#85 commit 2ad2ed8d1. mariadb's `<release>-ca-tls` is hyphenated and is left alone. Assisted-by: LLM Signed-off-by: Timofei Larkin <lllamnyp@gmail.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…ller delivers it The delivery mechanism in section 5 is the one that shipped: a chart renders a TenantProjection sentinel naming the engine's CA-bearing Secret, the controller copies ca.crt alone into <release>.tenant-ca, and the ResourceDefinition selects that copy by the tenant-ca label. The label leg that earlier revisions described was never merged and is gone from the text. The engine table carries structural facts only: who owns the PKI, which Secret holds the CA and whether it carries a key, how the anchor is delivered under this contract. Which engines have converged is tracked in cozystack/cozystack#2814, not here, so the table cannot go stale with the next merge. clickhouse serves no TLS at all; its only certificate input is the outbound trust bundle of the backup sidecar. postgres is described on its own, before the contract, which is what the whole Context section does. kafka's key-free CA Secret is cited from Strimzi's split between the CA certificate and CA key Secrets. The twelve Ready reasons match the reconciler one for one, and the content gate is described as all-or-nothing: a value is refused or passed whole, and the single-key copy strips unconditionally. The dot in the canonical name rests on two assumptions the platform does not enforce, and both are stated: no per-release suffix in use ends with .tenant-ca, and the release name the controller derives is dot-free, which the apps API guarantees and the Flux name label the controller actually reads does not. Dotted suffixes are already in use, so the argument is conditional on that set. References to work that never shipped are corrected: community#29 was closed, and the ExposureClass/ServiceExposure implementation was removed before v1.6.0 after riding its release candidates. Closed workstreams are marked as such. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
The repository convention asks for a decision record when an accepted design changes course, and this revision changes it twice. 0001 records that the trust anchor is declared by a namespaced TenantProjection sentinel and published as <release>.tenant-ca. It was decided in the rework of cozystack/cozystack#3299 that merged as cozystack/cozystack#3407, with #3408 and #3411 completing it. The record states the argument against the label mechanism in its own voice, lists the rejected alternatives with where each was argued, and names the two assumptions the dotted name rests on together with the mitigation: a Secret already sitting at the canonical name is refused, never adopted. 0002 records that mongodb sources the anchor from its leaf Secret rather than from the operator's CA Secret. The three reasons are verified against the pinned PSMDB operator v1.22.0: the self-signed fallback never writes the CA Secret, and a CA rotation merges old and new CA into the leaf's ca.crt while the CA Secret carries only the current one. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
…ervice The proposal's orchestration story pointed at a layer that reconciled expose entries into ServiceExposure objects. That API group was removed in favour of native Service type: LoadBalancer with loadBalancerClass, on the ground that managed-application charts own their Service, so the document named an integration point the platform does not have. Write the decision down where its alternatives can be weighed once: what the removal settled, the three shapes it turned down and where each was argued, and what follows for a design that wanted an object in between. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
… that exist Section 5 handed the trigger and the per-release route to a layer that reconciled expose entries into ServiceExposure objects. Reground it on what the tenant Gateway offers: one tls-<name> passthrough listener per tlsPassthroughListeners entry, nothing routed until a TLSRoute attaches by sectionName and names a backend, and the release's own chart rendering that route from values it already reads. Native LoadBalancer Services remain the path for the engines that cannot be SNI-routed. Claims the same reading falsifies go with it: the listeners declare no route kinds rather than restricting them to TLSRoute, declaring an entry withdraws termination from every name its SNI covers, route attachment is confined to the Gateway's own namespace, the listener cap fails the whole render rather than one listener, and the shared listener does not disappear with an engine's last release. The flat-hostname phase gains the second gate that rule puts on it, in each place that describes the phase. Who declares the per-engine entry, the app chart or the tenant chart, is stated as an open question with the cost on each side rather than answered. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
d819275 to
48ed9e8
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Everything from the previous round is closed, the DCO check is green, and the new material in the sibling proposal holds up against the tree. Two notes below, neither of which needs to gate the merge.
Reviewed at 48ed9e8 against merge-base ebbd9d4, which has not moved across any round. History was squashed to six commits and rebased, so I judged staleness by content rather than by commit ids.
Where the previous round stands
All four findings and both caveats are closed. I re-derived each against the tree and against cozystack/cozystack rather than reading the commit subjects.
The decision record's residual-assumption bullet (decisions/0001:39) is not just narrowed, it is better than what the review asked for. It now splits the residue into two assumptions and catches one I had missed: the controller derives the release from a helm.toolkit.fluxcd.io/name label value, where a dot is legal, so a hand-written HelmRelease sits outside the DNS-1035 and release-prefix guarantees that cover the apps API path. That is the sharper statement of the gap.
The rest: the clickhouse row now reads "no server TLS" with backup.endpointCA named as the outbound trust bundle it is; the postgres sentence drops the absolute quantifier for "on its own the engine hands the tenant one object" and frames the gap as engine-native with a pointer to §5; decisions/0002 drops the "has not independently verified" hedge for a statement that both PSMDB claims hold at the pinned operator version; decisions/0001:9 now carries #3411 with the reason it belongs there, and #3411 is indeed the merged change that requires a dot-free release prefix; and all six commits carry sign-off, with gh pr checks 36 reporting DCO pass.
The engine table deserves its own line. It carried a stale row in every previous round, and the fix this time is structural rather than another correction: the delivery column now states the shape of the operation under the contract, straight copy or strip or out of the series, instead of a per-engine merge-state verdict that goes stale the moment an engine converges. That retires the whole defect class. I re-checked the rows against main anyway. packages/apps/{postgres,mongodb,nats,qdrant}/templates/tenant-projection.yaml exist and the other engines' do not, which matches; mongodb's row correctly names the leaf <release>-ssl; and the mariadb row's "straight copy" is right, since #2680 names the operator's key-free <release>-ca-bundle as the extraction source in its own template comment.
Notes
Neither of these blocks.
[NIT] the new decision record is accurate on every claim I checked, and one of its citations has a loose end upstream rather than here. decisions/0001:8 describes cozystack/cozystack#3164 as the argument "settled by merging cozystack/cozystack#3218". #3218 did merge, on 2026-07-16, but #3164 is still open. Nothing in the record is wrong, since it never claims the issue was closed. It is just that the issue the record treats as resolved is still sitting in the tracker, and closing it with a pointer to the record would leave the two consistent.
[NIT] the mariadb row cites <release>.ca-tls, which is a fact about an open pull request rather than about main. That name changed under the row while this PR was in review: at #2680's earlier head it rendered <release>-ca-tls with a hyphen, and at its current head b24ed36 it renders the dotted form. The row is correct as of today. The general point is that rows tracking unmerged series will keep drifting by construction, and the table now says it is free of merge state without saying that some cells still track branches. A clause naming which rows follow open PRs would make that explicit. Worth noting rather than fixing: the same drift also resolved the previous round's redis-versus-mariadb separator divergence on its own, since both now use the dot.
What I verified on the new material
The exposure decision record checks out on every load-bearing claim. #3081 merged 2026-06-30 introducing network.cozystack.io/v1alpha1 with ExposureClass and ServiceExposure; #3164 was filed 2026-07-01, which is the "next day" the record describes; #3218 merged 2026-07-16, matching the record's date and its "two weeks after the filing"; #3342 is open, and the record cites it as specifying rather than shipping the TenantGateway field. cozystack/community#29 closed on 2026-07-23, so "open at the time" is right for a decision dated 2026-07-16. docs/changelogs/v1.6.0.md:119 confirms the group was "introduced and removed within this cycle", never shipped, and carries the same migration steps the record's third consequence gives. publishing.loadBalancerClass does reach charts as _cluster.load-balancer-class and packages/extra/ingress/templates/nginx-ingress.yaml:3 is what reads it; I checked the eleven managed-database charts and none reads that value, so the consequence that a managed database's Service lands on the cluster default is right. All four cited paths exist on main.
On conventions, the record sits at decisions/0001 under the proposal it amends, numbered per proposal, with the same header block as the existing records, an alternatives section that names the options that lost rather than the ones that were never viable, and a link from a Decisions section in the proposal.
The §5 rewrite is coherent with it: listener synthesis stays with the TenantGateway controller and the per-release TLSRoute moves to the release's own chart, which is derived from the decision rather than assumed, and the validation contract on TLSPassthroughListener is tighter than the version it replaces, moving to schema plus CEL on the CRD so a bad entry is refused on write.
Caveats
- I read the added prose in
external-database-exposure/README.mdfor coherence with the new decision record and for the §5 API surface, not line by line for its own internal consistency. That proposal has broken agreement with its sibling twice, both times on redis, and both directions now agree, but a full independent pass over its 57 added lines is not something this review did. - The
#3164and#2680observations are about upstream state that can change after this merges. Neither is a property of this diff. - No cluster was touched: no kubectl, no
--dry-run=server. Evidence came from the clone, from reads ofcozystack/cozystackmain, and fromghreads of#2680,#2729,#3081,#3164,#3218,#3342,#3407,#3408,#3411andcommunity#29.
3 participants
Implementing the accepted design surfaced four errors in it, and the implementation that landed (cozystack/cozystack#3407, #3408, and the release-prefix pattern in #3411) settled each one differently from what the proposal said. This PR brings both proposals in line with what shipped and adds the decision records the repository convention asks for when an accepted design changes course. The review that forced the redesign happened on cozystack/cozystack#3299, which was closed in favour of #3407.
The external-database-exposure proposal gets the same treatment in its section 5. It orchestrated exposure through the
ServiceExposurelayer that cozystack/cozystack#3218 removed before v1.6.0, after the API had ridden the release candidates. It now describes the objects that exist: aTenantGatewaypassthrough listener pertlsPassthroughListenersentry (cozystack/cozystack#3342), aTLSRouterendered by the release's own chart the way the harbor chart renders itsHTTPRoute, and a LoadBalancer Service for the engines that cannot be SNI-routed. Who declares the per-engine entry, the app chart or the tenant chart, stays an open question with the cost written on each side. A decision record for that proposal records what the removal settled and the shapes it turned down.The canonical trust anchor is
<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-cacollided across releases: for an applicationfoothe projection ispostgres-foo-tenant-ca, and for a siblingfoo-tenantCloudNativePG's own CA ispostgres-foo-tenant+-ca, the same string. One direction of that collision cannot be guarded, since the controller that wrote first was legitimate.<release>.tenant-caavoids it by character class: application names are DNS-1035 labels and cannot contain a dot, release prefixes are dot-free by validation, so<prefix><application><suffix>is dot-free whenever the appended suffix is. That last assumption is the one the platform does not enforce, and the proposal now says so and points at theCanonicalNameOccupiedguard for the residue rather than calling the argument a proof.The projection is owned by a declared sentinel, not by the application or the HelmRelease. The
apps.cozystack.iokinds are virtual, so there is no application object to reference. The shipped design has the chart render a namespacedTenantProjectionthat names the source Secret and the key; the controller resolves the release from thehelm.toolkit.fluxcd.io/namelabel Flux stamps, owner-references the projected Secret to the sentinel, and withdraws the projection when the sentinel stops declaring it. The source-selection label the proposal described never shipped, and the second release label it would have needed never existed.The trust-anchor guard parses and refuses. The boundary was specified as a header check, which PEM armour around arbitrary bytes satisfies. The controller decodes every block, parses each with
x509.ParseCertificate, and rejects the whole value on the first block that is not a certificate or on trailing bytes that are not a PEM block; nothing is published and the sentinel reportsReady=False,SourceRejected. The proposal's normative bullet and its failure list now say that, and the Ready reasons listed match the reconciler.The engine table no longer describes
main. It kept going stale on every merge, so it now carries only structural facts per engine (who owns the PKI, which Secret carries the CA and whether it holds a key, how the trust anchor is delivered under this contract) and points at cozystack/cozystack#2814 for which engines have converged. The separator rule for per-release Secret names is stated as what the contract asks of new work, with the collisions that exist on current series branches named.Decision records.
decisions/0001records why the label-based source selection and the two earlier canonical names lost to the declared sentinel and the dotted name;decisions/0002records why mongodb's sentinel names the leaf Secret rather than the operator's CA Secret. Both are linked from the proposal.The top commit before this revision was pushed by a reviewer; it is kept with its authorship, and only its trailers were brought to the repository's form.