MGMT-23732/MGMT-23759: define PublicIPPool proto type and service - #392
Conversation
|
Skipping CI for Draft Pull Request. |
|
@akshaynadkarni: This pull request references MGMT-23759 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 sub-task to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@akshaynadkarni: This pull request references MGMT-23759 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 sub-task to target the "4.22.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. |
|
@akshaynadkarni: This pull request references MGMT-23759 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 sub-task to target the "4.22.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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 12 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
WalkthroughAdds two new protobuf files in package Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proto/private/osac/private/v1/public_ip_pool_type.proto`:
- Around line 115-127: Fields total, allocated, and available in
public_ip_pool_type.proto use int32 and will overflow for IPv6-sized pools;
change their types to int64 (or sint64/uint64 if unsigned semantics are desired)
for the fields `total`, `allocated`, and `available` in the message defined in
public_ip_pool_type.proto (the three field names exactly) so they can represent
large IPv6 CIDR capacities, and then regenerate any protobuf stubs and update
any consumer code that assumes 32-bit values to use 64-bit integers.
🪄 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: 58de3620-ae2c-47aa-b0a5-198955b709de
⛔ Files ignored due to path filters (6)
internal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/private/v1/public_ip_pools_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pools_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
proto/private/osac/private/v1/public_ip_pool_type.protoproto/private/osac/private/v1/public_ip_pools_service.proto
|
/hold |
Code Review: PublicIPPool Proto Definition✅ Overall AssessmentExcellent proto design that follows established patterns. The implementation is clean, well-documented, and consistent with existing networking resources (VirtualNetwork, Subnet, SecurityGroup). Strengths:
🔴 Critical Issue: Integer Overflow Risk for IPv6 PoolsFile: Problem: The capacity fields use int32 total = 4;
int32 allocated = 5;
int32 available = 6;Impact: A single IPv6 /64 CIDR contains 2^64 addresses (~18.4 quintillion), which exceeds Recommended Fix: int64 total = 4; // Changed from int32
int64 allocated = 5; // Changed from int32
int64 available = 6; // Changed from int32Action Items:
📝 Note on Proto ConsistencyThis issue also exists in the osac-operator CRD ( Recommendation: Address the int64 change before merge, as it's a breaking schema change that's easier to fix now than after the BSR tag is published and consumed by osac-operator. |
|
@akshaynadkarni: This pull request references MGMT-23759 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 sub-task to target the "4.22.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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proto/private/osac/private/v1/public_ip_pool_type.proto (1)
42-43: Addgoogle.api.field_behavior = OUTPUT_ONLYannotation to thestatusfield.The
statusfield is documented as read-only but lacks the explicitOUTPUT_ONLYannotation used elsewhere in this file (e.g., onimplementation_strategy). This annotation clarifies the read-only contract for generated clients and aligns with Google Cloud API conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proto/private/osac/private/v1/public_ip_pool_type.proto` around lines 42 - 43, The proto's status field is documented read-only but missing the explicit annotation; update the status field declaration (the PublicIPPoolStatus status = 4 field) to include the google.api.field_behavior = OUTPUT_ONLY option—i.e., add [(google.api.field_behavior) = OUTPUT_ONLY] to the field—making sure the google/api/field_behavior.proto import is present (as used for implementation_strategy) and that the field syntax and semicolon remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proto/private/osac/private/v1/public_ip_pool_type.proto`:
- Around line 42-43: The proto's status field is documented read-only but
missing the explicit annotation; update the status field declaration (the
PublicIPPoolStatus status = 4 field) to include the google.api.field_behavior =
OUTPUT_ONLY option—i.e., add [(google.api.field_behavior) = OUTPUT_ONLY] to the
field—making sure the google/api/field_behavior.proto import is present (as used
for implementation_strategy) and that the field syntax and semicolon remain
valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9284d016-88b6-46d6-94b0-7a1922f2238e
⛔ Files ignored due to path filters (2)
internal/api/osac/private/v1/public_ip_pool_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_pool_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
proto/private/osac/private/v1/public_ip_pool_type.proto
@eranco74 I have updated the counters to use |
|
/lgtm |
|
/unhold |
|
/hold |
23d73ea to
0a07c5c
Compare
|
@akshaynadkarni: This pull request references MGMT-23759 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 sub-task to target the "4.22.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. |
af0c773 to
838cc56
Compare
Add the PublicIPPool resource to the private API with a type definition and a gRPC service supporting List, Get, Create, Update, Delete, and Signal RPCs. HTTP annotations are on all RPCs except Signal (internal controller use only). The spec follows the CRD shape from osac-operator: single `cidrs` list with an `ip_family` discriminator (not separate IPv4/IPv6 fields like Subnet). All spec fields are immutable after creation. The Update RPC is metadata-only (name, labels, annotations). `implementation_strategy` is OUTPUT_ONLY, set by the server. The state enum uses PENDING (not PROGRESSING), consistent with all other networking resources in the private API. CRD phase "Progressing" maps to proto PENDING via the feedback controller. Status includes capacity fields (total, allocated, available) as int32, matching the CRD. Assisted-by: Cursor/Claude
The total, allocated, and available fields in PublicIPPoolStatus used int32, which overflows for IPv6 CIDR ranges (a /96 alone exceeds int32 max). Changed to int64 to correctly represent address counts for both IPv4 and IPv6 pools. Assisted-by: Cursor/Claude
Region is a placeholder with no current use. Removed it from the PublicIPPoolSpec and all references in comments and filter examples. Converted ip_family from string to an IPFamily enum (IP_FAMILY_IPV4, IP_FAMILY_IPV6) for type safety, consistent with how the codebase uses enums for constrained value sets (Protocol, State enums). Assisted-by: Cursor/Claude
838cc56 to
cda16f4
Compare
|
/lgtm |
|
/unhold |
Summary
MGMT-23759: Define the
PublicIPPoolproto type and gRPC service in the private API, the first subtask of MGMT-23732 (PublicIPPool Backend).The type definition follows the CRD shape from osac-operator (single
cidrslist +ip_family, not separate IPv4/IPv6 fields like Subnet). All spec fields are immutable after creation. The state enum usesPENDING(consistent with all other networking resources), and status includes capacity fields (total,allocated,available).The service defines List, Get, Create, Update, Delete, and Signal RPCs with gRPC-gateway HTTP annotations on all except Signal.
Testing
Proto validation:
Container build and deploy to edge-22:
Pre-merge ToDos
buf.build/osac-project/private-apibuf.gen.yamlto reference the new BSR tag, thenbuf mod update && buf generate(needed for MGMT-23733 Phase 5 feedback controller)Related PRs
Ticket
MGMT-23759 (subtask of MGMT-23732, epic MGMT-23730)
Assisted-by: Cursor/Claude
Summary by CodeRabbit