OSAC-3210: reject Cluster creation when NetworkClass has no k8s_manager

[ori-amizur]

Validate during Cluster creation that the network attachment's NetworkClass (resolved via Subnet → VirtualNetwork → NetworkClass) has a k8s_manager configured. Without k8s_manager, the hosting cluster has no MetalLB IPAddressPool and CaaS VIP allocation cannot function.

Follows the BareMetalInstance pattern (OSAC-1464) which validates fabric_manager on the same chain. Dangling references are skipped — they fail independently during provisioning.

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

• New Features

  • Cluster creation now validates network attachments and their associated network configuration.
  • Creation is rejected when the selected network class lacks the required Kubernetes manager configuration.
  • Clusters without network attachments continue to be created without this validation.

• Bug Fixes

  • Missing referenced network resources are handled safely.
  • Network lookup failures now return an appropriate internal error.

[openshift-ci-robot]

@ori-amizur: This pull request references OSAC-3210 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.

Details

In response to this:

Validate during Cluster creation that the network attachment's NetworkClass (resolved via Subnet → VirtualNetwork → NetworkClass) has a k8s_manager configured. Without k8s_manager, the hosting cluster has no MetalLB IPAddressPool and CaaS VIP allocation cannot function.

Follows the BareMetalInstance pattern (OSAC-1464) which validates fabric_manager on the same chain. Dangling references are skipped — they fail independently during provisioning.

Assisted-by: Claude Code noreply@anthropic.com

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.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ori-amizur

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• fulfillment-service/OWNERS [ori-amizur]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 36f91bf2-2096-4359-bdb3-d4749ced9ab2

📥 Commits

Reviewing files that changed from the base of the PR and between 8f49bbc and 6b14da6.

📒 Files selected for processing (2)

• fulfillment-service/internal/servers/private_clusters_server.go
• fulfillment-service/internal/servers/private_clusters_server_test.go

---

Walkthrough

PrivateClustersServer now initializes network DAOs and validates subnet, virtual network, and network class references during cluster creation. Creation requires k8s_manager when a network attachment is present. Tests cover rejection, success, and skipped validation cases.

Changes

Network attachment validation

Layer / File(s)	Summary
Network DAO wiring fulfillment-service/internal/servers/private_clusters_server.go	The server constructs and stores DAOs for subnets, virtual networks, and network classes.
Network attachment checks fulfillment-service/internal/servers/private_clusters_server.go, fulfillment-service/internal/servers/private_clusters_server_test.go	Cluster creation resolves network references and returns internal errors for lookup failures. A network class without k8s_manager returns FailedPrecondition. Tests cover rejected, accepted, and unattached clusters.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Mergeability Score: ⚪ Minimal · up to 6b14d

The PR adds localized Cluster-creation validation with accompanying tests; no actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant PrivateClustersServer
  participant subnetDAO
  participant virtualNetworkDAO
  participant networkClassDAO
  participant GenericServer
  PrivateClustersServer->>subnetDAO: Resolve attached subnet
  subnetDAO->>virtualNetworkDAO: Resolve virtual network
  virtualNetworkDAO->>networkClassDAO: Resolve network class
  networkClassDAO-->>PrivateClustersServer: Return network class managers
  PrivateClustersServer->>GenericServer: Create cluster after validation

Loading

Possibly related PRs

• osac-project/osac#262: Extends the same PrivateClustersServer network-attachment validation flow.
• osac-project/osac#294: Modifies related PrivateClustersServer cluster-creation handling.
• osac-project/osac#84: Adds related network-attachment validation in cluster creation.

Suggested labels: enhancement, go

Suggested reviewers: eranco74, vladikr, omer-vishlitzky

🚥 Pre-merge checks | ✅ 11

✅ Passed checks (11 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: rejecting Cluster creation when NetworkClass lacks k8s_manager.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets	✅ Passed	The PR diff adds DAO/validation logic and test data only; scans found no added API keys, tokens, passwords, private keys, credential URLs, or long encoded strings.
No-Weak-Crypto	✅ Passed	The PR adds DAO lookups and k8s_manager validation only; added lines contain no weak algorithms, crypto APIs, custom crypto, or secret comparisons.
No-Injection-Vectors	✅ Passed	The added code only resolves references through DAO SetId calls; DAO binds IDs as SQL parameters. No added shell, eval/exec, pickle, unsafe YAML, OS command, or HTML injection sink exists.
Container-Privileges	✅ Passed	The PR changes only Go server and test files; it adds no container or Kubernetes manifests and no privilege settings such as hostNetwork, hostPID, SYS_ADMIN, or privileged.
No-Sensitive-Data-In-Logs	✅ Passed	The diff adds only resource keys and DAO errors to validation logs; it does not log passwords, tokens, PII, hostnames, or customer payloads, matching existing BareMetal logging patterns.
Ai-Attribution	✅ Passed	The PR commit includes the parsed Assisted-by: Claude Code <noreply@anthropic.com> trailer and has no AI Co-Authored-By trailer.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 10:43 AM UTC · Ended 10:49 AM UTC

Commit: 2ed5956 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:50 AM UTC · Completed 11:05 AM UTC

Commit: 711c661 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [naming-convention] fulfillment-service/internal/servers/private_clusters_server.go:755 — Log messages use lowercase verb (failed to query subnet for k8s_manager validation) while the established convention across all servers in this package is sentence-case (Failed to ...). The analogous function in private_baremetal_instances_server.go uses Failed to lookup subnet for fabric manager validation. The same file's own lookupCatalogItem method uses Failed to lookup catalog item.
  Remediation: Capitalise the log messages to match the codebase pattern: Failed to query subnet for k8s_manager validation, Failed to query virtual network for k8s_manager validation, Failed to query network class for k8s_manager validation.

• [naming-convention] fulfillment-service/internal/servers/private_clusters_server.go:756 — Structured log keys use _key suffix (subnet_key, virtual_network_key, network_class_key) but the established convention across the servers package is _id suffix (subnet_id in private_baremetal_instances_server.go, private_compute_instances_server.go; virtual_network_id in private_nat_gateways_server.go; network_class_id in private_nat_gateways_server.go).
  Remediation: Rename slog keys: subnet_key → subnet_id, virtual_network_key → virtual_network_id, network_class_key → network_class_id.

Low

• [test-inadequate] fulfillment-service/internal/servers/private_clusters_server_test.go — Tests do not cover the dangling-reference skip paths (subnet/virtual-network/network-class not found) or the internal-error paths (non-NotFound DAO errors). The BareMetalInstance equivalent also lacks these tests, so this is a pre-existing gap. Additionally, there is no test for the catalog-item creation path with a NetworkClass that has no k8s_manager — only the direct-template path is exercised.

Previous run

Review

Findings

Low

• [naming-convention] fulfillment-service/internal/servers/private_clusters_server.go:757 — Log messages use lowercase ("failed to query subnet for k8s_manager validation") while the established codebase pattern is title-case ("Failed to ..."). Same inconsistency on lines 773 and 789. Additionally, structured log keys use "_key" suffix (e.g. "subnet_key") while the BareMetalInstance analog uses "_id" suffix (e.g. "subnet_id").
  Remediation: Capitalize log messages to "Failed to query ..." and rename slog keys to "subnet_id", "virtual_network_id", "network_class_id".

• [code-organization] fulfillment-service/internal/servers/private_clusters_server.go:145 — The three new DAO creation blocks in Build() lack the "// Create the X DAO:" comment that precedes every other DAO block in the same method.
  Remediation: Add comments: "// Create the subnets DAO:", "// Create the virtual networks DAO:", "// Create the network classes DAO:".

• [edge-case] fulfillment-service/internal/servers/private_clusters_server_test.go — Tests cover the main paths (reject when no k8s_manager, allow when k8s_manager present, skip when no network_attachment) but do not cover dangling-reference skip paths or empty-key early returns. This matches the pre-existing pattern in the BareMetalInstance analog.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:18 PM UTC · Completed 12:35 PM UTC

Commit: 6b14da6 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] naming-convention

Log messages use lowercase verb (failed to query subnet for k8s_manager validation) while the established convention across all servers in this package is sentence-case (Failed to ...). The analogous function in private_baremetal_instances_server.go uses Failed to lookup subnet for fabric manager validation.

Suggested fix: Capitalise the log messages to match the codebase pattern.

[fullsend-ai-review]

[medium] naming-convention

Structured log keys use _key suffix (subnet_key, virtual_network_key, network_class_key) but the established convention across the servers package is _id suffix (subnet_id, virtual_network_id, network_class_id).

Suggested fix: Rename slog keys: subnet_key -> subnet_id, virtual_network_key -> virtual_network_id, network_class_key -> network_class_id.