This repository was archived by the owner on Sep 9, 2026. It is now read-only.
MGMT-23734/MGMT-23900: PublicIP pool validation, state machine and capacity tracking - #466
Merged
openshift-merge-bot[bot] merged 13 commits intoApr 30, 2026
Conversation
|
Skipping CI for Draft Pull Request. |
|
@akshaynadkarni: This pull request references MGMT-23900 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 "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. |
akshaynadkarni
changed the title
akshaynadkarni
force-pushed
the
mgmt-23734-publicip-state-machine
branch
2 times, most recently
from
April 29, 2026 15:29
4b5040f to
61ae781
Compare
akshaynadkarni
marked this pull request as ready for review
April 29, 2026 15:54
akshaynadkarni
requested review from
DakCrowder,
SiddarthR56,
adriengentil and
jhernand
and removed request for
larsks
April 29, 2026 15:54
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 selected for processing (4)
WalkthroughPrivatePublicIPsServer now uses a PublicIPPool DAO. Create verifies the referenced pool exists, is in READY state, and has available capacity, then atomically updates pool counters (available--, allocated++) and returns Aborted on optimistic-lock conflicts. Update requires object.id, enforces immutability of spec.pool when present in the mask, validates status.state transitions against an allowed-transition table, and delegates persistence to the generic updater. Delete requires id, blocks deletion when state is ATTACHED or RELEASING, deletes otherwise, and restores pool counters (allocated--, available++) when poolID is set. New helper functions centralize these checks and atomic updates. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 29 seconds.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: 2
🤖 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/private_public_ips_server.go`:
- Around line 172-177: validatePoolReference does a non-atomic availability
check and updatePoolCapacity decrements capacity without re-checking, allowing
available to go negative under concurrent creates; change the flow to perform an
atomic read-modify-write: within updatePoolCapacity (or a new single
transactional helper) fetch the Pool resource, verify
pool.GetStatus().GetCapacity() >= requestedDelta (or >0 for decrement by 1),
decrement and persist in one atomic update (use the storage/DB transaction,
Compare-And-Swap, or API's UpdateWithPrecondition) and return an error if the
precondition fails so callers (the code paths invoking validatePoolReference and
updatePoolCapacity) will abort instead of relying on the prior non-atomic
validatePoolReference check—apply the same atomic update pattern to all
referenced sites (validatePoolReference usages around the other create paths and
the decrement logic in updatePoolCapacity).
- Around line 359-367: The update error for s.publicIPPoolDao.Update() is being
unconditionally mapped to Aborted; modify the error handling to use errors.As to
detect an optimistic-lock conflict (the dao.ErrConflict type) and only return
grpcstatus.Errorf(grpccodes.Aborted, ...) for that case, otherwise return
grpcstatus.Errorf(grpccodes.Internal, "failed to update pool capacity"); keep
the existing s.logger.ErrorContext(...) log, but use errors.As(err,
&conflictErr) where conflictErr is a *dao.ErrConflict to distinguish the two
outcomes.
🪄 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: 260d1ced-44cb-4810-8172-c4903eb356e4
📒 Files selected for processing (3)
internal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server_test.go
akshaynadkarni
force-pushed
the
mgmt-23734-publicip-state-machine
branch
from
April 29, 2026 19:31
e694095 to
b78c3e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/private_public_ips_server_test.go`:
- Around line 699-710: The test "skips state validation when new state is
UNSPECIFIED" currently only checks the name update; add an assertion that the
PublicIP state was preserved after the Update call: capture the original state
from createPublicIPInState (e.g., PublicIPState_PUBLIC_IP_STATE_ALLOCATED), call
publicIPsServer.Update as shown, then assert
resp.GetObject().GetStatus().GetState() equals the original state to verify the
UNSPECIFIED update is a no-op for state (use the existing
object/GetStatus/SetState and resp/GetObject/GetStatus symbols).
- Around line 773-792: The test title says it exercises Delete when state is
PENDING but the test relies on the default/unspecified state; change the setup
to explicitly create a PENDING public IP by calling the helper that sets state
(e.g., replace the publicIPsServer.Create call with createPublicIPWithState(ctx,
poolID, privatev1.PublicIPState_PENDING) or the project's equivalent helper) so
the created object's state is PENDING, then call publicIPsServer.Delete (using
the created object's Id) and assert no error as before.
🪄 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: d0fe16da-5124-4ee2-8de4-c250f197981e
📒 Files selected for processing (3)
internal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/servers/private_public_ips_server.go
akshaynadkarni
force-pushed
the
mgmt-23734-publicip-state-machine
branch
from
April 29, 2026 20:27
b78c3e0 to
2b8a6b8
Compare
akshaynadkarni
changed the title
akshaynadkarni
changed the title
akshaynadkarni
changed the title
DakCrowder
reviewed
Apr 29, 2026
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 `@internal/servers/private_public_ips_server.go`:
- Around line 32-39: The update path currently skips validation when newState is
PUBLIC_IP_STATE_UNSPECIFIED which lets GenericServer.Update persist UNSPECIFIED
and corrupt state; to fix this, ensure validatePublicIPStateTransition runs for
every state change (including PUBLIC_IP_STATE_UNSPECIFIED) by removing the
special-case bypass and/or adding
publicv1.PublicIPState_PUBLIC_IP_STATE_UNSPECIFIED as a key in
validPublicIPTransitions with an empty target list, and make
validatePublicIPStateTransition explicitly reject UNSPECIFIED so Update cannot
persist it; update calls that previously skipped validation to instead call
validatePublicIPStateTransition (referencing validPublicIPTransitions and
validatePublicIPStateTransition) so UNSPECIFIED is rejected rather than saved.
🪄 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: 8c53ca91-8f39-47fa-8df3-5832217cecce
📒 Files selected for processing (3)
internal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/servers/private_public_ips_server_test.go
eranco74
reviewed
Apr 30, 2026
eranco74
left a comment
eranco74
left a comment
Contributor
There was a problem hiding this comment.
Nice work -- well-structured, follows existing codebase conventions, and thorough test coverage. A few suggestions below.
…d delete constraint Add PublicIPPool DAO injection for cross-resource validation. Create validates pool existence, READY state, and available capacity before allocating. Update enforces the 4-entry state machine (PENDING -> ALLOCATED -> ATTACHED -> RELEASING -> ALLOCATED) and pool immutability. Delete rejects ATTACHED state and reverses capacity counters. Pool capacity (allocated/available) updated atomically in the same DB transaction via the DAO. MGMT-23900 Assisted-by: Cursor/Claude
Simplify the loop in validatePublicIPStateTransition to use slices.Contains per linter suggestion. Assisted-by: Cursor/Claude
Cover all validation paths: pool validation on Create (existence, READY state, capacity), state machine enforcement on Update (4 valid + 3 invalid transitions), atomic capacity tracking (decrement on Create, increment on Delete), delete constraint when ATTACHED, and pool field immutability on Update. Fix existing tests in both private and public server test files to create real PublicIPPool records in the database, which became required after pool validation was added to the Create path. Assisted-by: Cursor/Claude
Verifies that Update requests that don't set status.state skip the state machine validation and succeed without error. Assisted-by: Cursor/Claude
Add doc comments to Create, Update, and Delete explaining reliance on the gRPC interceptor's database transaction for atomicity and optimistic locking for concurrent access safety. Assisted-by: Cursor/Claude
Assisted-by: Cursor/Claude
Assisted-by: Cursor/Claude
…odes Without a bounds check, a concurrent race (however unlikely with optimistic locking) could drive allocated/available counters below zero. Reject the update with FailedPrecondition if the result would be negative. Version conflicts from optimistic locking now return Aborted (retriable); other database errors return Internal. Previously all update errors were mapped to Aborted, which told clients to retry non-retriable failures. Assisted-by: Cursor/Claude
The "allows Delete when state is PENDING" test was relying on the default zero-value state after Create, which is UNSPECIFIED, not PENDING. Use createPublicIPWithState to explicitly set PENDING so the test exercises the state it claims to test. Assisted-by: Cursor/Claude
Delete was only blocked for ATTACHED state. A PublicIP in RELEASING state is still bound to a ComputeInstance while the controller processes the detach. Allowing deletion mid-release could leave the ComputeInstance referencing a deleted IP. Assisted-by: Cursor/Claude
Skip pool immutability and state machine validation when the updated fields are not in the UpdateMask. When the mask is nil (full object replacement), all validations still run. This prevents UNSPECIFIED state from being persisted on partial updates where the client omits status.state. Add a shared updateIncludesField helper in field_mask.go for mask-aware validation, reusable by any server in the package. Split the capacity bounds check into two distinct error messages: "no available capacity" for exhausted pools on Create, and a diagnostic message with actual values for allocation count inconsistencies on Delete. Consolidate duplicate test helpers (createPublicIPInState and createPublicIPWithState) into a single parameterized helper. Assisted-by: Cursor/Claude
akshaynadkarni
force-pushed
the
mgmt-23734-publicip-state-machine
branch
from
April 30, 2026 14:36
b66fd0d to
27347ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/servers/private_public_ips_server.go (1)
225-229:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDon’t treat
UNSPECIFIEDas a no-op whenstatus.stateis in the mask.With
update_mask: ["status.state"], this branch still lets clients persistPUBLIC_IP_STATE_UNSPECIFIEDand bypass the state machine. Once Line 225 determines the field is being updated,UNSPECIFIEDneeds to be rejected like any other invalid target state.Suggested fix
if updateIncludesField(mask, "status.state") { newState := request.GetObject().GetStatus().GetState() existingState := existingPublicIP.GetStatus().GetState() - if newState != privatev1.PublicIPState_PUBLIC_IP_STATE_UNSPECIFIED && newState != existingState { + if newState != existingState { if err = validatePublicIPStateTransition(existingState, newState); err != nil { return } } }🤖 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 225 - 229, The branch guarded by updateIncludesField(mask, "status.state") incorrectly treats PUBLIC_IP_STATE_UNSPECIFIED as a no-op; when the update mask includes status.state you must reject UNSPECIFIED instead of letting it bypass validation. Change the logic in the block that reads newState := request.GetObject().GetStatus().GetState() / existingState := existingPublicIP.GetStatus().GetState() so that if newState == privatev1.PublicIPState_PUBLIC_IP_STATE_UNSPECIFIED you return a validation error immediately (similar to other invalid targets) before calling validatePublicIPStateTransition(existingState, newState); ensure the error surface and message make clear the client submitted an UNSPECIFIED state for an explicit status.state update.
🤖 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/field_mask.go`:
- Around line 34-37: The current loop in updateIncludesField (iterating
mask.GetPaths() and comparing to prefixes) only treats exact matches and
descendants of the prefix, but not the case where the mask contains a parent
path (e.g., "spec") which should count as updating nested fields like
"spec.pool"; update the conditional inside the loop to also return true when the
prefix is a descendant of the mask path (add a check like
strings.HasPrefix(prefix, path+".") in addition to the existing checks so parent
mask paths are treated as updating their nested fields).
In `@internal/servers/private_public_ips_server.go`:
- Around line 399-406: The code currently only rejects pool changes when newPool
!= "" which allows callers to clear spec.pool; change the check to reject any
modification detected by updateIncludesField(mask, "spec.pool") (or at least
remove the newPool != "" guard) so that if the field is included and newPool !=
existingPool (including empty string) return the same Immutable field
InvalidArgument error; refer to newPublicIP.GetSpec().GetPool(),
existingPublicIP.GetSpec().GetPool(), and the updateIncludesField(mask,
"spec.pool") check to locate where to enforce this.
---
Duplicate comments:
In `@internal/servers/private_public_ips_server.go`:
- Around line 225-229: The branch guarded by updateIncludesField(mask,
"status.state") incorrectly treats PUBLIC_IP_STATE_UNSPECIFIED as a no-op; when
the update mask includes status.state you must reject UNSPECIFIED instead of
letting it bypass validation. Change the logic in the block that reads newState
:= request.GetObject().GetStatus().GetState() / existingState :=
existingPublicIP.GetStatus().GetState() so that if newState ==
privatev1.PublicIPState_PUBLIC_IP_STATE_UNSPECIFIED you return a validation
error immediately (similar to other invalid targets) before calling
validatePublicIPStateTransition(existingState, newState); ensure the error
surface and message make clear the client submitted an UNSPECIFIED state for an
explicit status.state update.
🪄 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: 30fac460-75bc-4d10-b2c9-55024aef0260
📒 Files selected for processing (4)
internal/servers/field_mask.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/servers/public_ips_server_test.go
- internal/servers/private_public_ips_server_test.go
… Create The pool deletion test from PR osac-project#456 creates a pool without setting its status. Our PR adds pool validation on Create (pool must be READY with available capacity), which caused the test to fail. Set the pool to READY state via Update before creating a PublicIP from it. Assisted-by: Cursor/Claude
Match parent mask paths in updateIncludesField: a mask containing "spec" now correctly matches prefix "spec.pool", since replacing the entire spec submessage replaces pool too. Remove the UNSPECIFIED state bypass in Update validation. Now that FieldMask guards skip validation when status.state is not in the mask, the UNSPECIFIED bypass is no longer needed. For nil-mask (full replacement), UNSPECIFIED is rejected by the state machine as an invalid transition target, preventing silent state corruption. Remove the empty-string guard on pool immutability. With the FieldMask guard ensuring we only validate when spec.pool is in the update, an empty pool value is a deliberate clear attempt and should be rejected. Assisted-by: Cursor/Claude
DakCrowder
approved these changes
Apr 30, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
This was referenced May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add business logic validation to the PublicIP server for MGMT-23734/MGMT-23900:
All validation and capacity updates run within the request's existing database transaction.
Testing
Unit Tests
33 new test cases covering:
E2E Tests (edge-22, osac-devel namespace)
Tested against the cluster with all components running the PR branch image
(
mgmt-23734-e2e). API calls viacurlto the REST gateway.pool 'nonexistent-pool-id-12345' does not exist(code 3)invalid state transition from PUBLIC_IP_STATE_PENDING to PUBLIC_IP_STATE_ATTACHED(code 9)cannot delete PublicIP: detach from ComputeInstance first(code 9)field 'spec.pool' is immutable and cannot be changed from '019dc144...' to 'some-other-pool-id'(code 3)7/7 E2E tests passed.
E2E environment: fulfillment-service
mgmt-23734-e2e, osac-operatormgmt-23734-e2e,OCP 4.20.0, Keycloak OAuth for controller auth, Authorino+OPA for API authorization.
Related PRs
Ticket
Assisted-by: Cursor/Claude
Summary by CodeRabbit
New Features
Bug Fixes
Tests