MGMT-23734: implement PublicIP gRPC servers (private + public) - #428
openshift-merge-bot[bot] merged 7 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
@akshaynadkarni: This pull request references MGMT-23734 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 story 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. |
|
/hold |
akshaynadkarni
changed the title
WalkthroughAdds PublicIPs support end-to-end: new private-side Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/servers/private_public_ips_server.go (1)
43-48: Unusedloggerfield onPrivatePublicIPsServer.The
loggerfield is stored but never referenced in any method (all operations delegate tos.generic, andvalidatePublicIPonly uses gRPC status errors). You can drop it from the struct and the builder assignment to reduce surface area, or use it for structured logging at the handler entry points if that is desired going forward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ips_server.go` around lines 43 - 48, The PrivatePublicIPsServer struct currently defines a logger field that is never used; remove the unused logger field from the PrivatePublicIPsServer type and any assignment to it in the server builder/constructor, or alternatively add structured entry/exit logs in each RPC handler (e.g., methods on PrivatePublicIPsServer that delegate to g.generic) and in validatePublicIP so the logger is actually referenced; update the constructor/builder that sets logger and any imports accordingly to avoid unused variable/compiler errors.internal/servers/public_ips_server.go (1)
31-52: Missing godoc on exported types/functions.
PublicIPsServerBuilder,PublicIPsServer,NewPublicIPsServer, andBuildlack godoc comments, whereas the siblingprivate_public_ips_server.go(e.g.,PrivatePublicIPsServerBuilder,PrivatePublicIPsServer,NewPrivatePublicIPsServer) documents each of them. Please add matching comments for consistency and to satisfyrevive/golint-style checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/public_ips_server.go` around lines 31 - 52, Add godoc-style comments for all exported symbols in this file to match the sibling private implementation: document PublicIPsServerBuilder, PublicIPsServer, NewPublicIPsServer, and the Build method on PublicIPsServerBuilder (if present) with a one-line summary followed by optional details; place the comment immediately above each declaration (e.g., above type PublicIPsServerBuilder, type PublicIPsServer, func NewPublicIPsServer and func (PublicIPsServerBuilder) Build) and use the same tone/structure used by PrivatePublicIPsServerBuilder/PrivatePublicIPsServer/NewPrivatePublicIPsServer in the sibling file so linters (revive/golint) are satisfied.internal/servers/private_public_ips_server_test.go (1)
267-268: UseSetNamefor consistency with other metadata mutations in the codebase.Line 268 directly assigns to the metadata field:
object.GetMetadata().Name = "updated-name". While this works, the codebase uses theSetName()method elsewhere for metadata updates (e.g.,generic_dao_test.go:1317), particularly in database operations. Useobject.GetMetadata().SetName("updated-name")to align with the established pattern.♻️ Proposed change
- // Update the name: - object.GetMetadata().Name = "updated-name" + // Update the name: + object.GetMetadata().SetName("updated-name")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ips_server_test.go` around lines 267 - 268, The test mutates metadata by assigning to the Name field directly; replace the direct assignment object.GetMetadata().Name = "updated-name" with the metadata setter call object.GetMetadata().SetName("updated-name") to match the codebase pattern used elsewhere (e.g., generic_dao_test.go) and keep metadata mutations consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/servers/public_ips_server.go`:
- Around line 66-76: The docstring is misleading: attribution is treated as
mandatory by GenericServer.Build but SetAttributionLogic on
PublicIPsServerBuilder is documented as optional; update
PublicIPsServerBuilder.Build (and PrivatePublicIPsServer.Build if present) to
validate b.attributionLogic and return a clear error early (e.g., "attribution
logic is mandatory for PublicIPsServer") before constructing
PrivatePublicIPsServer/GenericServer, or alternatively change the
SetAttributionLogic docstring to mark it required; locate SetAttributionLogic,
PublicIPsServerBuilder.Build, PrivatePublicIPsServer.SetAttributionLogic, and
GenericServer.Build to implement the chosen fix so callers receive a focused
failure message tied to this server.
---
Nitpick comments:
In `@internal/servers/private_public_ips_server_test.go`:
- Around line 267-268: The test mutates metadata by assigning to the Name field
directly; replace the direct assignment object.GetMetadata().Name =
"updated-name" with the metadata setter call
object.GetMetadata().SetName("updated-name") to match the codebase pattern used
elsewhere (e.g., generic_dao_test.go) and keep metadata mutations consistent.
In `@internal/servers/private_public_ips_server.go`:
- Around line 43-48: The PrivatePublicIPsServer struct currently defines a
logger field that is never used; remove the unused logger field from the
PrivatePublicIPsServer type and any assignment to it in the server
builder/constructor, or alternatively add structured entry/exit logs in each RPC
handler (e.g., methods on PrivatePublicIPsServer that delegate to g.generic) and
in validatePublicIP so the logger is actually referenced; update the
constructor/builder that sets logger and any imports accordingly to avoid unused
variable/compiler errors.
In `@internal/servers/public_ips_server.go`:
- Around line 31-52: Add godoc-style comments for all exported symbols in this
file to match the sibling private implementation: document
PublicIPsServerBuilder, PublicIPsServer, NewPublicIPsServer, and the Build
method on PublicIPsServerBuilder (if present) with a one-line summary followed
by optional details; place the comment immediately above each declaration (e.g.,
above type PublicIPsServerBuilder, type PublicIPsServer, func NewPublicIPsServer
and func (PublicIPsServerBuilder) Build) and use the same tone/structure used by
PrivatePublicIPsServerBuilder/PrivatePublicIPsServer/NewPrivatePublicIPsServer
in the sibling file so linters (revive/golint) are satisfied.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62837b85-23b9-48b8-93d8-416585649e05
⛔ Files ignored due to path filters (2)
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.go
📒 Files selected for processing (8)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/servers/generic_server.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server.gointernal/servers/public_ips_server_test.goproto/private/osac/private/v1/event_type.proto
GenericServer.Build() requires attribution logic but the PublicIP server builders did not check for it, producing a confusing error from the wrong layer. Fail fast with a clear message in both PrivatePublicIPsServerBuilder and PublicIPsServerBuilder, and update docstrings to reflect that SetAttributionLogic is mandatory. Addresses CodeRabbit review feedback on PR osac-project#428. Assisted-by: Cursor/Claude
GenericServer.Build() requires attribution logic but the PublicIP server builders did not check for it, producing a confusing error from the wrong layer. Fail fast with a clear message in both PrivatePublicIPsServerBuilder and PublicIPsServerBuilder, and update docstrings to reflect that SetAttributionLogic is mandatory. Addresses CodeRabbit review feedback on PR osac-project#428. Assisted-by: Cursor/Claude
b3db11f to
350bab5
Compare
akshaynadkarni
changed the title
350bab5 to
74e4640
Compare
GenericServer.Build() requires attribution logic but the PublicIP server builders did not check for it, producing a confusing error from the wrong layer. Fail fast with a clear message in both PrivatePublicIPsServerBuilder and PublicIPsServerBuilder, and update docstrings to reflect that SetAttributionLogic is mandatory. Addresses CodeRabbit review feedback on PR osac-project#428. Assisted-by: Cursor/Claude
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/servers/private_public_ips_server.go (1)
30-182: LGTM — builder, delegation, and pool-required validation are correctly implemented.Mandatory-field checks in
Build()match the test suite, andCreatevalidation returnsInvalidArgumentwith clear messages. Heavier validation (pool existence, state machine, immutability) deferred to MGMT-23732 is acknowledged in the PR objectives.Minor optional note: the
ctxparameter ofvalidatePublicIPis currently unused. Keeping it is fine if you plan to add DB-aware validation in the follow-up; otherwise it can be dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ips_server.go` around lines 30 - 182, The validatePublicIP method currently takes an unused ctx parameter; either keep it for future DB-aware checks or remove it to avoid unused parameter warnings—if removing, change the signature of validatePublicIP(ctx context.Context, publicIP *privatev1.PublicIP) to validatePublicIP(publicIP *privatev1.PublicIP) and update the caller in Create to call s.validatePublicIP(publicIP) (adjust the method reference in Create and any other callers), run `go vet`/tests to ensure no remaining references.internal/servers/public_ips_server.go (1)
31-272: LGTM — delegation via strict/lenient mappers is cleanly implemented and the past attribution-docstring issue is resolved.Builder validation matches the private server's contract, strict
inMapperprotects against silently dropping public-only fields on write, and non-strictoutMappertolerates private-only fields on read. Nil-object guard inCreateand gRPC status codes are appropriate.Minor note on
Create(lines 237-249): ifoutMapper.Copyfails after the privateCreatehas already persisted the object, the caller receivesInternalwhile the object exists server-side. This is acceptable (a subsequentGetwill retrieve it), but it means an out-mapper regression would surface as a transient-looking 5xx on successful writes. Consider a test that exercises the post-create mapping path, or alerting/logging on this specificErrorContextcall if/when you add observability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/public_ips_server.go` around lines 31 - 272, Create currently persists the object via s.delegate.Create and then calls s.outMapper.Copy in PublicIPsServer.Create; if that copy fails the client gets an Internal error even though the resource exists. Add a unit/integration test exercising the post-create mapping path (simulate outMapper.Copy failure after delegate.Create succeeds) to ensure behavior is acceptable, and/or augment the error handling in PublicIPsServer.Create: on outMapper.Copy failure log an explicit, high-visibility message (use s.logger.ErrorContext in the Create method referencing the failed outMapper.Copy) that includes the created object's ID/details and the error so operators can correlate the transient 5xx with the persisted resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/servers/private_public_ips_server.go`:
- Around line 30-182: The validatePublicIP method currently takes an unused ctx
parameter; either keep it for future DB-aware checks or remove it to avoid
unused parameter warnings—if removing, change the signature of
validatePublicIP(ctx context.Context, publicIP *privatev1.PublicIP) to
validatePublicIP(publicIP *privatev1.PublicIP) and update the caller in Create
to call s.validatePublicIP(publicIP) (adjust the method reference in Create and
any other callers), run `go vet`/tests to ensure no remaining references.
In `@internal/servers/public_ips_server.go`:
- Around line 31-272: Create currently persists the object via s.delegate.Create
and then calls s.outMapper.Copy in PublicIPsServer.Create; if that copy fails
the client gets an Internal error even though the resource exists. Add a
unit/integration test exercising the post-create mapping path (simulate
outMapper.Copy failure after delegate.Create succeeds) to ensure behavior is
acceptable, and/or augment the error handling in PublicIPsServer.Create: on
outMapper.Copy failure log an explicit, high-visibility message (use
s.logger.ErrorContext in the Create method referencing the failed
outMapper.Copy) that includes the created object's ID/details and the error so
operators can correlate the transient 5xx with the persisted resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf7e889d-0030-496c-8fd8-46ed3a0a7632
⛔ Files ignored due to path filters (2)
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.go
📒 Files selected for processing (8)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/servers/generic_server.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server.gointernal/servers/public_ips_server_test.goproto/private/osac/private/v1/event_type.proto
✅ Files skipped from review due to trivial changes (1)
- proto/private/osac/private/v1/event_type.proto
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/servers/generic_server.go
- internal/servers/public_ips_server_test.go
- internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
Add PublicIP as field 16 in the Event oneof payload block so the notifier can deliver change events for PublicIP resources. Add the corresponding case in GenericServer.setPayload to route PublicIP objects to Event.SetPublicIp. Assisted-by: Cursor/Claude
…ation Implement PrivatePublicIPsServer following the established GenericServer pattern (matching PrivateLeasesServer). The server delegates all six RPCs (List, Get, Create, Update, Delete, Signal) to GenericServer and adds pool-required validation on Create: rejects nil object, nil spec, and empty spec.pool with InvalidArgument. Pool existence checks, state machine validation, capacity tracking, delete constraints, and immutability enforcement are deferred to Phase 5 per project plan. Assisted-by: Cursor/Claude
PublicIPsServer implements 4 RPCs (List, Get, Create, Delete) and delegates all operations to PrivatePublicIPsServer via GenericMapper. The inMapper (public to private) uses strict mode to reject unknown fields from public input. The outMapper (private to public) uses non-strict mode so private-only fields like hub are dropped silently. Assisted-by: Cursor/Claude
Both public and private PublicIP servers are now registered in the gRPC server startup alongside existing resources (subnets, security groups). The public server uses DefaultAttributionLogic and DefaultTenancyLogic for tenant-scoped access. The private server uses SystemAttributionLogic and SystemTenancyLogic for controller/admin access. REST gateway handlers are registered for both public and private APIs, enabling HTTP/JSON access to PublicIP resources. Assisted-by: Cursor/Claude
Cover builder validation (logger, tenancy logic mandatory), pool-required validation (nil object, nil spec, empty pool), and full CRUD operations (create with ID generation, get by ID, list, update, soft delete) using the transaction-per-test pattern with GenericDAO. 12 test cases following the PrivateLeasesServer test pattern, exercising the validatePublicIP method and GenericServer delegation. Assisted-by: Cursor/Claude
Cover builder validation (logger, tenancy logic mandatory) and CRUD operations via public API delegation: create with ID generation, list all/with limit/with offset/with filter, get by ID with proto equality, and soft delete with finalizer-based verification. 10 test cases following the SubnetsServer test pattern, confirming the public-to-private mapping works end-to-end through GenericMapper. No Update test since the public server does not expose Update. Assisted-by: Cursor/Claude
GenericServer.Build() requires attribution logic but the PublicIP server builders did not check for it, producing a confusing error from the wrong layer. Fail fast with a clear message in both PrivatePublicIPsServerBuilder and PublicIPsServerBuilder, and update docstrings to reflect that SetAttributionLogic is mandatory. Addresses CodeRabbit review feedback on PR osac-project#428. Assisted-by: Cursor/Claude
74e4640 to
702f2af
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/servers/private_public_ips_server.go (2)
73-77: Docstring inconsistency:SetTenancyLogicis required, not optional.
Build()at lines 93-96 returns"tenancy logic is mandatory"whentenancyLogicis nil, but this docstring omits that. For consistency withSetLogger(line 55) andSetAttributionLogic(line 67), mark it mandatory.📝 Proposed fix
-// SetTenancyLogic sets the tenancy logic that will be used to determine the tenants for objects. +// SetTenancyLogic sets the tenancy logic that will be used to determine the tenants for objects. This is mandatory. func (b *PrivatePublicIPsServerBuilder) SetTenancyLogic(value auth.TenancyLogic) *PrivatePublicIPsServerBuilder {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ips_server.go` around lines 73 - 77, The docstring for SetTenancyLogic is misleading because tenancyLogic is required by Build; update the comment on PrivatePublicIPsServerBuilder.SetTenancyLogic to mark it mandatory (like SetLogger and SetAttributionLogic) and state that Build will return an error ("tenancy logic is mandatory") if tenancyLogic is nil; refer to the SetTenancyLogic method and the Build method on PrivatePublicIPsServerBuilder when making the docstring change.
167-182: Nit:ctxis accepted but never used.
validatePublicIPnever readsctx. Either drop the parameter or use it (e.g., for structured logging of rejections). Low priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ips_server.go` around lines 167 - 182, The validatePublicIP function currently accepts a ctx parameter that is unused; remove the unused ctx parameter from validatePublicIP's signature and all internal callers (or alternatively, use ctx for structured logging of validation failures via the logger available in the caller), updating references to the function accordingly; specifically modify the validatePublicIP method on PrivatePublicIPsServer (and any calls to s.validatePublicIP) to either drop ctx or to pass it through to a logging call that records rejection details.internal/servers/public_ips_server.go (2)
31-52: Missing doc comments on exported symbols.
PublicIPsServerBuilder,PublicIPsServer,NewPublicIPsServer,Build, and the gRPC methods (List,Get,Create,Delete) lack doc comments. The setters are documented and the siblingPrivatePublicIPsServer(lines 30, 41, 50, 86) documents its exported symbols — worth matching that forgo doc/lint consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/public_ips_server.go` around lines 31 - 52, Add Go doc comments for all exported symbols related to the public IPs server to satisfy lint/docs: add package-style one-line plus optional sentence comments above the types PublicIPsServerBuilder and PublicIPsServer, the constructor NewPublicIPsServer, the builder Build method, and each exported gRPC method names (List, Get, Create, Delete) on PublicIPsServer; follow the same wording/style used by the sibling PrivatePublicIPsServer comments (brief description of purpose and any notable behavior) so go doc and linters are happy.
116-126: Unnecessary dual instantiation of PrivatePublicIPsServer and its DAO.
PublicIPsServer.Build()internally constructs its ownPrivatePublicIPsServer(lines 116-126), whilestart_grpc_server_cmd.goalso builds a standalonePrivatePublicIPsServerindependently (lines 706-718). Both receive the samemetricsRegisterer.While Prometheus metric registration is idempotent—the shared
operation_durationhistogram reuses existing collectors on duplicate registration—this still results in two independentGenericDAOinstances for the samePublicIPtable. Each DAO maintains its own notifier subscription and database resources, causing unnecessary duplication.Consider accepting a pre-built
privatev1.PublicIPsServerdelegate viaSetDelegate(...)on the public server builder, or omitmetricsRegistererfrom the internal delegate build so only the standalone private server owns registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/public_ips_server.go` around lines 116 - 126, PublicIPsServer.Build() currently constructs a new PrivatePublicIPsServer via NewPrivatePublicIPsServer() causing duplicate GenericDAO/notifier and metric registration; change the PublicIPsServer builder to accept an optional pre-built privatev1.PublicIPsServer via SetDelegate(delegate) and, if provided, use that delegate instead of calling NewPrivatePublicIPsServer() in Build(); alternatively (if adding SetDelegate is not desired) avoid passing the shared metricsRegisterer into the internally-constructed delegate (remove SetMetricsRegisterer(b.metricsRegisterer) when building the internal PrivatePublicIPsServer) so only the standalone private server registers the collector and only one DAO/notifier is instantiated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/servers/private_public_ips_server.go`:
- Around line 73-77: The docstring for SetTenancyLogic is misleading because
tenancyLogic is required by Build; update the comment on
PrivatePublicIPsServerBuilder.SetTenancyLogic to mark it mandatory (like
SetLogger and SetAttributionLogic) and state that Build will return an error
("tenancy logic is mandatory") if tenancyLogic is nil; refer to the
SetTenancyLogic method and the Build method on PrivatePublicIPsServerBuilder
when making the docstring change.
- Around line 167-182: The validatePublicIP function currently accepts a ctx
parameter that is unused; remove the unused ctx parameter from
validatePublicIP's signature and all internal callers (or alternatively, use ctx
for structured logging of validation failures via the logger available in the
caller), updating references to the function accordingly; specifically modify
the validatePublicIP method on PrivatePublicIPsServer (and any calls to
s.validatePublicIP) to either drop ctx or to pass it through to a logging call
that records rejection details.
In `@internal/servers/public_ips_server.go`:
- Around line 31-52: Add Go doc comments for all exported symbols related to the
public IPs server to satisfy lint/docs: add package-style one-line plus optional
sentence comments above the types PublicIPsServerBuilder and PublicIPsServer,
the constructor NewPublicIPsServer, the builder Build method, and each exported
gRPC method names (List, Get, Create, Delete) on PublicIPsServer; follow the
same wording/style used by the sibling PrivatePublicIPsServer comments (brief
description of purpose and any notable behavior) so go doc and linters are
happy.
- Around line 116-126: PublicIPsServer.Build() currently constructs a new
PrivatePublicIPsServer via NewPrivatePublicIPsServer() causing duplicate
GenericDAO/notifier and metric registration; change the PublicIPsServer builder
to accept an optional pre-built privatev1.PublicIPsServer via
SetDelegate(delegate) and, if provided, use that delegate instead of calling
NewPrivatePublicIPsServer() in Build(); alternatively (if adding SetDelegate is
not desired) avoid passing the shared metricsRegisterer into the
internally-constructed delegate (remove
SetMetricsRegisterer(b.metricsRegisterer) when building the internal
PrivatePublicIPsServer) so only the standalone private server registers the
collector and only one DAO/notifier is instantiated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce3e161f-ffa7-4f08-823d-f1db31fbfc6b
⛔ Files ignored due to path filters (2)
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.go
📒 Files selected for processing (8)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/servers/generic_server.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server.gointernal/servers/public_ips_server_test.goproto/private/osac/private/v1/event_type.proto
✅ Files skipped from review due to trivial changes (2)
- internal/servers/generic_server.go
- internal/servers/public_ips_server_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cmd/service/start/restgateway/start_rest_gateway_cmd.go
- internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go
- internal/servers/private_public_ips_server_test.go
- proto/private/osac/private/v1/event_type.proto
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, akshaynadkarni, DakCrowder 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 |
5 participants
Summary
Implements the private and public PublicIP gRPC servers for MGMT-23734
(subtasks MGMT-23898 and MGMT-23899). The private server provides full
CRUD + Signal for admin use, while the public server exposes
tenant-scoped Create/Get/List/Delete. Both are registered in the gRPC
startup and REST gateway.
This PR also adds the
public_ippayload toevent_type.protoandthe corresponding
setPayloadcase ingeneric_server.go, so thenotifier handles PublicIP CRUD events correctly.
Basic validation is included (pool field required on Create). Heavier
validation (pool existence, state machine, capacity tracking) is
deferred to a follow-up PR once the PublicIPPool server/DAO is
available (MGMT-23732).
Testing
New test coverage:
private_public_ips_server_test.go: 13 tests (builder validationincluding attribution logic, pool-required on Create, CRUD operations)
public_ips_server_test.go: 11 tests (builder validation includingattribution logic, CRUD delegation, tenant isolation via TenancyLogic)
Ticket
MGMT-23734
Subtasks covered:
Assisted-by: Cursor/Claude
Summary by CodeRabbit
New Features
Bug Fixes / Validation