Skip to content
This repository was archived by the owner on Sep 9, 2026. It is now read-only.

MGMT-23763: Block IP pool deletion when allocated PublicIPs exist #456

Merged
SiddarthR56 merged 2 commits into from
Apr 30, 2026
Merged

MGMT-23763: Block IP pool deletion when allocated PublicIPs exist #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ddb9d4
Block IP pool deletion when allocated PublicIPs exist
SiddarthR56 Apr 27, 2026
aef3663
Switch to Hard fail
SiddarthR56 Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 59 additions & 4 deletions internal/servers/private_public_ip_pools_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@ package servers
import (
"context"
"errors"
"fmt"
"log/slog"

"github.com/prometheus/client_golang/prometheus"
grpccodes "google.golang.org/grpc/codes"
grpcstatus "google.golang.org/grpc/status"

privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1"
"github.com/osac-project/fulfillment-service/internal/auth"
"github.com/osac-project/fulfillment-service/internal/database"
"github.com/osac-project/fulfillment-service/internal/database/dao"
)

// PrivatePublicIPPoolsServerBuilder contains the data and logic needed to create a new private public IP pools server.
Expand All @@ -40,8 +44,9 @@ var _ privatev1.PublicIPPoolsServer = (*PrivatePublicIPPoolsServer)(nil)
type PrivatePublicIPPoolsServer struct {
privatev1.UnimplementedPublicIPPoolsServer

logger *slog.Logger
generic *GenericServer[*privatev1.PublicIPPool]
logger *slog.Logger
generic *GenericServer[*privatev1.PublicIPPool]
publicIPDAO *dao.GenericDAO[*privatev1.PublicIP]
}

// NewPrivatePublicIPPoolsServer creates a builder that can then be used to configure and create a new private public
Expand Down Expand Up @@ -92,6 +97,17 @@ func (b *PrivatePublicIPPoolsServerBuilder) Build() (result *PrivatePublicIPPool
return
}

// Create the PublicIP DAO used to check for allocated IPs on pool deletion:
publicIPDAO, err := dao.NewGenericDAO[*privatev1.PublicIP]().
SetLogger(b.logger).
SetTenancyLogic(b.tenancyLogic).
SetMetricsRegisterer(b.metricsRegisterer).
Build()
if err != nil {
err = fmt.Errorf("failed to create public IP DAO: %w", err)
return
}

generic, err := NewGenericServer[*privatev1.PublicIPPool]().
SetLogger(b.logger).
SetService(privatev1.PublicIPPools_ServiceDesc.ServiceName).

@akshaynadkarni akshaynadkarni Apr 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -105,8 +121,9 @@ func (b *PrivatePublicIPPoolsServerBuilder) Build() (result *PrivatePublicIPPool
}

result = &PrivatePublicIPPoolsServer{
logger: b.logger,
generic: generic,
logger: b.logger,
generic: generic,
publicIPDAO: publicIPDAO,
}
return
}
Expand Down Expand Up @@ -137,10 +154,48 @@ func (s *PrivatePublicIPPoolsServer) Update(ctx context.Context,

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
}
}
err = s.generic.Delete(ctx, request, &response)
return
}

// checkNoAllocatedIPs returns a FailedPrecondition error when at least one PublicIP still
// references this pool. Any database error is treated as a hard failure so that a transient
// connectivity issue cannot silently bypass the referential-integrity check and orphan IPs.
func (s *PrivatePublicIPPoolsServer) checkNoAllocatedIPs(ctx context.Context, poolID string) error {
filter := fmt.Sprintf("this.spec.pool == %q", poolID)
listResponse, err := s.publicIPDAO.List().
SetFilter(filter).
SetLimit(1).
Do(ctx)
if err != nil {
s.logger.ErrorContext(
ctx,
"Failed to verify allocated public IPs for pool",
slog.String("pool_id", poolID),
slog.Any("error", err),
)
return grpcstatus.Errorf(
grpccodes.Internal,
"failed to verify allocated public IPs for pool '%s'",
poolID,
)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if total := listResponse.GetTotal(); total > 0 {
return grpcstatus.Errorf(
grpccodes.FailedPrecondition,
"cannot delete public IP pool '%s': %d public IP(s) are still allocated from it",
poolID, total,
)
}
return nil
}

func (s *PrivatePublicIPPoolsServer) Signal(ctx context.Context,
request *privatev1.PublicIPPoolsSignalRequest) (response *privatev1.PublicIPPoolsSignalResponse, err error) {
err = s.generic.Signal(ctx, request, &response)
Expand Down
68 changes: 66 additions & 2 deletions internal/servers/private_public_ip_pools_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ var _ = Describe("Private public IP pools server", func() {
// Create the tables:
err = dao.CreateTables[*privatev1.PublicIPPool](ctx)
Expect(err).ToNot(HaveOccurred())
err = dao.CreateTables[*privatev1.PublicIP](ctx)
Expect(err).ToNot(HaveOccurred())
})

Describe("Creation", func() {
Expand Down Expand Up @@ -109,18 +111,29 @@ var _ = Describe("Private public IP pools server", func() {
})

Describe("Behaviour", func() {
var poolsServer *PrivatePublicIPPoolsServer
var (
poolsServer *PrivatePublicIPPoolsServer
ipsServer *PrivatePublicIPsServer
)

BeforeEach(func() {
var err error

// Create the server:
// Create the pools server:
poolsServer, err = NewPrivatePublicIPPoolsServer().
SetLogger(logger).
SetAttributionLogic(attribution).
SetTenancyLogic(tenancy).
Build()
Expect(err).ToNot(HaveOccurred())

// Create the IPs server (used to seed PublicIP objects for deletion tests):
ipsServer, err = NewPrivatePublicIPsServer().
SetLogger(logger).
SetAttributionLogic(attribution).
SetTenancyLogic(tenancy).
Build()
Expect(err).ToNot(HaveOccurred())
})

It("Creates a pool", func() {
Expand Down Expand Up @@ -274,5 +287,56 @@ var _ = Describe("Private public IP pools server", func() {
Expect(err).ToNot(HaveOccurred())
Expect(getResponse.GetObject().GetMetadata().GetDeletionTimestamp()).ToNot(BeNil())
})

It("Rejects deletion when allocated PublicIPs reference the pool", func() {
// Create a pool:
createPoolResponse, err := poolsServer.Create(ctx, privatev1.PublicIPPoolsCreateRequest_builder{
Object: privatev1.PublicIPPool_builder{
Spec: privatev1.PublicIPPoolSpec_builder{
Cidrs: []string{"10.0.0.0/24"},
IpFamily: privatev1.IPFamily_IP_FAMILY_IPV4,
}.Build(),
}.Build(),
}.Build())
Expect(err).ToNot(HaveOccurred())
poolID := createPoolResponse.GetObject().GetId()

// Allocate a PublicIP from the pool:
_, err = ipsServer.Create(ctx, privatev1.PublicIPsCreateRequest_builder{
Object: privatev1.PublicIP_builder{
Spec: privatev1.PublicIPSpec_builder{
Pool: poolID,
}.Build(),
}.Build(),
}.Build())
Expect(err).ToNot(HaveOccurred())

// Attempt to delete the pool — must fail:
_, 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"))
})

It("Allows deletion when no PublicIPs reference the pool", func() {
// Create a pool:
createPoolResponse, err := poolsServer.Create(ctx, privatev1.PublicIPPoolsCreateRequest_builder{
Object: privatev1.PublicIPPool_builder{
Spec: privatev1.PublicIPPoolSpec_builder{
Cidrs: []string{"10.1.0.0/24"},
IpFamily: privatev1.IPFamily_IP_FAMILY_IPV4,
}.Build(),
}.Build(),
}.Build())
Expect(err).ToNot(HaveOccurred())
poolID := createPoolResponse.GetObject().GetId()

// No PublicIPs reference this pool — deletion must succeed:
_, err = poolsServer.Delete(ctx, privatev1.PublicIPPoolsDeleteRequest_builder{
Id: poolID,
}.Build())
Expect(err).ToNot(HaveOccurred())
})
})
})
Loading