fix(rest): reject invalid RD names on s r rst and rg spawn before partial state lands

[Andrei Kvapil (kvaps)]

Bug

Bug-hunt v3 finding C.4. Two REST handlers that mint a fresh ResourceDefinition from a user-supplied name bypass the lowercase-RD-name validator the direct POST /v1/resource-definitions path enforces (Bug 97):

• linstor s r rst SRC --fs SNAP --tr <Bad> → POST /v1/resource-definitions/{rd}/snapshot-restore-resource/{snap}
• linstor rg spawn <RG> <Bad> → POST /v1/resource-groups/{rg}/spawn

Reproducer:

linstor s r rst dev-worker-1 dev-worker-2 --fr <good-rd> --fs snap1 --tr hunt3-Bad
linstor rg spawn hunt-zfs hunt3-A1 100M --partial

Root cause

Both handlers passed the raw to_resource / resource_definition_name value straight into Store.ResourceDefinitions().Create(). The k8s CRD store lowercased the metadata.name, and the result no longer matched the CRD admission webhook's metadata.name == <spec.resourceDefinitionName>.<spec.nodeName> invariant. By the time the rejection fired, a partial RD entry had already landed in the linstor view, and the operator saw a raw metadata.name must equal … leak — the same Bug-97 class the direct rd c path already gates against.

Fix

Call the existing validateLinstorName("resource definition", …) helper at the wire boundary of both handlers, BEFORE any Store.Create call, so the failure mode is one consistent LINSTOR envelope and no orphan state is left behind. The sibling snapshot-restore-volume-definition endpoint (Bug 225), which shares the ToResource field, gets the same gate.

The shape mirrors validateRDCreateBody on the direct rd c path: a small validateSnapshotRestoreRequest helper keeps both restore handlers under funlen and gives any future pre-Store gate one canonical home.

Tests

• pkg/rest/rd_name_validation_bulk_test.go

  • TestSnapshotRestoreRejectsInvalidRdName — table-driven across 7 invalid shapes (mixed-case, uppercase, underscore, leading/trailing hyphen, embedded dot, empty), asserts 400 + envelope + no orphan RD in the in-memory store.
  • TestSnapshotRestoreVolumeDefinitionRejectsInvalidRdName — same gate on the sibling VD-only restore endpoint.
  • TestResourceGroupSpawnRejectsInvalidRdName — same table for the rg-spawn handler.
  • TestSnapshotRestoreAcceptsValidRdName / TestResourceGroupSpawnAcceptsValidRdName — happy-path guards so the gate isn't over-strict on production pvc-<uuid> csi clone names.

• tests/e2e/rd-name-validation-bulk.sh — hits all 4 RD-minting REST routes through a live in-cluster apiserver via kubectl port-forward. For each invalid name, asserts (a) 4xx, (b) no metadata.name leak in the envelope, (c) no orphan RD CRD lingering after the request returns. Closes the gap that recurring class bugs in this area (94, 97, 98, 99, 100) keep falling into.

Test plan

• go test ./pkg/rest/... — full suite green, 0.35 s for the new bulk tests, ~60 s for full pkg/rest.
• golangci-lint run ./pkg/rest/ — 0 issues.
• go build ./... — clean.
• e2e on QEMU stand — stand was unreachable at fix-time; the tests/e2e/rd-name-validation-bulk.sh scenario auto-picks-up via ls tests/e2e/*.sh and will run on the next QEMU lane.

Summary by CodeRabbit

• Bug Fixes

  • Added validation to reject invalid resource definition names with HTTP 400 across REST endpoints, preventing invalid state creation and internal error detail leakage.

• Tests

  • Added unit tests for resource definition name validation across snapshot-restore and resource-group spawn endpoints.
  • Added end-to-end test validating rejection of invalid names and acceptance of valid RFC-1123 style names.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7df541ea-72ee-43c0-89e2-e9a489ced12f

📥 Commits

Reviewing files that changed from the base of the PR and between 010e1bb and 0cd54fa.

📒 Files selected for processing (4)

• pkg/rest/rd_name_validation_bulk_test.go
• pkg/rest/snapshot_restore.go
• pkg/rest/spawn.go
• tests/e2e/rd-name-validation-bulk.sh

---

📝 Walkthrough

Walkthrough

This PR adds centralized RFC-1123 name validation to four REST endpoints that create or restore resource definitions. Validation functions reject invalid names with HTTP 400 before any store mutation. Unit tests verify rejection and acceptance across all endpoints. E2E tests confirm validation behavior and absence of state leakage.

Changes

Resource Definition Name Validation

Layer / File(s)	Summary
Snapshot-restore validation gate pkg/rest/snapshot_restore.go	Adds validateSnapshotRestoreRequest to centralize validation of the to_resource field (non-empty and RFC-1123-compliant) across both snapshot-restore endpoints. Both handleSnapshotRestore and handleSnapshotRestoreVolumeDefinition call this gate after JSON decoding, rejecting invalid requests early with HTTP 400.
Spawn endpoint validation pkg/rest/spawn.go	Validates resource_definition_name in handleSpawn before calling Store.ResourceDefinitions().Create(), returning HTTP 400 on invalid names to prevent partial ResourceDefinition state from persisting.
Unit test coverage pkg/rest/rd_name_validation_bulk_test.go	Five test functions verify snapshot-restore (resource and volume paths) and spawn endpoints: three negative-path tests assert HTTP 400 rejection and no persisted ResourceDefinition for invalid names; two positive-path tests assert HTTP 201 acceptance and persistence for valid names.
E2E integration tests tests/e2e/rd-name-validation-bulk.sh	Bash script validates all four RD-minting REST endpoints with invalid name matrices. Helpers assert 4xx rejection without metadata.name leakage and absence of orphan Kubernetes ResourceDefinition CRs. Happy-path guard confirms valid names are accepted and persisted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A fuzzy test suite hops through the REST,
Validating names with RFC-1123 zest!
No orphans left behind, no leaks in the light,
Four endpoints now guard their gates good and tight.
whiskers twitch — the names are validated right!

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/rd-name-validation-bulk

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request addresses a bug where the lowercase resource definition (RD) name validator was bypassed on snapshot-restore and resource group spawn REST endpoints. It introduces validation gates at the wire boundary to reject invalid RD names before state mutation, preventing orphan states and internal Kubernetes metadata leaks. Additionally, comprehensive unit and end-to-end tests are added to verify the behavior. The review feedback points out a potential background process leak in the end-to-end test script if the port-forward wait fails, suggesting registering the trap immediately after starting the process.

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.

[gemini-code-assist]

If _wait_port_forward fails, the script exits immediately. Since the EXIT trap that kills the background kubectl port-forward process (PF_PID) is registered after _wait_port_forward, the background process will be leaked and keep the port bound. Registering the trap immediately after starting the background process prevents this leak.

Suggested change

	PF_PID=$!
	_wait_port_forward "$LPORT" "$PF_PID" || {
	echo "FAIL: could not port-forward apiserver"
	exit 1
	}
	trap 'kill "$PF_PID" 2>/dev/null || true; cleanup' EXIT
	PF_PID=$!
	trap 'kill "$PF_PID" 2>/dev/null || true; cleanup' EXIT
	_wait_port_forward "$LPORT" "$PF_PID" || {
	echo "FAIL: could not port-forward apiserver"
	exit 1
	}