fix: valid Kubernetes identifiers from any policy name - #152
Merged
Conversation
The operator had five independent builders for Kubernetes names and label
values, each with its own truncation and character rules. Three were
wrong, in three different ways, and `password.rs` already contained the
trailing-separator fix that `plan.rs` was missing — the knowledge existed
but was not shared.
Add `k8s_names` as the one place those rules live, covering both shapes
the operator produces: label values (63 chars, alphanumerics/`.`/`-`/`_`,
alphanumeric at each end) and resource names (RFC 1123 DNS subdomains).
Rewire every call site through it:
- `sanitize_label_value` delegates to `LabelValue::sanitize`, which trims
leading separators *before* the 63-char cut so they no longer consume
budget that is then discarded, and trims the separator a cut exposes.
- `generate_plan_name` uses `truncate_name_prefix`. It previously cut the
policy name at 215 chars and appended `-plan-{ts}-{hash}` without
trimming, so a legal 253-char name with a `.` on the cut produced
`...a.-plan-...`, whose segment after the dot starts with `-`. The API
server rejects that, and the policy stops reconciling.
- `is_valid_secret_name` delegates to `is_valid_resource_name`. It
required a lowercase *letter* first, but RFC 1123 permits a leading
digit, so a legal Secret such as `9db-creds` was rejected. Validation
is now per-DNS-label, so `db..creds` is correctly rejected too.
- `default_generated_secret_name` shares the same truncation helper.
Replace the string-formatted label selectors with `kube`'s typed
`Selector`/`Expression` and `ListParams::labels_from`, so selector
construction cannot go wrong by string manipulation.
Both bugs were of a shape that is easy to miss by example and immediate
to catch by property: an input long enough to truncate, with a separator
landing exactly on the cut. `tests/identifier_properties.rs` restates the
rules independently of the implementation and asserts every builder
satisfies them for arbitrary input, over an alphabet biased towards the
characters that break things (NUL, `=`, `/`, separators, multi-byte
UTF-8). It also pins two invariants the selectors depend on: sanitising
is idempotent, and an already-valid value is returned unchanged — either
failing would make label lookups disagree with label writes.
The composition property walks every cut position for each generated name
rather than sampling a random budget. A random budget almost never lands
on a separator, so the dangerous case would be sampled too rarely to
catch a regression; checked against the pre-fix code, the sampled version
passed while the exhaustive one failed. The checked-in regressions file
holds the minimal counterexamples that broke the old code: `"_"`,
`"0.0a"`, and a trailing-dash truncation.
Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
Co-authored-by: Aaron Shewkani <aaron.shewkani@partly.com>
Centralising only the final truncation left `sanitize_secret_name_segment` as a second, unshared source of Kubernetes naming rules — the exact thing this module exists to prevent — and it sat outside the property suite. Move it in as `sanitize_dns_label_segment` and property-test both it and the composition `default_generated_secret_name` actually performs, so the generated-Secret path is covered by the same invariants as the rest. Also cover the plan-SQL ConfigMap identifiers, which are derived by appending `-sql` to the plan name and carry three user-derived label values. `generate_plan_name` reserves exactly 4 bytes for that suffix, so a boundary-length policy name is where the reservation would be wrong. This is a unit test rather than an E2E assertion because plan SQL only spills to a ConfigMap above MAX_INLINE_SQL_BYTES, and generating 16 KiB of SQL in a cluster to re-verify string composition buys nothing the pure test does not already pin. Reported by Codex review on #147 and #149. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
`cargo run --bin crdgen` without `--output-dir` prints *both* CRDs to stdout separated by `---`, so redirecting it into `k8s/crd.yaml` writes a two-document file. `check-crd-drift.sh` compares that path against the single-document `postgrespolicies.pgroles.io.yaml`, so following the documented steps produced drift rather than resolving it. Document the `--output-dir` form the drift check actually models, and both copies it compares. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
The property re-composed `{policy}-pgr-{role}` itself instead of calling
the production builder, so a change to the separator, segment order, or
length budget could break real Secret names while the property stayed
green. Call `generated_secret_name` with no `secretName` override, which
is the path that derives the name.
Reported by CodeRabbit review on #147.
Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
Unit and property tests check the identifier rules as this repository restates them. The API server is the only complete oracle: it decides what it actually accepts, including anything the restated rules model wrongly. The kind cluster is already running in the operator E2E job, so exercise both directions there. A name one character over the label limit must be refused at admission, and the failure must carry the CEL rule's message — proving the rule is not merely present in the CRD but compiled and enforced by the API server. A dotted name exactly at the limit must reconcile and its plan must be discoverable through the label the operator wrote, which is the property the plan lookup, approval, and cleanup paths all depend on. The scenario then asserts the derived plan name is a name the API server accepted and that both label values it wrote satisfy the label rules, so a regression in either the truncation or the sanitising surfaces here. The sample uses manual approval because that, not `mode: plan`, is what makes the operator create a PostgresPolicyPlan and its SQL ConfigMap. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
… stack Add an E2E scenario for policy-name identifiers. Unit and property tests check the rules as this repository restates them; the API server is the only complete oracle for what it actually accepts. A name one character over the limit must be refused at admission *with the CEL rule's message*, which proves the rule is compiled and enforced rather than merely present in the CRD. A dotted name exactly at the limit must reconcile with its plan discoverable by the label the operator wrote, and the derived plan name and label values are then checked against the rules. The plan SQL stays inline at this size, so no plan-SQL ConfigMap is created; the sample and the scenario say so rather than implying coverage they do not have. That path is unit-tested instead. Gate the expensive jobs on stack position. In a stacked pull request the top entry contains the full changeset, so the integration matrix, the Docker and example smoke tests, and the three kind-based E2E jobs now run once per stack instead of once per entry, while lint and unit tests stay ungated so every entry keeps fast feedback. On a three-entry stack that is cheaper than the current behaviour, not just cheaper than removing the `branches` filter — which is unnecessary anyway, since GitHub now triggers workflows as if every entry targeted the stack's base. The gate is deliberately fail-safe: `stack` is null for pushes and for ordinary non-stacked pull requests, and anything not positively identified as a non-top stack entry runs the full suite, so a wrong guess costs CI time rather than coverage. Centralised in one job so the condition exists once instead of being copied into six. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
…ones The previous scenario asserted a 64-character name was refused at admission, which only held while the CRD carried a CEL length rule. That rule is gone: identifying plans by owner UID makes a lossy label harmless, so capping names at 63 restricted users for no remaining benefit. Test the boundary that still matters instead. The fixture is the maximum legal 253-byte DNS subdomain, and its 215th byte is a dot — exactly where `generate_plan_name` cuts the policy-name prefix. Without trimming the separator the cut exposes, the appended `-plan-...` begins a DNS label with `-` and the API server rejects the plan. This is a sharper test of `truncate_name_prefix` than the old 63-character fixture, which never reached the truncation path at all. The scenario now also asserts the plan's controller-owner UID matches the policy, and that the policy label really is truncated to 63 bytes, so the case cannot quietly stop being a boundary if the fixture changes. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
`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
Contributor
📝 WalkthroughWalkthroughThe operator adds shared Kubernetes identifier handling, uses controller-owner UIDs to isolate plans and ConfigMaps, adds boundary and property tests, documents orphan cleanup, and expands E2E coverage. CI now gates expensive jobs for stacked pull requests. ChangesOperator naming and ownership
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PostgresPolicy
participant Plan
participant SQLConfigMap
participant Reconciler
PostgresPolicy->>Plan: create plan with controller-owner UID
Plan->>SQLConfigMap: persist plan SQL
Reconciler->>Plan: list plans by policy label
Reconciler->>Plan: filter by controller-owner UID
Reconciler->>SQLConfigMap: clean up owned resources
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 |
This was referenced Aug 2, 2026
The two user-visible behaviour changes in this series had no changelog entry: valid identifiers from any policy name, and owner-UID identity replacing the truncated label. Both affect anyone running a policy whose name exceeds 63 characters, and the second carries an upgrade note about --cascade=orphan. Credits @aarons-afk for the original report and fix in #146. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac381f0e1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/pgroles-operator/src/k8s_names.rs (1)
122-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
sanitize_dns_label_segmenttrustsfallbackwithout validating it.The function guarantees a valid DNS label for every input except when
fallbackis returned.fallbackis passed through unchecked. Current callers inpassword.rspass"policy"and"role", so the behavior is correct today. A future caller that passes an empty or non-conforming fallback silently breaks the documented guarantee. Consider sanitizing the fallback too, or state the caller contract in the doc comment.♻️ Proposed doc/contract tightening
/// Input is lowercased; every run of characters outside `[a-z0-9]` collapses to /// a single `-`; leading and trailing `-` are dropped. `fallback` is returned if /// nothing survives, so the result is always a usable segment. +/// +/// The caller must pass a `fallback` that is already a valid DNS 1123 label. +/// The function does not sanitize it.🤖 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/k8s_names.rs` around lines 122 - 143, Update sanitize_dns_label_segment so its fallback path also returns a valid DNS label: sanitize the fallback using the same normalization rules and provide a safe non-empty default if the fallback is empty or invalid. Preserve the existing behavior for valid inputs and avoid recursive fallback handling.crates/pgroles-operator/src/plan.rs (1)
1109-1121: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe
215byte budget is derived from an unstatedtimestamp.len()assumption.
max_prefix_lensubtractstimestamp.len()at runtime, but the comment at line 355 and the reserved-4both assume the 15-byteYYYYMMDD-HHMMSSshape. The subtraction chain would underflow and panic ifformat_timestamp_compact()ever returned a longer string. Consider a debug assertion orsaturating_subso a future change to the timestamp format fails loudly rather than panicking in the reconcile loop.♻️ Proposed hardening
let max_name_len = crate::k8s_names::MAX_RESOURCE_NAME_LENGTH - 4; // 249 - let max_prefix_len = max_name_len - "-plan-".len() - timestamp.len() - "-".len() - suffix.len(); + let reserved = "-plan-".len() + timestamp.len() + "-".len() + suffix.len(); + debug_assert!(reserved < max_name_len, "plan-name suffix budget exhausted"); + let max_prefix_len = max_name_len.saturating_sub(reserved);🤖 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 1109 - 1121, Harden generate_plan_name against a timestamp longer than the assumed format by preventing max_prefix_len arithmetic from underflowing. Add an explicit invariant check that timestamp matches the expected compact format/length and fails loudly, or use saturating subtraction while preserving the resulting valid-name behavior; update the nearby budget assumption if needed..github/workflows/ci.yml (1)
396-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated
limit_nameliteral.
limit_namecopiesmetadata.namefromk8s/samples/max-length-name-policy.yaml, so an unrelated manifest rename makeswait_for_ready_status_reasonwait for a non-existent policy. Derive the name from the manifest instead of duplicating it.🤖 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 @.github/workflows/ci.yml around lines 396 - 406, Replace the hardcoded limit_name value in the CI workflow with the metadata.name read from k8s/samples/max-length-name-policy.yaml, then pass that derived value to wait_for_ready_status_reason. Keep the manifest application and readiness check behavior unchanged.
🤖 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 408-413: Validate that policy_uid and plan_owner_uid are non-empty
before comparing them in the long-name ownership check at
.github/workflows/ci.yml lines 408-413. Apply the same non-empty validation to
uid_a, uid_b, owner_a, and owner_b before the equality assertion at
.github/workflows/ci.yml lines 878-940; retain the existing failure behavior
when any value is missing or mismatched.
In `@AGENTS.md`:
- Around line 28-32: Update the neighboring CRD drift description in AGENTS.md
to say that CI enforces all four committed CRD copies match crdgen output,
aligning it with the “four committed copies” wording near the cargo run and copy
commands.
In `@k8s/samples/max-length-name-policy.yaml`:
- Around line 22-28: Resolve the contradiction between the policy’s mode and the
role comment: either change mode from apply to plan to preserve “Preview-only,”
or reword the comment under hostile_name_preview_user to state that the role is
created after manual approval.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 396-406: Replace the hardcoded limit_name value in the CI workflow
with the metadata.name read from k8s/samples/max-length-name-policy.yaml, then
pass that derived value to wait_for_ready_status_reason. Keep the manifest
application and readiness check behavior unchanged.
In `@crates/pgroles-operator/src/k8s_names.rs`:
- Around line 122-143: Update sanitize_dns_label_segment so its fallback path
also returns a valid DNS label: sanitize the fallback using the same
normalization rules and provide a safe non-empty default if the fallback is
empty or invalid. Preserve the existing behavior for valid inputs and avoid
recursive fallback handling.
In `@crates/pgroles-operator/src/plan.rs`:
- Around line 1109-1121: Harden generate_plan_name against a timestamp longer
than the assumed format by preventing max_prefix_len arithmetic from
underflowing. Add an explicit invariant check that timestamp matches the
expected compact format/length and fails loudly, or use saturating subtraction
while preserving the resulting valid-name behavior; update the nearby budget
assumption if needed.
🪄 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: e3fde8df-8d6b-4b74-bbc8-d1b4190b5e8b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/ci.ymlAGENTS.mdcrates/pgroles-operator/Cargo.tomlcrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/k8s_names.rscrates/pgroles-operator/src/lib.rscrates/pgroles-operator/src/main.rscrates/pgroles-operator/src/password.rscrates/pgroles-operator/src/plan.rscrates/pgroles-operator/tests/identifier_properties.proptest-regressionscrates/pgroles-operator/tests/identifier_properties.rsdocs/src/pages/docs/limitations.mdk8s/samples/max-length-name-policy.yaml
Codex caught a real gap in the cleanup added on the plan-name conflict path. create_plan_sql_configmap adopts an existing ConfigMap on 409 after checking the SQL hash alone, and the name embeds a truncated plan name, so two policies with colliding prefixes and identical SQL adopt each other's artifact. The rollback then deleted a ConfigMap belonging to another policy, breaking a plan already pending approval. Two changes, matching the discipline already applied elsewhere: - Check ownership before adopting. is_owned_by_another_policy is deliberately narrower than !is_owned_by_policy: only a live claim by a different policy blocks adoption, so an unowned orphan stays adoptable and recovery from --cascade=orphan still works. Fails closed when our own UID is absent. - Track whether this invocation created the ConfigMap, and roll back only then. The plan itself already had this guard via created_new_plan; the ConfigMap did not, and that asymmetry was the bug. Also from review: - Guard the E2E UID assertions against empty jsonpath output. Not vacuous today, since a live policy always has a UID, but = must never read as proof of ownership in a test whose whole purpose is that comparison. - Read the max-length policy name back from the applied object instead of repeating the 253-byte literal in the workflow. - Correct AGENTS.md: check-crd-drift.sh compares four committed copies, not two. - sanitize_dns_label_segment returns fallback verbatim, so document that it must already be a valid DNS label and assert it in debug builds. - Drop the Preview-only role comment in the sample; with mode: apply the role is created once the plan is approved. Not changed: the max_prefix_len subtraction in generate_plan_name is flagged as a potential underflow, but generate_plan_name_has_expected_format pins the timestamp suffix at 28 characters, so a format change fails in CI long before it could underflow. saturating_sub would instead emit a name starting with '-plan-', which the API server rejects. Claude-Session: https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the #147 → #149 → #151 stack, which GitHub's stacked-PR preview has wedged (see below). Same eleven commits, same tree — verified byte-identical to the stack tip — now as one PR off
main, plus a changelog entry and a review fix.Originates from @aarons-afk's report and fix in #146: a sanitised label value truncated at 63 bytes can land on a separator, producing an invalid label and a rejected plan ConfigMap. His fix is absorbed into
LabelValue::sanitize, and he is credited as co-author on that commit.What it does
A single source of truth for identifier rules (
k8s_names.rs). Label values, DNS-1123 labels, and resource names each had their rules restated inline at the call site, and they disagreed. Three latent bugs fell out of unifying them:generate_plan_namecut the policy-name prefix without trimming an exposed.or-, so the appended-plan-…began a DNS label with a separator.is_valid_secret_namerejected names beginning with a digit, which Kubernetes permits.default_generated_secret_nameopen-coded its own segment sanitising.truncate_name_prefixrespects UTF-8 char boundaries — an earlier attempt trimmed all non-alphanumerics and emptied a prefix of multi-byte characters.Identity by controller-owner UID, not by label (
plan.rs,main.rs). Thepgroles.io/policylabel is truncated at 63 bytes while a policy name may be 253, so two policies sharing a 63-byte prefix selected each other's plans and SQL ConfigMaps, and the plan→policy watch silently stopped waking the reconciler for any longer name. The label is now a server-side prefilter only;is_owned_by_policycompares UIDs at all six lookup sites, including the 409-conflict adopt path.Ownership on the SQL ConfigMap paths too.
create_plan_sql_configmapadopts an existing ConfigMap on a 409, and originally checked only the SQL hash — so two colliding policies with identical SQL could adopt each other's artifact, tying its lifetime to the wrong policy's garbage collection. Adoption is now gated onis_owned_by_another_policy, deliberately narrower than!is_owned_by_policy: only a live claim by a different policy blocks it, so an unowned orphan stays adoptable and--cascade=orphanrecovery still works. Rollback deletes only a ConfigMap this reconcile actually created, matching thecreated_new_planguard the plan itself already had.Property tests (
identifier_properties.rs). Eight properties over a hostile alphabet — NUL,/,=, separators,é,日— checking each builder against the API server's rules restated independently of the implementation. The secret-name property drives the real builder rather than re-composing the format, so a change to the separator or length budget can't pass vacuously.E2E coverage. A 253-byte policy name whose 215th byte is a dot, landing exactly on the truncation cut; and two policies with colliding 63-byte label prefixes, asserting UID-isolated lookup, approval, and cleanup.
Notes for review
stack-gateskips the six expensive jobs for non-top stack entries. It is fail-safe —stackis null for pushes and non-stacked PRs, so this PR runs the full suite — and it costs nothing to keep. Given what the stack feature just did, treat its savings as untrusted until the preview stabilises.max_prefix_lensubtraction ingenerate_plan_nameis flagged as a potential underflow.generate_plan_name_has_expected_formatpins the timestamp suffix at 28 characters, so a format change fails in CI long before it could underflow in production;saturating_subwould instead silently emit a name starting with-plan-, which the API server rejects.-(a.banda-b). Present onmaintoday; out of scope.Why the stack was abandoned
refs/pull/149/mergefroze atMerge 738b17e into 257c666and never recomputed, so CI kept building a tree containingcrd_cel_rules.rsandKubeSchema— files on no branch head. Six approaches failed: repointing the base (422, "part of a stack"), force-pushing the base branch to match its parent, closing the intermediate PR, no-op amends on both downstream heads, poking mergeability, and closing #149 to eject it (the base-change lock survives closure). A fresh PR offmainresolved its merge ref immediately — stack membership is a property of the PR, not the branch.Verification
All 13 CI checks green on
5203180, including the three kind-based E2E suites.cargo fmt --check,clippy --all-targets -D warnings, the full workspace suite, andcheck-crd-drift.shalso pass locally.https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4