refactor: single source of truth for Kubernetes identifier rules - #147
Closed
hardbyte wants to merge 4 commits into
Closed
refactor: single source of truth for Kubernetes identifier rules#147hardbyte wants to merge 4 commits into
hardbyte wants to merge 4 commits into
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
Contributor
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe operator adds shared Kubernetes identifier utilities for validation, sanitization, truncation, and typed values. Secret names, plan names, and policy selectors now use these utilities. Unit and property tests cover hostile input and UTF-8 boundaries. ChangesKubernetes identifier handling
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![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: 8b8f23f708
ℹ️ 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: 1
🤖 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 `@crates/pgroles-operator/src/plan.rs`:
- Around line 1108-1117: Update the five policy-scoped call
sites—create_or_update_plan, cleanup_old_plans, cleanup_orphan_sql_configmaps,
get_current_actionable_plan, and get_plan_by_phase—to retain only plans whose
spec.policy_ref.name exactly equals the requested full policy name after the
LABEL_POLICY selector list. For cleanup_orphan_sql_configmaps, apply the
equivalent full-name ownership check using the ConfigMap’s policy metadata or
its owning plan names derived from the filtered plans, so truncated label
collisions cannot affect deletion or state transitions.
🪄 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: 16349c92-f77e-42a4-977a-573892b7c7d4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
AGENTS.mdcrates/pgroles-operator/Cargo.tomlcrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/k8s_names.rscrates/pgroles-operator/src/lib.rscrates/pgroles-operator/src/password.rscrates/pgroles-operator/src/plan.rscrates/pgroles-operator/tests/identifier_properties.proptest-regressionscrates/pgroles-operator/tests/identifier_properties.rs
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
hardbyte
added a commit
that referenced
this pull request
Aug 1, 2026
`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
`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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
`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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/pgroles-operator/tests/identifier_properties.rs (1)
187-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest
default_generated_secret_namedirectly.This property reconstructs the production composition at Lines 195-198 instead of calling
default_generated_secret_name. A future change to the separator, segment order, or length budget can break production names while this property remains green. Move the property beside the private builder or expose a testable path.🤖 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/tests/identifier_properties.rs` around lines 187 - 210, The property test currently reconstructs the generated secret-name composition instead of exercising default_generated_secret_name. Update composed_generated_secret_names_are_valid to call default_generated_secret_name directly with the generated policy and role inputs, then validate that returned name against the API-server and internal validators while preserving the non-empty assertion.
🤖 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 `@crates/pgroles-operator/tests/identifier_properties.rs`:
- Around line 169-185: Update sanitize_dns_label_segment to truncate each
sanitized segment to the 63-byte DNS-label limit while preserving its non-empty
fallback and valid-label guarantees. Extend
sanitized_segments_are_valid_dns_labels and its hostile_input strategy to
generate explicitly overlong raw segments, verifying those segments remain valid
individually; keep default_generated_secret_name’s composed-name behavior
unchanged.
- Around line 187-210: Update composed_generated_secret_names_are_valid to
exercise default_generated_secret_name through the real
generate_generated_secret_name path, including owner-UID inputs, rather than
manually composing sanitized segments. Add a property regression asserting
distinct generated Secret names for colliding policy inputs such as a_b and a/b
and for roles on different owner-UID edges, while preserving the existing
API-server validity assertions.
---
Nitpick comments:
In `@crates/pgroles-operator/tests/identifier_properties.rs`:
- Around line 187-210: The property test currently reconstructs the generated
secret-name composition instead of exercising default_generated_secret_name.
Update composed_generated_secret_names_are_valid to call
default_generated_secret_name directly with the generated policy and role
inputs, then validate that returned name against the API-server and internal
validators while preserving the non-empty assertion.
🪄 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: e7ab2895-2e0c-465a-8280-c02f121a9488
📒 Files selected for processing (5)
AGENTS.mdcrates/pgroles-operator/src/k8s_names.rscrates/pgroles-operator/src/password.rscrates/pgroles-operator/src/plan.rscrates/pgroles-operator/tests/identifier_properties.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/pgroles-operator/src/password.rs
- crates/pgroles-operator/src/k8s_names.rs
- crates/pgroles-operator/src/plan.rs
Comment on lines
+169
to
+185
| /// A sanitised segment must always be a usable DNS label on its own: the | ||
| /// fallback guarantees it is never empty, so a composed name can never grow | ||
| /// an empty label. | ||
| #[test] | ||
| fn sanitized_segments_are_valid_dns_labels(input in hostile_input()) { | ||
| let segment = sanitize_dns_label_segment(&input, "fallback"); | ||
|
|
||
| prop_assert!(!segment.is_empty()); | ||
| prop_assert!( | ||
| is_resource_name_per_apiserver(&segment), | ||
| "segment {:?} from {:?} is not a valid DNS label", | ||
| segment, | ||
| input | ||
| ); | ||
| // A single label contains no dots, so it stays one label when composed. | ||
| prop_assert!(!segment.contains('.')); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'hostile_input|MAX_RESOURCE_NAME_LENGTH|is_dns1123_label|sanitize_dns_label_segment' \
crates/pgroles-operator/src crates/pgroles-operator/testsRepository: hardbyte/pgroles
Length of output: 29032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant validators/constructors and the production secret-name builder.
sed -n '1,130p' crates/pgroles-operator/src/k8s_names.rs | cat -n
printf '\n--- k8s_names sanitization and production usages below ---\n'
sed -n '130,260p' crates/pgroles-operator/src/k8s_names.rs | cat -n
printf '\n--- production secret-name builder ---\n'
sed -n '56,73p' crates/pgroles-operator/src/password.rs | cat -n
printf '\n--- tests around composed generated secret names ---\n'
sed -n '169,205p' crates/pgroles-operator/tests/identifier_properties.rs | cat -n
printf '\n--- tests around raw label-value sanitization ---\n'
sed -n '82,115p' crates/pgroles-operator/tests/identifier_properties.rs | cat -n
printf '\n--- behavioral probe of k8s_names semantics from source text ---\n'
python3 - <<'PY'
from pathlib import Path
import re
src = Path('crates/pgroles-operator/src/k8s_names.rs').read_text()
test_src = Path('crates/pgroles-operator/tests/identifier_properties.rs').read_text()
constants = dict(re.findall(r'(?:pub\s+)?(?:TYPE?)?const\s+([A-Z_]+)\s*:\s*usize\s*=\s*(\d+);', src))
print('constants:', constants)
hostile = re.search(r'fn hostile_input\(\) -> impl Strategy<Value = String>\s*\{\s*(.*?)\n(\s*\.prop_map\(|\n\}|\s*///)', test_src, re.S)
print('hostile_input contains dots:', bool(hostile) and bool(re.search(r'Just\([\'"].{0,5}[\'"]\)|\'\.\'\s*|Just\(\\\dot', hostile.group(1) if hostile else '', re.S)))
sanitize = re.search(r'pub fn sanitize_dns_label_segment\(.*?\n(?=///\s*\[|pub fn|impl)', src, re.S)
print('sanitize_dns_label_segment contains length limit:', bool(sanitize) and re.search(r'(len\(|\.len\(|MAX_|63|253)'), sanitize.group(0))
is_valid_label_value = re.search(r'pub fn is_valid_label_value\(.*?\n(?=///\s*\[|pub fn|impl|fn is_dns)', src, re.S)
print('is_valid_label_value contains MAX_LABEL_VALUE_LENGTH:', bool(is_valid_label_value) and 'MAX_LABEL_VALUE_LENGTH' in (is_valid_label_value.group(0) if is_valid_label_value else ''))
sanitize_label_value = re.search(r'pub fn sanitize_label_value\(.*?\n(?=///\s*\[|pub fn|impl)', src, re.S)
print('sanitize_label_value contains length limit:', bool(sanitize_label_value) and re.search(r'(len\(|\.len\(|MAX_|63|253)', sanitize_label_value.group(0) if sanitize_label_value else ''))
print('is_resource_name_per_apiserver uses segment length:', 'MAX_RESOURCE_NAME_LENGTH' in re.search(r'fn is_resource_name_per_apiserver\(.*?\n\}\n\n///', test_src, re.S).group(0) if re.search(r'fn is_resource_name_per_apiserver\(.*?\n\}\n\n///', test_src, re.S) else False)
PYRepository: hardbyte/pgroles
Length of output: 15832
🌐 Web query:
Kubernetes object metadata name labels RFC 1123 label max length 63 resource name max length 253
💡 Result:
In Kubernetes, the distinction between DNS labels and DNS subdomains is fundamental to how object metadata, such as resource names and labels, is validated [1][2][3]. DNS Label (RFC 1123) A DNS label is used for segments that must be compatible with hostnames [1][3]. It has a maximum length of 63 characters [1][4]. It must contain only lowercase alphanumeric characters or '-', and it must start and end with an alphanumeric character [1][3]. DNS Subdomain (RFC 1123) A DNS subdomain consists of one or more DNS labels separated by dots ('.') [2][3]. It has a maximum length of 253 characters [1][4]. While the subdomain as a whole can be up to 253 characters, each individual label within that subdomain is still subject to the 63-character limit [2]. Application in Kubernetes 1. Resource Names (metadata.name): Most Kubernetes resources require a name that follows the DNS subdomain standard (RFC 1123), allowing for a maximum of 253 characters [1][2]. 2. Labels (metadata.labels): Kubernetes labels are key/value pairs [5]. The "name" segment of a label key must be 63 characters or less [5][6]. If a label key includes an optional prefix, that prefix must be a DNS subdomain (max 253 characters) followed by a slash [5]. The label value must also be 63 characters or less [5]. Validation Implementation Kubernetes enforces these constraints in its validation logic [4][6]. While some historical issues existed regarding the validation of nested labels within subdomains [2], the system generally validates these constraints during API object creation [7][4]. These limits are critical for ensuring compatibility with DNS-based service discovery and other infrastructure components [1][3].
Citations:
- 1: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
- 2: Incorrect max length check on metadata.name kubernetes/kubernetes#79351
- 3: https://github.com/kubernetes/design-proposals-archive/blob/main/architecture/identifiers.md
- 4: https://github.com/kubernetes/kubernetes/blob/HEAD/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go
- 5: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
- 6: https://github.com/kubernetes/apimachinery/blob/5a348c53/pkg/api/validate/strfmt.go
- 7: https://kubernetes.io/docs/reference/kubernetes-api/definitions/object-meta-v1-meta/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact hostile_input definition ---'
sed -n '55,77p' crates/pgroles-operator/tests/identifier_properties.rs | cat -n
printf '%s\n' '--- direct length-boundary static check in tests ---'
python3 - <<'PY'
from pathlib import Path
p = Path('crates/pgroles-operator/tests/identifier_properties.rs')
text = p.read_bytes()
needle = b"sanitize_dns_label_segment(&input, \"fallback\")"
pos = text.find(needle)
start = text.rfind(b"prop_assert!(\n", 0, pos)
block = text[start:text.find(b"\n },", pos)].decode()
print(block)
print("contains 253:", b"253" in block)
print("contains LENGTH:", b"LENGTH" in block)
PYRepository: hardbyte/pgroles
Length of output: 3757
Enforce the DNS-label length limit in sanitize_dns_label_segment.
default_generated_secret_name() truncates the composed name to 253 bytes, but an overlong policy_segment or role_segment can remain a single invalid DNS label after sanitization. Truncate each segment to 63 bytes during sanitize_dns_label_segment, and include long raw segments in the property test so this no longer depends only on total-name truncation.
🤖 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/tests/identifier_properties.rs` around lines 169 -
185, Update sanitize_dns_label_segment to truncate each sanitized segment to the
63-byte DNS-label limit while preserving its non-empty fallback and valid-label
guarantees. Extend sanitized_segments_are_valid_dns_labels and its hostile_input
strategy to generate explicitly overlong raw segments, verifying those segments
remain valid individually; keep default_generated_secret_name’s composed-name
behavior unchanged.
Comment on lines
+187
to
+210
| /// The real composition in `default_generated_secret_name`: two sanitised | ||
| /// segments joined by `-pgr-` and truncated to the resource-name limit must | ||
| /// always be a name the API server accepts. | ||
| #[test] | ||
| fn composed_generated_secret_names_are_valid( | ||
| policy in hostile_input(), | ||
| role in hostile_input(), | ||
| ) { | ||
| let policy_segment = sanitize_dns_label_segment(&policy, "policy"); | ||
| let role_segment = sanitize_dns_label_segment(&role, "role"); | ||
| let composed = format!("{policy_segment}-pgr-{role_segment}"); | ||
| let truncated = truncate_name_prefix(&composed, MAX_RESOURCE_NAME_LENGTH); | ||
|
|
||
| prop_assert!(!truncated.is_empty()); | ||
| prop_assert!( | ||
| is_resource_name_per_apiserver(truncated), | ||
| "composed secret name {:?} is invalid", | ||
| truncated | ||
| ); | ||
| prop_assert_eq!( | ||
| is_valid_resource_name(truncated), | ||
| is_resource_name_per_apiserver(truncated) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 'default_generated_secret_name|owner.*uid|owner_references|controller_owner_ref' \
crates/pgroles-operator/src crates/pgroles-operator/testsRepository: hardbyte/pgroles
Length of output: 11380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search generated_secret_name calls/usages =="
rg -n -C 8 'generated_secret_name|GENERATED_VERIFIER_KEY|managed_secret_name|secret_name' crates/pgroles-operator/src crates/pgroles-operator/tests || true
echo
echo "== password.rs relevant section =="
wc -l crates/pgroles-operator/src/password.rs
sed -n '1,290p' crates/pgroles-operator/src/password.rs
echo
echo "== relevant properties section =="
sed -n '140,220p' crates/pgroles-operator/tests/identifier_properties.rs
echo
echo "== k8s_names module =="
fd -a 'k8s_names\.rs$' . | sed 's#^\./##' | while read -r f; do
echo "--- $f"
wc -l "$f"
sed -n '1,260p' "$f"
doneRepository: hardbyte/pgroles
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== password.rs full relevant implementation =="
awk '/^fn default_generated_secret_name/,/^[[:space:]]*[^[:space:]#\/][^[:space:]]*\(/' crates/pgroles-operator/src/password.rs | sed -n '1,140p'
sed -n '180,250p' crates/pgroles-operator/src/password.rs
echo
echo "== references to generated secret name in password code only =="
rg -n -C 4 'default_generated_secret_name|generate_generated_secret_name|GeneratedPasswordSecret|ensure_generated_secret|missing_generated_secret_source_version|source_version|owner_references' crates/pgroles-operator/src/password.rs
echo
echo "== deterministic naming collision probe from source definitions =="
python3 - <<'PY'
from pathlib import Path
text = Path("crates/pgroles-operator/src/password.rs").read_text()
start = text.index("fn sanitize_dns_label_segment")
end = text.index("fn truncate_name_prefix")
print(text[start:start+2000])
def sanitize_dns_label_segment(input_str, part):
# Minimal reproduction of the regex from the visible Rust code.
import re
s = re.sub(r'[^A-Za-z0-9\-]', '-', input_str)
s = re.sub(r'-+', '-', s)
s = re.sub(r'^-|-$', '', s)
if not s:
return f"{part}-" + ("z" * 40)
return s[:50]
def truncate_name_prefix(s, length):
return s[:length]
def default_generated_secret_name(policy_name, role_name):
name = f"{sanitize_dns_label_segment(policy_name, 'policy')}-pgr-{sanitize_dns_label_segment(role_name, 'role')}"
truncated = truncate_name_prefix(name, 253)
return truncated if truncated else "pgroles-generated-password"
cases = [("a" * 30, "b" * 30), ("a-b", "b"), ("a_b", "b"), ("a/b", "b")]
for policy, role in cases:
print(f"({policy!r}, {role!r}) -> {default_generated_secret_name(policy, role)!r}")
collisions = []
import itertools
inputs = ["ab", "a_b", "a/b", "a-b"]
for p, n in itertools.product(inputs, inputs):
for r1, r2 in itertools.product(inputs, repeat=2):
if (p, r1) != (n, r2) and default_generated_secret_name(p, r1) == default_generated_secret_name(n, r2):
collisions.append(((p, r1), (n, r2), default_generated_secret_name(p, r1)))
print("collisions_count=", len(collisions))
for c in collisions[:20]:
print(c)
PY
echo
echo "== generate_generated_secret_name definitions across repo =="
rg -n 'pub\s+fn\s+generated_secret_name|fn\s+generated_secret_name|pub\s+fn\s+default_generated_secret_name|fn\s+default_generated_secret_name|pub\s+(struct|fn)\s+GeneratedPasswordSecret|struct\s+GeneratedPasswordSecret|pub\s+struct\s+GeneratedPasswordSecret' crates/pgroles-operator/src crates/pgroles-operator/testsRepository: hardbyte/pgroles
Length of output: 8488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact source helper bodies =="
python3 - <<'PY'
from pathlib import Path
text = Path("crates/pgroles-operator/src/k8s_names.rs").read_text()
needle = "pub fn sanitize_dns_label_segment"
idx = text.find(needle)
print("== sanitize_dns_label_segment ==")
print("start", idx)
end = text.find("\nfn ", idx + 1)
if end == -1:
end = text.find("pub fn is_resource_name_per_apiserver", idx + 1)
print(text[idx:end if end != -1 else idx+1000])
print("== truncate_name_prefix ==")
idx2 = text.find("pub fn truncate_name_prefix")
print(text[idx2: idx2+800])
PY
echo
echo "== existing properties around generated secret names =="
sed -n '1,230p' crates/pgroles-operator/tests/identifier_properties.rs
echo
echo "== generated secret collision probe using source definitions =="
python3 - <<'PY'
from pathlib import Path
import re, itertools
k8s_text = Path("crates/pgroles-operator/src/k8s_names.rs").read_text()
m = re.search(r'pub fn sanitize_dns_label_segment\([^)]*\) -> String \{(.*?)\n}\n\npub \(crate\) fn truncate_name_prefix', k8s_text, re.S)
if not m:
raise SystemExit("could not locate sanitize_dns_label_segment body")
body = re.sub(r'/\*.*?\*/', '', m.group(1), flags=re.S)
print(body)
print("--- extracted calls ---")
print([c.strip() for c in re.findall(r'[^;\n]+', body)])
def sanitize_dns_label_segment(input_str, part):
s = re.sub(r'[^A-Za-z0-9\-]', '-', input_str)
s = re.sub(r'-+', '-', s)
s = re.sub(r'^-|-$', '', s)
if not s:
return f"{part}-" + ("z" * 40)
return s[:50]
def truncate_name_prefix(s, length):
return s[:length]
def default_generated_secret_name(policy_name, role_name):
name = f"{sanitize_dns_label_segment(policy_name, 'policy')}-pgr-{sanitize_dns_label_segment(role_name, 'role')}"
truncated = truncate_name_prefix(name, 253)
return truncated if truncated else "pgroles-generated-password"
print("samples:")
for pairs in [(("ab", "abc"), ("a_b", "abc")), (("a" * 60, "abc"), ("aaaa", "abc"))]:
for a, b in pairs:
print((a, b), "=>", default_generated_secret_name(a, b))
print("generated collisions for small inputs:")
values = ["ab", "a_b", "a/b", "a-b", "abc", "abc-d"]
seen = {}
for pair in itertools.product(values, repeat=2):
out = default_generated_secret_name(*pair)
if out in seen:
prev = seen[out]
if prev != pair:
print(prev, "collides with", pair, "as", out)
else:
seen[out] = pair
print("policies producing same sanitized segment:", {(sanitize_dns_label_segment("ab", "policy"), [x for x in values if sanitize_dns_label_segment(x, "policy") == sanitize_dns_label_segment("ab", "policy")])})
PYRepository: hardbyte/pgroles
Length of output: 10827
Add a collision-isolation regression for generated Secret names.
default_generated_secret_name only builds {policy}--pgr-{role}; generate_generated_secret_name has owner-UID isolation. The current property still measures the wrong name shape and misses collisions such as policies a_b/a/b or roles on separate owner-UID edges. Pin the real generated Secret name path and add a collision regression property.
🤖 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/tests/identifier_properties.rs` around lines 187 -
210, Update composed_generated_secret_names_are_valid to exercise
default_generated_secret_name through the real generate_generated_secret_name
path, including owner-UID inputs, rather than manually composing sanitized
segments. Add a property regression asserting distinct generated Secret names
for colliding policy inputs such as a_b and a/b and for roles on different
owner-UID edges, while preserving the existing API-server validity assertions.
Source: Learnings
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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
`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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
`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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
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
hardbyte
added a commit
that referenced
this pull request
Aug 2, 2026
`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
Owner
Author
|
Superseded by #152, which carries these five commits unchanged plus the rest of the stack, as one PR off Closing not because anything was wrong here — this passed all ten checks — but because the downstream stack entries became unbuildable: Generated by Claude Code |
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.
Stack 1 of 3 — base
main. Follow-ups build on this branch.Prompted by #146: the label-value truncation bug there is one instance of a wider class. Reviewing it turned up two more live bugs and five independent builders for Kubernetes identifiers, each with its own rules.
The class
sanitize_label_value(plan.rs)plan_label_value(plan.rs)generate_plan_name(plan.rs)default_generated_secret_name(password.rs)-is_valid_secret_name(crd.rs)password.rsalready contained the trailing-separator fix thatplan.rswas missing. The knowledge existed in the repo and wasn't shared — which is the argument for consolidating rather than patching each site.Bugs fixed
generate_plan_name. Cuts the policy name at 215 chars, then appends-plan-{ts}-{hash}without trimming. A legal 253-char name with a.on the cut yields…a.-plan-…, whose segment after the dot starts with-. Not a valid DNS-1123 subdomain, so the API server rejects the plan and the policy stops reconciling. Same failure mode as #146, different function.is_valid_secret_name. Required a lowercase letter first, but RFC 1123 permits a leading digit, so a legal Secret named9db-credswas rejected. Now validated per DNS label, which also correctly rejectsdb..creds— previously accepted here and then rejected by the API server.Approach
New
k8s_namesmodule owns both shapes — label values and RFC 1123 resource names — with the upstream rules as the documented spec. Every call site delegates to it. Label selectors move fromformat!("{LABEL_POLICY}={}", …)tokube's typedSelector/ExpressionwithListParams::labels_from.LabelValue::sanitizeis documented as lossy and explicitly not injective, with a pointer to the hash-basedplan_label_valuefor cases where uniqueness matters.Tests
Both bugs share a shape that's 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.rsrestates the rules independently of the implementation and asserts every builder satisfies them for arbitrary input, over an alphabet biased toward what actually breaks things (NUL,=,/, separators, multi-byte UTF-8).I verified the tests have teeth by reverting each fix — they fail with minimal counterexamples
"_"and"0.0a", both checked into the regressions file.One coverage note worth flagging: the composition property walks every cut position per generated name rather than sampling a random budget. With a random budget it passed against the pre-fix code, because a random cut almost never lands on a separator. The exhaustive version fails as it should.
Verification
cargo fmt --check,clippy -D warnings, full workspace tests, andcheck-crd-drift.shall pass. No CRD change in this PR.https://claude.ai/code/session_01KKdSeCxJEVcrH4MPLgwUG4
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests