OSAC-3273, OSAC-4036: enforce Volume spec field immutability in Update

[akshaynadkarni]

Summary

Fixes a bug (found during E2E testing, TC-A5) where Volume spec fields (storage_tier, size_gib, access_mode) could be silently modified after creation via PATCH. Adds server-side immutability validation to PrivateVolumesServer.Update() following the same pattern used by PrivateBareMetalInstanceTypesServer. No operator or Helm changes.

Why

The volumes table has a check_immutable_columns DB trigger that protects top-level columns (id, name, tenant, project), but spec fields live inside the data JSONB column which the trigger cannot inspect. Update() previously delegated directly to generic.Update() with no validation, so any spec value could be overwritten.

Spec fields are provisioned directly into the vendor CSI call: the storage tier selects the backend, the size and access mode are set on the PVC. Changing them after creation would produce a split-brain between what the fulfillment-service records and what the vendor actually provisioned.

Testing

cd fulfillment-service

# Spec immutability tests (4 specs: 3 rejection + 1 positive)
go run github.com/onsi/ginkgo/v2/ginkgo run --focus="Spec immutability" internal/servers

# Full servers suite — 1587 specs, 0 failures
go run github.com/onsi/ginkgo/v2/ginkgo run -r internal

Ticket

Feature: OSAC-2872 (Storage Control Plane)

Epic: OSAC-3273 (Volume API, Inventory & Storage Logic)

Task: OSAC-4036 (Enforce spec field immutability in Volume Update endpoint)

Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Claude Code noreply@anthropic.com

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akshaynadkarni

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• fulfillment-service/OWNERS [akshaynadkarni]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@akshaynadkarni: This pull request references OSAC-3273 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set.

This pull request references OSAC-4036 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

Fixes a bug found during E2E testing (TC-A5) where Volume spec fields
(storage_tier, size_gib, access_mode) could be modified after
creation via PATCH. This adds server-side immutability validation to
PrivateVolumesServer.Update in the fulfillment-service.

Standalone fix — no operator or Helm changes. Part of the OSAC-2872
storage control plane work.

Why

The volumes table has a check_immutable_columns DB trigger that
protects top-level columns (id, name, tenant, project), but
spec fields live inside the data JSONB column which the trigger
cannot inspect. The Update method had no validation and accepted
any spec change silently.

Spec fields must be immutable after creation: the storage tier selects
the vendor backend, the size and access mode are provisioned into the
CSI PVC, and none of these can be changed after the vendor call
completes.

What changed

fulfillment-service/internal/servers/private_volumes_server.go:

• Update() now fetches the existing object, clones it, applies the
  field-masked request via applyVolumeUpdate(), and validates immutable
  fields via validateVolumeSpecImmutability() before calling
  generic.Update()
• Returns codes.InvalidArgument with a field-specific message if
  spec.storage_tier, spec.size_gib, or spec.access_mode changed
• Status-only updates (state, vendor_volume_id, backend, protocol)
  are unaffected

fulfillment-service/internal/servers/private_volumes_server_test.go:

• Three new specs under Describe("Spec immutability") covering
  rejection of changes to each immutable field
• The size_gib test also verifies the stored value is unchanged
  after a rejected update

Testing

cd fulfillment-service

# New spec immutability tests (3 specs)
go run github.com/onsi/ginkgo/v2/ginkgo run --focus="Spec immutability" internal/servers

# Full servers suite (1586 specs, 0 failures)
go run github.com/onsi/ginkgo/v2/ginkgo run -r internal

Ticket

Feature: OSAC-2872
(Storage Control Plane)

Epic: OSAC-3273
(Volume API, Inventory & Storage Logic)

Task: OSAC-4036
(Enforce spec field immutability in Volume Update endpoint)

Signed-off-by: akshaynadkarni 25892229+akshaynadkarni@users.noreply.github.com
Assisted-by: Cursor/Claude

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: dada0d24-4813-4ecf-b2f1-3e44c5a2799a

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae7fbe and 873384a.

📒 Files selected for processing (2)

• fulfillment-service/internal/servers/private_volumes_server.go
• fulfillment-service/internal/servers/private_volumes_server_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• fulfillment-service/internal/servers/private_volumes_server.go
• fulfillment-service/internal/servers/private_volumes_server_test.go

---

Walkthrough

Private volume updates now support protobuf field masks. The server retrieves the existing volume, merges requested changes, rejects modifications to immutable specification fields, and persists valid updates. Tests cover changed and unchanged specification values.

Changes

Private volume update flow

Layer / File(s)	Summary
Update processing and immutability validation fulfillment-service/internal/servers/private_volumes_server.go	The update path requires an object identifier, retrieves the existing volume, applies full or masked updates, rejects changes to storage_tier, size_gib, and access_mode, and persists valid updates.
Immutable specification validation coverage fulfillment-service/internal/servers/private_volumes_server_test.go	Tests verify InvalidArgument errors for immutable field changes, confirm that the stored size remains 100, and accept unchanged specification values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 87338

The change adds server-side protection against modifying immutable Volume specification fields after creation, with focused and full-suite tests reported; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant PrivateVolumesServer
  participant ExistingVolume
  participant GenericUpdateOperation
  Caller->>PrivateVolumesServer: Submit Update with object ID and field mask
  PrivateVolumesServer->>ExistingVolume: Retrieve current volume
  PrivateVolumesServer->>PrivateVolumesServer: Merge fields and validate immutable specification
  PrivateVolumesServer->>GenericUpdateOperation: Persist validated update
  GenericUpdateOperation-->>Caller: Return update result

Loading

Possibly related PRs

• osac-project/osac#201: Both changes modify private_volumes_server.go and its update tests. This change adds field-mask merging and immutable specification validation.

Suggested labels: enhancement

Suggested reviewers: rgolangh, avishayt, danniesh

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	The PR adds only Go logic and tests; added lines contain no API keys, tokens, passwords, private-key material, credential URLs, or long base64/hex blobs.
No-Weak-Crypto	✅ Passed	The aggregate PR diff adds protobuf/field-mask update logic and tests only; its added-line scan found no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB, custom crypto, or secret comparisons.
No-Injection-Vectors	✅ Passed	The PR adds protobuf merge and field-mask logic only; no SQL concatenation, shell, eval/exec, unsafe YAML, pickle, or innerHTML APIs were added. The ID uses parameterized DAO arguments.
Container-Privileges	✅ Passed	The cumulative PR diff changes only two Go files and tests; it adds no container/Kubernetes manifests or privilege settings such as privileged, hostPID, hostNetwork, SYS_ADMIN, or allowPrivilegeEsc...
No-Sensitive-Data-In-Logs	✅ Passed	The PR diff adds no logging calls or sensitive values. It only fetches, merges, and validates volumes; existing generic error logs record a resource ID, not a password, token, or PII.
Ai-Attribution	✅ Passed	AI use is explicit, and both PR commits include an Assisted-by: Claude Code trailer; the PR range contains no AI Co-Authored-By trailer.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: enforcing Volume spec field immutability during Update.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:18 PM UTC · Ended 2:23 PM UTC

Commit: 5ae7fbe · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:24 PM UTC · Completed 2:43 PM UTC

Commit: 5ae7fbe · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [test-inadequate] fulfillment-service/internal/servers/private_volumes_server_test.go — The immutability tests only cover the field-mask code path (where specific paths are listed in the UpdateMask). The no-mask path — where mask is nil and proto.Merge is used — is not tested for immutability enforcement. Sibling servers (PrivateInstanceTypesServer, PrivateBareMetalInstanceTypesServer) exercise the no-mask path in their immutability tests.
  Remediation: Add a test case that sends an update with a changed immutable field and no UpdateMask, verifying it is rejected with InvalidArgument.

Low

• [test-inadequate] fulfillment-service/internal/servers/private_volumes_server_test.go — No test verifies that spec.pvc_ref can be updated via field mask. The code marks pvc_ref as intentionally mutable, but this positive-path behavior is untested. The proto doc states "All fields are immutable after creation" while the code carves out an exception for pvc_ref — a test would document this intentional exception.
  Remediation: Add a test that updates spec.pvc_ref via field mask and verifies success.

• [edge-case] fulfillment-service/internal/servers/private_volumes_server.go — The Update method performs an unlocked Get to fetch the existing object for immutability validation, then delegates to s.generic.Update() which performs its own locked fetch. The check_immutable_columns DB trigger only protects table-level columns (id, name, tenant, project), not the spec fields inside the JSONB data column, making this server-side validation the sole enforcement mechanism. Both reads run within the same gRPC-interceptor-managed DB transaction, so the practical exploitation window is extremely narrow. This follows the same double-fetch pattern used by sibling servers.

• [pattern-inconsistency] fulfillment-service/internal/servers/private_volumes_server.go — The immutability error messages do not include old and new values. Sibling servers include both values for debuggability, e.g., "field 'spec.cores' is immutable and cannot be changed from '%d' to '%d'".
  Remediation: Include old and new values in the error messages.

Previous run

Review

Findings

Medium

• [pattern deviation / missing step] fulfillment-service/internal/servers/private_volumes_server.go:186 — The Update method does not set the merged spec back into the request before calling generic.Update, diverging from the reference pattern in PrivateBareMetalInstanceTypesServer.Update(). The merged object from applyVolumeUpdate is used only for immutability validation; its state is then discarded. generic.Update performs its own independent merge, making the local merge effectively dead code for any purpose beyond validation. Any future mutable spec field added to applyVolumeUpdate would be merged twice independently, and the results could diverge.
  Remediation: Add request.GetObject().SetSpec(merged.GetSpec()) before the s.generic.Update(ctx, request, &response) call.

• [incomplete immutability enforcement] fulfillment-service/internal/servers/private_volumes_server.go:210 — The proto comment on VolumeSpec states "All fields are immutable after creation," and spec.pvc_ref is handled in applyVolumeUpdate, but validateVolumeSpecImmutability does not check whether pvc_ref has changed. If pvc_ref is intentionally mutable (e.g., set by the CSI driver post-creation), the proto comment should be updated; if truly immutable, the validation is incomplete.
  Remediation: Either add a pvc_ref immutability check (e.g., proto.Equal comparison) or update the proto comment to clarify that pvc_ref is an exception.

Low

• [naming convention] fulfillment-service/internal/servers/private_volumes_server.go — Function validateVolumeSpecImmutability includes an extra Spec qualifier not present in the established pattern (validateBareMetalInstanceTypeImmutability, validateInstanceTypeImmutability). Consider renaming to validateVolumeImmutability.

• [pattern consistency] fulfillment-service/internal/servers/private_volumes_server.go — Missing cloneVolume helper function. Reference implementations extract the proto.Clone call into a named helper with a doc comment.

• [pattern consistency] fulfillment-service/internal/servers/private_volumes_server.go — The Update method has no inline section comments demarcating each step, unlike reference implementations which use comments like // Get the object identifier:, // Fetch the existing object:, etc.

• [documentation comments] fulfillment-service/internal/servers/private_volumes_server.go — The new applyVolumeUpdate and validateVolumeSpecImmutability functions lack Go doc comments. Reference implementations provide doc comments on all helper functions.

• [pattern consistency] fulfillment-service/internal/servers/private_volumes_server.go — The applyVolumeUpdate switch statement has no default case. Reference implementations include a default: branch with a comment.

• [test coverage gap] fulfillment-service/internal/servers/private_volumes_server_test.go:451 — The new immutability tests cover only rejection cases. No positive test confirms that an update preserving immutable field values (e.g., sending the same spec values or updating only status fields) still succeeds. An existing test covers status-only updates, but a no-op spec update test would guard against overly strict validation.

• [TOCTOU race condition] fulfillment-service/internal/servers/private_volumes_server.go:172 — The immutability check uses an unlocked Get before generic.Update's locked read. Practical impact is negligible — immutable fields should never change post-creation, and the reference pattern (PrivateBareMetalInstanceTypesServer) uses the same design.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:07 PM UTC · Ended 6:14 PM UTC

Commit: 18bf70a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:16 PM UTC · Completed 6:37 PM UTC

Commit: 873384a · View workflow run →

[wgordon17]

/lgtm