Repository navigation
fix(cacert): make the comments match the reconciler - #4064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change updates CA reconciler documentation comments and renames one test to define its collision property more precisely. No functional code or test logic changes. ChangesCA reconciler documentation
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change clarifies CA reconciler ownership and naming documentation and makes the collision test’s dot-free suffix scope explicit. Runtime behavior and test execution remain unchanged, with no current merge-readiness risk identified. 🚥 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 |
Comments in the CA-extraction package described guards the code does not have. ManagedLabel: the anti-clobber decision is made by isOurProjection, from the Secret type and an owner reference back to the sentinel, not from the marker, which no decision keys on beyond the drift check. managedByCozystackLabel: the reconciler does mutate it. It deletes the label on a selectors-digest change, which is what hands the projection back to the lineage webhook for a fresh verdict. projectionSuffix: the projection name is kept clear of engine CA Secrets by a dot, and a release name cannot carry one, but the suffix an operator appends can and nothing validates it. A suffix ending in ".tenant-ca" is the only one that reaches a canonical name. TestProjectionNameCannotCollideWithADotFreeEngineSuffix: both its doc and its name claimed the property for any engine suffix while the loop samples only dot-free ones, which is the same overstatement one file over. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
86c1c4e to
9ea4a96
Compare
github-actions
Bot
added
area/uncategorized
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. I checked both factual claims against the code rather than reading the diff for plausibility, because a comment fix is only worth merging if the new comment is true.
isOurProjection decides on two things — s.Type != corev1.SecretTypeOpaque and an owner reference naming this TenantProjection — and never reads ManagedLabel. So the old claim that the reconciler "only ever writes over a Secret bearing this label" was false, and the replacement is accurate about where the decision actually lives.
delete(existing.Labels, managedByCozystackLabel) is there, guarded so only a selector change reaches it, with the reasoning that a data rotation cannot change the webhook's verdict and re-admitting on one would be pure load. The old "the reconciler never writes it" was false in the other direction.
The projectionSuffix rewrite is the one I would have wanted most. The old text asserted collision impossibility outright; the new text says where the guarantee comes from and — more usefully — where it stops: the release name is read from the helm.toolkit.fluxcd.io/name label, where a dot is legal, so a hand-written HelmRelease is outside the DNS-1035 argument, and the suffix an operator or chart appends is validated by nothing. Naming that the guarantee holds on the validated path and not beyond it is what makes the comment usable by whoever next considers "tidying" the dot.
Renaming the test to TestProjectionNameCannotCollideWithADotFreeEngineSuffix is the right call too: the loop only ever sampled dot-free suffixes, so the old name promised a property the test did not check — the same class of overclaim as the comments, and the more dangerous one, because a test name is what people trust when they skim.
A comment that overstates a guard is worse than no comment: it is what a reader relies on when deciding whether a change is safe. Worth having.
2 participants
What this PR does
Comments in the CA-extraction package claim guards the code does not have. No behaviour change; one test is renamed to match the property it pins.
ManagedLabelsaid the reconciler only ever writes over a Secret carrying that label. It does not.isOurProjectiondecides that from the Secret type and the owner reference, and never looks at the label, which any writer in the namespace can strip.managedByCozystackLabelsaid the reconciler never writes it. It deletes it whenever the selectors digest changes, which is what gets the projection re-admitted by the lineage webhook.projectionSuffixsaid the dotted canonical name cannot collide with an operator-owned object. A release the apps API created cannot carry a dot, since application names are DNS-1035 labels andrelease.prefixis restricted to^[a-z0-9-]*$. The guarantee stops there: the controller reads the release from thehelm.toolkit.fluxcd.io/namelabel, where a dot is legal, so a hand-written HelmRelease sits outside it. On the validated path the dot can only come from the suffix an operator or chart appends, which nothing validates at all, and since a canonical name ends in.tenant-caand carries that one dot, a suffix ending in.tenant-cais the only one that reaches a canonical name. The comment says all of that now.TestProjectionNameCannotCollideWithADotFreeEngineSuffix, and its doc says what the loop covers.Screenshots
Not a UI change.
Downstream repositories
Comments and one test name in two Go files. No package, values key, schema field, CRD, metric, release asset or
hack/target changes, so nothing in the trigger map fires.Release note
Summary by CodeRabbit
Documentation
Tests