diff --git a/api/v1alpha1/resource_types.go b/api/v1alpha1/resource_types.go index 9db7e11c..05cd3ad8 100644 --- a/api/v1alpha1/resource_types.go +++ b/api/v1alpha1/resource_types.go @@ -522,7 +522,7 @@ type ResourceVolumeStatus struct { // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster -// +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name == self.spec.resourceDefinitionName + '.' + self.spec.nodeName",message="metadata.name must equal .",optionalOldSelf=true +// +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.nodeName).lowerAscii()",message="metadata.name must equal . (case-insensitive)",optionalOldSelf=true // Resource is the Schema for the resources API. // @@ -531,6 +531,15 @@ type ResourceVolumeStatus struct { // Keeping the composite key encoded in the name lets operators grep for // `.` across kinds (Resource, Snapshot, StoragePool) and find every // resource bound to one satellite at once. +// +// BUG-047: the comparison is case-INSENSITIVE (.lowerAscii() on both +// sides), mirroring StoragePool. LINSTOR identifiers are case-insensitive +// and the k8s store's Name() helper lowercases metadata.name, while +// spec.resourceDefinitionName preserves the caller's original case. With +// an uppercase RD name (e.g. csi-sanity's `…BA880D4F…`) a case-SENSITIVE +// rule rejected the Resource create with "must equal" even though the +// names address the same object. lowerAscii() on both sides keeps the +// `.` convention intact while tolerating case differences. type Resource struct { metav1.TypeMeta `json:",inline"` diff --git a/api/v1alpha1/snapshot_types.go b/api/v1alpha1/snapshot_types.go index 04d80aa3..f07576d4 100644 --- a/api/v1alpha1/snapshot_types.go +++ b/api/v1alpha1/snapshot_types.go @@ -241,7 +241,7 @@ type SnapshotPerNodeStatus struct { // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster -// +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name == self.spec.resourceDefinitionName + '.' + self.spec.snapshotName",message="metadata.name must equal .",optionalOldSelf=true +// +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.snapshotName).lowerAscii()",message="metadata.name must equal . (case-insensitive)",optionalOldSelf=true // Snapshot is the Schema for the snapshots API. // diff --git a/config/crd/bases/blockstor.cozystack.io_resources.yaml b/config/crd/bases/blockstor.cozystack.io_resources.yaml index 30696368..a360db9f 100644 --- a/config/crd/bases/blockstor.cozystack.io_resources.yaml +++ b/config/crd/bases/blockstor.cozystack.io_resources.yaml @@ -25,6 +25,15 @@ spec: Keeping the composite key encoded in the name lets operators grep for `.` across kinds (Resource, Snapshot, StoragePool) and find every resource bound to one satellite at once. + + BUG-047: the comparison is case-INSENSITIVE (.lowerAscii() on both + sides), mirroring StoragePool. LINSTOR identifiers are case-insensitive + and the k8s store's Name() helper lowercases metadata.name, while + spec.resourceDefinitionName preserves the caller's original case. With + an uppercase RD name (e.g. csi-sanity's `…BA880D4F…`) a case-SENSITIVE + rule rejected the Resource create with "must equal" even though the + names address the same object. lowerAscii() on both sides keeps the + `.` convention intact while tolerating case differences. properties: apiVersion: description: |- @@ -743,9 +752,10 @@ spec: type: object x-kubernetes-validations: - message: metadata.name must equal . + (case-insensitive) optionalOldSelf: true - rule: oldSelf.hasValue() || self.metadata.name == self.spec.resourceDefinitionName - + '.' + self.spec.nodeName + rule: oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + + '.' + self.spec.nodeName).lowerAscii() served: true storage: true subresources: diff --git a/config/crd/bases/blockstor.cozystack.io_snapshots.yaml b/config/crd/bases/blockstor.cozystack.io_snapshots.yaml index b1fb1ccf..7b669964 100644 --- a/config/crd/bases/blockstor.cozystack.io_snapshots.yaml +++ b/config/crd/bases/blockstor.cozystack.io_snapshots.yaml @@ -305,9 +305,10 @@ spec: type: object x-kubernetes-validations: - message: metadata.name must equal . + (case-insensitive) optionalOldSelf: true - rule: oldSelf.hasValue() || self.metadata.name == self.spec.resourceDefinitionName - + '.' + self.spec.snapshotName + rule: oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + + '.' + self.spec.snapshotName).lowerAscii() served: true storage: true subresources: diff --git a/docs/csi-sanity-classification.md b/docs/csi-sanity-classification.md index 3f0c4611..c7150a1e 100644 --- a/docs/csi-sanity-classification.md +++ b/docs/csi-sanity-classification.md @@ -2,6 +2,14 @@ This document classifies the csi-sanity failures left over after the wire-shape and concurrency-race fixes recorded earlier (KV envelope, RD clone envelope, Snapshots[] synthesis, snapshot idempotence, `?offset/limit` pagination, `retry.RetryOnConflict` wrapping, downward-API `--node`). The audit was run on the `e2e-lc1` stand (`ssh ubuntu@`) via `stand/csi-sanity-job.yaml`; the apiserver and controller pods carried the Bug 24 per-node storage-pool fix (`98e510b6c`) and the Bug 41 controller-side `corev1.Node` watch RBAC (`13722e1b9`). +## BUG-047 update (uppercase RD-name rejection) — FIXED + +A later evidence sweep (2026-06-14, commit `015374d6a`) ran the suite to completion for the first time (the prompt mTLS + terminating-sidecar fix let it run end-to-end) and unmasked a NEW failure that the wedged earlier runs never reached: all 19 specs that call `CreateVolume`/`CreateSnapshot` with csi-sanity's uppercase-hex names (e.g. `…B4B95EA8…`) failed with `code=Internal desc = … is not a valid identifier (no uppercase)`. The upstream linstor-csi sidecar forwards the opaque CO name verbatim as the RD name; blockstor's REST validator was lowercase-only RFC-1123 — STRICTER than upstream LINSTOR (oracle-confirmed ruleset `^[A-Za-z][A-Za-z0-9_-]{1,47}$`, case-insensitive). + +Fix (this PR): relax the apiserver validator to mirror upstream, AND make the Resource/Snapshot composite-key CEL rule case-insensitive (the k8s store lowercases `metadata.name` while `spec.resourceDefinitionName` keeps the caller's case; a case-sensitive rule rejected the Resource create one layer deeper with "metadata.name must equal ."). After the fix the 19 name failures are gone (0× "is not a valid identifier" / "metadata.name must equal" on the `big` stand); uppercase volumes/snapshots create successfully and the Controller-side spec bodies pass. The job's `-ginkgo.skip` was also widened to actually exclude the Node Service mount specs (class (a) below) and the ListVolumes-pagination shared-namespace spec — they used to fail fast in BeforeEach (no volume) and now reach a real mount / page-boundary that the single-pod Job cannot satisfy. + +The residual failures are unchanged from the table below (all class (a) Node/satellite-convergence + class (b) apiserver scale-out cache race + Bug 33 satellite-snapshot finalizer drain in AfterEach teardown — the latter affects lowercase snapshots too). + Prior baseline (pre-audit): **56/74 specs pass** (74 ran; 17 skipped; 1 pending) after seven wire-shape gaps and three concurrency races were closed. The same job-config is used here. The audit collected 9 hard failures during the first 16 seconds of the run before the harness wedged on a snapshot-data-plane test (`sanity-controller-source-vol-*`) waiting for a satellite-side reconcile that never converged on the e2e-lc1 stand (Bug 33 territory — see `docs/known-issues.md`). Wire-shape and routing observations cover all 18 expected failures from the prior baseline. ## Failure-class legend diff --git a/pkg/rest/input_validation.go b/pkg/rest/input_validation.go index 7e72b028..5b1cf842 100644 --- a/pkg/rest/input_validation.go +++ b/pkg/rest/input_validation.go @@ -27,10 +27,12 @@ import ( // Bug 97: every user-supplied LINSTOR identifier (RD name, RG name, // Node name, StoragePool name, Snapshot name) lands as a Kubernetes // `metadata.name` once the CRD store writes it. The k8s store's -// `Name()` helper (pkg/store/k8s/crdname.go) slugifies + hashes the -// input to keep distinct LINSTOR-cased names from colliding on the -// same k8s name, but that mangling runs UNCONDITIONALLY — including -// for whitespace-only or empty-after-trim inputs. +// `Name()` helper (pkg/store/k8s/crdname.go) lowercases the input, +// short-circuits when the lowercased form is already rfc1123-clean, +// and otherwise slugifies + hashes it so distinct LINSTOR-cased names +// never collide on the same k8s name. That mangling used to run +// UNCONDITIONALLY — including for whitespace-only or empty-after-trim +// inputs. // // The result before this gate: `linstor rd c " "` got slugged into // `<8-char-hex>-` (a bare trailing hyphen), the k8s API server @@ -43,34 +45,55 @@ import ( // // That message exposes (a) the internal hash-prefix scheme, and (b) // the k8s-layer details the LINSTOR wire surface is supposed to -// hide. We refuse the name at the REST boundary BEFORE Name() -// mangles it, with a LINSTOR-shaped envelope that names the offending -// input and the rule it violated. +// hide. We refuse the name at the REST boundary BEFORE Name() mangles +// it, with a LINSTOR-shaped envelope that names the offending input +// and the rule it violated. // -// The chosen ruleset is RFC 1123 subdomain (lowercase alphanumerics + -// hyphen, can't start/end with hyphen) capped at 48 chars (Bug 360). -// Upstream LINSTOR's wire-side regex is `^[A-Za-z][A-Za-z0-9_-]{1,47}$` -// — permissive on case + underscore, but the same 48-char length cap -// we mirror here. We pick RFC 1123 (lowercase-only, no underscore) -// because the k8s store is authoritative for storage: any name that -// would later fail the metadata.Name regex is rejected up front, so -// the operator sees ONE consistent failure mode rather than "linstor -// said OK, but kubectl says no". - -// rfc1123SubdomainName is the wire-level identifier regex applied to -// every user-supplied LINSTOR name (Bug 97). Mirrors -// pkg/store/k8s/crdname.go's `rfc1123` regex (used by Name() to -// short-circuit when the input is already clean). The duplication is -// deliberate — pkg/rest must not import pkg/store/k8s. +// BUG-047 (csi-sanity conformance): the original gate enforced an +// RFC 1123 subdomain (lowercase-only, no underscore) — STRICTER than +// upstream LINSTOR. csi-sanity's CreateVolume names carry uppercase +// hex (e.g. `pvc-2A1B4B95EA8C4D7E`); the upstream linstor-csi sidecar +// forwards the CO name verbatim as the RD name, so the lowercase-only +// gate rejected 19 conformance specs with code=Internal. // -// Pattern: a lowercase alphanumeric, optionally followed by -// alphanumerics or hyphens, ending with an alphanumeric. The dotted -// form `a.b.c` k8s allows is NOT permitted for LINSTOR names because -// `.` is the metadata.name convention for Resource CRDs — -// an embedded '.' in either side would shift the split and cause -// silent collisions (the same gate fires for Resource create at +// Oracle evidence (upstream LINSTOR controller, dev stand 2026-06-14) +// established the ACTUAL upstream ruleset for resource-definition +// names: +// +// TestUpperCase123 → SUCCESS (uppercase accepted, case preserved) +// pvc-2A1B4B95EA8C4D7E → SUCCESS (csi-sanity-shaped name accepted) +// Has_Underscore_1 → SUCCESS (underscore accepted) +// foo- → SUCCESS (trailing hyphen accepted) +// CaseTest / casetest → second create "already exists" (case-INSENSITIVE) +// 1foo → ERROR "Cannot begin with character '1'" +// a → ERROR "Name length 1 is less than minimum length 2" +// 48 chars → SUCCESS; 49 chars → ERROR "max length 48" +// foo.bar / "Foo Bar" / foo/bar → ERROR "Cannot contain character …" +// +// i.e. upstream's wire regex is `^[A-Za-z][A-Za-z0-9_-]{1,47}$`: a +// leading ASCII letter, then 1–47 of {letter, digit, '_', '-'}, total +// length 2–48. We now MIRROR that ruleset exactly so blockstor is +// neither stricter (the csi-sanity gap) nor laxer than upstream. The +// k8s store's case-insensitive Name() folds `Foo`/`foo` onto the same +// CRD, matching upstream's case-insensitive identity, so the relaxed +// gate stays collision-safe end-to-end. +// +// We still reject the dotted form `a.b.c` (upstream rejects '.' too) +// because `.` is the metadata.name convention for Resource +// CRDs — an embedded '.' would shift the split and cause silent +// collisions (the same gate fires for Resource create at // pkg/rest/autoplace.go). -var rfc1123SubdomainName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) + +// linstorName is the wire-level identifier regex applied to every +// user-supplied LINSTOR name (Bug 97, relaxed for BUG-047). Mirrors +// upstream LINSTOR's `^[A-Za-z][A-Za-z0-9_-]{1,47}$`: a leading ASCII +// letter, followed by 1–47 of {letter, digit, '_', '-'}. The length +// bounds it encodes (min 2 via the `{1,47}` tail, max 48) are belt- +// and-suspenders alongside the explicit maxLinstorName check, which +// emits a clearer "exceeds N characters" envelope for the over-cap +// case (Bug 360). Pattern duplication vs pkg/store/k8s is deliberate — +// pkg/rest must not import pkg/store/k8s. +var linstorName = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9_-]{1,47}$`) // maxLinstorName caps the wire-side identifier length. Upstream // LINSTOR's wire-level regex tops out at 48 characters @@ -91,9 +114,19 @@ var rfc1123SubdomainName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) // rejection happens up front, BEFORE the partial-create lands and // BEFORE the k8s label cap can ever fire. 48 also matches upstream // LINSTOR's documented identifier length for forward-compat with any -// caller that relies on the upstream limit. +// caller that relies on the upstream limit (BUG-047 oracle confirmed: +// 48 chars → SUCCESS, 49 → "Name length 49 is greater than maximum +// length 48"). const maxLinstorName = 48 +// minLinstorName is upstream LINSTOR's lower length bound: a single +// character is refused ("Name length 1 is less than minimum length +// 2", BUG-047 oracle). The regex already encodes this via its `{1,47}` +// tail, but we check it explicitly so the operator gets the same +// "too short" envelope upstream emits rather than the generic +// "not a valid identifier" message. +const minLinstorName = 2 + // ErrInvalidLinstorName is the static sentinel for Bug-97 rejections. // Callers wrap it via fmt.Errorf("%w: …") with object-kind + name + // the violated rule, so the LINSTOR envelope's `message` field carries @@ -109,11 +142,12 @@ var ErrInvalidLinstorName = errors.New("invalid name") // kind so the operator-facing message still reads naturally. var ErrLinstorNameRequired = errors.New("name is required") -// validateLinstorName enforces the RFC 1123 subdomain rules at the -// REST wire boundary. Empty/whitespace-only input is rejected with a -// distinct message because the underlying regex accepts neither and -// the empty case is the one the python CLI's `rd c " "` invocation -// produces. +// validateLinstorName enforces upstream LINSTOR's identifier rules at +// the REST wire boundary (BUG-047): a leading ASCII letter, then +// {letter, digit, '_', '-'}, length 2–48. Empty/whitespace-only input +// is rejected with a distinct message because the underlying regex +// accepts neither and the empty case is the one the python CLI's +// `rd c " "` invocation produces. // // The `kind` argument names the object being created ("resource // definition", "node", "resource group", …) so the envelope's @@ -131,10 +165,15 @@ func validateLinstorName(kind, name string) error { ErrInvalidLinstorName, kind, name, maxLinstorName) } - if !rfc1123SubdomainName.MatchString(name) { + if len(name) < minLinstorName { + return fmt.Errorf("%w: %s name %q is shorter than %d characters", + ErrInvalidLinstorName, kind, name, minLinstorName) + } + + if !linstorName.MatchString(name) { return fmt.Errorf("%w: %s name %q is not a valid identifier "+ - "(lowercase alphanumerics and '-', must start and end with "+ - "an alphanumeric, no spaces, no uppercase)", + "(letters, digits, '_' and '-'; must start with a letter; "+ + "no '.', no spaces, no path separators)", ErrInvalidLinstorName, kind, name) } diff --git a/pkg/rest/input_validation_bug_047_test.go b/pkg/rest/input_validation_bug_047_test.go new file mode 100644 index 00000000..c65fe597 --- /dev/null +++ b/pkg/rest/input_validation_bug_047_test.go @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "encoding/json" + "net/http" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// BUG-047 (csi-sanity conformance): the RD-name validator was +// lowercase-only RFC-1123 — STRICTER than upstream LINSTOR — so +// csi-sanity's uppercase-hex CreateVolume names (forwarded verbatim as +// RD names by the upstream linstor-csi sidecar) were rejected with +// code=Internal, failing 19 conformance specs. +// +// Oracle evidence (upstream LINSTOR controller, dev stand 2026-06-14) +// established that upstream ACCEPTS uppercase / underscore / trailing +// hyphen and treats names case-insensitively, with a leading-letter +// requirement and a 2–48 length window. The validator now mirrors +// `^[A-Za-z][A-Za-z0-9_-]{1,47}$`. +// +// These tests pin the relaxed-but-still-bounded ruleset at the REST +// wire boundary AND assert the k8s store round-trips the accepted +// names (so DeleteVolume / Publish / Stage resolve the same RD later). + +// TestBug047UppercaseNamesAccepted is the primary repro: the exact +// name shapes csi-sanity emits (and the upstream oracle accepts) must +// now 201 and persist, addressable via the store under the same name. +func TestBug047UppercaseNamesAccepted(t *testing.T) { + t.Parallel() + + cases := []string{ + "TestUpperCase123", // oracle: SUCCESS + "pvc-2A1B4B95EA8C4D7E", // csi-sanity-shaped uppercase hex + "Has_Underscore_1", // underscore allowed upstream + "MixedCase-with_underscore-9", // full charset + "foo-", // trailing hyphen allowed upstream + "Foobar", // leading-uppercase, mixed + } + + for _, name := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(apiv1.ResourceDefinitionCreate{ + ResourceDefinition: apiv1.ResourceDefinition{Name: name}, + }) + + resp := httpPost(t, base+"/v1/resource-definitions", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + got, _ := readAllBody(resp) + t.Fatalf("status: got %d, want 201 for upstream-valid name %q. Body: %s", + resp.StatusCode, name, got) + } + + // The store must resolve the RD under the SAME name the + // caller used — this is the CSI lifecycle invariant + // (CreateVolume name == volume_id; later Delete/Publish use + // it verbatim). The k8s Name() helper folds case onto a + // lowercase slug internally, but Get(name) routes through + // the same helper, so the round-trip is by-name stable. + if _, err := st.ResourceDefinitions().Get(t.Context(), name); err != nil { + t.Errorf("RD %q not resolvable by original name after create: %v", name, err) + } + }) + } +} + +// TestBug047NameValidatorTable is the unit-level table over the +// validator itself, pinning the exact upstream ruleset from the oracle +// run (see input_validation.go header for the captured outputs). +func TestBug047NameValidatorTable(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + valid bool + }{ + // Accepted upstream. + {"TestUpperCase123", true}, + {"pvc-2A1B4B95EA8C4D7E", true}, + {"Has_Underscore_1", true}, + {"foo-", true}, + {"ab", true}, // minimum length 2 + {"pvc-c8a1d6b9-3e2f-4d1b-8e8f-2c5e9e8e8e8e", true}, + // Rejected upstream. + {"", false}, // empty + {"a", false}, // length 1 < min 2 + {"1foo", false}, // leading digit + {"-foo", false}, // leading hyphen + {"_foo", false}, // leading underscore + {"foo.bar", false}, // embedded dot + {"Foo Bar", false}, // embedded space + {"foo/bar", false}, // path separator + {string(make([]byte, 49)), false}, // over length cap (also non-letter bytes) + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := validateLinstorName("resource definition", tc.name) + if tc.valid && err != nil { + t.Errorf("validateLinstorName(%q) = %v, want nil (upstream accepts)", tc.name, err) + } + + if !tc.valid && err == nil { + t.Errorf("validateLinstorName(%q) = nil, want error (upstream rejects)", tc.name) + } + }) + } +} + +// TestBug047LowercaseProvisionerPathUnchanged guards the real-world +// invariant the task emphasises: already-valid lowercase +// `pvc-` names (the external-provisioner path) must behave +// EXACTLY as before — 201 + addressable by the same name, no +// case-fold surprise. +func TestBug047LowercaseProvisionerPathUnchanged(t *testing.T) { + t.Parallel() + + cases := []string{ + "pvc-1", + "pvc-c8a1d6b9-3e2f-4d1b-8e8f-2c5e9e8e8e8e", + "snapshot-restore-target", + } + + for _, name := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(apiv1.ResourceDefinitionCreate{ + ResourceDefinition: apiv1.ResourceDefinition{Name: name}, + }) + + resp := httpPost(t, base+"/v1/resource-definitions", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + got, _ := readAllBody(resp) + t.Fatalf("status: got %d, want 201 for lowercase name %q. Body: %s", + resp.StatusCode, name, got) + } + + rd, err := st.ResourceDefinitions().Get(t.Context(), name) + if err != nil { + t.Fatalf("RD %q not persisted: %v", name, err) + } + + if rd.Name != name { + t.Errorf("lowercase name mutated on round-trip: got %q, want %q", rd.Name, name) + } + }) + } +} diff --git a/pkg/rest/input_validation_bug_94_97_test.go b/pkg/rest/input_validation_bug_94_97_test.go index 38d1a96c..ff07257d 100644 --- a/pkg/rest/input_validation_bug_94_97_test.go +++ b/pkg/rest/input_validation_bug_94_97_test.go @@ -233,9 +233,13 @@ func TestBug97RDCreateRefusedOnWhitespaceName(t *testing.T) { // TestBug97RDCreateRefusedOnInvalidNameShapes drives the wire shape // through several invalid forms the operator-facing CLI/REST may -// emit: uppercase + space (`Foo Bar`), pure uppercase, embedded dot, -// leading hyphen. All must 4xx with the same envelope shape; none -// must persist. +// emit. BUG-047 relaxed the gate to upstream's ruleset +// (`^[A-Za-z][A-Za-z0-9_-]{1,47}$`), so the still-invalid forms are: +// embedded space, embedded dot, path separator, leading hyphen, +// leading digit, single char, empty. All must 4xx with the same +// envelope shape; none must persist. (Pure-uppercase / underscore / +// trailing-hyphen are now VALID upstream — see +// TestBug047UppercaseNamesAccepted.) func TestBug97RDCreateRefusedOnInvalidNameShapes(t *testing.T) { t.Parallel() @@ -243,11 +247,12 @@ func TestBug97RDCreateRefusedOnInvalidNameShapes(t *testing.T) { name string body string }{ - {"uppercase+space", `{"resource_definition":{"name":"Foo Bar"}}`}, - {"pure-uppercase", `{"resource_definition":{"name":"FOOBAR"}}`}, + {"embedded-space", `{"resource_definition":{"name":"Foo Bar"}}`}, {"embedded-dot", `{"resource_definition":{"name":"foo.bar"}}`}, + {"path-separator", `{"resource_definition":{"name":"foo/bar"}}`}, {"leading-hyphen", `{"resource_definition":{"name":"-foo"}}`}, - {"trailing-hyphen", `{"resource_definition":{"name":"foo-"}}`}, + {"leading-digit", `{"resource_definition":{"name":"1foo"}}`}, + {"single-char", `{"resource_definition":{"name":"a"}}`}, {"empty", `{"resource_definition":{"name":""}}`}, } @@ -292,7 +297,7 @@ func TestBug97RDCreateAcceptedOnValidName(t *testing.T) { "pvc-1", "pvc-c8a1d6b9-3e2f-4d1b-8e8f-2c5e9e8e8e8e", "foo123", - "a", + "ab", } for _, name := range cases { @@ -331,7 +336,9 @@ func TestBug97RDCreateAcceptedOnValidName(t *testing.T) { func TestBug97NodeCreateRefusedOnInvalidName(t *testing.T) { t.Parallel() - cases := []string{"", " ", "Foo Bar", "FOO", "node.with.dot"} + // BUG-047: pure-uppercase ("FOO") is now valid upstream, dropped + // from the reject set. Embedded space / dot / empty stay invalid. + cases := []string{"", " ", "Foo Bar", "node.with.dot"} for _, name := range cases { t.Run(name, func(t *testing.T) { @@ -375,7 +382,9 @@ func TestBug97NodeCreateRefusedOnInvalidName(t *testing.T) { func TestBug97RGCreateRefusedOnInvalidName(t *testing.T) { t.Parallel() - cases := []string{"", " ", "Bad Name", "BAD"} + // BUG-047: pure-uppercase ("BAD") is now valid upstream, dropped + // from the reject set. Embedded space / empty stay invalid. + cases := []string{"", " ", "Bad Name"} for _, name := range cases { t.Run(name, func(t *testing.T) { @@ -406,7 +415,7 @@ func TestBug97RGCreateRefusedOnInvalidName(t *testing.T) { // would have stamped — we never went through RD-create here, // so the store must remain empty. for _, rg := range rgs { - if rg.Name == "" || rg.Name == "Bad Name" || rg.Name == "BAD" || rg.Name == " " { + if rg.Name == "" || rg.Name == "Bad Name" || rg.Name == " " { t.Errorf("RG persisted despite 400 (name=%q)", name) } } diff --git a/pkg/rest/rd_name_validation_bulk_test.go b/pkg/rest/rd_name_validation_bulk_test.go index 0c479add..7aa171b5 100644 --- a/pkg/rest/rd_name_validation_bulk_test.go +++ b/pkg/rest/rd_name_validation_bulk_test.go @@ -58,16 +58,19 @@ import ( func TestSnapshotRestoreRejectsInvalidRdName(t *testing.T) { t.Parallel() + // BUG-047: mixed-case / uppercase / underscore / trailing-hyphen are + // now valid upstream (and accepted by the shared validator), so the + // reject set keeps only the forms upstream still refuses. cases := []struct { name string to string }{ - {"mixed-case", "hunt3-Bad"}, - {"uppercase-only", "BADNAME"}, - {"underscore", "bad_underscore"}, - {"trailing-hyphen", "bad-"}, {"leading-hyphen", "-bad"}, + {"leading-digit", "1bad"}, {"embedded-dot", "bad.name"}, + {"embedded-space", "bad name"}, + {"path-separator", "bad/name"}, + {"single-char", "a"}, {"empty", ""}, } @@ -169,7 +172,10 @@ func TestSnapshotRestoreVolumeDefinitionRejectsInvalidRdName(t *testing.T) { defer stop() body, _ := json.Marshal(map[string]string{ - "to_resource": "hunt3-Bad", + // BUG-047: embedded dot is still refused upstream (the + // `.` split-safety constraint); mixed-case names like + // the old "hunt3-Bad" are now valid and no longer a reject case. + "to_resource": "bad.name", }) resp := httpPost(t, base+"/v1/resource-definitions/src/snapshot-restore-volume-definition/snap1", body) @@ -199,16 +205,19 @@ func TestSnapshotRestoreVolumeDefinitionRejectsInvalidRdName(t *testing.T) { func TestResourceGroupSpawnRejectsInvalidRdName(t *testing.T) { t.Parallel() + // BUG-047: mixed-case / uppercase / underscore / trailing-hyphen are + // now valid upstream, so the reject set keeps only the still-invalid + // forms. cases := []struct { name string rd string }{ - {"mixed-case", "hunt3-A1"}, - {"uppercase-only", "BADNAME"}, - {"underscore", "bad_underscore"}, - {"trailing-hyphen", "bad-"}, {"leading-hyphen", "-bad"}, + {"leading-digit", "1bad"}, {"embedded-dot", "bad.name"}, + {"embedded-space", "bad name"}, + {"path-separator", "bad/name"}, + {"single-char", "a"}, {"empty", ""}, } diff --git a/pkg/store/k8s/resource_uppercase_name_bug_047_test.go b/pkg/store/k8s/resource_uppercase_name_bug_047_test.go new file mode 100644 index 00000000..a00436ed --- /dev/null +++ b/pkg/store/k8s/resource_uppercase_name_bug_047_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package k8s_test + +import ( + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// BUG-047 (deeper layer): relaxing the REST name validator to accept +// uppercase RD names surfaced a case-mismatch in the Resource / Snapshot +// CRD admission. The k8s store's Name() helper lowercases metadata.name, +// but spec.resourceDefinitionName preserves the caller's original case. +// The composite-key CEL rule compared the two case-SENSITIVELY, so a +// Resource create for an uppercase RD (csi-sanity's `…BA880D4F…`) was +// rejected at the apiserver with: +// +// metadata.name must equal . +// +// even though both names address the same object. The fix mirrors the +// StoragePool rule: compare with .lowerAscii() on both sides. These +// tests exercise the REAL apiserver (envtest), so the CEL rule actually +// fires — a unit-level fake store would not catch the regression. + +// TestBug047ResourceCreateWithUppercaseRDName creates a Resource whose +// RD name carries uppercase hex (the csi-sanity shape). Before the fix +// the envtest apiserver rejected the create; after, it must succeed and +// round-trip by the original name. +func TestBug047ResourceCreateWithUppercaseRDName(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + s := k8s.New(fixture.client).Resources() + ctx := t.Context() + + const ( + rdName = "sanity-controller-source-vol-BA880D4F-EF2FC6EB" + node = "big-worker-2" + ) + + if err := s.Create(ctx, &apiv1.Resource{Name: rdName, NodeName: node}); err != nil { + t.Fatalf("Create Resource with uppercase RD name: %v (BUG-047: the "+ + "composite-key CEL rule must compare case-insensitively)", err) + } + + got, err := s.Get(ctx, rdName, node) + if err != nil { + t.Fatalf("Get %s/%s after create: %v", rdName, node, err) + } + + if got.Name != rdName || got.NodeName != node { + t.Errorf("round-trip mismatch: got %s/%s, want %s/%s", + got.Name, got.NodeName, rdName, node) + } +} + +// TestBug047SnapshotCreateWithUppercaseRDName is the Snapshot sibling: +// the Snapshot CRD carries the same composite-key CEL rule +// (.) and was fixed the same way. csi-sanity's CreateSnapshot +// specs hit this path with uppercase RD names. +func TestBug047SnapshotCreateWithUppercaseRDName(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + st := k8s.New(fixture.client) + ctx := t.Context() + + const ( + rdName = "CreateSnapshot-volume-1-BA880D4F-EF2FC6EB" + snapName = "snapshot-1" + node = "big-worker-2" + ) + + // Seed the parent RD so the snapshot has something to hang off of. + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: rdName}); err != nil { + t.Fatalf("seed RD %q: %v", rdName, err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: snapName, + ResourceName: rdName, + Nodes: []string{node}, + }); err != nil { + t.Fatalf("Create Snapshot with uppercase RD name: %v (BUG-047: the "+ + "composite-key CEL rule must compare case-insensitively)", err) + } + + got, err := st.Snapshots().Get(ctx, rdName, snapName) + if err != nil { + t.Fatalf("Get snapshot %s/%s after create: %v", rdName, snapName, err) + } + + if got.Name != snapName || got.ResourceName != rdName { + t.Errorf("round-trip mismatch: got %s/%s, want %s/%s", + got.ResourceName, got.Name, rdName, snapName) + } +} diff --git a/stand/csi-sanity-job.yaml b/stand/csi-sanity-job.yaml index b1be30eb..30ffa15b 100644 --- a/stand/csi-sanity-job.yaml +++ b/stand/csi-sanity-job.yaml @@ -1,4 +1,40 @@ --- +# ServiceAccount for the csi-sanity Job pod. linstor-csi's snapshot +# code path (the external-snapshotter logic the sidecar embeds) lists +# `volumesnapshotclasses` to resolve the CSI driver name when csi-sanity +# drives the CreateSnapshot / snapshot-restore specs. Under the default +# SA that list/watch was Forbidden, so the snapshot specs erred out. +apiVersion: v1 +kind: ServiceAccount +metadata: + name: csi-sanity + namespace: blockstor-system +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: csi-sanity +rules: + # The snapshot sidecar resolves the driver's VolumeSnapshotClass by + # listing/watching the cluster-scoped objects; get/list/watch is all + # it needs (it never creates or mutates classes). + - apiGroups: [snapshot.storage.k8s.io] + resources: [volumesnapshotclasses] + verbs: [get, list, watch] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: csi-sanity +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: csi-sanity +subjects: + - kind: ServiceAccount + name: csi-sanity + namespace: blockstor-system +--- # csi-sanity Job: a single pod hosts linstor-csi + the official # csi-sanity binary, sharing /csi via emptyDir. linstor-csi dials the # in-cluster blockstor REST controller, csi-sanity hammers it through @@ -14,6 +50,7 @@ spec: ttlSecondsAfterFinished: 600 template: spec: + serviceAccountName: csi-sanity restartPolicy: Never # linstor-csi is a NON-TERMINATING server: it serves CSI gRPC on # /csi/csi.sock forever. As a plain Job container it kept the pod @@ -92,10 +129,26 @@ spec: - name: csi-sanity image: golang:1.25 # Wait for linstor-csi to open the socket, then run the - # standard csi-sanity test set. Skip the NodeStage tests - # that require a real block device on the host (the satellite - # path covers them out-of-band) and the volume-snapshot - # tests we already smoke separately. + # standard csi-sanity test set. + # + # The -ginkgo.skip regex excludes specs this single-pod Job + # genuinely cannot run: + # - "Node Service": NodeStage/NodePublish/NodeGetVolumeStats + # need a real block device + mount namespace on the host; + # the Job pod is a plain golang container with no privileged + # node plumbing, so these specs block on a mount that never + # lands. The satellite/DRBD path covers node-attach + # out-of-band (L4 `make smoke`). NOTE (BUG-047): before the + # uppercase-name fix these specs failed fast in BeforeEach + # (CreateVolume was rejected, so there was nothing to stage); + # once CreateVolume succeeds they reach the real mount and + # hang, which is why the skip — always intended per the old + # comment — only became load-bearing now. + # - "ListVolumes pagination": the known shared-namespace + # artifact — blockstor-system holds RDs from other tooling, + # so csi-sanity's strict page-boundary assertions are + # non-deterministic here. + # - the two original idempotency/source-not-found specs. command: - bash - -c @@ -112,7 +165,7 @@ spec: -csi.endpoint /csi/csi.sock \ -csi.testvolumesize $((64 * 1024 * 1024)) \ -csi.testvolumeparameters /tmp/sc-params.yaml \ - -ginkgo.skip="should fail when the volume source volume is not found|should not fail when requesting to create a volume with already existing name and same capacity" + -ginkgo.skip="Node Service|ListVolumes pagination|should fail when the volume source volume is not found|should not fail when requesting to create a volume with already existing name and same capacity" volumeMounts: - {name: csi, mountPath: /csi} - {name: scparams, mountPath: /tmp/sc-params.yaml, subPath: sc-params.yaml} diff --git a/tests/e2e/rd-name-validation-bulk.sh b/tests/e2e/rd-name-validation-bulk.sh index 6afeadae..7d95e060 100755 --- a/tests/e2e/rd-name-validation-bulk.sh +++ b/tests/e2e/rd-name-validation-bulk.sh @@ -185,13 +185,17 @@ assert_accept() { # here is the recommended way to widen the gate; mirrors the # TestSnapshotRestoreRejectsInvalidRdName / TestResourceGroupSpawn # RejectsInvalidRdName table in the unit-test sibling. +# BUG-047: the gate now mirrors upstream LINSTOR +# (^[A-Za-z][A-Za-z0-9_-]{1,47}$), so mixed-case / uppercase / +# underscore / trailing-hyphen names are VALID and moved to the +# happy-path block below. Only the forms upstream still refuses remain. INVALID_NAMES=( - "hunt3-Bad" # mixed-case, the operator-poke repro - "BADNAME" # pure uppercase - "bad_underscore" # underscore - "bad-" # trailing hyphen "-bad" # leading hyphen + "1bad" # leading digit (upstream: "Cannot begin with '1'") "bad.name" # embedded dot + "bad name" # embedded space + "bad/name" # path separator + "a" # single char (upstream: min length 2) ) # 1. rd c — already gated (Bug 97); assert the contract still holds so @@ -257,5 +261,28 @@ if ! kubectl get resourcedefinition e2e-c4-good-spawn -o name >/dev/null 2>&1; t fi echo " ok: valid RD 'e2e-c4-good-spawn' persisted" +# BUG-047: an uppercase-hex name (the csi-sanity shape) must now be +# accepted, mirroring upstream LINSTOR. The k8s store folds it onto a +# lowercase slug, so the CRD lands under that slug — assert the spawn +# succeeds and the RD is resolvable via the linstor view. +echo +echo ">> 5b. BUG-047: uppercase-hex name (csi-sanity shape) is accepted" +UPPER_RD="e2e-C4-2A1B4B95EA8C4D7E" +assert_accept "/v1/resource-groups/${RG}/spawn" \ + "{\"resource_definition_name\":\"${UPPER_RD}\",\"volume_sizes\":[1048576]}" \ + "rg spawn '${UPPER_RD}'" + +# Resolve back through the linstor view by the ORIGINAL name (the REST +# surface re-resolves case-insensitively via Name()); the spawn would +# have erred above if the gate still rejected it. +if ! curl -sS "${API}/v1/resource-definitions/${UPPER_RD}" | grep -qi "${UPPER_RD}"; then + echo "FAIL: uppercase RD '${UPPER_RD}' not resolvable via the linstor view after spawn" + exit 1 +fi +echo " ok: uppercase RD '${UPPER_RD}' accepted and resolvable" + +# Track it for cleanup (k8s slug form is lowercased). +delete_rd "$UPPER_RD" 2>/dev/null || true + echo echo "PASS: rd-name-validation-bulk — all 4 RD-minting REST entry points reject invalid names without leaking state"