MGMT-23736: PublicIP attach/detach validation for ComputeInstances - #480
Conversation
|
Skipping CI for Draft Pull Request. |
|
@akshaynadkarni: This pull request references MGMT-23736 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis change inserts an intermediate ATTACHING state into the PublicIP lifecycle and renumbers subsequent enum values in both public and private protos. The PrivatePublicIPsServer gains a ComputeInstance DAO and Update logic to treat changes to spec.compute_instance as attach/detach operations (validating source state, compute instance existence/state, and uniqueness), moving status.state to ATTACHING or RELEASING and adjusting the protobuf updateMask as needed. PublicIPsServer gets a new Update RPC handler that delegates to the private server. Tests, a DB migration adding a partial unique index on spec.compute_instance, and DAO error mapping for PG unique violations were added. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 60 minutes.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: 3
🤖 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 250-267: The update path handling attach/detach for
spec.compute_instance can result in a sparse PATCH being treated as a full
replace and dropping fields; modify this code so that when
updateIncludesField(mask, "spec.compute_instance") is true but the incoming
request has no update_mask you either (a) synthesize an update mask that
includes both spec.compute_instance and status.state before calling
GenericServer.Update, or (b) merge the existingPublicIP's preserved fields
(e.g., spec.pool, metadata, status.address) into request.GetObject() so the
Update call receives a complete object; apply the same fix to the analogous
block around the 515-529 range; use the existing helper functions
(validateAttachToComputeInstance, validateDetachFromComputeInstance,
setStateOnRequest) and ensure you mutate the request object or mask server-side
rather than relying on client-sent fields.
- Around line 490-500: The check in the create-attachment flow (using filter :=
fmt.Sprintf("this.spec.compute_instance == %q", computeInstanceID) and
s.generic.dao.List().SetFilter(...).Do(ctx)) is racy and needs a DB-enforced
uniqueness guarantee; add a UNIQUE constraint or partial index on
spec.compute_instance in the public_ips table schema (or create a new migration
that adds the constraint/index similar to the network_classes migration 28
pattern) so duplicate attachments cannot be committed concurrently, or
alternatively change the create path to perform the check-and-insert inside a
serializable transaction with proper row locking; update the schema migration
files and any DAO/schema registration code that defines public_ips to include
the new constraint/index.
In `@proto/public/osac/public/v1/public_ip_type.proto`:
- Around line 29-30: The lifecycle comment for PublicIP states no intermediate
preservation of the address on detach but runtime actually transitions ATTACHED
-> RELEASING -> ALLOCATED and preserves the address while in RELEASING; update
the comment text around the PublicIP lifecycle to reflect that clearing
spec.compute_instance triggers ATTACHED -> RELEASING (address preserved
temporarily) -> ALLOCATED, and clarify that RELEASING does not immediately
return the address to the pool. Make the same edits to the other matching
comment block later in the file so both descriptions of the
ATTACHING/ATTACHED/RELEASING/ALLOCATED flow are consistent with runtime
behavior.
🪄 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: ee55fb89-aa28-477e-b3d1-a11e34d3b17b
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/public/v1/public_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
internal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server.goproto/private/osac/private/v1/public_ip_type.protoproto/public/osac/public/v1/public_ip_type.protoproto/public/osac/public/v1/public_ips_service.proto
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/database/dao/generic_dao_update.go`:
- Around line 153-157: The code currently maps a Postgres unique-violation
(pgerrcode.UniqueViolation) to ErrConflict which is intended for
optimistic-lock/version errors and yields misleading messages; change this to
return a distinct error shape (e.g., ErrUniqueConflict or ErrAborted) when
errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation, populate that
new error with the conflicting key/info (or at minimum the ID) and return it
instead of ErrConflict, or alternatively modify ErrConflict to detect “no
version info” and provide a generic conflict message; update the error
construction site (the block that sets err = &ErrConflict{ID: id}) and any
callers that switch on ErrConflict to recognize the new error type.
In `@proto/public/osac/public/v1/public_ip_type.proto`:
- Around line 122-149: The enum values for PUBLIC_IP_STATE_* were renumbered
causing wire incompatibility; restore the original numeric values for
PUBLIC_IP_STATE_ATTACHED, PUBLIC_IP_STATE_RELEASING, and PUBLIC_IP_STATE_FAILED
and assign PUBLIC_IP_STATE_ATTACHING the next unused value (e.g., change
PUBLIC_IP_STATE_ATTACHING to 6) so existing numeric mappings remain stable;
update the enum in proto/public/osac/public/v1/public_ip_type.proto and make the
identical change in proto/private/osac/private/v1/public_ip_type.proto,
referencing the enum symbols PUBLIC_IP_STATE_ATTACHING,
PUBLIC_IP_STATE_ATTACHED, PUBLIC_IP_STATE_RELEASING, and PUBLIC_IP_STATE_FAILED.
🪄 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: 50c42176-8ac1-4cb7-a6b0-6b25c3c25166
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
internal/database/dao/generic_dao_update.gointernal/database/migrations/35_add_public_ips_compute_instance_unique_index.up.sqlinternal/reflection/reflection_helper_test.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.goproto/private/osac/private/v1/public_ip_type.protoproto/public/osac/public/v1/public_ip_type.proto
✅ Files skipped from review due to trivial changes (1)
- internal/database/migrations/35_add_public_ips_compute_instance_unique_index.up.sql
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/hold |
DakCrowder
left a comment
DakCrowder
left a comment
There was a problem hiding this comment.
One ask wrt the migration but otherwise LGTM
There was a problem hiding this comment.
Can we also add a corresponding down migration? I know we aren't using them yet in any automated way but am trying to get into the habit.
There was a problem hiding this comment.
Good suggestion. Added in f2d0b38.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, DakCrowder, SiddarthR56 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 |
… to public API Insert PUBLIC_IP_STATE_ATTACHING=3 in both private and public proto enum definitions, renumbering ATTACHED=4, RELEASING=5, FAILED=6. Add Update RPC with FieldMask support to the public PublicIPs service proto, following the VirtualNetworks Update pattern. Assisted-by: Claude
… state transition map Regenerate Go code from updated proto definitions, producing ATTACHING enum constants and PublicIPsUpdateRequest/Response types. Add Update method to PublicIPsServer that maps public to private format and delegates to the private server with FieldMask support. Add validPublicIPTransitions map to private server defining allowed state transitions including ALLOCATED->ATTACHING and ATTACHING->ATTACHED. Assisted-by: Claude
Attach validation (MGMT-23987): when a user sets spec.compute_instance on an ALLOCATED PublicIP, the server validates the ComputeInstance exists, is RUNNING, belongs to the same tenant, and has no other PublicIP attached. On success the state transitions to ATTACHING. Detach validation (MGMT-23988): when a user clears spec.compute_instance on an ATTACHED PublicIP, the state transitions to RELEASING. The IP address is preserved through the detach. Direct CI swaps (changing from one CI to another) are rejected. The user must detach first, then attach to the new instance. Unit tests (MGMT-23994) cover all five attach rejection scenarios, successful attach, detach rejection, and successful detach with IP address preservation. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
The public PublicIPs service is now discoverable via gRPC reflection after adding the Update RPC. The reflection helper test's expected type lists need to include it. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
The RELEASING state serves two purposes: detach (address preserved, returns to ALLOCATED) and deallocation (address returned to pool). The previous comments described only the deallocation path, which would mislead API consumers about what to expect after clearing spec.compute_instance. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Synthesize a FieldMask when a caller sends an attach/detach Update without one. Without this, a nil mask causes GenericServer to treat the sparse request as a full replacement, wiping fields like spec.pool and status.address. Add a partial unique index on compute_instance to prevent two PublicIPs from attaching to the same ComputeInstance concurrently. The List-based uniqueness check provides the user-facing error message; the index is the database-level safety net for the TOCTOU race window. Handle UniqueViolation in the DAO Update path (matching the existing Create behavior) so the error surfaces as Aborted rather than Internal. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
ErrConflict is designed for optimistic locking and produces a misleading "requested version 0 but current version is 0" message for unique constraint violations. Using ErrAlreadyExists matches the existing Create behavior and surfaces as AlreadyExists to the caller. Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
Assisted-by: Cursor/Claude Signed-off-by: akshaynadkarni <25892229+akshaynadkarni@users.noreply.github.com>
6266d1b to
f2d0b38
Compare
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_public_ips_server_test.go`:
- Around line 641-646: The Delete method in private_public_ips_server.go must
prevent hard-deletes when a PublicIP is in ATTACHING (which holds
spec.compute_instance) similar to ATTACHED and RELEASING; update the guard in
Delete (check existingPublicIP.GetStatus().GetState()) to include
privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHING alongside
PUBLIC_IP_STATE_ATTACHED and PUBLIC_IP_STATE_RELEASING and return the same grpc
FailedPrecondition error message ("cannot delete PublicIP: detach from
ComputeInstance first") to block deletion while attachment is in progress.
🪄 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: 4bd84beb-c7b5-4361-acb4-618d3d12a930
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ip_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service.pb.gw.gois excluded by!**/*.pb.gw.gointernal/api/osac/public/v1/public_ips_service_grpc.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/public_ips_service_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
internal/database/dao/generic_dao_update.gointernal/database/migrations/35_add_public_ips_compute_instance_unique_index.down.sqlinternal/database/migrations/35_add_public_ips_compute_instance_unique_index.up.sqlinternal/reflection/reflection_helper_test.gointernal/servers/generic_server.gointernal/servers/private_public_ips_server.gointernal/servers/private_public_ips_server_test.gointernal/servers/public_ips_server.goproto/private/osac/private/v1/public_ip_type.protoproto/public/osac/public/v1/public_ip_type.protoproto/public/osac/public/v1/public_ips_service.proto
✅ Files skipped from review due to trivial changes (3)
- proto/public/osac/public/v1/public_ips_service.proto
- internal/reflection/reflection_helper_test.go
- internal/servers/private_public_ips_server.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/database/dao/generic_dao_update.go
- internal/database/migrations/35_add_public_ips_compute_instance_unique_index.up.sql
- proto/public/osac/public/v1/public_ip_type.proto
- proto/private/osac/private/v1/public_ip_type.proto
| It("accepts ALLOCATED to ATTACHING transition", func() { | ||
| object := createPublicIPInState(publicIPsServer, privatev1.PublicIPState_PUBLIC_IP_STATE_PENDING) | ||
| object = transitionTo(object, privatev1.PublicIPState_PUBLIC_IP_STATE_ALLOCATED) | ||
| updated := transitionTo(object, privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHING) | ||
| Expect(updated.GetStatus().GetState()).To(Equal(privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHING)) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether PrivatePublicIPsServer.Delete guards the ATTACHING state.
rg -n "ATTACHING\|PublicIPState_PUBLIC_IP_STATE_ATTACHING" \
--type=go \
-g "private_public_ips_server.go" \
-A3 -B3Repository: osac-project/fulfillment-service
Length of output: 58
🏁 Script executed:
fd -e go "private_public_ips_server" --type fRepository: osac-project/fulfillment-service
Length of output: 173
🏁 Script executed:
cat -n internal/servers/private_public_ips_server.go | head -100Repository: osac-project/fulfillment-service
Length of output: 5170
🏁 Script executed:
wc -l internal/servers/private_public_ips_server.goRepository: osac-project/fulfillment-service
Length of output: 126
🏁 Script executed:
rg -n "func.*Delete" internal/servers/private_public_ips_server.go -A 20Repository: osac-project/fulfillment-service
Length of output: 888
🏁 Script executed:
rg -n "func.*Delete" internal/servers/private_public_ips_server.go -A 40Repository: osac-project/fulfillment-service
Length of output: 1484
Delete method must guard against the new ATTACHING state.
The production Delete method (line 307-308 in private_public_ips_server.go) only guards against ATTACHED and RELEASING states, but the new ATTACHING state introduced in this PR is missing from the guard. Since ATTACHING holds a spec.compute_instance reference (analogous to RELEASING), a PublicIP can currently be hard-deleted while an attachment is in progress, leaving the ComputeInstance with a dangling reference.
Add ATTACHING to the delete guard condition:
Suggested fix
state := existingPublicIP.GetStatus().GetState()
if state == privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHED ||
state == privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHING ||
state == privatev1.PublicIPState_PUBLIC_IP_STATE_RELEASING {
err = grpcstatus.Errorf(grpccodes.FailedPrecondition,
"cannot delete PublicIP: detach from ComputeInstance first")
return
}This must be fixed in the production code before the test case is added.
🤖 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_public_ips_server_test.go` around lines 641 - 646,
The Delete method in private_public_ips_server.go must prevent hard-deletes when
a PublicIP is in ATTACHING (which holds spec.compute_instance) similar to
ATTACHED and RELEASING; update the guard in Delete (check
existingPublicIP.GetStatus().GetState()) to include
privatev1.PublicIPState_PUBLIC_IP_STATE_ATTACHING alongside
PUBLIC_IP_STATE_ATTACHED and PUBLIC_IP_STATE_RELEASING and return the same grpc
FailedPrecondition error message ("cannot delete PublicIP: detach from
ComputeInstance first") to block deletion while attachment is in progress.
|
/unhold |
4 participants
Summary
MGMT-23736: Implement PublicIP attachment to ComputeInstances with full validation
on attach and detach operations. Covers MGMT-23986 (Update RPC), MGMT-23987 (attach
validation), MGMT-23988 (detach validation), and MGMT-23994 (unit tests).
Why
Users need to associate a public IP address with a ComputeInstance so traffic can
be routed to the instance via MetalLB. The attach/detach lifecycle requires
server-side validation to ensure only valid operations succeed: the ComputeInstance
must exist, be in RUNNING state, belong to the same tenant, and not already have
a PublicIP attached. Without this validation, invalid attachments could leave the
system in an inconsistent state.
Changes
Proto (commit 1):
PUBLIC_IP_STATE_ATTACHINGto both private and publicPublicIPStateenums(value 3, subsequent states renumbered)
UpdateRPC to publicPublicIPsservice with FieldMask support andHTTP PATCH annotation
Server + codegen (commit 2):
buf generatePublicIPsServer.Update()to public server (maps to private, passes FieldMask)Validation + tests (commit 3):
computeInstancesDaotoPrivatePublicIPsServerfor cross-resource lookupssame tenant (automatic via DAO), no duplicate attachment (uniqueness query). Sets
state to ATTACHING on success.
Testing
Unit tests (643 passed):
E2E on edge-22 (6 tests, all passed):
publicip-attach-v1-260501-1026images to edge-22 clustertenant-akshay-cudnosac/ticket/mgmt-23736/260501-1100-e2e-attach-detach-results.mdTicket
MGMT-23986
MGMT-23987
MGMT-23988
MGMT-23994
Assisted-by: Cursor/Claude
Summary by CodeRabbit
New Features
Bug Fixes
Tests