fix: identify a policy's plans by owner UID, not by a truncated label - #151
fix: identify a policy's plans by owner UID, not by a truncated label#151hardbyte wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe operator now identifies policies by controller owner UID and filters plans and ConfigMaps by that ownership. Tests cover collisions and lifecycle cases. Documentation records policy-name and orphaning limits. CI validates stacked pull request metadata before running the heavy suite. ChangesOperator ownership isolation
Stack metadata validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Plan
participant PostgresPolicy API
participant Reconciliation
Plan->>PostgresPolicy API: provide controller owner UID
PostgresPolicy API-->>Reconciliation: return matching PostgresPolicy
Reconciliation->>Plan: create reconciliation reference
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
@coderabbitai review Auto-review was skipped here because this is a stacked PR whose base is Note for reviewers: this PR currently has no CI. It was created by setting Generated by Claude Code |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pgroles-operator/src/plan.rs (1)
348-374: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLeaked SQL ConfigMap on cross-policy name collision.
The sql-content ConfigMap is created at line 279, before the ownership check at line 358. When
is_owned_by_policy(&existing, policy)is false, the function returnsReconcileError::PlanSqlStorageat line 359 without callingdelete_configmap_best_effort. The generic error branch at lines 368-372 does clean up the ConfigMap, but this branch does not.The orphan reaper (
cleanup_orphan_sql_configmaps) eventually removes this ConfigMap onceORPHAN_GRACE_SECSpasses, since it is owned by the attempting policy's UID. This limits the impact to a transient orphan rather than a permanent leak, but the same cleanup call used in the other error branch should apply here too.🧹 Proposed fix to clean up the ConfigMap on ownership mismatch
if !is_owned_by_policy(&existing, policy) { + if let Some(configmap_name) = sql_configmap_name.as_deref() { + delete_configmap_best_effort(client, &namespace, configmap_name).await; + } return Err(ReconcileError::PlanSqlStorage(format!( "plan {plan_name} already exists and is owned by another policy" ))); }🤖 Prompt for 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. In `@crates/pgroles-operator/src/plan.rs` around lines 348 - 374, Update the ownership-mismatch branch in the plan creation match, before returning ReconcileError::PlanSqlStorage, to call delete_configmap_best_effort with the existing client, namespace, and sql_configmap_name. Preserve the current ownership validation and error message, and apply cleanup only when is_owned_by_policy returns false.
🧹 Nitpick comments (1)
crates/pgroles-operator/src/plan.rs (1)
1510-1637: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a test for the create_or_update_plan collision-with-foreign-owner path.
The new tests cover
is_owned_by_policydirectly and its callers indirectly through unit-level assertions, but none exercise thecreate_or_update_plan409-collision branch (lines 353-362) where an existing plan is owned by a different policy. That branch is the one mutation site the comment at line 354-357 calls out as not covered by the general owner-UID discipline, so it is the highest-value path to test end-to-end.🤖 Prompt for 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. In `@crates/pgroles-operator/src/plan.rs` around lines 1510 - 1637, Extend the ownership tests around create_or_update_plan to cover a 409 conflict when an existing plan is owned by a different policy, using distinct owner UIDs and colliding policy labels. Assert that the collision branch preserves the foreign-owned plan and does not update or delete it, while retaining the existing is_owned_by_policy assertions.
🤖 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 @.github/workflows/ci.yml:
- Around line 49-60: Update the stack metadata validation in the workflow before
the range comparison to reject any STACK_POSITION or STACK_SIZE value that is
not a positive decimal integer. Preserve the existing partial-population and
1..STACK_SIZE range checks, and exit 1 with an appropriate error before numeric
comparisons when either value is invalid.
---
Outside diff comments:
In `@crates/pgroles-operator/src/plan.rs`:
- Around line 348-374: Update the ownership-mismatch branch in the plan creation
match, before returning ReconcileError::PlanSqlStorage, to call
delete_configmap_best_effort with the existing client, namespace, and
sql_configmap_name. Preserve the current ownership validation and error message,
and apply cleanup only when is_owned_by_policy returns false.
---
Nitpick comments:
In `@crates/pgroles-operator/src/plan.rs`:
- Around line 1510-1637: Extend the ownership tests around create_or_update_plan
to cover a 409 conflict when an existing plan is owned by a different policy,
using distinct owner UIDs and colliding policy labels. Assert that the collision
branch preserves the foreign-owned plan and does not update or delete it, while
retaining the existing is_owned_by_policy assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d65faca-dae7-44ea-a606-f632fdb20495
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/pgroles-operator/src/main.rscrates/pgroles-operator/src/plan.rsdocs/src/pages/docs/limitations.md
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
7ad4d42 to
29c8ac3
Compare
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
29c8ac3 to
b1ef718
Compare
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
b1ef718 to
eb7d264
Compare
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
eb7d264 to
27682d2
Compare
|
Both E2E failures share one cause, and it is the stack topology rather than the new scenarios.
That CEL rule was removed along with #148. Neither branch head still has it — The reason it is still enforced is that a Both merged trees contain No code change here can fix it. The only edits that would go green are reinstating the rule or shrinking the fixtures under 63 bytes, and the latter would delete precisely the boundary coverage these scenarios exist to provide: the 253-byte name places a dot on This clears once #148 is removed from the stack, which has to happen in the stack UI — Everything else is green at the tip: Generated by Claude Code |
`pgroles.io/policy` was the only link between a policy and its plans and plan-SQL ConfigMaps, but the label value is truncated at 63 characters while a policy name may be 253. Two policies sharing a 63-character prefix therefore selected each other's objects, and that selector result drives deletion in `cleanup_old_plans` and `cleanup_orphan_sql_configmaps` as well as approval and deduplication in the three lookup paths. The module documentation for `LabelValue::sanitize` warns against exactly this use. Filter every one of those six paths by controller-owner UID. The label still narrows server-side, which keeps the list cheap; the UID decides. Owner references are already written on both plans and ConfigMaps, so this needs no new label and no migration for existing objects. The ConfigMap path is why this has to be done as one change rather than plan-side only. `is_orphan_sql_configmap` treats a ConfigMap whose plan label is absent from the known set as an orphan and deletes it, so exact-filtering the plans while leaving the ConfigMap list truncation-matched would have made a colliding policy's ConfigMap look *more* like an orphan — turning a lookup bug into a deletion bug. Apply the same fix to the plan-to-policy watch mapping in `main.rs`, which compared the truncated label against `metadata.name` and so silently never matched for any name over 63 characters, leaving plan status changes unable to wake the policy reconciler. UID rather than `spec.policyRef.name` because it is exact per object *lifetime*: a replacement policy with the same name is a different object and must not inherit the deleted one's plans. An absent UID is treated as owning nothing rather than as a wildcard. Reported by CodeRabbit review on #147. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
Self-review follow-ups on the identifier stack. The 409 create-conflict path adopted whatever plan held the colliding name and patched its status without an ownership check, making it the one mutation site the owner-UID discipline did not cover. Reaching it across policies needs two names sharing a 215-byte truncated prefix colliding in the same second, so this is defence in depth rather than a live bug, but the check belongs there for the same reason as the other six. Harden the stack gate. Stacked pull requests are in public preview, and if `position`/`size` semantics ever changed then `position == size` would be silently false for the real top entry — the stack would merge with the expensive suite never having run. Assert the documented 1-based range and fail loudly instead of skipping wrongly. Document two migration hazards that CI cannot catch, since the E2E kind cluster runs a Kubernetes version where neither applies: - Before Kubernetes 1.30 there is no CRD validation ratcheting, so an existing policy whose name exceeds the new 63-character limit has every write rejected, including the operator's own status updates. It stops making progress with nothing in its status to explain why. - `--cascade=orphan` strips the owner references that both re-adoption and garbage collection now depend on, so orphaned plans and plan-SQL ConfigMaps persist until deleted by hand. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
The unit tests cover `is_owned_by_policy` directly, but nothing exercised two genuinely colliding policies against a real API server — which is the scenario the whole layer exists for. Two policies sharing a 63-byte prefix produce an identical `pgroles.io/policy` label, and the scenario asserts that collision holds before relying on it, so the fixture cannot silently stop colliding. It then checks the three things the label previously decided and the owner UID now decides: each plan is owned by its own policy's UID, approving one policy's plan leaves the other's Pending with its role uncreated, and deleting one policy garbage-collects only its own plan while the other stays discoverable and still applies cleanly afterwards. That last step is the important one. `is_orphan_sql_configmap` deletes objects whose plan label is absent from the known set, so a half-applied ownership filter would have turned this collision into cross-policy deletion rather than merely a confused lookup. Also drop the policy-name limit from the limitations page, which described a CRD rule that no longer exists, keeping only the `--cascade=orphan` note, which is a genuine consequence of matching by owner UID. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
27682d2 to
7a3b3a7
Compare
…match The stack-metadata guard could be bypassed by the thing it guards against. `[ non-numeric -lt 1 ]` writes "integer expression expected" to stderr and evaluates *false*, and a failing command inside an `if` condition does not trip `bash -e`, so the step continued and the expensive suite could still be skipped — exactly the silent-skip the guard was added to prevent. Reproduced it, then validated both values as non-negative integers before comparing, forcing decimal with `10#` so a zero-padded value is never read as octal. Verified against non-numeric, negative, exponent-form, zero-padded, partial, in-range and out-of-range inputs; the unstacked empty case still fails open as intended. The ownership-mismatch arm of the plan-creation conflict path returned without deleting the SQL ConfigMap it had already created, unlike the sibling error arm. The orphan reaper would collect it, since it carries this policy's UID, but the leak is pointless. Clean it up. Reported by CodeRabbit review on #151. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
|
Superseded by #152, which carries these four commits unchanged along with the rest of the stack. The UID-ownership change was never the problem — CI here was building Generated by Claude Code |
1 participant
Stack 4 of 4 — base is #149. Review #147, #148 and #149 first.
Closes the collision CodeRabbit raised on #147. #148 prevents new lossy labels; this makes the lookups exact regardless, so a policy that predates the CEL rule is covered too.
The bug
pgroles.io/policywas the only link between a policy and its plans and plan-SQL ConfigMaps, but the label truncates at 63 characters while a policy name may be 253. Two policies sharing a 63-character prefix selected each other's objects, and that selector drives deletion incleanup_old_plansandcleanup_orphan_sql_configmaps, plus approval and deduplication in the three lookup paths.LabelValue::sanitize's own docs in #147 warn against this exact use.Why UID, and why all six paths at once
Filter by controller-owner UID. The label still narrows server-side so the list stays cheap; the UID decides. Owner references are already written on both plans and ConfigMaps, so no new label and no migration.
The ConfigMap path is why this can't be plan-side only, and it's worth being explicit because the obvious partial fix is actively harmful:
is_orphan_sql_configmapdeletes a ConfigMap whose plan label is absent from the known set. Exact-filtering the plans while leaving the ConfigMap list truncation-matched would make a colliding policy's ConfigMap look more like an orphan — converting a lookup bug into a deletion bug. So I did not take the suggestedspec.policy_ref.namepost-filter as written.UID over
spec.policyRef.namefor a second reason: it's exact per object lifetime. A replacement policy with the same name is a different object and must not inherit the deleted one's plans. An absent UID owns nothing rather than acting as a wildcard.Same fix in
main.rs, which compared the truncated label againstmetadata.nameand so silently never matched above 63 characters — plan status changes could not wake the policy reconciler at all. That also retires the last use of the label as an identity.Tests
Five unit tests, each pinning a way this could regress: a genuine 63-character-prefix collision (asserting first that the two labels really are identical, so the test can't rot into vacuity), a missing owner reference, a non-controller owner reference, an empty UID not acting as a wildcard, a recreated same-name policy, and the ConfigMap path specifically.
Verification
cargo fmt --check,clippy -D warnings, full workspace (274 operator unit tests), andcheck-crd-drift.shall pass.https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation