MGMT-23763: Block IP pool deletion when allocated PublicIPs exist - #456
Conversation
WalkthroughBuild() now instantiates and stores a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 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: 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_ip_pools_server.go`:
- Around line 174-182: The referential-integrity guard currently logs lookup
errors and returns nil, allowing deletion to proceed; change the behavior so
lookup failures block deletion by returning the error (or a wrapped error)
instead of nil. In the code block using s.logger.WarnContext with
slog.String("pool_id", poolID) and slog.Any("error", err), replace the final
"return nil" with "return err" (or fmt.Errorf/wrap) so the caller sees the
failure and deletion is aborted; keep the warning log but ensure the method (the
private_public_ip_pools_server delete/verify function) propagates the error.
🪄 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: 32e43c8d-f13d-46e4-be6f-0d6155c8ad6a
📒 Files selected for processing (2)
internal/servers/private_public_ip_pools_server.gointernal/servers/private_public_ip_pools_server_test.go
|
@SiddarthR56 Can you please add a PR description and capture what tests you ran with the results? |
|
|
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Review comment on error handling approach.
|
|
||
| generic, err := NewGenericServer[*privatev1.PublicIPPool](). | ||
| SetLogger(b.logger). | ||
| SetService(privatev1.PublicIPPools_ServiceDesc.ServiceName). |
There was a problem hiding this comment.
Is the soft-fail here intentional?
Currently if the DAO query errors (e.g., DB connectivity issue), pool deletion proceeds anyway (return nil), which could orphan allocated PublicIPs.
An alternative is hard-fail, returning the error so the delete is blocked until the check can actually run:
if err \!= nil {
return grpcstatus.Errorf(grpccodes.Internal,
"failed to verify allocated public IPs for pool '%s': %v", poolID, err)
}For referential integrity checks, hard-fail is usually safer since it prevents data inconsistency at the cost of blocking deletion during transient failures. Curious about the reasoning if soft-fail is intentional here.
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Please check the comment and add a PR description containing test output.
SiddarthR56
changed the title
|
@SiddarthR56: This pull request references MGMT-23763 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/servers/private_public_ip_pools_server_test.go (1)
315-320: Strengthen rejection assertions to validate gRPC contract, not just message substring.Line 319 currently checks only
err.Error()text. Please also assertFailedPreconditionand include checks for pool ID / count so the test protects the API contract.Proposed test assertion hardening
import ( "context" "fmt" "github.com/jackc/pgx/v5/pgxpool" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + grpccodes "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/types/known/fieldmaskpb" @@ _, err = poolsServer.Delete(ctx, privatev1.PublicIPPoolsDeleteRequest_builder{ Id: poolID, }.Build()) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("public IP(s) are still allocated")) + st, ok := grpcstatus.FromError(err) + Expect(ok).To(BeTrue()) + Expect(st.Code()).To(Equal(grpccodes.FailedPrecondition)) + Expect(st.Message()).To(ContainSubstring(poolID)) + Expect(st.Message()).To(ContainSubstring("public IP(s) are still allocated")) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ip_pools_server_test.go` around lines 315 - 320, The test currently only checks the error string after calling poolsServer.Delete with publicv1.PublicIPPoolsDeleteRequest_builder; update the assertions to validate the gRPC contract by converting err to a gstatus (using status.FromError) and assert the Code() equals codes.FailedPrecondition, then also assert the returned error details or message contains the specific poolID and the allocation count (e.g., verify the error message or details includes poolID and number of allocated IPs) so the test asserts both the status code and that the pool identifier/count are reported by Delete.internal/servers/private_public_ip_pools_server.go (1)
157-162: Avoid fail-open behavior whenpublicIPDAOis unexpectedly nil.Line 157-Line 162 skips integrity checks if
publicIPDAOis nil. For delete safety, this should fail closed withInternalinstead of proceeding.Proposed fail-closed guard
func (s *PrivatePublicIPPoolsServer) Delete(ctx context.Context, request *privatev1.PublicIPPoolsDeleteRequest) (response *privatev1.PublicIPPoolsDeleteResponse, err error) { - if s.publicIPDAO != nil { - err = s.checkNoAllocatedIPs(ctx, request.GetId()) - if err != nil { - return - } - } + if s.publicIPDAO == nil { + err = grpcstatus.Error( + grpccodes.Internal, + "public IP DAO is not configured", + ) + return + } + err = s.checkNoAllocatedIPs(ctx, request.GetId()) + if err != nil { + return + } err = s.generic.Delete(ctx, request, &response) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/servers/private_public_ip_pools_server.go` around lines 157 - 162, The current code skips deletion integrity checks when s.publicIPDAO is nil, causing a fail-open; change the guard so that if s.publicIPDAO == nil you return a failing Internal gRPC error instead of proceeding. Specifically, in the method containing the s.publicIPDAO block, replace the silent no-op path with an explicit error return (use the gRPC/internal error type your codebase uses) explaining that publicIPDAO is unexpectedly nil; keep the existing call to s.checkNoAllocatedIPs(ctx, request.GetId()) when publicIPDAO is present.
🤖 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_ip_pools_server_test.go`:
- Around line 315-320: The test currently only checks the error string after
calling poolsServer.Delete with publicv1.PublicIPPoolsDeleteRequest_builder;
update the assertions to validate the gRPC contract by converting err to a
gstatus (using status.FromError) and assert the Code() equals
codes.FailedPrecondition, then also assert the returned error details or message
contains the specific poolID and the allocation count (e.g., verify the error
message or details includes poolID and number of allocated IPs) so the test
asserts both the status code and that the pool identifier/count are reported by
Delete.
In `@internal/servers/private_public_ip_pools_server.go`:
- Around line 157-162: The current code skips deletion integrity checks when
s.publicIPDAO is nil, causing a fail-open; change the guard so that if
s.publicIPDAO == nil you return a failing Internal gRPC error instead of
proceeding. Specifically, in the method containing the s.publicIPDAO block,
replace the silent no-op path with an explicit error return (use the
gRPC/internal error type your codebase uses) explaining that publicIPDAO is
unexpectedly nil; keep the existing call to s.checkNoAllocatedIPs(ctx,
request.GetId()) when publicIPDAO is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b52a894-193a-4433-bfd8-4d2e9abbba1d
📒 Files selected for processing (2)
internal/servers/private_public_ip_pools_server.gointernal/servers/private_public_ip_pools_server_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, 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 |
… 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
4 participants
PrivatePublicIPPoolsServer.Delete now checks for allocated PublicIP objects before proceeding. If any PublicIP references the pool via spec.pool, the call is rejected with FailedPrecondition including the pool ID and remaining IP count.
Added two unit tests cover the happy and rejection paths (both pass), Manully tested the below 4 scenarios:
Summary by CodeRabbit
Bug Fixes
Tests