docs(postgres): name the trust anchor as the tenant actually sees it - #3463
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughUpdated CA sentinel RBAC documentation and PostgreSQL TLS instructions. PostgreSQL guidance now references the tenant-accessible CA secret and identifies which secret contains the CA private key. ChangesCA access documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
9786ef8 to
ea40dfc
Compare
ed02cd8 to
293fc19
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
293fc19 to
cdd1387
Compare
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Both claims check out against the code, and the second one is the more valuable half.
The naming fix is right and there is an independent witness for it. packages/system/postgres-rd/cozyrds/postgres.yaml:13 sets prefix: postgres- and api/v1alpha1/applicationdefinitions_types.go:82 defines release names as prefix plus app name, so a Postgres named foo is release postgres-foo. hack/e2e-chainsaw/postgres/chainsaw-test.yaml:155 asserts the Secret postgres-test.tenant-ca for an instance named test and line 232 runs the same command shape the README now prints. Read literally as a resource name, the old text gave NotFound.
The ValidatingAdmissionPolicy retraction is the part I would highlight. There is no VAP on internal.cozystack.io or tenantprojections anywhere in the tree, and git log -S "companion ValidatingAdmissionPolicy" shows the phrase arrived in the feature commit itself, so it was aspirational and never shipped. Deleting it is a security-documentation win, because a comment claiming a defence in depth that does not exist makes the next reviewer budget against it. The replacement claim is also true, the only RBAC grant on that group is the controller's own at packages/system/cozystack-controller/templates/rbac.yaml:29.
One loose end. The vocabulary migration stops inside the same paragraph. README.md:180 and :184 now say postgres-<name>, and :188 four lines later still says CNPG creates its own <release>-ca. The premise of this PR is that a tenant does not know what <release> expands to, and that does not stop being true two sentences later. It is postgres-<name>-ca, per hack/e2e-chainsaw/postgres/chainsaw-test.yaml:175. Same <release> survives at :169 for the external hostname, which renders as .Release.Name plus host, so it means the same thing.
Worth adding a half-line in internal/controller/cacert/reconciler.go noting the VAP was considered and rejected in favour of RBAC. That rationale is good and it currently lives only in the commit body, so the next reader will re-run your investigation.
21eccfc to
7a99967
Compare
7a99967 to
6d39648
Compare
The README told a tenant to fetch <release>.tenant-ca, but the release name carries the postgres- prefix the application definition applies, so the object is postgres-<name>.tenant-ca. Following the old text returned NotFound. Also drop two comments promising a companion ValidatingAdmissionPolicy that pins sentinel writes to helm-controller. No such policy ships: the proposal for it was rejected because authorization by username duplicates RBAC in a more brittle form. The RBAC boundary is the whole control. Signed-off-by: Aleksei Sviridkin <f@lex.la>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
6d39648 to
9513227
Compare
What this PR does
Two documentation defects that both landed with the CA trust-anchor work in #3407.
The Postgres README told a tenant to fetch
<release>.tenant-ca. The release name carries thepostgres-prefix the application definition applies, so the object ispostgres-<name>.tenant-ca, and the postgres chainsaw test shows exactly that for a resource namedtest. A tenant following the README gotNotFound. I confirmed the real name on a live cluster while converting another engine onto the same contract.The second is a comment in the controller and one in its RBAC chart, both promising a companion
ValidatingAdmissionPolicythat pins sentinel writes to helm-controller. No such policy ships. It was proposed in #3410 and rejected: authorizing byrequest.userInfo.usernameduplicates RBAC in a more brittle form, and the RBAC boundary already holds because no tenant role has any verb oninternal.cozystack.io. The comments now say that instead of pointing at a guard that does not exist.Downstream repositories
Documentation only, no API or chart behaviour changes.
cozystack/websiteregenerates the Postgres reference page from this README on the next stable tag, so no manual follow-up is needed there.Release note
Summary by CodeRabbit