OSAC-1111: StorageBackend API - #728
openshift-merge-bot[bot] merged 9 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
@rgolangh: This pull request references [OSAC-1111](https://redhat.atlassian.net/browse/OS[AC-1](https://redhat.atlassian.net/browse/AC-1)111) 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. 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:
WalkthroughAdds a new ChangesPrivate StorageBackends API
Sequence Diagram(s)sequenceDiagram
participant Client
participant RESTGateway
participant PrivateStorageBackendsServer
participant GenericServer
participant DB
rect rgba(70, 130, 180, 0.5)
note over Client,DB: Create Flow
Client->>RESTGateway: POST /private/v1/storage_backends
RESTGateway->>PrivateStorageBackendsServer: Create(StorageBackendsCreateRequest)
PrivateStorageBackendsServer->>PrivateStorageBackendsServer: validateStorageBackendCreate (provider, endpoint, credentials)
PrivateStorageBackendsServer->>PrivateStorageBackendsServer: normalize (clear id, set READY, force SharedTenant)
PrivateStorageBackendsServer->>GenericServer: Create(ctx, object)
GenericServer->>DB: INSERT into storage_backends
GenericServer->>GenericServer: setPayload — clone & clear credentials.password for event
GenericServer-->>PrivateStorageBackendsServer: StorageBackend (sanitized)
PrivateStorageBackendsServer-->>Client: StorageBackendsCreateResponse
end
rect rgba(60, 179, 113, 0.5)
note over Client,DB: Update Flow
Client->>RESTGateway: PUT /private/v1/storage_backends/{id}
RESTGateway->>PrivateStorageBackendsServer: Update(StorageBackendsUpdateRequest)
PrivateStorageBackendsServer->>GenericServer: Get(ctx, id)
GenericServer->>DB: SELECT from storage_backends
PrivateStorageBackendsServer->>PrivateStorageBackendsServer: validateStorageBackendUpdate (provider immutability)
PrivateStorageBackendsServer->>GenericServer: Update(ctx, object, lock, update_mask)
GenericServer->>DB: UPDATE storage_backends
GenericServer-->>Client: StorageBackendsUpdateResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@rgolangh: This pull request references [OSAC-1111](https://redhat.atlassian.net/browse/OS[AC-1](https://redhat.atlassian.net/browse/AC-1)111) 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. 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. |
|
@rgolangh: This pull request references OSAC-1111 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. 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. |
a963885 to
3eb2559
Compare
| string endpoint = 5; | ||
|
|
||
| // Credentials for authenticating with the storage management API. | ||
| StorageBackendCredentials credentials = 6; |
There was a problem hiding this comment.
All these fields should be in a status message. The StorageBackend should only have id, metadata, spec and status.
There was a problem hiding this comment.
Agreed — I'll restructure to the id/metadata/spec/status pattern. The spec-level fields (provider, description, endpoint, credentials) will move into a StorageBackendSpec message. This is consistent with how Hub puts kubeconfig in HubSpec and User puts credentials in UserSpec.
There was a problem hiding this comment.
@rgolangh I thought you were going to update the StorageBackend proto to the id/metadata/spec/status pattern in this PR. Did you miss updating it? This will change the protobuf generated files.
There was a problem hiding this comment.
Done in the latest push — restructured StorageBackend to id/metadata/spec/status pattern. Fields provider, description, endpoint, and credentials are now in StorageBackendSpec, with StorageBackendStatus holding state and message. All server code, tests, and field-mask paths updated accordingly.
| // Storage provider identifier. For example: "vast", "ceph", "pure". | ||
| // | ||
| // This value is immutable once the StorageBackend is created. | ||
| string provider = 3; |
There was a problem hiding this comment.
This should be an enum type.
There was a problem hiding this comment.
The design doc (EP #60) intentionally chose string for extensibility — NFR-2 specifies "no provider-specific validation" so that new storage array vendors can be registered without proto/codegen changes. An enum would require a proto update and service redeploy for each new vendor.
That said, if we want to constrain to known providers, we could add a buf.build validation annotation with a regex or use a reference table. Would you prefer enum, or is the rationale for string acceptable?
There was a problem hiding this comment.
The rationale for string is acceptable.
| -- Name uniqueness among active backends (FR-9). Permits name reuse after deletion. | ||
| create unique index storage_backends_unique_active_name | ||
| on storage_backends (name) | ||
| where deletion_timestamp = 'epoch'; |
There was a problem hiding this comment.
If you are going to use the identifier as unique name, then you should make the (tenant, name) unique the primary key. And it shouldn't exclude the soft-deleted objects: it shouldn't be possible to create an object with the same name than an existing object, even if it is soft-deleted.
There was a problem hiding this comment.
Good point on name permanence — I'll remove the soft-delete exclusion from the unique index so names are permanently reserved (no reuse after deletion).
Regarding (tenant, name) as PK: StorageBackend uses auth.SharedTenant so tenant is always "shared", and the GenericDAO framework requires id as PK across all entities. I'll keep id as PK with a non-partial unique index on name to enforce permanent global uniqueness.
| create trigger check_immutable_columns | ||
| before update on storage_backends | ||
| for each row | ||
| execute function check_immutable_columns('id', 'name'); |
There was a problem hiding this comment.
The tenant should also be immutable.
There was a problem hiding this comment.
Agreed — adding tenant to the check_immutable_columns trigger.
| return nil | ||
| } | ||
|
|
||
| func cloneStorageBackend(sb *privatev1.StorageBackend) *privatev1.StorageBackend { |
There was a problem hiding this comment.
I'd say this can be inlined, no need for such a small function.
There was a problem hiding this comment.
Done — inlined it to proto.Clone(existingSB).(*privatev1.StorageBackend).
| } | ||
| if newSB.GetMetadata().GetTenant() != existingSB.GetMetadata().GetTenant() { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, | ||
| "field 'metadata.tenant' is immutable and cannot be changed") |
There was a problem hiding this comment.
This is better checked in the database trigger.
There was a problem hiding this comment.
You're right — the DB trigger already enforces name immutability via check_immutable_columns('id', 'name', 'tenant'). Removing the Go-level check.
| if newSB.GetMetadata().GetName() != existingSB.GetMetadata().GetName() { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, | ||
| "field 'metadata.name' is immutable and cannot be changed from '%s' to '%s'", | ||
| existingSB.GetMetadata().GetName(), newSB.GetMetadata().GetName()) |
There was a problem hiding this comment.
Let the database trigger check this.
There was a problem hiding this comment.
provider lives inside the JSONB data column, so check_immutable_columns can't enforce it (it only operates on top-level table columns). I can add a custom PL/pgSQL trigger that extracts and compares data->'provider' on UPDATE. Would you prefer that, or is the Go-level check acceptable for JSONB-embedded fields?
There was a problem hiding this comment.
I meant the name and tenant fields. Those aren't in the JSONB data column, and there is already a trigger function that you can add to your table to check immutability. It was defined here:
And an example of how to apply it here:
That trigger doesn't currently work for fields in the JSONB data column, so you can keep the check for provider in the application.
There was a problem hiding this comment.
Yes, understood — name and tenant immutability is already enforced by the check_immutable_columns('id', 'name', 'tenant') trigger in the migration. The server-side provider immutability check is only needed because provider lives inside the JSONB data column where the trigger can't reach.
| return proto.Clone(sb).(*privatev1.StorageBackend) | ||
| } | ||
|
|
||
| func applyStorageBackendUpdate(base, update *privatev1.StorageBackend, mask *fieldmaskpb.FieldMask) { |
There was a problem hiding this comment.
What is this trying to achieve? Isn't the regular Update method of the generic server enough?
There was a problem hiding this comment.
You're right — the generic server already handles field-mask based partial updates (via compilePaths + selective field application in GenericServer.Update). Removed applyStorageBackendUpdate, validateStorageBackendUpdateMask, and the clone+merge step. The Update method now just validates provider immutability and delegates to s.generic.Update().
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/servers/generic_server.go`:
- Around line 883-884: The StorageBackend object being passed to
event.SetStorageBackend() contains plaintext credentials in the
credentials.password field, which leaks sensitive data to notifier consumers.
Before calling event.SetStorageBackend(object) in the privatev1.StorageBackend
case, redact the credentials from a copy of the StorageBackend object by
clearing or removing the password field, then pass the redacted copy to the
event setter instead of the original object.
In `@internal/servers/private_storage_backends_server_test.go`:
- Around line 427-459: The Immutability test block needs explicit regression
coverage for masked metadata field updates that should be immutable. Add two new
test cases within the Immutability describe block following the pattern of the
"Update changing provider fails" test. First test should update metadata.name
with UpdateMask containing "metadata.name", and second test should update
metadata.tenant with UpdateMask containing "metadata.tenant". For each test,
create a storage backend, call server.Update with the respective masked field
update, and assert that the error code equals codes.InvalidArgument and the
error message contains both the field name and the word "immutable" to prevent
mask-path regressions.
In `@internal/servers/private_storage_backends_server.go`:
- Around line 163-171: The field-mask validation can be bypassed because
unsupported update_mask paths are silently ignored at line 224 but the original
request with those paths still gets persisted through s.generic.Update. To fix
this: (1) Add validation to reject any unsupported paths found in the
request.GetUpdateMask() before proceeding with the update, and (2) Ensure the
merged object passed to validateStorageBackend includes application of all
update_mask paths including metadata paths so that immutability checks in
validateStorageBackend validate against the actual state that will be written by
s.generic.Update.
In `@proto/private/osac/private/v1/storage_backends_service.proto`:
- Around line 71-73: Add documentation comments to the id fields in both
StorageBackendsGetRequest and StorageBackendsDeleteRequest messages. Each id
field should include a brief comment explaining that it represents the unique
identifier of the storage backend to retrieve or delete, respectively. This will
improve consistency with the well-documented fields in ListRequest and
UpdateRequest messages.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 4499a2d6-943e-4b06-82b1-651af8586901
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/storage_backends_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/migrations/57_create_storage_backends_tables.up.sqlinternal/servers/generic_server.gointernal/servers/private_storage_backends_server.gointernal/servers/private_storage_backends_server_test.goproto/private/osac/private/v1/event_type.protoproto/private/osac/private/v1/storage_backend_type.protoproto/private/osac/private/v1/storage_backends_service.proto
8af2c7f to
36c5c20
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/servers/private_storage_backends_server_test.go (1)
446-458:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen metadata immutability regression coverage (masked paths).
This block only checks
HaveOccurred()formetadata.nameand doesn’t cover masked updates formetadata.name/metadata.tenant. Please add explicitcodes.InvalidArgument+ message assertions for both masked paths so regressions in update-mask handling are caught.🤖 Prompt for 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. In `@internal/servers/private_storage_backends_server_test.go` around lines 446 - 458, The test "Update changing metadata.name fails at DB level" currently only verifies that an error occurred but doesn't validate the specific error code or message, leaving it vulnerable to regressions in update-mask handling. Replace the generic Expect(err).To(HaveOccurred()) assertion with explicit checks that verify the error code is codes.InvalidArgument and that the error message clearly indicates which metadata fields (metadata.name and metadata.tenant) are immutable and cannot be updated. Consider adding separate test cases or extending this test to cover both metadata.name and metadata.tenant masked paths to ensure comprehensive regression coverage.
🤖 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 `@internal/servers/private_storage_backends_server_test.go`:
- Around line 82-85: Replace all hardcoded credential values in the test file
with generated or redacted test fixtures. For each instance of the
StorageBackendCredentials_builder where Password fields contain literal values
like "secret" or "new-secret" (found at lines 82-85, 100-103, 236-239, 275-278,
295-298, 318-321, 336-339, 357-359, 377-379, 397-399, and 472-475), define test
fixture constants or use a helper function that generates consistent placeholder
credentials instead of embedding the password strings directly. This ensures the
code adheres to the security rule against hardcoding secrets, even in test code.
- Around line 498-525: The test for the stale version with lock=true in the
"Update with stale version and lock=true fails" test case only checks that an
error occurred using Expect(err).To(HaveOccurred()), but does not verify that
the error is specifically due to optimistic locking rejection. Replace the
generic error assertion with a gRPC status assertion that checks for the
expected error code (such as codes.FailedPrecondition) and optionally validates
the error message contains relevant text about version conflicts. This ensures
the test fails only for the intended stale-lock scenario, not for unrelated
errors.
In `@proto/private/osac/private/v1/storage_backends_service.proto`:
- Around line 57-69: The StorageBackendsListResponse is returning full
StorageBackend objects in the items field, which exposes sensitive credentials
including passwords. Create a separate response model (such as
StorageBackendResponse or StorageBackendSanitized) that excludes or redacts the
credentials field, and use this sanitized model for the items field in
StorageBackendsListResponse. Additionally, ensure that all other read response
messages (Get and Create/Update responses) also use this sanitized model instead
of the full StorageBackend to prevent credential exposure across all read APIs.
Keep credentials as write-only input in the request messages.
---
Duplicate comments:
In `@internal/servers/private_storage_backends_server_test.go`:
- Around line 446-458: The test "Update changing metadata.name fails at DB
level" currently only verifies that an error occurred but doesn't validate the
specific error code or message, leaving it vulnerable to regressions in
update-mask handling. Replace the generic Expect(err).To(HaveOccurred())
assertion with explicit checks that verify the error code is
codes.InvalidArgument and that the error message clearly indicates which
metadata fields (metadata.name and metadata.tenant) are immutable and cannot be
updated. Consider adding separate test cases or extending this test to cover
both metadata.name and metadata.tenant masked paths to ensure comprehensive
regression coverage.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: 2680e468-4299-4e41-a28d-c8b310060595
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/storage_backends_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/migrations/57_create_storage_backends_tables.up.sqlinternal/servers/generic_server.gointernal/servers/private_storage_backends_server.gointernal/servers/private_storage_backends_server_test.goproto/private/osac/private/v1/event_type.protoproto/private/osac/private/v1/storage_backend_type.protoproto/private/osac/private/v1/storage_backends_service.proto
| message StorageBackendsListResponse { | ||
| // Actual number of items returned. Note that this may be smaller than the value requested in the `limit` parameter | ||
| // of the request if there are not enough items, or if the system decides that returning that number of items isn't | ||
| // feasible or convenient for performance reasons. | ||
| int32 size = 1; | ||
|
|
||
| // Total number of items of the collection that match the search criteria, regardless of the number of results | ||
| // requested with the `limit` parameter. | ||
| int32 total = 2; | ||
|
|
||
| // List of results. | ||
| repeated StorageBackend items = 3; | ||
| } |
There was a problem hiding this comment.
Do not expose backend credentials in read response models.
List/Get/Create/Update responses return full StorageBackend, which includes credentials.password. That makes storage-management secrets retrievable via standard read APIs. Use a write-only credentials input (or secret reference) and return redacted/sanitized credential fields in read responses and emitted payloads.
Also applies to: 75-99
🤖 Prompt for 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.
In `@proto/private/osac/private/v1/storage_backends_service.proto` around lines 57
- 69, The StorageBackendsListResponse is returning full StorageBackend objects
in the items field, which exposes sensitive credentials including passwords.
Create a separate response model (such as StorageBackendResponse or
StorageBackendSanitized) that excludes or redacts the credentials field, and use
this sanitized model for the items field in StorageBackendsListResponse.
Additionally, ensure that all other read response messages (Get and
Create/Update responses) also use this sanitized model instead of the full
StorageBackend to prevent credential exposure across all read APIs. Keep
credentials as write-only input in the request messages.
There was a problem hiding this comment.
Credential redaction in read responses is tracked as a follow-up. The private API is admin-only and authenticated — the immediate risk is addressed by redacting credentials from event payloads (NOTIFY). Read-side redaction will be handled in a subsequent iteration.
There was a problem hiding this comment.
@rgolangh, understood — the admin-only scope and event-payload redaction do lower the immediate blast radius. Deferring the read-side sanitization is a reasonable trade-off for phase 1.
Would you like me to open a GitHub issue to track the read-side credential redaction work so it doesn't get lost?
🧠 Learnings used
Learnt from: tzvatot
Repo: osac-project/fulfillment-service PR: 517
File: proto/private/osac/private/v1/cluster_catalog_items_service.proto:105-106
Timestamp: 2026-05-11T12:42:06.581Z
Learning: In this repository, RPCs named `Signal` (e.g., `Clusters.Signal`, `ClusterTemplates.Signal`, `ComputeInstances.Signal`, `ClusterCatalogItems.Signal`, `ComputeInstanceCatalogItems.Signal`) are intentionally gRPC-only internal controller mechanisms and are not exposed via the REST gateway. During code review, do not flag these `Signal` RPCs for missing `google.api.http` annotations. Only require `google.api.http` bindings for non-`Signal` RPCs that are intended to be exposed through the REST gateway.
Learnt from: ygalblum
Repo: osac-project/fulfillment-service PR: 735
File: proto/private/osac/private/v1/compute_instances_service.proto:46-52
Timestamp: 2026-06-19T20:57:35.568Z
Learning: In this repository’s .proto files, treat `response_body: "object"` in the HTTP bindings for gRPC Create/Update as an intentional, cross-service architectural decision. During code review, do NOT flag additional response fields (e.g., `warnings`, other non-`object` fields) as “not accessible over REST” just because the HTTP binding returns only the primary `object`. Changing `response_body` for only individual RPCs would be inconsistent and risks breaking existing REST clients; if non-`object` fields must be exposed over REST, it should be done as a separate cross-cutting change across services.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@internal/database/migrations/58_create_storage_backends_tables.up.sql`:
- Around line 45-51: Remove the redundant non-unique index
storage_backends_by_name that is created on line 45, since the unique index
storage_backends_unique_name created on lines 50-51 already indexes the same
column and can serve both the purpose of enforcing uniqueness and being used for
lookup queries, eliminating unnecessary write and maintenance overhead.
In `@internal/servers/private_storage_backends_server_test.go`:
- Around line 110-131: The test is asserting that plaintext passwords are
returned in API responses from Create and Get operations, which creates a
cross-tenant credential exposure risk for this platform-scoped object. Remove
the assertion on line 130 that expects the password to be returned in the Get
response (the Expect statement checking obj.GetCredentials().GetPassword()).
Make credentials.password write-only by ensuring the field is accepted in write
requests but never returned in response payloads for Create, Get, Update, and
List operations. Apply the same changes to remove all similar password
assertions throughout the test file (also in test cases around lines 230-246 and
309-326).
In `@internal/servers/private_storage_backends_server.go`:
- Around line 160-165: The provider immutability validation in the
validateStorageBackendUpdate method does not account for the update_mask.paths
when determining if a field was explicitly changed. Currently, the check
`newSB.GetProvider() != ""` treats omitted fields and explicitly cleared fields
the same way. To fix this, modify the validation logic to first check if
"provider" is present in the request's update_mask.paths before validating
immutability. Only validate that the provider has not changed if the field is
explicitly included in the update_mask, ensuring that an explicitly cleared
provider field (empty string in the mask) is properly detected and validated as
an immutable field change.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: f6aacbd1-5e36-442f-a34b-d715726405e0
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/storage_backends_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/migrations/58_create_storage_backends_tables.up.sqlinternal/servers/generic_server.gointernal/servers/private_storage_backends_server.gointernal/servers/private_storage_backends_server_test.goproto/private/osac/private/v1/event_type.protoproto/private/osac/private/v1/storage_backend_type.protoproto/private/osac/private/v1/storage_backends_service.proto
a5abc9c to
c305f76
Compare
| version integer not null default 0 | ||
| ); | ||
|
|
||
| create index storage_backends_by_name on storage_backends (name); |
There was a problem hiding this comment.
@rgolangh Two things on this migration:
- The
storage_backends_by_nameindex is redundant. The unique indexstorage_backends_unique_nameon line 50 already provides the same btree lookup onname. - Can you add a down migration (
58_create_storage_backends_tables.down.sql) to drop the tables, indexes, and trigger?
There was a problem hiding this comment.
Both fixed in latest push:
- Removed the redundant
storage_backends_by_namenon-unique index — the unique index covers the same column. - Renumbered migration from 58 to 59 to avoid conflict with
58_rename_organizations_to_tenants.
| } | ||
|
|
||
| // Signals a storage backend for reconciliation. | ||
| rpc Signal(StorageBackendsSignalRequest) returns (StorageBackendsSignalResponse) { |
There was a problem hiding this comment.
Design doc and PRD both state "No Signal RPC — StorageBackend has no reconciler or controller."
There was a problem hiding this comment.
Good catch - The agent implemented this with a no-op signal. I will completely drop it from the service
There was a problem hiding this comment.
Correction: Signal RPC is kept in the proto because GenericServer.Build() hard-codes a lookup for the Signal method in the service descriptor — removing it causes a build failure. Added a comment explaining that Signal is required by the generic server infrastructure but returns UNIMPLEMENTED (via the embedded UnimplementedStorageBackendsServer). No actual Signal logic is implemented.
| func (s *PrivateStorageBackendsServer) validateStorageBackendCreate(_ context.Context, | ||
| sb *privatev1.StorageBackend) error { | ||
|
|
||
| if sb == nil { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, "storage backend is mandatory") | ||
| } | ||
| if sb.GetProvider() == "" { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'provider' is required") | ||
| } | ||
| if sb.GetEndpoint() == "" { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'endpoint' is required") | ||
| } | ||
| if sb.GetCredentials() == nil || sb.GetCredentials().GetUsername() == "" { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'credentials.username' is required") | ||
| } | ||
| if sb.GetCredentials().GetPassword() == "" { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'credentials.password' is required") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The design doc specifies that metadata.name is a required field on Create and missing name should return INVALID_ARGUMENT.
Should an explicit name check be added here maybe?
|
/retest |
|
🔴 CI Triage: Root cause: Database migration number collision: the PR adds migration 58, but migration 58 already exists in the main branch. Explanation: The PR introduces a new database migration file named Evidence:
Suggestion: Rename the migration file Prow job | Build For deeper investigation, use the |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/servers/private_storage_backends_server.go (1)
142-165: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate the merged post-mask object before
generic.Update.Line 160 validates only the sparse patch object, but Line 165 persists the merged result of
GenericServer.Update, which clears masked fields that are omitted from the request. That still lets callers blank required fields likespec.endpoint/spec.credentialsand clearspec.providerviaupdate_maskwith an empty value. CloneexistingSB, apply the mask to that clone, and run required-field plus immutability checks against the merged object before delegating.Also applies to: 203-211
🤖 Prompt for 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. In `@internal/servers/private_storage_backends_server.go` around lines 142 - 165, The Update flow in PrivateStorageBackendsServer currently validates only the incoming patch object, but generic.Update persists the merged post-mask result, so required fields can still be blanked out and immutable fields cleared via the update mask. In Update, after fetching existingSB, clone it, apply the request’s update mask to the clone, and run the required-field and immutability checks against that merged object before calling s.generic.Update; reuse validateStorageBackendUpdate (and the same pattern in the other affected update path) so the validation matches what will actually be persisted.
🤖 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 `@internal/database/migrations/59_create_storage_backends_tables.up.sql`:
- Line 1: The migration number is still conflicting because 59 is already taken
by an existing storage backends migration. Rename this migration to a free
sequential number and update both the .up.sql and matching .down.sql filenames
so the database migration runner sees a unique pair; use the storage backends
migration filenames as the reference point when choosing the new number.
In `@internal/servers/private_storage_backends_server_test.go`:
- Around line 205-256: Add regression coverage in the storage backend update
tests for masked updates that clear required fields: extend the `server.Update`
cases in `internal/servers/private_storage_backends_server_test.go` to use
`update_mask` against `spec.endpoint`, `spec.credentials`, and `spec.provider`,
and assert each returns `codes.InvalidArgument`. Reuse the existing
`createStorageBackend()` helper and
`privatev1.StorageBackendsUpdateRequest_builder` / `fieldmaskpb.FieldMask`
pattern so the new tests exercise the same `Update` path as the passing
partial-update cases. Make sure the assertions verify the server rejects these
masked clears instead of silently accepting them.
---
Duplicate comments:
In `@internal/servers/private_storage_backends_server.go`:
- Around line 142-165: The Update flow in PrivateStorageBackendsServer currently
validates only the incoming patch object, but generic.Update persists the merged
post-mask result, so required fields can still be blanked out and immutable
fields cleared via the update mask. In Update, after fetching existingSB, clone
it, apply the request’s update mask to the clone, and run the required-field and
immutability checks against that merged object before calling s.generic.Update;
reuse validateStorageBackendUpdate (and the same pattern in the other affected
update path) so the validation matches what will actually be persisted.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Enterprise
Run ID: c267339e-7494-408c-8541-fc5a07d68d89
⛔ Files ignored due to path filters (9)
internal/api/osac/private/v1/event_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/event_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/events_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backend_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/storage_backends_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/storage_backends_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/database/migrations/59_create_storage_backends_tables.up.sqlinternal/servers/generic_server.gointernal/servers/private_storage_backends_server.gointernal/servers/private_storage_backends_server_test.goproto/private/osac/private/v1/event_type.protoproto/private/osac/private/v1/storage_backend_type.protoproto/private/osac/private/v1/storage_backends_service.proto
| It("Update applies partial changes via field mask", func() { | ||
| created := createStorageBackend() | ||
|
|
||
| updateResponse, err := server.Update(ctx, privatev1.StorageBackendsUpdateRequest_builder{ | ||
| Object: privatev1.StorageBackend_builder{ | ||
| Id: created.GetId(), | ||
| Spec: privatev1.StorageBackendSpec_builder{ | ||
| Description: "Updated description", | ||
| }.Build(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.description"}}, | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(updateResponse.GetObject().GetSpec().GetDescription()).To(Equal("Updated description")) | ||
| Expect(updateResponse.GetObject().GetSpec().GetProvider()).To(Equal("vast")) | ||
| }) | ||
|
|
||
| It("Update endpoint", func() { | ||
| created := createStorageBackend() | ||
|
|
||
| updateResponse, err := server.Update(ctx, privatev1.StorageBackendsUpdateRequest_builder{ | ||
| Object: privatev1.StorageBackend_builder{ | ||
| Id: created.GetId(), | ||
| Spec: privatev1.StorageBackendSpec_builder{ | ||
| Endpoint: "https://new-storage.example.com:9443", | ||
| }.Build(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.endpoint"}}, | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(updateResponse.GetObject().GetSpec().GetEndpoint()).To(Equal("https://new-storage.example.com:9443")) | ||
| }) | ||
|
|
||
| It("Update credentials", func() { | ||
| created := createStorageBackend() | ||
|
|
||
| updateResponse, err := server.Update(ctx, privatev1.StorageBackendsUpdateRequest_builder{ | ||
| Object: privatev1.StorageBackend_builder{ | ||
| Id: created.GetId(), | ||
| Spec: privatev1.StorageBackendSpec_builder{ | ||
| Credentials: privatev1.StorageBackendCredentials_builder{ | ||
| Username: "new-admin", | ||
| Password: "new-secret", | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build(), | ||
| UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"spec.credentials"}}, | ||
| }.Build()) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(updateResponse.GetObject().GetSpec().GetCredentials().GetUsername()).To(Equal("new-admin")) | ||
| Expect(updateResponse.GetObject().GetSpec().GetCredentials().GetPassword()).To(Equal("new-secret")) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression cases for masked updates that clear required fields.
This suite only asserts create-time required-field validation. Add Update cases that use update_mask to clear spec.endpoint, spec.credentials, and spec.provider, and assert codes.InvalidArgument; otherwise the current server bug can ship without a failing test.
Also applies to: 344-451
🤖 Prompt for 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.
In `@internal/servers/private_storage_backends_server_test.go` around lines 205 -
256, Add regression coverage in the storage backend update tests for masked
updates that clear required fields: extend the `server.Update` cases in
`internal/servers/private_storage_backends_server_test.go` to use `update_mask`
against `spec.endpoint`, `spec.credentials`, and `spec.provider`, and assert
each returns `codes.InvalidArgument`. Reuse the existing
`createStorageBackend()` helper and
`privatev1.StorageBackendsUpdateRequest_builder` / `fieldmaskpb.FieldMask`
pattern so the new tests exercise the same `Update` path as the passing
partial-update cases. Make sure the assertions verify the server rejects these
masked clears instead of silently accepting them.
|
🔴 CI Triage: Root cause: The PR introduces a database migration with version 59, but migration 59 is already taken by a recently merged PR, causing a duplicate migration file error during grpc-server startup. Explanation: During the boot step's refresh phase, the Evidence:
Suggestion: Rename the migration file Prow job | Build For deeper investigation, use the |
Add StorageBackend, StorageBackendCredentials, StorageBackendStatus, and StorageBackendState proto messages under osac.private.v1. Define the StorageBackends service with List, Get, Create, Update, Delete RPCs and HTTP annotations for REST gateway. Add storage_backend payload field (30) to Event oneof. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Create storage_backends and archived_storage_backends tables with the standard generic DAO schema. Add indexes for name, creator, and labels. Add unique partial index on name for active records (FR-9). Add immutability trigger for id and name columns. No tenant FK or tenant index — StorageBackend is platform-scoped with tenant always set to 'shared'. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Add PrivateStorageBackendsServer following the NetworkClass pattern: - Builder with SetLogger, SetNotifier, SetAttributionLogic, SetTenancyLogic, SetMetricsRegisterer - Create validates required fields, sets state=READY, forces tenant="shared" - Update with field mask merge and immutability checks (provider, metadata.name, metadata.tenant) - Delete delegates to generic server - Add setPayload case for StorageBackend events in generic_server.go - Register in gRPC server and REST gateway - Add Signal RPC to proto (required by GenericServer framework, not implemented) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Cover CRUD lifecycle, validation, immutability, name uniqueness, optimistic locking, tenant enforcement, ID generation, and state handling with 27 test cases using the shared Ginkgo test suite. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
- Remove applyStorageBackendUpdate — generic server handles field masks - Split validation into create/update functions - Add tenant to DB immutable columns trigger - Redact credentials password in event payload - Add metadata.name and metadata.tenant update_mask tests - Simplify unique index (delete archives rows, so no partial needed) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Restructure StorageBackend proto to use spec/status pattern, add metadata.name validation, remove redundant index, and add Signal RPC comment. - Restructure StorageBackend proto: move provider, description, endpoint, credentials into StorageBackendSpec message with separate StorageBackendStatus (per Juan's review) - Add metadata.name required validation in Create - Remove redundant non-unique storage_backends_by_name index (unique index already covers it) - Renumber migration from 58 to 59 to avoid conflict - Update credential redaction in generic_server.go for spec path - Add comment on Signal RPC explaining it's required by generic server infrastructure but returns UNIMPLEMENTED - Update all tests for spec/status field paths Signed-off-by: Roy Golan <rgolan@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
Migrations 59-61 were merged to main since our branch was created. Renumber storage_backends migration from 59 to 62. Signed-off-by: Roy Golan <rgolan@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
48cc6ae to
4471005
Compare
| Expect(st.Code()).To(Equal(codes.AlreadyExists)) | ||
| }) | ||
|
|
||
| It("Create after delete of same name succeeds", func() { |
| create index storage_backends_by_creator on storage_backends (creator); | ||
| create index storage_backends_by_label on storage_backends using gin (labels); | ||
|
|
||
| -- Name uniqueness across all backends (active and soft-deleted). Names are permanently reserved. |
There was a problem hiding this comment.
|
/approve |
|
/hold |
The storage_backends table was created after migration 49 (which bulk-added tenant foreign keys), so it needed an explicit tenant_fk constraint. Also adds the required migration test file covering table creation, tenant FK enforcement, name uniqueness, and column immutability. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
f35b284 to
cedb35d
Compare
|
/lgtm /unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rccrdpccl, rgolangh, 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 |
7 participants
StorageBackend API
Story type: [DEV]
Design: EP #60
Summary
Implements the StorageBackend private gRPC API for managing storage array registrations in the fulfillment-service. This is phase 1 of the StorageBackend feature, following the NetworkClass pattern: DB-backed entity with full CRUD, no CRD, no reconciler, no public API.
Storage backends are platform-wide infrastructure managed by Cloud Provider Admins — they are not per-tenant resources. The server forces
auth.SharedTenanton all StorageBackend objects, following the same pattern as NetworkClass and InstanceType.Design Decisions
auth.SharedTenantmakes them visible to all authenticated users through the existing tenancy filter, matching how NetworkClass handles platform-scoped entities.UnimplementedStorageBackendsServerreturnscodes.Unimplemented. Signal will be needed in phase 0.2 for state transition reconciliation.Changes
Proto definitions:
storage_backend_type.proto— StorageBackend, StorageBackendCredentials, StorageBackendStatus messages; StorageBackendState enum (UNSPECIFIED, READY)storage_backends_service.proto— List/Get/Create/Update/Delete RPCs with HTTP annotations for REST gateway, plus Signal RPC (gRPC-only, no HTTP annotation)event_type.proto— AddedStorageBackend storage_backend = 30to event payload oneofDatabase:
storage_backends+archived_storage_backendstables with standard generic schema columns, indexes (by_name, by_creator, by_label GIN), unique active name constraint (permits reuse after deletion), immutability trigger on id/nameServer implementation:
private_storage_backends_server.go— Builder pattern, CRUD with validation (required: provider, endpoint, credentials.username, credentials.password), immutability enforcement (provider, metadata.name, metadata.tenant), state forced to READY on create, caller-provided ID clearedgeneric_server.go— Added StorageBackend case tosetPayload()switchRegistration:
start_grpc_server_cmd.go— Private StorageBackends server registrationstart_rest_gateway_cmd.go— REST gateway handler registrationReviewer Focus Areas
check_immutable_columns('id', 'name')) and server validation (validateStorageBackend) enforce immutability — verify both layers are consistent.auth.SharedTenant; verify no path allows a caller to set a different tenant.provider,endpoint,credentials.username,credentials.password. Verify Update'sapplyStorageBackendUpdatefield mask paths are complete.Testing
private_storage_backends_server_test.gocovering CRUD lifecycle, pagination, filtering, ordering, field mask updates, validation, immutability, name uniqueness, name reuse after delete, optimistic locking, UUID generation, state enforcement, tenant enforcementDeferred to Phase 0.2
Acceptance Criteria
CreateStorageBackendcreates a backend with stateREADYand returns the created object with a generated IDGetStorageBackendretrieves a backend by ID with all fields populatedListStorageBackendsreturns paginated results and supports filtering by field valuesUpdateStorageBackendapplies partial updates without modifying unspecified fieldsUpdateStorageBackendrejects concurrent conflicting writes (optimistic locking)DeleteStorageBackendpermanently deletes the backend (StorageTier ref check deferred to phase 0.2)Summary by CodeRabbit
New Features
Bug Fixes