diff --git a/internal/servers/cidr_validation.go b/internal/servers/cidr_validation.go new file mode 100644 index 000000000..0b7dc4fa7 --- /dev/null +++ b/internal/servers/cidr_validation.go @@ -0,0 +1,54 @@ +/* +Copyright (c) 2026 Red Hat Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the +License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific +language governing permissions and limitations under the License. +*/ + +package servers + +import ( + "net/netip" + + grpccodes "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" +) + +const ( + cidrIPv4 = "IPv4" + cidrIPv6 = "IPv6" +) + +// parseAndValidateCIDR parses cidrStr, validates it matches the expected IP version, +// and returns the canonical CIDR notation. +func parseAndValidateCIDR(cidrStr string, ipVersion string) (string, error) { + prefix, err := netip.ParsePrefix(cidrStr) + if err != nil { + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, + "invalid %s CIDR format '%s': %v", ipVersion, cidrStr, err) + } + + isIPv4 := prefix.Addr().Is4() + if ipVersion == cidrIPv4 && !isIPv4 { + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, + "field 'ipv4_cidr' contains IPv6 address: %s", cidrStr) + } + if ipVersion == cidrIPv6 && isIPv4 { + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, + "field 'ipv6_cidr' contains IPv4 address: %s", cidrStr) + } + + return prefix.Masked().String(), nil +} + +// 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 +} diff --git a/internal/servers/private_security_groups_server.go b/internal/servers/private_security_groups_server.go index e54b6ec87..cfca8d1ea 100644 --- a/internal/servers/private_security_groups_server.go +++ b/internal/servers/private_security_groups_server.go @@ -341,7 +341,7 @@ func validateSecurityRule(rule *privatev1.SecurityRule, ruleType string, index i // Validate IPv4 CIDR format if present if rule.GetIpv4Cidr() != "" { - if err := validateCIDR(rule.GetIpv4Cidr(), "IPv4"); err != nil { + if err := validateCIDR(rule.GetIpv4Cidr(), cidrIPv4); err != nil { return grpcstatus.Errorf(grpccodes.InvalidArgument, "%s rule at index %d: invalid IPv4 CIDR: %v", ruleType, index, err) } @@ -349,7 +349,7 @@ func validateSecurityRule(rule *privatev1.SecurityRule, ruleType string, index i // Validate IPv6 CIDR format if present if rule.GetIpv6Cidr() != "" { - if err := validateCIDR(rule.GetIpv6Cidr(), "IPv6"); err != nil { + if err := validateCIDR(rule.GetIpv6Cidr(), cidrIPv6); err != nil { return grpcstatus.Errorf(grpccodes.InvalidArgument, "%s rule at index %d: invalid IPv6 CIDR: %v", ruleType, index, err) } diff --git a/internal/servers/private_subnets_server.go b/internal/servers/private_subnets_server.go index 5ccd4d7d4..122cebbc9 100644 --- a/internal/servers/private_subnets_server.go +++ b/internal/servers/private_subnets_server.go @@ -228,16 +228,26 @@ func (s *PrivateSubnetsServer) validateSubnet(ctx context.Context, // SUB-VAL-01: Validate IPv4 CIDR format if spec.GetIpv4Cidr() != "" { - if err := validateCIDR(spec.GetIpv4Cidr(), "IPv4"); err != nil { + canonical, err := parseAndValidateCIDR(spec.GetIpv4Cidr(), cidrIPv4) + if err != nil { return err } + // Canonicalize on Create only; Update must not rewrite immutable CIDR fields (SUB-VAL-11+). + if existingSubnet == nil { + spec.SetIpv4Cidr(canonical) + } } // SUB-VAL-02: Validate IPv6 CIDR format if spec.GetIpv6Cidr() != "" { - if err := validateCIDR(spec.GetIpv6Cidr(), "IPv6"); err != nil { + canonical, err := parseAndValidateCIDR(spec.GetIpv6Cidr(), cidrIPv6) + if err != nil { return err } + // Canonicalize on Create only; Update must not rewrite immutable CIDR fields (SUB-VAL-11+). + if existingSubnet == nil { + spec.SetIpv6Cidr(canonical) + } } // SUB-VAL-04, SUB-VAL-05, SUB-VAL-06, SUB-VAL-07, SUB-VAL-08: Validate parent VirtualNetwork @@ -331,7 +341,7 @@ func (s *PrivateSubnetsServer) validateVirtualNetworkReference(ctx context.Conte "subnet has IPv4 CIDR but parent VirtualNetwork does not support IPv4") } // SUB-VAL-06: Validate IPv4 CIDR is subset of parent - if err := validateCIDRSubset(spec.GetIpv4Cidr(), parentSpec.GetIpv4Cidr(), "IPv4"); err != nil { + if err := validateCIDRSubset(spec.GetIpv4Cidr(), parentSpec.GetIpv4Cidr(), cidrIPv4); err != nil { return err } } @@ -343,7 +353,7 @@ func (s *PrivateSubnetsServer) validateVirtualNetworkReference(ctx context.Conte "subnet has IPv6 CIDR but parent VirtualNetwork does not support IPv6") } // SUB-VAL-06: Validate IPv6 CIDR is subset of parent - if err := validateCIDRSubset(spec.GetIpv6Cidr(), parentSpec.GetIpv6Cidr(), "IPv6"); err != nil { + if err := validateCIDRSubset(spec.GetIpv6Cidr(), parentSpec.GetIpv6Cidr(), cidrIPv6); err != nil { return err } } diff --git a/internal/servers/private_subnets_server_test.go b/internal/servers/private_subnets_server_test.go index 7346d484b..30bcd74b3 100644 --- a/internal/servers/private_subnets_server_test.go +++ b/internal/servers/private_subnets_server_test.go @@ -308,6 +308,75 @@ var _ = Describe("Private subnets server", func() { }) }) + Context("SUB-VAL-01: IPv4 CIDR canonicalization on Create", func() { + It("canonicalizes non-canonical IPv4 CIDR on Create", func() { + vn := createVirtualNetwork(ctx, "10.0.0.0/16", "") + + subnet := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + VirtualNetwork: vn.GetId(), + }.Build(), + }.Build() + + err := server.validateSubnet(ctx, subnet, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(subnet.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + + It("accepts non-canonical subnet CIDR within legacy non-canonical parent", func() { + vn := createVirtualNetwork(ctx, "10.0.0.5/16", "") + + subnet := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + VirtualNetwork: vn.GetId(), + }.Build(), + }.Build() + + err := server.validateSubnet(ctx, subnet, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(subnet.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + }) + + Context("SUB-VAL-02: IPv6 CIDR canonicalization on Create", func() { + It("canonicalizes non-canonical IPv6 CIDR on Create", func() { + vn := createVirtualNetwork(ctx, "", "2001:db8::/32") + + subnet := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv6Cidr: new("2001:db8:1::1/64"), + VirtualNetwork: vn.GetId(), + }.Build(), + }.Build() + + err := server.validateSubnet(ctx, subnet, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(subnet.GetSpec().GetIpv6Cidr()).To(Equal("2001:db8:1::/64")) + }) + }) + + Context("Round-trip: server.Create canonicalizes subnet CIDRs", func() { + It("stores canonical IPv4 CIDR on Create", func() { + vn := createVirtualNetwork(ctx, "10.0.0.0/16", "") + + createResponse, err := server.Create(ctx, privatev1.SubnetsCreateRequest_builder{ + Object: privatev1.Subnet_builder{ + Metadata: privatev1.Metadata_builder{ + Tenant: auth.SharedTenant, + }.Build(), + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + VirtualNetwork: vn.GetId(), + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(createResponse.GetObject().GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + }) + Context("SUB-VAL-03: At least one CIDR required", func() { It("rejects both IPv4 and IPv6 CIDRs empty", func() { vn := createVirtualNetwork(ctx, "10.0.0.0/16", "2001:db8::/32") diff --git a/internal/servers/private_virtual_networks_server.go b/internal/servers/private_virtual_networks_server.go index 9e4f593a5..010a4f480 100644 --- a/internal/servers/private_virtual_networks_server.go +++ b/internal/servers/private_virtual_networks_server.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" "log/slog" - "net/netip" "github.com/prometheus/client_golang/prometheus" grpccodes "google.golang.org/grpc/codes" @@ -236,16 +235,28 @@ func (s *PrivateVirtualNetworksServer) validateVirtualNetwork(ctx context.Contex // VN-VAL-01: Validate IPv4 CIDR format if spec.GetIpv4Cidr() != "" { - if err = validateCIDR(spec.GetIpv4Cidr(), "IPv4"); err != nil { + var canonical string + canonical, err = parseAndValidateCIDR(spec.GetIpv4Cidr(), cidrIPv4) + if err != nil { return } + // Canonicalize on Create only; Update must not rewrite immutable CIDR fields (VN-VAL-11/12). + if existingVN == nil { + spec.SetIpv4Cidr(canonical) + } } // VN-VAL-02: Validate IPv6 CIDR format if spec.GetIpv6Cidr() != "" { - if err = validateCIDR(spec.GetIpv6Cidr(), "IPv6"); err != nil { + var canonical string + canonical, err = parseAndValidateCIDR(spec.GetIpv6Cidr(), cidrIPv6) + if err != nil { return } + // Canonicalize on Create only; Update must not rewrite immutable CIDR fields (VN-VAL-11/12). + if existingVN == nil { + spec.SetIpv6Cidr(canonical) + } } // VN-VAL-04, VN-VAL-05, VN-VAL-06: Validate NetworkClass @@ -261,28 +272,6 @@ func (s *PrivateVirtualNetworksServer) validateVirtualNetwork(ctx context.Contex return } -// validateCIDR validates a CIDR string and checks if it matches the expected IP version. -func validateCIDR(cidrStr string, ipVersion string) error { - prefix, err := netip.ParsePrefix(cidrStr) - if err != nil { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "invalid %s CIDR format '%s': %v", ipVersion, cidrStr, err) - } - - // Validate IP version matches field name - isIPv4 := prefix.Addr().Is4() - if ipVersion == "IPv4" && !isIPv4 { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'ipv4_cidr' contains IPv6 address: %s", cidrStr) - } - if ipVersion == "IPv6" && isIPv4 { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'ipv6_cidr' contains IPv4 address: %s", cidrStr) - } - - return nil -} - // validateImmutableFields validates that immutable fields have not been changed. func validateImmutableFields(newVN *privatev1.VirtualNetwork, existingVN *privatev1.VirtualNetwork) error { if existingVN == nil { diff --git a/internal/servers/private_virtual_networks_server_test.go b/internal/servers/private_virtual_networks_server_test.go index d54b35d2a..306a497f8 100644 --- a/internal/servers/private_virtual_networks_server_test.go +++ b/internal/servers/private_virtual_networks_server_test.go @@ -276,6 +276,89 @@ var _ = Describe("Private virtual networks server", func() { }) }) + Context("VN-VAL-01: IPv4 CIDR canonicalization on Create", func() { + It("canonicalizes non-canonical IPv4 CIDR on Create", func() { + nc := createNetworkClass(ctx, privatev1.NetworkClassState_NETWORK_CLASS_STATE_READY) + + vn := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + NetworkClass: nc.GetId(), + Region: "us-west-1", + }.Build(), + }.Build() + + _, err := server.validateVirtualNetwork(ctx, vn, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(vn.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + }) + + Context("VN-VAL-02: IPv6 CIDR canonicalization on Create", func() { + It("canonicalizes non-canonical IPv6 CIDR on Create", func() { + nc := createNetworkClass(ctx, privatev1.NetworkClassState_NETWORK_CLASS_STATE_READY) + + vn := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Ipv6Cidr: new("2001:db8::1/32"), + NetworkClass: nc.GetId(), + Region: "us-west-1", + }.Build(), + }.Build() + + _, err := server.validateVirtualNetwork(ctx, vn, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(vn.GetSpec().GetIpv6Cidr()).To(Equal("2001:db8::/32")) + }) + }) + + Context("Round-trip: server.Create canonicalizes CIDRs", func() { + It("stores canonical IPv4 CIDR on Create", func() { + nc := createNetworkClass(ctx, privatev1.NetworkClassState_NETWORK_CLASS_STATE_READY) + + createResponse, err := server.Create(ctx, privatev1.VirtualNetworksCreateRequest_builder{ + Object: privatev1.VirtualNetwork_builder{ + Metadata: privatev1.Metadata_builder{ + Tenant: auth.SharedTenant, + }.Build(), + Spec: privatev1.VirtualNetworkSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + NetworkClass: nc.GetId(), + Region: "us-west-1", + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(createResponse.GetObject().GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + }) + + Context("Create-only: no canonicalization on Update", func() { + It("does not canonicalize ipv4_cidr on Update", func() { + nc := createNetworkClass(ctx, privatev1.NetworkClassState_NETWORK_CLASS_STATE_READY) + + existing := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Region: "us-west-1", + NetworkClass: nc.GetId(), + Ipv4Cidr: new("10.0.1.5/24"), + }.Build(), + }.Build() + + updated := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Region: "us-west-1", + NetworkClass: nc.GetId(), + Ipv4Cidr: new("10.0.1.5/24"), + }.Build(), + }.Build() + + _, err := server.validateVirtualNetwork(ctx, updated, existing) + Expect(err).ToNot(HaveOccurred()) + Expect(updated.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.5/24")) + }) + }) + Context("VN-VAL-03: At least one CIDR required", func() { It("rejects empty IPv4 and IPv6 CIDRs", func() { vn := privatev1.VirtualNetwork_builder{