OSAC-3995: Public Storage Tier API — fulfillment-service backend - #292
OSAC-3995: Public Storage Tier API — fulfillment-service backend#292wgordon17 wants to merge 44 commits into
Conversation
|
@wgordon17: This pull request references OSAC-3995 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. DetailsIn response to this:
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds public StorageTiers List and Get APIs, moves protocol configuration to ChangesStorage-tier contracts and migration
Public Storage Tiers
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds tenant-facing storage-tier reads and changes persisted storage-tier schema data. Merge readiness remains high risk because conflicting protocol values may be silently lost during migration, the installer Job is not constrained to non-root execution, and pagination metadata can mislead consumers when malformed tiers exist. Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTGateway
participant StorageTiersServer
participant PrivateStorageTiersServer
participant StorageBackendsDAO
Client->>RESTGateway: Request public List or Get
RESTGateway->>StorageTiersServer: Translated gRPC request
StorageTiersServer->>PrivateStorageTiersServer: Validated delegated request
PrivateStorageTiersServer->>StorageBackendsDAO: Resolve backend associations
StorageBackendsDAO-->>PrivateStorageTiersServer: Backend data
PrivateStorageTiersServer-->>StorageTiersServer: Private tier result
StorageTiersServer-->>Client: Flattened public StorageTier response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 47.37% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 24 files. (20 skipped: 20 unsupported.) Full details: No-Hardcoded-SecretsExplanation No hardcoded secret was introduced. The only added credential literal is Full details: No-Weak-CryptoExplanation No weak-crypto usage was introduced. The aggregate diff from Full details: No-Injection-VectorsExplanation No prohibited injection vector was introduced. The changed installer hook passes the API-derived Full details: Container-PrivilegesExplanation No prohibited container privilege setting was introduced. The only changed Kubernetes manifest is the local-storage hook, and its security context is unchanged from Full details: No-Sensitive-Data-In-LogsExplanation No changed production log statement records a password, token, API key, PII, session ID, or backend endpoint. The new server logs only mapper errors, a storage-tier ID with an association count, and an enum number. The installer prints only the local resource ID and fixed validation text. The generated gateway endpoint log is standard boilerplate and its FromEndpoint helper is not used by the service; the REST gateway uses the shared connection registration path. CLI rendering is user output, not application logging. Full details: Ai-AttributionExplanation AI use is explicitly documented in the PR and commit history. The pull-request range contains multiple ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 4:00 PM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewReason: stale-head The review agent reviewed commit Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsLow
Previous run (11)ReviewFindingsMedium
Low
Previous run (12)ReviewFindingsLow
Labels: PR adds Go-implemented public StorageTiers gRPC server, CLI commands, and tests — matches convention of applying tech labels alongside existing type/domain labels. Previous run (13)ReviewFindingsHigh
Low
Next steps:
Previous run (14)ReviewFindingsLow
Previous run (15)ReviewFindingsLow
Previous run (16)Looks good to me FindingsLow
Info
Previous run (17)ReviewFindingsMedium
Low
Previous run (18)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (19)ReviewFindingsMedium
Low
Previous run (20)ReviewFindingsMedium
Low
Previous run (21)ReviewFindingsLow
Labels: PR adds new public StorageTiers API to the fulfillment-service storage subsystem |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
ad0f7d5 to
ac29b87
Compare
|
🤖 Review · Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fulfillment-service/internal/cmd/cli/get/storagetier/get_storagetier_cmd.go`:
- Around line 106-159: Update renderTierTable and renderTierDetail in
fulfillment-service/internal/cmd/cli/get/storagetier/get_storagetier_cmd.go
(lines 106-159) to return errors from every fmt.Fprint* call and
tabwriter.Writer.Flush, then propagate those errors through renderStorageTier
and the respective command at lines 66-84. Apply the same output-error
propagation to the storage-tier describe command in
fulfillment-service/internal/cmd/cli/describe/storagetier/describe_storagetier_cmd.go
(lines 88-120), ensuring broken-pipe failures are returned instead of producing
success.
In `@fulfillment-service/internal/servers/storage_tiers_server.go`:
- Around line 214-223: Update the List conversion loop around privateItems and
toPublicTier to log malformed tiers and continue instead of returning the
conversion error; build the result slice with only successfully converted items
and adjust size to match the filtered items count. Keep Get’s existing Internal
error behavior unchanged, and update the affected List test expectations
accordingly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8696598c-879a-4f50-b239-5a4d1b122027
⛔ Files ignored due to path filters (6)
fulfillment-service/internal/api/osac/public/v1/storage_tier_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/storage_tier_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/storage_tiers_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/storage_tiers_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/public/v1/storage_tiers_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/storage_tiers_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
fulfillment-service/internal/auth/policies/authz.regofulfillment-service/internal/cmd/cli/describe/storagetier/describe_storagetier_cmd.gofulfillment-service/internal/cmd/cli/get/get_cmd.gofulfillment-service/internal/cmd/cli/get/storagetier/get_storagetier_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/cmd/service/start/restgateway/start_rest_gateway_cmd.gofulfillment-service/internal/servers/storage_tiers_server.gofulfillment-service/internal/servers/storage_tiers_server_test.gofulfillment-service/it/it_public_storage_tiers_test.gofulfillment-service/proto/public/osac/public/v1/storage_tier_type.protofulfillment-service/proto/public/osac/public/v1/storage_tiers_service.proto
|
🤖 Review · Commit: |
A tenant can't act on a malformed tier (0 or >1 backend associations) -- that's a cloud-provider-admin data problem, not something the caller can fix. Failing the entire List response for every tenant over one corrupted admin-side row is bad UX. List now logs and omits malformed tiers (adjusting size to match); Get is unchanged and still returns Internal for the specific tier requested. Addresses CodeRabbit review feedback on PR osac-project#292. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Auto-dismissed: only Prow labels gate merging
GetQuotaGib() no longer exists on the public StorageTierSpec. Removes the detail-view Quota line from both describe and get, and the compact-table QUOTA column from get. Protocol rendering is untouched -- same data path, no functional change. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
… field Moves protocol out of the backends array element into the top-level spec object in both the create payload and the idempotency-check parsing logic, matching the private proto restructure. Left unfixed, this hook would still return HTTP 2xx on first run (creating a tier with protocol silently unset) then fail its own idempotency check on every subsequent hook re-run. Also fixes two issues found while rewriting this line: BACKEND_ID was bash-spliced directly into the python3 -c source string, letting a crafted backend_id value break out of the string literal and execute inside this admin-scoped hook -- now passed via an environment variable instead. And the idempotency check's `stderr.write(...) or sys.exit(1)` never actually exited on failure, since write() returns a truthy byte count that short-circuits the `or` -- fixed to unconditionally exit. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
describe storagetier was switched to the public StorageTiers API client back when it was first added, but was still registered with help.MarkPrivateAPI -- hiding it from `osac describe --help` for any tenant not running with --private, even though it works fine for them and its sibling `get storagetier` command is correctly discoverable. A test locked in the stale contract by asserting storagetier must carry the private-API annotation. Removes the wrapper and updates the test's expectation to match. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Will Gordon <wgordon@redhat.com>
Fixes 6 pr-review findings on PR osac-project#292: test coverage for toPublicStorageProtocol's default branch, a Total/Size divergence in StorageTiersServer.List when malformed tiers are dropped, duplicate get/describe storagetier rendering logic, a missing describe storagetier test file, an opaque BUG-005 test reference, and a grammar fix in the local-storage installer hook. A follow-up quality-gate pass found and fixed a pagination-consistency bug in the Total fix itself (only adjust Total when the response provably covers the entire result set) plus a related simplification and test-helper deduplication.
osac-operator's buf.gen.yaml now reads fulfillment-service's proto locally (c8f4cb7 migrated it off the BSR pin), removing the tag-and-republish dependency that previously blocked this change. Regenerates the private StorageTier client and updates resolveTierDefinitions() to read protocol from the tier's spec instead of its first backend association, and drops the removed quota_gib field from TierDefinition and its extra_vars serialization.
Drops VAST quota creation/validation now that the StorageTier API no longer carries a quota field, removing the corresponding examples and documentation across playbooks, roles, and samples.
Aligns the StorageTier creation payload with the same env-var pattern already used for the existing-tier verification path, and produces correctly-escaped JSON via json.dumps instead of raw printf substitution.
Renumbers the relocate-protocol-and-remove-quota migration to 101 to resolve a collision with a migration added on main, and migrates the StorageTiersServer's filter validator to the new non-generic FilterTranslator API (SetDescriptor instead of a type parameter). Also fixes a latent bug this surfaced: List unconditionally called SetLimit on the private request, marking it present with value 0 even when the public request had no limit set, causing the underlying GenericServer's HasLimit() check to apply a literal zero-row limit. Guarded the call the same way GenericServer itself does.
…etering proto generation
|
🤖 Review · ❌ Terminated · Started 9:34 AM UTC · Ended 10:13 AM UTC Commit: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, wgordon17, zszabo-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
E2E on
|
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/workflows/check-generated-code.yaml(file-level): Line 90 · [medium] CI coverage regression
The PR adds osac-metering/metering-service to the workflow path triggers and changes job path filter, but does not add it to the matrix.component list. The generated-code check for metering will never run because no matrix job matches it.
Suggested fix: Add osac-metering/metering-service to the matrix.component list.
.github/workflows/check-generated-code.yaml(file-level): Line 9 · [low] scope-creep
The osac-metering CI path triggers and cleanapi dummy package stub are infrastructure fixes necessitated by the private proto gaining the cleanapi annotation. They are a direct consequence of the proto change but could be mentioned in the PR description.
osac-installer/charts/osac/templates/hooks/register-local-storage.yaml(file-level): Line 51 · [low] Command Injection (Residual)
Line 51 still interpolates ${BACKEND_RESP} (a mktemp path) into a Python string literal. The PR correctly fixed BACKEND_ID interpolation by using os.environ, but did not apply the same pattern to BACKEND_RESP. Not practically exploitable since mktemp paths have no shell-special characters.
Suggested fix: Consider passing BACKEND_RESP via environment variable for defense-in-depth consistency.
| return err | ||
| } | ||
|
|
||
| renderTierTable(c.console, []*publicv1.StorageTier{tier}) |
There was a problem hiding this comment.
[low] convention-consistency
When get storagetier resolves to a single tier, it renders a table. The sibling get externalippool command renders a detail view for single-item gets. Both approaches are legitimate UX choices.
| osac get storagetier tier-abc123` | ||
|
|
||
| // renderTierTable writes a compact table of storage tiers — used when listing all tiers. | ||
| func renderTierTable(w *terminal.Console, tiers []*publicv1.StorageTier) { |
There was a problem hiding this comment.
[low] convention-consistency
renderTierTable takes *terminal.Console while renderStorageTier in the describe command takes io.Writer. Since *terminal.Console implements io.Writer, this is not functional but is an inconsistency within the same resource CLI commands.
Suggested fix: Pick one writer type consistently for both get and describe render functions.
| } | ||
|
|
||
| // Create private request with same parameters: | ||
| privateRequest := &privatev1.StorageTiersListRequest{} |
There was a problem hiding this comment.
[low] edge-case
The List method calls SetOffset unconditionally (passing 0 when unset) while SetLimit uses a HasLimit guard. The asymmetry is benign since offset=0 is the default behavior.
|
🤖 Finished Review · ✅ Success · Started 9:34 AM UTC · Completed 10:13 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $13.64 |
4 participants
OSAC-3995: Public Storage Tier API — fulfillment-service backend
Jira: https://redhat.atlassian.net/browse/OSAC-3995 (child of epic OSAC-3014)
Summary
Adds a public, tenant-facing
StorageTiersgRPC service (Get+Listonly) tofulfillment-service, wrapping the existing private StorageTier API via the same
builder/delegate/mapper pattern already used for
BareMetalInstanceTypes. Tenants can now browseavailable storage tiers to choose one when provisioning a ComputeInstance.
Also removes storage quota tracking entirely and moves
protocolfrom the privateBackendAssociationmessage up to the tier's own spec, since it's a property of the tier as awhole rather than of an individual backend association. This includes a data migration so
pre-existing rows don't silently lose their
protocolvalue, and updates every affected call siteacross the private/public servers, the admin CLI, and the
osac-installerlocal-storageregistration hook.
Changes
public.v1.StorageTier/StorageTierSpec/StorageTierStatusproto types andStorageTiersservice (Get/List only, no Create/Update/Delete — matches the
BareMetalInstanceTypes/ExternalIPPoolsprecedent for read-only platform-catalog resources).StorageTiersServerimplementation with two-layer CEL filter validation to prevent tenants fromprobing excluded private fields via the filter side-channel (a scoped fix for BUG-018; the
broader pattern across 24 other servers is tracked separately in OSAC-3609).
get storagetier,describe storagetierswitched to the public client).quota_gibfrom the privateBackendAssociationmessage and the (not-yet-shipped)public
StorageTierSpecfield, with a backfill migration forstorage_tiers/archived_storage_tiersso existing rows are correctly cleaned up rather than silentlycorrupted.
protocolfromBackendAssociationtoStorageTierSpecon both the private andpublic proto, updating the public server's flattening logic, the admin CLI, both server test
suites, the integration tests, and the
osac-installerlocal-storage registration hook (whichalso had a pre-existing command-injection vector in the same line, fixed alongside the field
move).
protocolnow lives at the same path (this.spec.protocol) on both schemas, filtering byprotocol is now forwardable to the private delegate — previously rejected as a schema-mismatch.
Testing
internal/servers/storage_tiers_server_test.go,private_storage_tiers_server_test.go— builder validation, List/Get delegation and fieldflattening, pagination, both filter-validation layers (including a new test proving the
now-shared
protocolfilter path is genuinely forwarded and discriminates correctly, not just"isn't rejected"), defensive handling of malformed backend-association counts, and a
schema-drift regression test.
internal/database/migrations/100_relocate_storage_tier_protocol_remove_quota_test.go— backfill correctness for nested and legacy flat-shape rows in both
storage_tiersandarchived_storage_tiers, including the zero-backend default case, run against a real Postgresinstance.
it/it_public_storage_tiers_test.go— end-to-end List/Get, pagination,filtering (including the new protocol-forwarding path), the private-API permission boundary, and
documented BUG-005 pass-through behavior.
suite (
internal/..., 2000+ specs across two full runs) and repo-wide build/lint pass. The fulllocal integration suite could not be completed on the development machine (traced to a
Docker-Desktop-for-Mac-specific networking hang in an unrelated, pre-existing CLI-login test —
confirmed unrelated to this PR's code, since the identical test already passes in this PR's own
CI in ~13 minutes) — CI on this push is the source of truth for the integration suite.
Acceptance Criteria
StorageTiers/GetRPC implemented in fulfillment-serviceStorageTiers/ListRPC implemented in fulfillment-servicepublic.v1.StorageTiermessage, mirrors private minusbackend_idosac-operator/osac-aapportion is a separate follow-up PR, blocked until this PR merges and a newfulfillment-servicetag publishes a new Buf Schema Registry label thatosac-operatorcan regenerate against.protocolmoved fromBackendAssociationto the storage tier's own spec — fulfillment-service and osac-installer portions;osac-operator's copy is unaffected until the same follow-up PR (its own generated bindings are still pinned to the old shape, so it isn't broken by this change today)Summary by CodeRabbit
New Features
Changes