OSAC-365: canonicalize CIDR values on VirtualNetwork and Subnet Create

[tchughesiv]

Canonicalize CIDR values on Create for Subnet and VirtualNetwork

Jira: https://redhat.atlassian.net/browse/OSAC-365
Story type: Task

Summary

Normalize CIDR values to canonical form when creating VirtualNetworks and Subnets so stored values are consistent regardless of how callers express the network address (e.g., 10.0.1.5/24 → 10.0.1.0/24). Canonicalization applies on Create only; Update and SecurityGroup rule validation are unchanged.

Changes

• Extract shared parseAndValidateCIDR and validateCIDR helpers into internal/servers/cidr_validation.go
• Canonicalize ipv4_cidr and ipv6_cidr on VirtualNetwork Create in validateVirtualNetwork
• Canonicalize ipv4_cidr and ipv6_cidr on Subnet Create in validateSubnet (before subset/overlap checks)
• Add contract tests for VN/Subnet Create canonicalization, Create round-trip, Update no-op, and legacy non-canonical parent VN

Testing

• Unit tests: 8 new Ginkgo specs in private_virtual_networks_server_test.go and private_subnets_server_test.go
• Integration tests: N/A (not run locally; CI will exercise it/ on PR)
• Coverage: parseAndValidateCIDR and validateCIDR at 100%; full internal/ suite passes (918/918 server specs, 64 suites)

### VirtualNetwork IPv4 canonicalization 
% osac create virtualnetwork \
  --name osac-365-vn-ipv4 \
  --network-class "$NC" \
  --ipv4-cidr 10.0.1.5/24
Created virtual network 'osac-365-vn-ipv4' (ID: 019ebcd1-b9a3-7216-9256-e0fbd4cabe14).

% osac get virtualnetwork osac-365-vn-ipv4 -o yaml | grep -i cidr
  ipv4_cidr: 10.0.1.0/24

### VirtualNetwork IPv6 canonicalization
% osac create virtualnetwork \
  --name osac-365-vn-ipv6 \
  --network-class "$NC" \
  --ipv6-cidr 2001:db8::1/32
osac get virtualnetwork osac-365-vn-ipv6 -o yaml | grep -i cidr

Created virtual network 'osac-365-vn-ipv6' (ID: 019ebcd2-1a9c-7b93-8b44-0b80376c32bc).
  ipv6_cidr: 2001:db8::/32

### Subnet IPv4 canonicalization
% osac create subnet \
  --name osac-365-subnet-ipv4 \
  --virtual-network osac-365-vn-parent \
  --ipv4-cidr 10.0.1.5/24
Created subnet 'osac-365-subnet-ipv4' (ID: 019ebcd3-ad68-748e-957c-ba51ad846c03).

% osac get subnet osac-365-subnet-ipv4 -o yaml | grep -i cidr
  ipv4_cidr: 10.0.1.0/24

Deployment note

Before rollout, audit staging/prod for existing non-canonical CIDRs (operational prerequisite noted in the Jira ticket — not a code deliverable for this PR).

Acceptance Criteria

• Non-canonical CIDRs normalized on Create (e.g., 10.0.1.5/24 → 10.0.1.0/24)
• Scope limited to VirtualNetwork and Subnet Create; both IPv4 and IPv6 fields
• Existing validation preserved (invalid format, wrong IP version)
• Tested in OSAC cluster
• Pre-flight data audit (operational — verify in staging/prod before rollout)

Summary by CodeRabbit

• Improvements

  • Private subnet and virtual network CIDR blocks are now automatically canonicalized on creation, ensuring consistent representation across both IPv4 and IPv6.

• Tests

  • Added validation tests for CIDR canonicalization behavior.

[openshift-ci-robot]

@tchughesiv: This pull request references [OSAC-365](https://redhat.atlassian.net/browse/OS[AC-3](https://redhat.atlassian.net/browse/AC-3)65) 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:

[OSAC-365](https://redhat.atlassian.net/browse/OS[AC-3](https://redhat.atlassian.net/browse/AC-3)65): Canonicalize CIDR values on Create for Subnet and VirtualNetwork

Jira: [OSAC-365](https://redhat.atlassian.net/browse/OS[AC-3](https://redhat.atlassian.net/browse/AC-3)65)
Story type: Task

Summary

Normalize CIDR values to canonical form when creating VirtualNetworks and Subnets so stored values are consistent regardless of how callers express the network address (e.g., 10.0.1.5/24 → 10.0.1.0/24). Canonicalization applies on Create only; Update and SecurityGroup rule validation are unchanged.

Changes

• Extract shared parseAndValidateCIDR and validateCIDR helpers into internal/servers/cidr_validation.go
• Canonicalize ipv4_cidr and ipv6_cidr on VirtualNetwork Create in validateVirtualNetwork
• Canonicalize ipv4_cidr and ipv6_cidr on Subnet Create in validateSubnet (before subset/overlap checks)
• Add contract tests for VN/Subnet Create canonicalization, Create round-trip, Update no-op, and legacy non-canonical parent VN

Testing

• Unit tests: 8 new Ginkgo specs in private_virtual_networks_server_test.go and private_subnets_server_test.go
• Integration tests: N/A (not run locally; CI will exercise it/ on PR)
• Coverage: parseAndValidateCIDR and validateCIDR at 100%; full internal/ suite passes (918/918 server specs, 64 suites)

Deployment note

AC-4 (pre-flight audit of existing non-canonical CIDRs in staging/prod) is an operational prerequisite noted in the Jira ticket — not a code deliverable for this PR.

Acceptance Criteria

• AC-1: Non-canonical CIDRs normalized on Create (e.g., 10.0.1.5/24 → 10.0.1.0/24)
• AC-2: Scope limited to VirtualNetwork and Subnet Create; both IPv4 and IPv6 fields
• AC-3: Existing validation preserved (invalid format, wrong IP version)
• AC-4: Pre-flight data audit (operational — verify in staging/prod before rollout)

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]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tchughesiv
Once this PR has been reviewed and has the lgtm label, please assign trewest for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci-robot]

@tchughesiv: This pull request references [OSAC-365](https://redhat.atlassian.net/browse/OS[AC-3](https://redhat.atlassian.net/browse/AC-3)65) 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:

OSAC-365: Canonicalize CIDR values on Create for Subnet and VirtualNetwork

Jira: [OSAC-365](https://redhat.atlassian.net/browse/OS[AC-3](https://redhat.atlassian.net/browse/AC-3)65)
Story type: Task

Summary

Normalize CIDR values to canonical form when creating VirtualNetworks and Subnets so stored values are consistent regardless of how callers express the network address (e.g., 10.0.1.5/24 → 10.0.1.0/24). Canonicalization applies on Create only; Update and SecurityGroup rule validation are unchanged.

Changes

• Extract shared parseAndValidateCIDR and validateCIDR helpers into internal/servers/cidr_validation.go
• Canonicalize ipv4_cidr and ipv6_cidr on VirtualNetwork Create in validateVirtualNetwork
• Canonicalize ipv4_cidr and ipv6_cidr on Subnet Create in validateSubnet (before subset/overlap checks)
• Add contract tests for VN/Subnet Create canonicalization, Create round-trip, Update no-op, and legacy non-canonical parent VN

Testing

• Unit tests: 8 new Ginkgo specs in private_virtual_networks_server_test.go and private_subnets_server_test.go
• Integration tests: N/A (not run locally; CI will exercise it/ on PR)
• Coverage: parseAndValidateCIDR and validateCIDR at 100%; full internal/ suite passes (918/918 server specs, 64 suites)

Deployment note

AC-4 (pre-flight audit of existing non-canonical CIDRs in staging/prod) is an operational prerequisite noted in the Jira ticket — not a code deliverable for this PR.

Acceptance Criteria

• AC-1: Non-canonical CIDRs normalized on Create (e.g., 10.0.1.5/24 → 10.0.1.0/24)
• AC-2: Scope limited to VirtualNetwork and Subnet Create; both IPv4 and IPv6 fields
• AC-3: Existing validation preserved (invalid format, wrong IP version)
• AC-4: Pre-flight data audit (operational — verify in staging/prod before rollout)

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]

Warning

Review limit reached

@tchughesiv, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 43 minutes and 8 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

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

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 944c2503-27ad-4936-9d21-211e3c2ad4fe

📥 Commits

Reviewing files that changed from the base of the PR and between 8a26dc2 and b64bf27.

📒 Files selected for processing (6)

• internal/servers/cidr_validation.go
• internal/servers/private_security_groups_server.go
• internal/servers/private_subnets_server.go
• internal/servers/private_subnets_server_test.go
• internal/servers/private_virtual_networks_server.go
• internal/servers/private_virtual_networks_server_test.go

Walkthrough

This PR introduces CIDR canonicalization across subnet and virtual network server validation. A new parseAndValidateCIDR utility handles parsing, IP-version enforcement, and canonical string generation. Both servers now canonicalize CIDR values on Create operations, with comprehensive test coverage verifying the canonicalization behavior and CIDR immutability during updates.

Changes

CIDR Canonicalization on Create

Layer / File(s)	Summary
CIDR Canonicalization Utility internal/servers/cidr_validation.go	New parseAndValidateCIDR function parses input CIDR strings, enforces IP-family constraints (rejecting IPv4 when expecting IPv6 and vice versa), and returns canonical CIDR strings via network.String() or gRPC InvalidArgument errors with field-specific messages. Includes thin validateCIDR wrapper for error-only returns.
VirtualNetwork Server CIDR Canonicalization internal/servers/private_virtual_networks_server.go, internal/servers/private_virtual_networks_server_test.go	validateVirtualNetwork now uses parseAndValidateCIDR for IPv4 and IPv6 validation; on Create, canonicalized values are persisted into the spec via SetIpv4Cidr and SetIpv6Cidr. The local validateCIDR helper and net import are removed. Test contexts verify IPv4/IPv6 canonicalization on Create, persistent canonical values in the response, and that Update operations do not canonicalize (respecting immutable-field checks).
Subnet Server CIDR Canonicalization internal/servers/private_subnets_server.go, internal/servers/private_subnets_server_test.go	validateSubnet now uses parseAndValidateCIDR and canonicalizes CIDR values in the subnet spec on Create. Update operations are unchanged, deferring correctness to immutable-field enforcement. Tests verify IPv4 and IPv6 canonicalization (including scenarios with non-canonical parent VirtualNetwork CIDRs) and confirm canonical CIDR values persist in created Subnet response objects.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• osac-project/fulfillment-service#402: Implements and expands immutable-field enforcement for CIDR changes during Update operations on Subnet and VirtualNetwork resources, complementing this PR's canonicalization on Create.

Suggested labels

approved, lgtm

Suggested reviewers

• trewest
• eranco74
• jhernand

Poem

📐 CIDRs canonicalize with grace,
Each subnet finds its proper place,
Non-canonical inputs transform and align,
Creating networks both valid and fine—
Validation flows pure, immutability thrives. ✨

🚥 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 specifically identifies the main change: CIDR value canonicalization on VirtualNetwork and Subnet Create operations, directly aligned with the changeset's core objective.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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	Scanned the PR’s described changed files (incl. new cidr_validation.go) for PEM keys, embedded-cred URLs, and api_key/secret/token/password string literals + long base64; no hardcoded secrets found.
No-Weak-Crypto	✅ Passed	Repo-wide scan found 0 occurrences of MD5/SHA1/DES/RC4/3DES/Blowfish/ECB (and no crypto keywords) in the PR-related CIDR server/test files.
No-Injection-Vectors	✅ Passed	Scanned PR’s touched files (cidr_validation.go + VN/Subnet server + tests) for SQL concat, shell=True, eval/exec, pickle.loads, yaml.load, os.system, dangerouslySetInnerHTML; none found. Severity low.
Container-Privileges	✅ Passed	PR #687 changes only internal/servers/*.go (CIDR validation/canonicalization); no Kubernetes/container manifests updated and no privileged/hostPID/hostNetwork/hostIPC/SYS_ADMIN/allowPrivilegeEscala...
No-Sensitive-Data-In-Logs	✅ Passed	PR adds CIDR parsing/canonicalization only; no new slog/logger calls. User-supplied CIDRs appear only in returned gRPC InvalidArgument errors, not in log statements.
Ai-Attribution	✅ Passed	Low risk: repo commit message and workspace files contain no AI-tool mentions or trailers (Assisted-by/Generated-by/Co-Authored-By), so no AI attribution issue detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 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 and usage tips.}

[openshift-ci-robot]

@tchughesiv: This pull request references OSAC-365 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:

Canonicalize CIDR values on Create for Subnet and VirtualNetwork

Jira: https://redhat.atlassian.net/browse/OSAC-365
Story type: Task

Summary

Normalize CIDR values to canonical form when creating VirtualNetworks and Subnets so stored values are consistent regardless of how callers express the network address (e.g., 10.0.1.5/24 → 10.0.1.0/24). Canonicalization applies on Create only; Update and SecurityGroup rule validation are unchanged.

Changes

• Extract shared parseAndValidateCIDR and validateCIDR helpers into internal/servers/cidr_validation.go
• Canonicalize ipv4_cidr and ipv6_cidr on VirtualNetwork Create in validateVirtualNetwork
• Canonicalize ipv4_cidr and ipv6_cidr on Subnet Create in validateSubnet (before subset/overlap checks)
• Add contract tests for VN/Subnet Create canonicalization, Create round-trip, Update no-op, and legacy non-canonical parent VN

Testing

• Unit tests: 8 new Ginkgo specs in private_virtual_networks_server_test.go and private_subnets_server_test.go
• Integration tests: N/A (not run locally; CI will exercise it/ on PR)
• Coverage: parseAndValidateCIDR and validateCIDR at 100%; full internal/ suite passes (918/918 server specs, 64 suites)

Deployment note

Before rollout, audit staging/prod for existing non-canonical CIDRs (operational prerequisite noted in the Jira ticket — not a code deliverable for this PR).

Acceptance Criteria

• Non-canonical CIDRs normalized on Create (e.g., 10.0.1.5/24 → 10.0.1.0/24)
• Scope limited to VirtualNetwork and Subnet Create; both IPv4 and IPv6 fields
• Existing validation preserved (invalid format, wrong IP version)
• Pre-flight data audit (operational — verify in staging/prod before rollout)

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.

[ygalblum]

Should IPv4 and IPv6 defined as constants somewhere? Shouldn't break on a typo

[tchughesiv]

Good catch... I’ll add constants in cidr_validation.go and use them at call sites in this PR.

[tchughesiv]

added cidrIPv4/cidrIPv6 constants in cidr_validation.go.

[ygalblum]

Is this method used anywhere? I couldn't find it in this PR, but maybe somewhere else

[tchughesiv]

Yes... still used by SG rule validation:

fulfillment-service/internal/servers/private_security_groups_server.go

Lines 344 to 354 in 8a26dc2

	if err := validateCIDR(rule.GetIpv4Cidr(), "IPv4"); err != nil {
	return grpcstatus.Errorf(grpccodes.InvalidArgument,
	"%s rule at index %d: invalid IPv4 CIDR: %v", ruleType, index, err)
	}
	}
	// Validate IPv6 CIDR format if present
	if rule.GetIpv6Cidr() != "" {
	if err := validateCIDR(rule.GetIpv6Cidr(), "IPv6"); err != nil {
	return grpcstatus.Errorf(grpccodes.InvalidArgument,
	"%s rule at index %d: invalid IPv6 CIDR: %v", ruleType, index, err)

VN/Subnet use parseAndValidateCIDR to canonicalize on Create; SG keeps validateCIDR (validate only) per OSAC-365 scope:

fulfillment-service/internal/servers/cidr_validation.go

Lines 45 to 49 in 8a26dc2

	// validateCIDR validates a CIDR string and checks if it matches the expected IP version.
	func validateCIDR(cidrStr string, ipVersion string) error {
	_, err := parseAndValidateCIDR(cidrStr, ipVersion)
	return err
	}

If you want SG (or PublicIPPool spec.cidrs[]) canonicalized here too, say the word... otherwise I’d track those as follow-ups.

[ygalblum]

I guess this is done to facilitate further validation. But, it's not clear why and why it depends on existingSubnet being nil can you add a comment about it?

[tchughesiv]

added comments on both paths.

existingVN == nil / existingSubnet == nil distinguishes Create from Update. We canonicalize only on Create today because:

• OSAC-365 scope is new Create behavior, not a data migration.
• Legacy rows may already store non-canonical strings; normalizing on Update would rewrite stored values on unrelated updates.
• Immutability is string-based - a client sending the canonical form against a legacy stored value would fail even though the network is the same.

We can do Update canonicalization, but it’s a product decision about migrating existing data... we’d want agreed Update/migration rules, tests for legacy rows, and a quick check that anything downstream wouldn’t break if the stored CIDR text changed but the network didn’t.

[tchughesiv]

/retest-required