OSAC-2341: create default networking resources at tenant onboarding - #964
Conversation
|
@danmanor: This pull request references OSAC-2341 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 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor 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 |
WalkthroughAdds a default networking provisioner that creates tenant networking resources from ChangesDefault tenant networking
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant PrivateTenantsServer
participant DefaultNetworkingProvisioner
participant NetworkClassDAO
participant NetworkingDAOs
Client->>PrivateTenantsServer: Create tenant
PrivateTenantsServer->>DefaultNetworkingProvisioner: Provision(ctx, tenantName)
DefaultNetworkingProvisioner->>NetworkClassDAO: Find default NetworkClass
NetworkClassDAO-->>DefaultNetworkingProvisioner: Return defaults or none
DefaultNetworkingProvisioner->>NetworkingDAOs: Create default VN, subnets, security group, and optional NAT
NetworkingDAOs-->>DefaultNetworkingProvisioner: Persisted resources or error
DefaultNetworkingProvisioner-->>PrivateTenantsServer: Provisioning result
PrivateTenantsServer-->>Client: Tenant response or Internal error
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ 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 |
danmanor
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/default_networking_provisioner_test.go`:
- Line 154: Update the owner-reference assertions in the affected networking
provisioner tests to capture the VN ID from the earlier list and use
HaveKeyWithValue for "osac.openshift.io/owner-reference" with that vnID,
replacing the key-only HaveKey checks at all three locations.
- Around line 303-313: Remove the boolPtr, int32Ptr, and stringPtr helper
functions, and replace their call sites with Go 1.26 new expressions using the
corresponding typed expressions where needed, preserving the existing pointer
values and behavior.
- Around line 212-221: The NAT gateway provisioning path lacks coverage beyond
the disabled case. Add specs around provisionNATGateway that exercise an
eligible external IP pool and verify ExternalIP and NATGateway creation,
allocated/available capacity changes, and attached=true; also add a
no-eligible-pool case asserting the error from
findExternalIPPool/provisionNATGateway propagates.
In `@internal/servers/default_networking_provisioner.go`:
- Around line 231-268: The five default resource creation helpers duplicate
metadata construction and should share a common defaultMetadata(tenantName,
name, ownerID string) helper. Add that helper to centralize the default label,
system creator, and optional owner-reference annotation, then update
createDefaultVirtualNetwork and the corresponding create helpers to use it while
preserving each resource’s existing DAO creation and ID return behavior.
- Around line 325-330: Update the SecurityGroupSpec builder in the default
networking provisioner to use the NetworkClass implementation strategy, via the
same nc.GetImplementationStrategy() value propagated to the VirtualNetwork,
instead of hardcoding "network_policy". Keep the ingress, egress, and virtual
network assignments unchanged.
- Around line 195-224: Make
internal/servers/default_networking_provisioner.go#L195-L224 explicitly document
on the exported Provision method that its dependent networking writes must run
inside a caller-supplied PostgreSQL transaction; add the corresponding brief
comment at the Provision call site in
internal/servers/private_tenants_server.go#L209-L223 stating that tenant
insertion and networking writes share the request transaction and roll back
together when the RPC returns Internal. Do not add resource-level rollback or
idempotency beyond this documentation.
- Around line 79-82: Remove the unused no-op newDAO closure and its discard
assignment from Build, leaving the surrounding provisioning logic unchanged.
- Around line 446-464: Update updatePoolCapacity to initialize and attach an
ExternalIPPool status object when pool.GetStatus() is nil before mutating
allocation fields. Also validate the resulting available capacity under the
locked pool state and return an error without updating when the decrement would
make it negative; preserve the existing wrapped errors for DAO operations.
In `@internal/servers/private_tenants_server_test.go`:
- Around line 484-506: Add a test under the “with default networking
provisioner” context that configures a NetworkClass with defaults forcing
provisioning failure, such as enable_nat_gateway without an eligible ExternalIP
pool, then exercises PrivateTenantsServer.Internal and asserts the returned
error code and message from the new failure branch. Reuse the existing
provisionerServer setup and test fixtures.
- Around line 551-569: Add per-spec database isolation around the setup used by
the Create test suite, ensuring each BeforeEach transaction rolls back or resets
the schema/container state before the next spec runs. Preserve the “no
NetworkClass” precondition in the “creates tenant without default networking
when no NetworkClass exists” spec so its VirtualNetwork assertion is independent
of prior specs.
In `@internal/servers/private_tenants_server.go`:
- Around line 209-223: Add a concise contract comment near the default
networking Provision call in the tenant creation flow, or on
DefaultNetworkingProvisioner.Provision, stating that atomic cleanup depends on
the surrounding single PostgreSQL transaction and callers outside that
transaction must not assume rollback of partial provisioning. Keep the existing
error handling unchanged.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 852cb5f8-cd9d-43f0-88a9-479eb163a0ba
📒 Files selected for processing (5)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/servers/default_networking_provisioner.gointernal/servers/default_networking_provisioner_test.gointernal/servers/private_tenants_server.gointernal/servers/private_tenants_server_test.go
| Spec: privatev1.SecurityGroupSpec_builder{ | ||
| VirtualNetwork: vnID, | ||
| Ingress: defaults.GetIngressRules(), | ||
| Egress: defaults.GetEgressRules(), | ||
| ImplementationStrategy: "network_policy", | ||
| }.Build(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
implementation_strategy hardcoded on the SecurityGroup while the VirtualNetwork gets it from the NetworkClass.
Line 249 propagates nc.GetImplementationStrategy(); here it's pinned to "network_policy". If a NetworkClass declares netris, the VN and its SecurityGroup disagree. Intentional?
🤖 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/default_networking_provisioner.go` around lines 325 - 330,
Update the SecurityGroupSpec builder in the default networking provisioner to
use the NetworkClass implementation strategy, via the same
nc.GetImplementationStrategy() value propagated to the VirtualNetwork, instead
of hardcoding "network_policy". Keep the ingress, egress, and virtual network
assignments unchanged.
| func (p *DefaultNetworkingProvisioner) updatePoolCapacity(ctx context.Context, poolID string, delta int64) error { | ||
| getResponse, err := p.externalIPPoolDao.Get(). | ||
| SetId(poolID). | ||
| SetLock(true). | ||
| Do(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get ExternalIPPool for capacity update: %w", err) | ||
| } | ||
|
|
||
| pool := getResponse.GetObject() | ||
| pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta) | ||
| pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta) | ||
|
|
||
| _, err = p.externalIPPoolDao.Update().SetObject(pool).Do(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update ExternalIPPool capacity: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded status mutation and unchecked capacity floor.
Two problems:
pool.GetStatus()returns nil when the pool has no status set; the subsequentSetAllocatedcall on a nil status message panics.availableis only checked in the CEL filter at selection time (Line 377). Between selection and this update nothing re-validates it, soavailablecan be driven negative.
🛡️ Proposed fix
pool := getResponse.GetObject()
+ if !pool.HasStatus() {
+ return fmt.Errorf("ExternalIPPool '%s' has no status", poolID)
+ }
+ if pool.GetStatus().GetAvailable() < delta {
+ return fmt.Errorf(
+ "ExternalIPPool '%s' has %d available addresses, need %d",
+ poolID, pool.GetStatus().GetAvailable(), delta,
+ )
+ }
pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta)
pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *DefaultNetworkingProvisioner) updatePoolCapacity(ctx context.Context, poolID string, delta int64) error { | |
| getResponse, err := p.externalIPPoolDao.Get(). | |
| SetId(poolID). | |
| SetLock(true). | |
| Do(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to get ExternalIPPool for capacity update: %w", err) | |
| } | |
| pool := getResponse.GetObject() | |
| pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta) | |
| pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta) | |
| _, err = p.externalIPPoolDao.Update().SetObject(pool).Do(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to update ExternalIPPool capacity: %w", err) | |
| } | |
| return nil | |
| } | |
| func (p *DefaultNetworkingProvisioner) updatePoolCapacity(ctx context.Context, poolID string, delta int64) error { | |
| getResponse, err := p.externalIPPoolDao.Get(). | |
| SetId(poolID). | |
| SetLock(true). | |
| Do(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to get ExternalIPPool for capacity update: %w", err) | |
| } | |
| pool := getResponse.GetObject() | |
| if !pool.HasStatus() { | |
| return fmt.Errorf("ExternalIPPool '%s' has no status", poolID) | |
| } | |
| if pool.GetStatus().GetAvailable() < delta { | |
| return fmt.Errorf( | |
| "ExternalIPPool '%s' has %d available addresses, need %d", | |
| poolID, pool.GetStatus().GetAvailable(), delta, | |
| ) | |
| } | |
| pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta) | |
| pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta) | |
| _, err = p.externalIPPoolDao.Update().SetObject(pool).Do(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to update ExternalIPPool capacity: %w", err) | |
| } | |
| return nil | |
| } |
🤖 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/default_networking_provisioner.go` around lines 446 - 464,
Update updatePoolCapacity to initialize and attach an ExternalIPPool status
object when pool.GetStatus() is nil before mutating allocation fields. Also
validate the resulting available capacity under the locked pool state and return
an error without updating when the decrement would make it negative; preserve
the existing wrapped errors for DAO operations.
| Context("with default networking provisioner", func() { | ||
| var ( | ||
| provisionerServer *PrivateTenantsServer | ||
| provisioner *DefaultNetworkingProvisioner | ||
| ) | ||
|
|
||
| BeforeEach(func() { | ||
| var err error | ||
| provisioner, err = NewDefaultNetworkingProvisioner(). | ||
| SetLogger(logger). | ||
| SetTenancyLogic(tenancy). | ||
| Build() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| provisionerServer, err = NewPrivateTenantsServer(). | ||
| SetLogger(logger). | ||
| SetAttributionLogic(attribution). | ||
| SetTenancyLogic(tenancy). | ||
| SetDefaultNetworkingProvisioner(provisioner). | ||
| Build() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| }) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing coverage for the failure branch.
The new Internal error path in private_tenants_server.go Lines 213-222 has no test. A provisioner pointed at a NetworkClass whose defaults force a failure (e.g. enable_nat_gateway with no eligible ExternalIP pool) would cover both the error code and the message.
🤖 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_tenants_server_test.go` around lines 484 - 506, Add
a test under the “with default networking provisioner” context that configures a
NetworkClass with defaults forcing provisioning failure, such as
enable_nat_gateway without an eligible ExternalIP pool, then exercises
PrivateTenantsServer.Internal and asserts the returned error code and message
from the new failure branch. Reuse the existing provisionerServer setup and test
fixtures.
| It("creates tenant without default networking when no NetworkClass exists", func() { | ||
| request := privatev1.TenantsCreateRequest_builder{ | ||
| Object: privatev1.Tenant_builder{ | ||
| Metadata: privatev1.Metadata_builder{ | ||
| Name: "plain-tenant", | ||
| }.Build(), | ||
| }.Build(), | ||
| }.Build() | ||
|
|
||
| response, err := provisionerServer.Create(ctx, request) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(response).ToNot(BeNil()) | ||
|
|
||
| vnList, err := provisioner.virtualNetworkDao.List(). | ||
| SetFilter("this.metadata.tenant == 'plain-tenant'"). | ||
| Do(ctx) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(vnList.GetItems()).To(BeEmpty()) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'suite_test.go' internal/servers --exec cat -n {}Repository: osac-project/fulfillment-service
Length of output: 4584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate test file size:"
wc -l internal/servers/private_tenants_server_test.go
echo "Suite hooks:"
rg -n "Before(Suite|Each|All)|After(Suite|Each|All)|DeferCleanup|RunSpecs|BeforeEach|AfterEach" internal/servers/private_tenants_server_test.go internal/servers/suite_test.go
echo "Relevant preceding spec area:"
sed -n '520,580p' internal/servers/private_tenants_server_test.go | cat -n
echo "Virtual network creation paths in internal/servers/private_tenants_server_test.go:"
rg -n "NetworkClass|plain-tenant|default networking|VirtualNetwork|virtualNetworkDao|List\\(|Create\\(" internal/servers/private_tenants_server_test.go
echo "Broader test cleanup patterns in repo:"
rg -n "tx\\.Rollback|Rollback\\(ctx\\)|TxIntoContext|AfterEach\\(func|BeforeEach\\(func|DeferCleanup\\(func\\{" -g '*_test.go' internal | head -200Repository: osac-project/fulfillment-service
Length of output: 482
Use per-spec database resets or rollback cleanup.
BeforeEach starts a fresh transaction from the same database.Container, with no rollback or schema reset, so any NetworkClass created by previous specs persists in shared DB state across the suite. Add per-spec cleanup/rollback before relying on invariants like “no VirtualNetwork for plain-tenant”.
🤖 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_tenants_server_test.go` around lines 551 - 569, Add
per-spec database isolation around the setup used by the Create test suite,
ensuring each BeforeEach transaction rolls back or resets the schema/container
state before the next spec runs. Preserve the “no NetworkClass” precondition in
the “creates tenant without default networking when no NetworkClass exists” spec
so its VirtualNetwork assertion is independent of prior specs.
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| if s.defaultNetworking != nil { | ||
| if provisionErr := s.defaultNetworking.Provision(ctx, name); provisionErr != nil { | ||
| s.logger.ErrorContext(ctx, "Failed to provision default networking", | ||
| slog.String("tenant", name), | ||
| slog.Any("error", provisionErr)) | ||
| err = grpcstatus.Errorf(grpccodes.Internal, | ||
| "failed to provision default networking resources: %v", provisionErr) | ||
| return | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Atomicity here rests on an unstated assumption.
Returning Internal cleans up the half-created tenant only because the gRPC startup wraps each RPC in a single database transaction. That's correct today, but nothing in this file or in DefaultNetworkingProvisioner says so, and Provision is a public method that a future caller (a reconciler, a CLI, a background job) could invoke outside a transaction — at which point a mid-way failure leaves orphaned PENDING networking with no owner. A short comment here, or a doc note on Provision, would pin the contract down.
Based on learnings: the grpcserver startup configures an interceptor chain that wraps each incoming RPC handler in a single PostgreSQL transaction.
🤖 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_tenants_server.go` around lines 209 - 223, Add a
concise contract comment near the default networking Provision call in the
tenant creation flow, or on DefaultNetworkingProvisioner.Provision, stating that
atomic cleanup depends on the surrounding single PostgreSQL transaction and
callers outside that transaction must not assume rollback of partial
provisioning. Keep the existing error handling unchanged.
Source: Learnings
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
Create default networking resources (VirtualNetwork, IPv4/IPv6 Subnets, SecurityGroup, and optionally ExternalIP + NATGateway) when a tenant is created. Resources are created via DAOs directly to bypass server-level async preconditions (VN must be READY for Subnet, ExternalIP must be ALLOCATED for NATGateway). All resources start in PENDING state and are reconciled by the operator. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Add DefaultNetworkingProvisioner as an optional dependency on PrivateTenantsServer. When set, Provision() is called after tenant creation within the same DB transaction. Wire provisioner in the grpc server startup. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
- Remove dead newDAO stub from Build() - Add transaction contract doc on Provision() - Assert owner-reference values (not just key presence) in tests - Add NATGateway positive test (pool capacity, attached flag) and no-pool error test - Replace boolPtr/int32Ptr/stringPtr helpers with new() expressions - Fix ExternalIP pool selection to filter in Go (CEL filter cannot compare proto enum fields as strings) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
ae32894 to
83c94a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/servers/default_networking_provisioner.go (3)
324-329: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
implementation_strategystill hardcoded on SecurityGroup while VirtualNetwork gets it from NetworkClass.Line 248 propagates
nc.GetImplementationStrategy()to the VN; the SecurityGroup at line 328 is pinned to"network_policy", so anetrisNetworkClass produces a VN/SecurityGroup pair with disagreeing strategies. This was flagged previously and remains unresolved.🐛 Proposed fix
func (p *DefaultNetworkingProvisioner) createDefaultSecurityGroup( ctx context.Context, - tenantName, vnID string, + tenantName, vnID, implementationStrategy string, defaults *privatev1.NetworkDefaults, ) (string, error) { ... Spec: privatev1.SecurityGroupSpec_builder{ VirtualNetwork: vnID, Ingress: defaults.GetIngressRules(), Egress: defaults.GetEgressRules(), - ImplementationStrategy: "network_policy", + ImplementationStrategy: implementationStrategy, }.Build(),And update the call site in
Provisionto passnc.GetImplementationStrategy().🤖 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/default_networking_provisioner.go` around lines 324 - 329, Update the SecurityGroupSpec construction in Provision to use the NetworkClass implementation strategy instead of the hardcoded "network_policy" value, and pass nc.GetImplementationStrategy() through the relevant call path so the VirtualNetwork and SecurityGroup strategies remain consistent.
230-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFive near-identical create helpers still duplicated.
Each helper builds metadata (
defaultlabel,systemcreator, optional owner-reference annotation), calls<dao>.Create().SetObject(x).Do(ctx), and returnsresp.GetObject().GetId(). A shareddefaultMetadata(tenantName, name, ownerID string) *privatev1.Metadatahelper removes the copy-paste. Flagged previously, still open.Also applies to: 269-306, 307-341, 397-424, 425-453
🤖 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/default_networking_provisioner.go` around lines 230 - 267, Extract the repeated metadata construction from createDefaultVirtualNetwork and the other four default create helpers into a shared defaultMetadata(tenantName, name, ownerID string) helper returning *privatev1.Metadata. Preserve the default label, system creator, and optional owner-reference annotation behavior, then update each helper to reuse it while leaving their DAO creation and ID return flows unchanged.
454-472: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPool capacity update still unguarded against nil status and negative availability.
pool.GetStatus()returns nil for a pool without status, soSetAllocatedon line 464 panics. Nothing re-validatesavailable >= deltabetween selection (line 384-386) and this update, soavailablecan go negative. Same finding as the prior review, still unaddressed.🛡️ Proposed fix
pool := getResponse.GetObject() + if !pool.HasStatus() { + return fmt.Errorf("ExternalIPPool '%s' has no status", poolID) + } + if pool.GetStatus().GetAvailable() < delta { + return fmt.Errorf( + "ExternalIPPool '%s' has %d available addresses, need %d", + poolID, pool.GetStatus().GetAvailable(), delta, + ) + } pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta) pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta)🤖 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/default_networking_provisioner.go` around lines 454 - 472, Update updatePoolCapacity to safely initialize or reject a nil pool status before accessing allocation fields, and validate that the current available capacity is at least delta before applying the update. Return a descriptive error without persisting changes when the status is missing or availability would become negative; preserve the existing locked fetch and update error handling.internal/servers/private_tenants_server_test.go (1)
484-570: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNo coverage for the provisioning-failure branch.
PrivateTenantsServer.Createnow returnsInternalwhenProvisionfails (private_tenants_server.go lines 213-221), but this context only exercises the success and no-default-NetworkClass paths. Add a spec that forcesProvisionto fail (e.g.enable_nat_gateway: truewith no eligibleExternalIPPool) and assert the returnedInternalcode/message.🤖 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_tenants_server_test.go` around lines 484 - 570, Add a spec in the “with default networking provisioner” context that creates a default NetworkClass requiring NAT (enable_nat_gateway true) without an eligible ExternalIPPool, then calls PrivateTenantsServer.Create and asserts it returns an Internal error with the expected provisioning-failure message. Reuse the existing provisionerServer setup and verify the failure branch in Create rather than asserting successful tenant provisioning.
🤖 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/default_networking_provisioner.go`:
- Around line 69-163: Refactor DefaultNetworkingProvisionerBuilder.Build by
introducing a local or private generic helper that constructs each DAO with the
shared logger, tenancy logic, metrics registerer, and Build chain. Replace the
repeated construction blocks for networkClassDao, virtualNetworkDao, subnetDao,
securityGroupDao, externalIPDao, externalIPPoolDao, natGatewayDao, and tenantDao
with helper calls, preserving existing error propagation and result
initialization.
---
Duplicate comments:
In `@internal/servers/default_networking_provisioner.go`:
- Around line 324-329: Update the SecurityGroupSpec construction in Provision to
use the NetworkClass implementation strategy instead of the hardcoded
"network_policy" value, and pass nc.GetImplementationStrategy() through the
relevant call path so the VirtualNetwork and SecurityGroup strategies remain
consistent.
- Around line 230-267: Extract the repeated metadata construction from
createDefaultVirtualNetwork and the other four default create helpers into a
shared defaultMetadata(tenantName, name, ownerID string) helper returning
*privatev1.Metadata. Preserve the default label, system creator, and optional
owner-reference annotation behavior, then update each helper to reuse it while
leaving their DAO creation and ID return flows unchanged.
- Around line 454-472: Update updatePoolCapacity to safely initialize or reject
a nil pool status before accessing allocation fields, and validate that the
current available capacity is at least delta before applying the update. Return
a descriptive error without persisting changes when the status is missing or
availability would become negative; preserve the existing locked fetch and
update error handling.
In `@internal/servers/private_tenants_server_test.go`:
- Around line 484-570: Add a spec in the “with default networking provisioner”
context that creates a default NetworkClass requiring NAT (enable_nat_gateway
true) without an eligible ExternalIPPool, then calls PrivateTenantsServer.Create
and asserts it returns an Internal error with the expected provisioning-failure
message. Reuse the existing provisionerServer setup and verify the failure
branch in Create rather than asserting successful tenant provisioning.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b597fe6-e72b-40dd-acd3-7b0a86a6f250
📒 Files selected for processing (5)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/servers/default_networking_provisioner.gointernal/servers/default_networking_provisioner_test.gointernal/servers/private_tenants_server.gointernal/servers/private_tenants_server_test.go
| func (b *DefaultNetworkingProvisionerBuilder) Build() (result *DefaultNetworkingProvisioner, err error) { | ||
| if b.logger == nil { | ||
| err = errors.New("logger is mandatory") | ||
| return | ||
| } | ||
| if b.tenancyLogic == nil { | ||
| err = errors.New("tenancy logic is mandatory") | ||
| return | ||
| } | ||
|
|
||
| networkClassDao, err := dao.NewGenericDAO[*privatev1.NetworkClass](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| virtualNetworkDao, err := dao.NewGenericDAO[*privatev1.VirtualNetwork](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| subnetDao, err := dao.NewGenericDAO[*privatev1.Subnet](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| securityGroupDao, err := dao.NewGenericDAO[*privatev1.SecurityGroup](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| externalIPDao, err := dao.NewGenericDAO[*privatev1.ExternalIP](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| externalIPPoolDao, err := dao.NewGenericDAO[*privatev1.ExternalIPPool](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| natGatewayDao, err := dao.NewGenericDAO[*privatev1.NATGateway](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| tenantDao, err := dao.NewGenericDAO[*privatev1.Tenant](). | ||
| SetLogger(b.logger). | ||
| SetTenancyLogic(b.tenancyLogic). | ||
| SetMetricsRegisterer(b.metricsRegisterer). | ||
| Build() | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| result = &DefaultNetworkingProvisioner{ | ||
| logger: b.logger, | ||
| networkClassDao: networkClassDao, | ||
| virtualNetworkDao: virtualNetworkDao, | ||
| subnetDao: subnetDao, | ||
| securityGroupDao: securityGroupDao, | ||
| externalIPDao: externalIPDao, | ||
| externalIPPoolDao: externalIPPoolDao, | ||
| natGatewayDao: natGatewayDao, | ||
| tenantDao: tenantDao, | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Build repeats the same 4-line DAO construction 7 times.
Every DAO (networkClassDao, virtualNetworkDao, subnetDao, securityGroupDao, externalIPDao, externalIPPoolDao, natGatewayDao, tenantDao) is built with an identical SetLogger/SetTenancyLogic/SetMetricsRegisterer/Build chain. A small generic helper collapses ~70 lines into one function call per DAO.
♻️ Proposed refactor
+func buildDAO[T dao.Object](
+ logger *slog.Logger,
+ tenancyLogic auth.TenancyLogic,
+ registerer prometheus.Registerer,
+) (*dao.GenericDAO[T], error) {
+ return dao.NewGenericDAO[T]().
+ SetLogger(logger).
+ SetTenancyLogic(tenancyLogic).
+ SetMetricsRegisterer(registerer).
+ Build()
+}
+
func (b *DefaultNetworkingProvisionerBuilder) Build() (result *DefaultNetworkingProvisioner, err error) {
...
- networkClassDao, err := dao.NewGenericDAO[*privatev1.NetworkClass]().
- SetLogger(b.logger).
- SetTenancyLogic(b.tenancyLogic).
- SetMetricsRegisterer(b.metricsRegisterer).
- Build()
+ networkClassDao, err := buildDAO[*privatev1.NetworkClass](b.logger, b.tenancyLogic, b.metricsRegisterer)
if err != nil {
return
}
// ... repeat for the remaining 7 DAOs🤖 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/default_networking_provisioner.go` around lines 69 - 163,
Refactor DefaultNetworkingProvisionerBuilder.Build by introducing a local or
private generic helper that constructs each DAO with the shared logger, tenancy
logic, metrics registerer, and Build chain. Replace the repeated construction
blocks for networkClassDao, virtualNetworkDao, subnetDao, securityGroupDao,
externalIPDao, externalIPPoolDao, natGatewayDao, and tenantDao with helper
calls, preserving existing error propagation and result initialization.
- Add owner-reference annotation on NATGateway pointing to VN ID, consistent with Subnet and SecurityGroup - Remove internal error details from gRPC response (already logged via ErrorContext) Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
danmanor
changed the title
ori-amizur
left a comment
ori-amizur
left a comment
There was a problem hiding this comment.
Review comments on a few spots — mostly clarification questions and one potential gap.
| VirtualNetwork: vnID, | ||
| Ingress: defaults.GetIngressRules(), | ||
| Egress: defaults.GetEgressRules(), | ||
| ImplementationStrategy: "network_policy", |
There was a problem hiding this comment.
Is "network_policy" intentionally hardcoded here? The VirtualNetwork correctly copies implementation_strategy from the NetworkClass, but the SecurityGroup uses a static value. Same question about Region: "default" on the VN (line 246) — should these come from the NetworkClass or deployment config, or are they always fixed?
There was a problem hiding this comment.
Both are intentional:
-
"network_policy"— matches the existing pattern inprivate_security_groups_server.go:31(const securityGroupImplementationStrategy = "network_policy"). The SG server always hardcodes this on Create (line 161). SGs are enforced via Kubernetes NetworkPolicy, not via the fabric manager. -
"default"region — matches the public VN server behavior when region is unset (virtual_networks_server.go:244-245). Region is a private-API field; the public server always sets it to"default".
| } | ||
|
|
||
| func (p *DefaultNetworkingProvisioner) findExternalIPPool(ctx context.Context) (*privatev1.ExternalIPPool, error) { | ||
| listResponse, err := p.externalIPPoolDao.List().Do(ctx) |
There was a problem hiding this comment.
findExternalIPPool lists all pools with no tenant filter. In a multi-tenant deployment, this would return pools from every tenant. Should it be scoped to system-level pools or pools available to the target tenant?
There was a problem hiding this comment.
ExternalIPPools are platform-level resources, not tenant-scoped. The existing private_external_ips_server.go also does unscoped pool lookups via validatePoolReference (line 275) — pools are managed by Cloud Provider Admins and shared across tenants. The DAO List() here follows the same pattern.
|
|
||
| pool := getResponse.GetObject() | ||
| pool.GetStatus().SetAllocated(pool.GetStatus().GetAllocated() + delta) | ||
| pool.GetStatus().SetAvailable(pool.GetStatus().GetAvailable() - delta) |
There was a problem hiding this comment.
After the locked Get, there is no check that available >= delta before decrementing. The available > 0 filter in findExternalIPPool runs before the lock is acquired, so by the time the row is locked and re-read here, the capacity could have changed. A guard like:
if pool.GetStatus().GetAvailable() < delta {
return fmt.Errorf("ExternalIP pool %s has insufficient capacity", poolID)
}after the locked read would close this gap.
There was a problem hiding this comment.
Good catch — fixed in commit 90fb398. Added newAvailable < 0 guard after the locked read, matching the existing pattern in private_external_ips_server.go:325.
Check newAvailable >= 0 after acquiring the row lock in updatePoolCapacity, matching the existing pattern in private_external_ips_server.go. Closes the TOCTOU gap between the unlocked findExternalIPPool check and the locked capacity update. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/servers/default_networking_provisioner.go (1)
466-473:⚠️ Potential issue | 🟠 MajorGuard missing pool status before mutation.
pool.GetStatus()can be nil; theSetAllocated/SetAvailablecalls then panic even though the capacity check is otherwise correct. Return an error before reading or mutating the status.🛡️ Proposed fix
pool := getResponse.GetObject() + if !pool.HasStatus() { + return fmt.Errorf("ExternalIP pool '%s' has no status", poolID) + } newAllocated := pool.GetStatus().GetAllocated() + delta🤖 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/default_networking_provisioner.go` around lines 466 - 473, In the pool status update logic, validate that pool.GetStatus() is non-nil before calculating newAllocated/newAvailable or mutating it. Return an appropriate error when status is missing, while preserving the existing capacity check and updates for valid statuses.
🤖 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/default_networking_provisioner_test.go`:
- Around line 324-334: Extend the NAT gateway assertions in the test around ng
and vnID to verify ng.GetSpec().GetVirtualNetwork() equals vnID, preserving the
existing owner-reference and other state checks.
---
Duplicate comments:
In `@internal/servers/default_networking_provisioner.go`:
- Around line 466-473: In the pool status update logic, validate that
pool.GetStatus() is non-nil before calculating newAllocated/newAvailable or
mutating it. Return an appropriate error when status is missing, while
preserving the existing capacity check and updates for valid statuses.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 799727e9-0dbd-4cc7-9c7a-1c5c3da62f86
📒 Files selected for processing (3)
internal/servers/default_networking_provisioner.gointernal/servers/default_networking_provisioner_test.gointernal/servers/private_tenants_server.go
| vnList, err := provisioner.virtualNetworkDao.List(). | ||
| SetFilter("this.metadata.tenant == 'nat-tenant'"). | ||
| Do(ctx) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| vnID := vnList.GetItems()[0].GetId() | ||
|
|
||
| ng := ngList.GetItems()[0] | ||
| Expect(ng.GetMetadata().GetLabels()).To(HaveKeyWithValue("osac.openshift.io/default", "true")) | ||
| Expect(ng.GetMetadata().GetAnnotations()).To(HaveKeyWithValue("osac.openshift.io/owner-reference", vnID)) | ||
| Expect(ng.GetSpec().GetExternalIp()).To(Equal(eip.GetId())) | ||
| Expect(ng.GetStatus().GetState()).To(Equal(privatev1.NATGatewayState_NAT_GATEWAY_STATE_PENDING)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the NATGateway’s VirtualNetwork link too.
The test captures vnID but only verifies the owner-reference annotation. Add an assertion for ng.GetSpec().GetVirtualNetwork() so a regression can’t leave the NATGateway associated with the wrong or empty VirtualNetwork.
🧪 Proposed assertion
ng := ngList.GetItems()[0]
Expect(ng.GetMetadata().GetLabels()).To(HaveKeyWithValue("osac.openshift.io/default", "true"))
Expect(ng.GetMetadata().GetAnnotations()).To(HaveKeyWithValue("osac.openshift.io/owner-reference", vnID))
+ Expect(ng.GetSpec().GetVirtualNetwork()).To(Equal(vnID))
Expect(ng.GetSpec().GetExternalIp()).To(Equal(eip.GetId()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vnList, err := provisioner.virtualNetworkDao.List(). | |
| SetFilter("this.metadata.tenant == 'nat-tenant'"). | |
| Do(ctx) | |
| Expect(err).ToNot(HaveOccurred()) | |
| vnID := vnList.GetItems()[0].GetId() | |
| ng := ngList.GetItems()[0] | |
| Expect(ng.GetMetadata().GetLabels()).To(HaveKeyWithValue("osac.openshift.io/default", "true")) | |
| Expect(ng.GetMetadata().GetAnnotations()).To(HaveKeyWithValue("osac.openshift.io/owner-reference", vnID)) | |
| Expect(ng.GetSpec().GetExternalIp()).To(Equal(eip.GetId())) | |
| Expect(ng.GetStatus().GetState()).To(Equal(privatev1.NATGatewayState_NAT_GATEWAY_STATE_PENDING)) | |
| vnList, err := provisioner.virtualNetworkDao.List(). | |
| SetFilter("this.metadata.tenant == 'nat-tenant'"). | |
| Do(ctx) | |
| Expect(err).ToNot(HaveOccurred()) | |
| vnID := vnList.GetItems()[0].GetId() | |
| ng := ngList.GetItems()[0] | |
| Expect(ng.GetMetadata().GetLabels()).To(HaveKeyWithValue("osac.openshift.io/default", "true")) | |
| Expect(ng.GetMetadata().GetAnnotations()).To(HaveKeyWithValue("osac.openshift.io/owner-reference", vnID)) | |
| Expect(ng.GetSpec().GetVirtualNetwork()).To(Equal(vnID)) | |
| Expect(ng.GetSpec().GetExternalIp()).To(Equal(eip.GetId())) | |
| Expect(ng.GetStatus().GetState()).To(Equal(privatev1.NATGatewayState_NAT_GATEWAY_STATE_PENDING)) |
🤖 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/default_networking_provisioner_test.go` around lines 324 -
334, Extend the NAT gateway assertions in the test around ng and vnID to verify
ng.GetSpec().GetVirtualNetwork() equals vnID, preserving the existing
owner-reference and other state checks.
|
/override "E2E BMaaS Full Install / e2e" |
|
@danmanor: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
/override e2e-bmaas-full-install / e2e |
|
@danmanor: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
/override "e2e-bmaas-full-install / e2e" |
|
@danmanor: Overrode contexts on behalf of danmanor: e2e-bmaas-full-install / e2e 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 kubernetes-sigs/prow repository. |
|
/lgtm |
e20fdfc
into
osac-project:main
This branch was previously deployed
3 participants
Summary
DefaultNetworkingProvisionerthat creates default networking resources (VirtualNetwork, IPv4 Subnet, IPv6 Subnet, SecurityGroup, and optionally ExternalIP + NATGateway) when a tenant is createdPrivateTenantsServer.Createas an optional dependencyJira
https://redhat.atlassian.net/browse/OSAC-2341
Design
Default Networking EP — §Tenant Onboarding
Changes
New file:
internal/servers/default_networking_provisioner.goDefaultNetworkingProvisionerwith builder patternProvision(ctx, tenantName)— reads default NetworkClass, creates VN/Subnets/SG/NATGatewayosac.openshift.io/default: "true"with correct tenant annotationosac.openshift.io/owner-referenceannotation pointing to VNModified:
internal/servers/private_tenants_server.goDefaultNetworkingProvisionerdependency viaSetDefaultNetworkingProvisioner()Create()callsProvision()after successful tenant creation (same DB transaction)Modified:
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.goTest plan
DefaultNetworkingProvisionercovering:enable_nat_gatewayis falsegolangci-lint: 0 issuesgofmt: no formatting changes🤖 Generated with Claude Code
Summary by CodeRabbit