OSAC-3277: resolve StorageTier at volume creation

[akshaynadkarni]

Summary

OSAC-3277: Add tier resolution to the Volume Create handler in fulfillment-service.

When a Volume is created, the server now resolves spec.storage_tier to a concrete
backend and protocol before persisting. Resolution looks up the StorageTier by name,
validates it is ACTIVE with at least one BackendAssociation, and selects the first
association. The resolved backend ID and protocol are written to status.backend and
status.protocol so the reconciler and operator controller have the routing information
without a second lookup.

Fails fast with gRPC status codes: NotFound if the tier does not exist,
FailedPrecondition if the tier is not active or has no backends.

Why

Without tier resolution, volumes were persisted with an empty backend and protocol.
The Volume controller on the operator side needs to know which vendor CSI driver to
call, and the feedback controller needs to propagate the protocol back. Resolving at
creation time is the natural place since the fulfillment-service has direct DAO access
to StorageTier records.

Testing

go build ./...           # passes
uv run dev.py lint go    # 0 issues
buf lint                 # passes

Integration test coverage for the full Volume create flow is tracked under OSAC-4046.

Ticket

OSAC-3277 (under OSAC-3273 epic, under OSAC-2872 feature)

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

Summary by CodeRabbit

• New Features

  • Volume creation now resolves and validates the requested storage tier.
  • Volumes record the selected storage backend and protocol.

• Bug Fixes

  • Volume creation is rejected when the storage tier cannot be found, is inactive, lacks a backend, or cannot be resolved.
  • Service startup now fails clearly if storage-tier configuration cannot be initialized.

[openshift-ci-robot]

@akshaynadkarni: This pull request references OSAC-3277 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.1.0" version, but no target version was set.

Details

In response to this:

Summary

OSAC-3277: Add tier resolution to the Volume Create handler in fulfillment-service.

When a Volume is created, the server now resolves spec.storage_tier to a concrete
backend and protocol before persisting. Resolution looks up the StorageTier by name,
validates it is ACTIVE with at least one BackendAssociation, and selects the first
association. The resolved backend ID and protocol are written to status.backend and
status.protocol so the reconciler and operator controller have the routing information
without a second lookup.

Fails fast with gRPC status codes: NotFound if the tier does not exist,
FailedPrecondition if the tier is not active or has no backends.

Why

Without tier resolution, volumes were persisted with an empty backend and protocol.
The Volume controller on the operator side needs to know which vendor CSI driver to
call, and the feedback controller needs to propagate the protocol back. Resolving at
creation time is the natural place since the fulfillment-service has direct DAO access
to StorageTier records.

Testing

go build ./...           # passes
uv run dev.py lint go    # 0 issues
buf lint                 # passes

Integration test coverage for the full Volume create flow is tracked under OSAC-4046.

Ticket

OSAC-3277 (under OSAC-3273 epic, under OSAC-2872 feature)

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.

[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

[coderabbitai]

Warning

Review limit reached

@akshaynadkarni, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c646a15-cacf-4a88-9e1c-84a1f15aabfe

📥 Commits

Reviewing files that changed from the base of the PR and between 1af7885 and e39c728.

📒 Files selected for processing (3)

• fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
• fulfillment-service/internal/servers/private_volumes_server.go
• fulfillment-service/internal/servers/private_volumes_server_test.go

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: c86e4463-2b28-47f3-85ea-e67e875964eb

📥 Commits

Reviewing files that changed from the base of the PR and between fa35b54 and 1af7885.

📒 Files selected for processing (2)

• fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
• fulfillment-service/internal/servers/private_volumes_server.go

---

Walkthrough

The gRPC startup path creates and injects a tenancy-aware storage-tier DAO. Private volume creation resolves the requested tier, validates it, selects its first backend association, and records the backend ID and protocol in volume status.

Changes

Storage-tier volume resolution

Layer / File(s)	Summary
Storage-tier DAO wiring fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go, fulfillment-service/internal/servers/private_volumes_server.go	Startup creates the tenancy-aware DAO and injects it into the private volumes server builder. The builder requires and retains the DAO.
Volume tier resolution fulfillment-service/internal/servers/private_volumes_server.go	Volume creation looks up the requested tier, rejects lookup failures, missing tiers, inactive tiers, and tiers without backends, then records the selected backend ID and protocol in volume status.

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

Merge Risk: ⚪ Minimal · up to 1af78

The Volume creation path now resolves storage tiers before persisting routing details; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: crystalchun, danmanor, eliorerz

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: resolving StorageTier during volume creation.
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 diff adds DAO wiring, status/error literals, and a quoted CEL filter; no API keys, tokens, passwords, private keys, credential URLs, or secret-shaped blobs were introduced.
No-Weak-Crypto	✅ Passed	The PR diff adds DAO wiring and tier resolution only; it introduces no MD5, SHA1, DES, RC4, Blowfish, 3DES, ECB, custom crypto, or secret comparisons.
No-Injection-Vectors	✅ Passed	The changed code uses strconv.Quote for the user-supplied tier name in a CEL filter; no SQL concatenation, shell execution, eval/exec, unsafe YAML, pickle, or DOM sink was introduced.
Container-Privileges	✅ Passed	The PR diff contains only two Go source files and adds no container or Kubernetes manifest privilege settings; existing fulfillment manifests use runAsNonRoot and allowPrivilegeEscalation: false.
No-Sensitive-Data-In-Logs	✅ Passed	The PR adds no log calls or sensitive log fields. It wires a DAO and returns tier-name gRPC errors; existing interceptor logging is unchanged.
Ai-Attribution	✅ Passed	Both PR commits contain the trailer “Assisted-by: Cursor/Claude,” and the PR commit range contains no Co-Authored-By trailer.

✨ 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]

🤖 Finished Review · ✅ Success · Started 4:40 PM UTC · Completed 4:58 PM UTC

Commit: 521211b · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [test-adequacy] fulfillment-service/internal/servers/private_volumes_server_test.go:151 — The "Creates and gets a volume" test verifies the status state is CREATING but does not assert that the resolved backend and protocol are persisted in the volume's status. The stub resolver returns BackendID="test-backend" and Protocol=STORAGE_PROTOCOL_BLOCK, but no Create test checks GetStatus().GetBackend() or GetStatus().GetProtocol(). A regression that silently drops tier resolution results would go undetected.
  Remediation: Add assertions verifying the created volume's status contains the expected backend and protocol values.

• [test-adequacy] fulfillment-service/internal/servers/private_volumes_server_test.go:90 — All Behaviour tests use a stub resolver that always succeeds. No tests exercise resolver error paths (tier not found, tier not active, tier with no backends). The Create handler's error propagation when the resolver fails is untested at the unit level.
  Remediation: Add a test case with a failing resolver (e.g., returning codes.NotFound) and verify Create returns the expected error code without persisting.

• [architectural-coherence] fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go:1224 — The new storageTiersDAO is created without SetMetricsRegisterer(metricsRegisterer). This is consistent with the established pattern for auxiliary/helper DAOs (storageBackendsDAO at line 1199 also omits it), so no action required.

• [pattern-inconsistency] fulfillment-service/internal/servers/private_volumes_server.go:40 — TierResolverFunc is a bare function type, while most cross-resource dependencies use interfaces. However, ReferenceLookupFunc in the same codebase uses the same pattern, making this a minor stylistic preference rather than a violation.
  Remediation: Consider defining a TierResolver interface if this pattern grows to multiple methods.

• [code-organization] fulfillment-service/internal/servers/private_volumes_server.go:33 — TierResolution and TierResolverFunc are exported types placed at the top of the volumes server file. They are tightly coupled to this server (8 lines total), so this is a minor organizational note.
  Remediation: Consider moving to a dedicated tier_resolver.go if the type grows.

• [pattern-inconsistency] fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go:1222 — The new DAO/resolver creation block does not emit a c.logger.InfoContext log line, unlike most resource-creation blocks in this function (though storageBackendsDAO also omits it).
  Remediation: Add c.logger.InfoContext(ctx, "Creating tier resolver for volumes server") before the DAO creation.

Previous run

Review

Findings

Critical

• [test-inadequate] fulfillment-service/internal/servers/private_volumes_server_test.go — The PR makes storageTiersDAO mandatory in Build() but does not update the test file. All 4 builder call sites in the test file lack SetStorageTiersDAO(), causing the builder tests and every BeforeEach setup to fail with "storage tiers DAO is mandatory". Additionally, the createVolume helpers reference StorageTier: "gold" but no corresponding tier is seeded in the test database, so resolveTier would return NotFound on every Create call. All existing volume behavioural tests are broken.
  Remediation: Update the test file to (1) create and inject a storageTiersDAO, (2) seed an ACTIVE StorageTier named "gold" with at least one BackendAssociation, and (3) add test cases covering tier resolution edge cases (not found, not active, no backends).

Medium

• [edge-case] fulfillment-service/internal/servers/private_volumes_server.go:207 — resolveTier selects backends[0] unconditionally without validating that the selected BackendAssociation has a non-empty backend_id. An empty backend_id would persist an empty status.backend, which downstream consumers (CSI driver, operator reconcilers) would likely fail on.
  Remediation: Validate selected.GetBackendId() is non-empty after selection. Return FailedPrecondition if empty.

• [information-disclosure] fulfillment-service/internal/servers/private_volumes_server.go:197 — resolveTier wraps internal DAO errors into the gRPC response via %v formatting (grpcstatus.Errorf(grpccodes.Internal, "failed to look up storage tier %q: %v", tierName, err)), potentially leaking SQL fragments, table names, or connection details to the API caller. The analogous lookup in private_storage_tiers_server.go intentionally omits the raw error from the response.
  Remediation: Log the underlying error server-side via s.logger.ErrorContext(ctx, ...) and return a generic message without the %v error detail.

Low

• [naming-convention] fulfillment-service/internal/servers/private_volumes_server.go:51 — The field uses storageTiersDAO (all-caps suffix) but the dominant convention across the servers/ package is camelCaseDao (e.g., virtualNetworkDao, subnetsDao, storageTiersDao in private_compute_instances_server.go). Only private_storage_tiers_server.go uses the all-caps DAO suffix.
  Remediation: Rename to storageTiersDao / SetStorageTiersDao in both files.

• [authorization/tenant-isolation] fulfillment-service/internal/servers/private_volumes_server.go:193 — resolveTier queries the storage tiers DAO using the calling user's gRPC context, scoping results via tenancy logic. StorageTiers are platform-scoped (SharedTenant). If a deployment's tenancy configuration excludes SharedTenant from a user's visible set, tier resolution would silently return "not found" for valid tiers.

---

Labels: PR modifies storage tier resolution in fulfillment-service

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (2)

Review

Findings

High

• [logic-error] fulfillment-service/internal/servers/private_volumes_server.go:186 — The filter expression metadata.name == '%s' is missing the this. prefix. The CEL environment created by FilterTranslator.createCelEnv() declares only two variables: this and now. The bare identifier metadata will fail CEL compilation with an unknown identifier error. Every other server-side filter in the codebase uses this. (e.g., cluster_version_resolution.go:38, private_clusters_server.go:294). This means every call to resolveTier will fail, making volume creation entirely non-functional.
  Remediation: Change the filter to fmt.Sprintf("this.metadata.name == %s", strconv.Quote(tierName)). This also addresses the quoting issue below.

• [test-adequacy] fulfillment-service/internal/servers/private_volumes_server_test.go:32 — Build() now requires storageTiersDAO (returns error "storage tiers DAO is mandatory" if nil), but the test file is not updated. The builder success test at line 32 and the BeforeEach at line 64 both omit SetStorageTiersDAO(). The entire test suite for volumes is broken by this PR.
  Remediation: Update the test file to: (1) add SetStorageTiersDAO() to the builder test, (2) add a "fails if storage tiers DAO is not set" test, (3) create and seed a storageTiersDAO in BeforeEach before volume creation tests.

Medium

• [injection] fulfillment-service/internal/servers/private_volumes_server.go:186 — CEL filter uses fmt.Sprintf("metadata.name == '%s'", tierName) with raw single-quote wrapping instead of strconv.Quote(). A crafted tierName with embedded single quotes could alter the CEL expression semantics. The established codebase pattern (cluster_version_resolution.go:38, private_clusters_server.go:294,329,1007) uses strconv.Quote() for properly escaped double-quoted CEL string literals. See also: [logic-error] finding at this location.
  Remediation: Use strconv.Quote(tierName) instead of manually wrapping in single quotes.

• [missing-test] fulfillment-service/internal/servers/private_volumes_server_test.go — The PR adds non-trivial resolveTier logic with three distinct error paths (tier not found / NotFound, tier not active / FailedPrecondition, no backends / FailedPrecondition) and a happy path that selects backends[0]. None of these paths have test coverage. The comparable compute instances server tests cover the "storage tier does not exist" case.
  Remediation: Add Ginkgo It blocks covering each error path and a success case that asserts on status.backend and status.protocol values.

Low

• [data-exposure] fulfillment-service/internal/servers/private_volumes_server.go:192 — Internal error details leaked to gRPC client via %v in the DAO lookup error path. The error message includes the raw DAO error, which may contain database connection details.

• [naming-convention] fulfillment-service/internal/servers/private_volumes_server.go:38 — The field storageTiersDAO uses uppercase DAO suffix. The dominant codebase convention is camelCase Dao (e.g., storageTiersDao, templatesDao, subnetsDao in private_compute_instances_server.go). The uppercase variant exists only in private_storage_tiers_server.go.

• [error-handling-idiom] fulfillment-service/internal/servers/private_volumes_server.go:192 — The resolveTier method uses %q formatting for error messages. The established codebase pattern uses single-quoted '%s' (e.g., storage tier '%s' does not exist in private_compute_instances_server.go:651). See also: [data-exposure] finding at this location.

• [scope-boundary] fulfillment-service/internal/servers/private_volumes_server.go:189 — The resolveTier method uses a filter-based List query instead of direct Get-by-ID. Since PrivateStorageTiersServer.Create sets id = metadata.name, the tier can be looked up via s.storageTiersDAO.Get().SetId(tierName).Do(ctx) — the pattern used by private_compute_instances_server.go:644 for the same resource. Using Get-by-ID would also eliminate the CEL filter issues entirely.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Auto-dismissed: only Prow labels gate merging

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 6:01 PM UTC · Ended 6:09 PM UTC

Commit: 1af7885 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:10 PM UTC · Completed 6:30 PM UTC

Commit: 14807cf · View workflow run →

Auto-dismissed: only Prow labels gate merging

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:55 PM UTC · Completed 8:10 PM UTC

Commit: e39c728 · View workflow run →

[fullsend-ai-review]

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• fulfillment-service/internal/servers/private_volumes_server_test.go (file-level): Line 151 · [low] test-adequacy

Create test doesn't assert backend/protocol values in status. Stub resolver returns BackendID='test-backend' and Protocol=STORAGE_PROTOCOL_BLOCK but no Create test checks GetStatus().GetBackend() or GetStatus().GetProtocol(). A regression that silently drops tier resolution results would go undetected by unit tests.

Suggested fix: Add assertions to the Create test verifying status contains expected backend and protocol values.

• fulfillment-service/internal/servers/private_volumes_server_test.go:90: [low] test-adequacy

All Behaviour tests use a stub resolver that always succeeds. No tests exercise resolver error paths (tier not found, not active, no backends). Create handler error propagation when resolver fails is untested at the unit level.

Suggested fix: Add test with failing resolver returning grpcstatus error and verify Create returns expected error code without persisting.

• fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go:1224: [low] architectural-coherence

The new storageTiersDAO is created without SetMetricsRegisterer(metricsRegisterer). This is consistent with the established pattern for auxiliary/helper DAOs (storageBackendsDAO at line 1199 also omits it).

• fulfillment-service/internal/servers/private_volumes_server.go:40: [low] pattern-inconsistency

TierResolverFunc is a bare function type while most cross-resource dependencies use interfaces. However, ReferenceLookupFunc uses the same pattern, making this a minor stylistic preference.

Suggested fix: Consider defining a TierResolver interface if this pattern grows to multiple methods.

• fulfillment-service/internal/servers/private_volumes_server.go:33: [low] code-organization

TierResolution and TierResolverFunc are exported types placed at top of volumes server file. They are tightly coupled to this server (8 lines total).

Suggested fix: Consider moving to a dedicated tier_resolver.go if the type grows.

• fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go:1222: [low] pattern-inconsistency

The new DAO/resolver creation block does not emit a c.logger.InfoContext log line, unlike most resource-creation blocks in this function (though storageBackendsDAO also omits it).

Suggested fix: Add c.logger.InfoContext(ctx, 'Creating tier resolver for volumes server') before the DAO creation.