fix(rest): accept uppercase LINSTOR identifiers for CSI conformance (BUG-047)#163
Conversation
The wire-side identifier validator enforced a lowercase-only RFC-1123
subdomain, stricter than upstream LINSTOR. csi-sanity CreateVolume
names carry uppercase hex (e.g. pvc-2A1B4B95EA8C4D7E) which the
upstream linstor-csi sidecar forwards verbatim as the RD name, so the
gate rejected 19 CSI-conformance specs with code=Internal.
Oracle evidence against the upstream LINSTOR controller established the
actual upstream ruleset for resource-definition names: a leading ASCII
letter, then {letter, digit, '_', '-'}, length 2-48, case-insensitive
(^[A-Za-z][A-Za-z0-9_-]{1,47}$). Mirror it exactly so blockstor is
neither stricter (the conformance gap) nor laxer than upstream. The
k8s store's Name() helper already folds case onto a deterministic
lowercase slug, so the relaxed gate round-trips by name and stays
collision-safe end-to-end; the lowercase pvc-<uuid> provisioner path
is unchanged.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The csi-sanity Job ran under the default ServiceAccount, which has no permission to list/watch volumesnapshotclasses. The linstor-csi snapshot path the sidecar embeds needs that read to resolve the driver's VolumeSnapshotClass, so the snapshot-related conformance specs erred with Forbidden. Add a dedicated ServiceAccount + ClusterRole granting get/list/watch on snapshot.storage.k8s.io/volumesnapshotclasses and bind it to the job pod. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The sibling-path regression tests (snapshot-restore, rg-spawn) and the e2e bulk script encoded the old lowercase-only divergence: they asserted mixed-case / uppercase / underscore / trailing-hyphen names were refused. Those are valid upstream, so move them out of the reject set and keep only the forms upstream still refuses (leading hyphen / digit, embedded dot / space, path separator, single char, empty). The e2e script also gains a happy-path assertion for an uppercase-hex name (the csi-sanity shape) to prove the conformance gap is closed end to end. The gate-fires-on-sibling-paths coverage is unchanged. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Relaxing the REST validator to accept uppercase RD names surfaced a case-mismatch one layer deeper: the k8s store's Name() helper lowercases metadata.name, while spec.resourceDefinitionName preserves the caller's original case. The Resource and Snapshot composite-key CEL rules compared the two case-sensitively, so creating a Resource for an uppercase RD (e.g. csi-sanity's "...BA880D4F...") was rejected by the apiserver with "metadata.name must equal <rd>.<node>" even though both names address the same object. Mirror the StoragePool rule, which already handles this: compare with .lowerAscii() on both sides. LINSTOR identifiers are case-insensitive, so this keeps the <rd>.<node> naming convention intact while tolerating case differences. Add an envtest regression that creates a Resource and a Snapshot with uppercase RD names against the real apiserver. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The Node Service specs (NodeStage/NodePublish/NodeGetVolumeStats) need a real block device and mount namespace on the host, which the single-pod csi-sanity Job does not provide. They used to fail fast in BeforeEach because CreateVolume was rejected (the uppercase-name bug); now that CreateVolume succeeds they reach the real mount and hang. The job's comment always intended to skip them, but the skip regex never matched them. Make the skip load-bearing and also exclude the known ListVolumes-pagination shared-namespace artifact, so the suite completes and exercises the Controller-side specs (including the uppercase-name CreateVolume/CreateSnapshot paths) deterministically. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Add a BUG-047 section to the csi-sanity failure classification: the uppercase RD-name rejection that the first end-to-end suite run unmasked, the two-layer fix (REST validator relax + Resource/Snapshot CEL case-insensitivity), and confirmation that the remaining failures are the previously-documented class (a)/(b) artifacts plus the Bug 33 satellite-snapshot finalizer drain in teardown. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Warning Review limit reached
More reviews will be available in 16 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses BUG-047 by relaxing the REST name validator to accept uppercase letters, underscores, and trailing hyphens, matching upstream LINSTOR's identifier rules. Additionally, the CEL validation rules for Resource and Snapshot CRDs have been updated to compare names case-insensitively using .lowerAscii(), preventing validation failures when using uppercase resource definition names. New unit, integration, and E2E tests have been added to verify these changes, and the csi-sanity job configuration has been updated with appropriate RBAC permissions and test skips. As there are no review comments, no feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
1 participant
Summary
The apiserver wire-side name validator rejected uppercase LINSTOR identifiers (lowercase-only RFC-1123) — stricter than upstream LINSTOR. csi-sanity sends CreateVolume/CreateSnapshot names with uppercase hex; the upstream linstor-csi sidecar forwards the opaque CO name verbatim as the RD name, so the gate rejected 19 CSI-conformance specs with code=Internal.
Layer decision (oracle evidence)
Live oracle run established upstream's actual RD-name ruleset: leading ASCII letter, then {letter,digit,_,-}, length 2-48, case-INSENSITIVE (
^[A-Za-z][A-Za-z0-9_-]{1,47}$). Upstream ACCEPTSTestUpperCase123,pvc-2A1B4B95EA8C4D7E, underscores, trailing hyphen; rejects leading-digit, <2/>48 chars,./space//; treats names case-insensitively. So blockstor's lowercase-only validator was a stricter-than-upstream divergence (parity bug) → relax the validator to mirror upstream, NOT change the CSI driver (the real piraeus-csi sidecar is on the request path, not the in-repo shim).What changed
./space/path-separators (preserves<rd>.<node>split-safety + Bug-97 no-k8s-leak).-ginkgo.skipto actually exclude Node-Service kernel-mount specs + the ListVolumes-pagination shared-namespace spec (single-pod Job can't satisfy them).Validation
go test ./..., -race (rest/csi-driver/store), golangci-lint 0 issues. Stand big: uppercase RD POSTs 201 + round-trips; 0 name-validation errors across csi-sanity (baseline 19). Residual csi-sanity failures are pre-documented artifacts (Node-mount out of scope; ControllerPublishVolume scale-out cache 404; Bug-33 snapshot-finalizer teardown drain — affects lowercase too) — none name-validation.