From e4ca22660e7f005cf4db0900da80ac309eea369d Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Wed, 17 Jun 2026 10:06:45 -0500 Subject: [PATCH 1/2] OSAC-365: canonicalize CIDR values across networking resources Normalize CIDR strings to masked canonical form (netip.ParsePrefix + Masked()) wherever fulfillment-service already validates notation. - Shared helpers in internal/servers/cidr_validation.go and internal/utils/cidr.go - VirtualNetwork/Subnet: Create and Update canonicalization with canonical-form immutability and legacy heal on omitted-field Updates - SecurityGroup rules, PublicIPPool spec.cidrs, Cluster pod/service CIDRs Operator note: string-exact config-version hashing may reprovision READY resources when legacy notation is rewritten to an equivalent prefix. Assisted-by: Claude Code --- internal/servers/cidr_validation.go | 132 ++++++++++++++++++ internal/servers/cidr_validation_test.go | 80 +++++++++++ internal/servers/private_clusters_server.go | 27 ---- .../servers/private_public_ip_pools_server.go | 21 +-- .../private_public_ip_pools_server_test.go | 13 ++ .../servers/private_security_groups_server.go | 20 +-- internal/servers/private_subnets_server.go | 68 +++++---- .../servers/private_subnets_server_test.go | 89 ++++++++++++ .../private_virtual_networks_server.go | 87 ++++-------- .../private_virtual_networks_server_test.go | 80 +++++++++++ internal/utils/cidr.go | 44 ++++++ internal/utils/cluster_spec_defaults.go | 24 ++-- internal/utils/cluster_spec_defaults_test.go | 15 ++ 13 files changed, 543 insertions(+), 157 deletions(-) create mode 100644 internal/servers/cidr_validation.go create mode 100644 internal/servers/cidr_validation_test.go create mode 100644 internal/utils/cidr.go diff --git a/internal/servers/cidr_validation.go b/internal/servers/cidr_validation.go new file mode 100644 index 000000000..7f89a668d --- /dev/null +++ b/internal/servers/cidr_validation.go @@ -0,0 +1,132 @@ +/* +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 +} + +// cidrPrefixesEqual reports whether two CIDR strings denote the same network prefix. +func cidrPrefixesEqual(a, b string) (bool, error) { + if a == b { + return true, nil + } + prefixA, err := netip.ParsePrefix(a) + if err != nil { + return false, err + } + prefixB, err := netip.ParsePrefix(b) + if err != nil { + return false, err + } + return prefixA.Masked() == prefixB.Masked(), nil +} + +// validateImmutableCIDR rejects updates that change the network prefix; canonically equivalent +// notation (e.g. 10.0.1.5/24 vs 10.0.1.0/24) is allowed. +func validateImmutableCIDR(fieldName, existingCIDR, newCIDR, ipVersion string) error { + equal, err := cidrPrefixesEqual(existingCIDR, newCIDR) + if err == nil && equal { + return nil + } + from, to := existingCIDR, newCIDR + if existingCIDR != "" { + if canonical, err := parseAndValidateCIDR(existingCIDR, ipVersion); err == nil { + from = canonical + } + } + if canonical, err := parseAndValidateCIDR(newCIDR, ipVersion); err == nil { + to = canonical + } + return grpcstatus.Errorf(grpccodes.InvalidArgument, + "field '%s' is immutable and cannot be changed from '%s' to '%s'", + fieldName, from, to) +} + +// immutableCIDRField groups optional CIDR field accessors for preserve-and-validate on Update. +type immutableCIDRField struct { + fieldName string + ipVersion string + existingHasCIDR func() bool + existingCIDR func() string + newHasCIDR func() bool + newCIDR func() string + setNewCIDR func(string) +} + +func (f immutableCIDRField) preserveAndValidate() error { + if f.existingHasCIDR() && !f.newHasCIDR() { + f.setNewCIDR(f.existingCIDR()) + } + if !f.newHasCIDR() { + return nil + } + existing := f.existingCIDR() + if !f.existingHasCIDR() { + existing = "" + } + return validateImmutableCIDR(f.fieldName, existing, f.newCIDR(), f.ipVersion) +} + +// canonicalizeDualStackCIDRs validates and persists masked IPv4/IPv6 CIDRs when present. +func canonicalizeDualStackCIDRs( + ipv4 func() string, setIPv4 func(string), + ipv6 func() string, setIPv6 func(string), +) error { + if cidr := ipv4(); cidr != "" { + canonical, err := parseAndValidateCIDR(cidr, cidrIPv4) + if err != nil { + return err + } + setIPv4(canonical) + } + if cidr := ipv6(); cidr != "" { + canonical, err := parseAndValidateCIDR(cidr, cidrIPv6) + if err != nil { + return err + } + setIPv6(canonical) + } + return nil +} diff --git a/internal/servers/cidr_validation_test.go b/internal/servers/cidr_validation_test.go new file mode 100644 index 000000000..9ab8a8371 --- /dev/null +++ b/internal/servers/cidr_validation_test.go @@ -0,0 +1,80 @@ +/* +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 ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("cidr validation helpers", func() { + Describe("parseAndValidateCIDR", func() { + It("masks host bits in canonical notation", func() { + canonical, err := parseAndValidateCIDR("10.0.1.5/24", cidrIPv4) + Expect(err).ToNot(HaveOccurred()) + Expect(canonical).To(Equal("10.0.1.0/24")) + }) + }) + + Describe("cidrPrefixesEqual", func() { + It("returns true for identical strings", func() { + equal, err := cidrPrefixesEqual("10.0.1.0/24", "10.0.1.0/24") + Expect(err).ToNot(HaveOccurred()) + Expect(equal).To(BeTrue()) + }) + + It("returns true for canonically equivalent prefixes", func() { + equal, err := cidrPrefixesEqual("10.0.1.5/24", "10.0.1.0/24") + Expect(err).ToNot(HaveOccurred()) + Expect(equal).To(BeTrue()) + }) + + It("returns false for different networks", func() { + equal, err := cidrPrefixesEqual("10.0.0.0/16", "192.168.0.0/16") + Expect(err).ToNot(HaveOccurred()) + Expect(equal).To(BeFalse()) + }) + }) + + Describe("validateImmutableCIDR", func() { + It("allows canonically equivalent rewrite", func() { + err := validateImmutableCIDR("spec.ipv4_cidr", "10.0.1.5/24", "10.0.1.0/24", cidrIPv4) + Expect(err).ToNot(HaveOccurred()) + }) + + It("rejects a real network change with canonical values in the error", func() { + err := validateImmutableCIDR("spec.ipv4_cidr", "10.0.0.0/16", "192.168.0.0/16", cidrIPv4) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("immutable")) + Expect(err.Error()).To(ContainSubstring("10.0.0.0/16")) + Expect(err.Error()).To(ContainSubstring("192.168.0.0/16")) + }) + }) + + Describe("canonicalizeDualStackCIDRs", func() { + It("canonicalizes both address families when present", func() { + ipv4 := "10.0.1.5/24" + ipv6 := "2001:db8::1/32" + err := canonicalizeDualStackCIDRs( + func() string { return ipv4 }, + func(v string) { ipv4 = v }, + func() string { return ipv6 }, + func(v string) { ipv6 = v }, + ) + Expect(err).ToNot(HaveOccurred()) + Expect(ipv4).To(Equal("10.0.1.0/24")) + Expect(ipv6).To(Equal("2001:db8::/32")) + }) + }) +}) diff --git a/internal/servers/private_clusters_server.go b/internal/servers/private_clusters_server.go index 2d3d75861..1237f679a 100644 --- a/internal/servers/private_clusters_server.go +++ b/internal/servers/private_clusters_server.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" "log/slog" - "net/netip" "sort" "strconv" "strings" @@ -600,11 +599,6 @@ func (s *PrivateClustersServer) validateAndTransformCluster(ctx context.Context, ) cluster.GetSpec().SetTemplateParameters(actualClusterParameters) - // Validate network CIDRs if provided: - if err = validateNetworkCIDRs(cluster.GetSpec()); err != nil { - return err - } - // Make sure that the template and the host types of the node sets are referenced by their identifiers, as // that is what we want to save to the database. cluster.GetSpec().SetTemplate(template.GetId()) @@ -745,27 +739,6 @@ func mergeNodeSetsWithTemplate( cluster.GetSpec().SetNodeSets(actualNodeSets) } -// validateNetworkCIDRs validates pod and service CIDR formats if provided. -func validateNetworkCIDRs(spec *privatev1.ClusterSpec) error { - if !spec.HasNetwork() { - return nil - } - network := spec.GetNetwork() - if network.HasPodCidr() { - if _, err := netip.ParsePrefix(network.GetPodCidr()); err != nil { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "invalid pod_cidr format '%s': %v", network.GetPodCidr(), err) - } - } - if network.HasServiceCidr() { - if _, err := netip.ParsePrefix(network.GetServiceCidr()); err != nil { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "invalid service_cidr format '%s': %v", network.GetServiceCidr(), err) - } - } - return nil -} - func (s *PrivateClustersServer) validateAndTransformCatalogItem(ctx context.Context, cluster *privatev1.Cluster) error { if cluster == nil { return grpcstatus.Errorf(grpccodes.InvalidArgument, "object is mandatory") diff --git a/internal/servers/private_public_ip_pools_server.go b/internal/servers/private_public_ip_pools_server.go index 28e4d1811..7fda2898f 100644 --- a/internal/servers/private_public_ip_pools_server.go +++ b/internal/servers/private_public_ip_pools_server.go @@ -195,13 +195,17 @@ func (s *PrivatePublicIPPoolsServer) validateCreate(ctx context.Context, return grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'spec.cidrs' is required") } + canonicalCIDRs := make([]string, len(cidrs)) for i, cidr := range cidrs { - if err := validatePoolCIDRFormat(cidr, spec.GetIpFamily(), i); err != nil { + canonical, err := validatePoolCIDRFormat(cidr, spec.GetIpFamily(), i) + if err != nil { return err } + canonicalCIDRs[i] = canonical } + spec.SetCidrs(canonicalCIDRs) - if err := validateNoCIDRSelfOverlap(cidrs); err != nil { + if err := validateNoCIDRSelfOverlap(canonicalCIDRs); err != nil { return err } @@ -234,11 +238,12 @@ func validateUpdate(newPool *privatev1.PublicIPPool, existing *privatev1.PublicI return nil } -// validatePoolCIDRFormat validates CIDR string is parseable and belongs to the specified IP family -func validatePoolCIDRFormat(cidrStr string, ipFamily privatev1.IPFamily, idx int) error { +// validatePoolCIDRFormat validates CIDR string is parseable, belongs to the specified IP family, +// and returns the canonical CIDR notation. +func validatePoolCIDRFormat(cidrStr string, ipFamily privatev1.IPFamily, idx int) (string, error) { prefix, err := netip.ParsePrefix(cidrStr) if err != nil { - return grpcstatus.Errorf(grpccodes.InvalidArgument, + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "invalid CIDR format in field 'spec.cidrs[%d]': '%s': %v", idx, cidrStr, err) } @@ -246,17 +251,17 @@ func validatePoolCIDRFormat(cidrStr string, ipFamily privatev1.IPFamily, idx int switch ipFamily { case privatev1.IPFamily_IP_FAMILY_IPV4: if !isIPv4 { - return grpcstatus.Errorf(grpccodes.InvalidArgument, + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'spec.cidrs[%d]' contains IPv6 address but pool ip_family is IPv4: %s", idx, cidrStr) } case privatev1.IPFamily_IP_FAMILY_IPV6: if isIPv4 { - return grpcstatus.Errorf(grpccodes.InvalidArgument, + return "", grpcstatus.Errorf(grpccodes.InvalidArgument, "field 'spec.cidrs[%d]' contains IPv4 address but pool ip_family is IPv6: %s", idx, cidrStr) } } - return nil + return prefix.Masked().String(), nil } // validateNoCIDRSelfOverlap checks that no two CIDRs within the same pool overlap each other. diff --git a/internal/servers/private_public_ip_pools_server_test.go b/internal/servers/private_public_ip_pools_server_test.go index 6b93bded7..7f369424e 100644 --- a/internal/servers/private_public_ip_pools_server_test.go +++ b/internal/servers/private_public_ip_pools_server_test.go @@ -456,6 +456,19 @@ var _ = Describe("Private public IP pools server", func() { Expect(err).ToNot(HaveOccurred()) }) + It("canonicalizes non-canonical CIDRs on Create", func() { + pool := privatev1.PublicIPPool_builder{ + Spec: privatev1.PublicIPPoolSpec_builder{ + Cidrs: []string{"10.0.1.5/24"}, + IpFamily: privatev1.IPFamily_IP_FAMILY_IPV4, + }.Build(), + }.Build() + + err := poolsServer.validateCreate(ctx, pool) + Expect(err).ToNot(HaveOccurred()) + Expect(pool.GetSpec().GetCidrs()).To(Equal([]string{"10.0.1.0/24"})) + }) + }) Context("Cross-pool CIDR overlap detection", func() { diff --git a/internal/servers/private_security_groups_server.go b/internal/servers/private_security_groups_server.go index e54b6ec87..a7a2c57a9 100644 --- a/internal/servers/private_security_groups_server.go +++ b/internal/servers/private_security_groups_server.go @@ -339,20 +339,12 @@ func validateSecurityRule(rule *privatev1.SecurityRule, ruleType string, index i "%s rule at index %d: at least one of ipv4_cidr or ipv6_cidr must be provided", ruleType, index) } - // Validate IPv4 CIDR format if present - if rule.GetIpv4Cidr() != "" { - 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) - } + if err := canonicalizeDualStackCIDRs( + rule.GetIpv4Cidr, rule.SetIpv4Cidr, + rule.GetIpv6Cidr, rule.SetIpv6Cidr, + ); err != nil { + return grpcstatus.Errorf(grpccodes.InvalidArgument, + "%s rule at index %d: %v", ruleType, index, err) } return nil diff --git a/internal/servers/private_subnets_server.go b/internal/servers/private_subnets_server.go index 5ccd4d7d4..212dc8f2f 100644 --- a/internal/servers/private_subnets_server.go +++ b/internal/servers/private_subnets_server.go @@ -226,18 +226,12 @@ func (s *PrivateSubnetsServer) validateSubnet(ctx context.Context, "at least one of 'spec.ipv4_cidr' or 'spec.ipv6_cidr' must be provided") } - // SUB-VAL-01: Validate IPv4 CIDR format - if spec.GetIpv4Cidr() != "" { - if err := validateCIDR(spec.GetIpv4Cidr(), "IPv4"); err != nil { - return err - } - } - - // SUB-VAL-02: Validate IPv6 CIDR format - if spec.GetIpv6Cidr() != "" { - if err := validateCIDR(spec.GetIpv6Cidr(), "IPv6"); err != nil { - return err - } + // SUB-VAL-01, SUB-VAL-02: Validate and canonicalize CIDRs + if err := canonicalizeDualStackCIDRs( + spec.GetIpv4Cidr, spec.SetIpv4Cidr, + spec.GetIpv6Cidr, spec.SetIpv6Cidr, + ); err != nil { + return err } // SUB-VAL-04, SUB-VAL-05, SUB-VAL-06, SUB-VAL-07, SUB-VAL-08: Validate parent VirtualNetwork @@ -331,7 +325,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 +337,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 } } @@ -456,28 +450,30 @@ func validateImmutableFieldsSubnet(newSubnet *privatev1.Subnet, existingSubnet * existingSpec.GetVirtualNetwork(), newSpec.GetVirtualNetwork()) } - // SUB-VAL-14: Preserve and check immutable ipv4_cidr field. - // If the request omits ipv4_cidr (HasIpv4Cidr() false), copy the existing value to prevent - // erasure — the private API has no Merge() step to preserve absent optional fields. - if existingSpec.HasIpv4Cidr() && !newSpec.HasIpv4Cidr() { - newSpec.SetIpv4Cidr(existingSpec.GetIpv4Cidr()) - } - if newSpec.HasIpv4Cidr() && newSpec.GetIpv4Cidr() != existingSpec.GetIpv4Cidr() { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'spec.ipv4_cidr' is immutable and cannot be changed from '%s' to '%s'", - existingSpec.GetIpv4Cidr(), newSpec.GetIpv4Cidr()) - } - - // SUB-VAL-15: Preserve and check immutable ipv6_cidr field. - // If the request omits ipv6_cidr (HasIpv6Cidr() false), copy the existing value to prevent - // erasure — the private API has no Merge() step to preserve absent optional fields. - if existingSpec.HasIpv6Cidr() && !newSpec.HasIpv6Cidr() { - newSpec.SetIpv6Cidr(existingSpec.GetIpv6Cidr()) - } - if newSpec.HasIpv6Cidr() && newSpec.GetIpv6Cidr() != existingSpec.GetIpv6Cidr() { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'spec.ipv6_cidr' is immutable and cannot be changed from '%s' to '%s'", - existingSpec.GetIpv6Cidr(), newSpec.GetIpv6Cidr()) + // SUB-VAL-14, SUB-VAL-15: Preserve and check immutable CIDR fields. + for _, field := range []immutableCIDRField{ + { + fieldName: "spec.ipv4_cidr", + ipVersion: cidrIPv4, + existingHasCIDR: existingSpec.HasIpv4Cidr, + existingCIDR: existingSpec.GetIpv4Cidr, + newHasCIDR: newSpec.HasIpv4Cidr, + newCIDR: newSpec.GetIpv4Cidr, + setNewCIDR: newSpec.SetIpv4Cidr, + }, + { + fieldName: "spec.ipv6_cidr", + ipVersion: cidrIPv6, + existingHasCIDR: existingSpec.HasIpv6Cidr, + existingCIDR: existingSpec.GetIpv6Cidr, + newHasCIDR: newSpec.HasIpv6Cidr, + newCIDR: newSpec.GetIpv6Cidr, + setNewCIDR: newSpec.SetIpv6Cidr, + }, + } { + if err := field.preserveAndValidate(); err != nil { + return err + } } return nil diff --git a/internal/servers/private_subnets_server_test.go b/internal/servers/private_subnets_server_test.go index 7346d484b..cc805a5f3 100644 --- a/internal/servers/private_subnets_server_test.go +++ b/internal/servers/private_subnets_server_test.go @@ -308,6 +308,95 @@ var _ = Describe("Private subnets server", func() { }) }) + Context("CIDR canonicalization", func() { + It("canonicalizes non-canonical IPv4 and IPv6 CIDRs on Create", func() { + vn4 := createVirtualNetwork(ctx, "10.0.0.0/16", "") + subnet4 := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + VirtualNetwork: vn4.GetId(), + }.Build(), + }.Build() + err := server.validateSubnet(ctx, subnet4, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(subnet4.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + + vn6 := createVirtualNetwork(ctx, "", "2001:db8::/32") + subnet6 := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv6Cidr: new("2001:db8:1::1/64"), + VirtualNetwork: vn6.GetId(), + }.Build(), + }.Build() + err = server.validateSubnet(ctx, subnet6, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(subnet6.GetSpec().GetIpv6Cidr()).To(Equal("2001:db8:1::/64")) + }) + + 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")) + }) + + It("canonicalizes on Update including preserved legacy CIDR when omitted", func() { + const fakeVNID = "vn-fake-id" + + existing := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.5/24"), + VirtualNetwork: fakeVNID, + }.Build(), + }.Build() + + equivalentRewrite := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + Ipv4Cidr: new("10.0.1.0/24"), + VirtualNetwork: fakeVNID, + }.Build(), + }.Build() + err := server.validateSubnet(ctx, equivalentRewrite, existing) + Expect(err).ToNot(HaveOccurred()) + Expect(equivalentRewrite.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + + omittedField := privatev1.Subnet_builder{ + Spec: privatev1.SubnetSpec_builder{ + VirtualNetwork: fakeVNID, + }.Build(), + }.Build() + err = server.validateSubnet(ctx, omittedField, existing) + Expect(err).ToNot(HaveOccurred()) + Expect(omittedField.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + + It("stores canonical IPv4 CIDR on Create round-trip", 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..45ccc2883 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" @@ -234,18 +233,12 @@ func (s *PrivateVirtualNetworksServer) validateVirtualNetwork(ctx context.Contex return } - // VN-VAL-01: Validate IPv4 CIDR format - if spec.GetIpv4Cidr() != "" { - if err = validateCIDR(spec.GetIpv4Cidr(), "IPv4"); err != nil { - return - } - } - - // VN-VAL-02: Validate IPv6 CIDR format - if spec.GetIpv6Cidr() != "" { - if err = validateCIDR(spec.GetIpv6Cidr(), "IPv6"); err != nil { - return - } + // VN-VAL-01, VN-VAL-02: Validate and canonicalize CIDRs + if err = canonicalizeDualStackCIDRs( + spec.GetIpv4Cidr, spec.SetIpv4Cidr, + spec.GetIpv6Cidr, spec.SetIpv6Cidr, + ); err != nil { + return } // VN-VAL-04, VN-VAL-05, VN-VAL-06: Validate NetworkClass @@ -261,28 +254,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 { @@ -306,28 +277,32 @@ func validateImmutableFields(newVN *privatev1.VirtualNetwork, existingVN *privat existingSpec.GetNetworkClass(), newSpec.GetNetworkClass()) } - // VN-VAL-11: Preserve and check immutable ipv4_cidr field. - // If the request omits ipv4_cidr (HasIpv4Cidr() false), copy the existing value to prevent + // VN-VAL-11, VN-VAL-12: Preserve and check immutable CIDR fields. + // If the request omits a CIDR (Has*Cidr() false), copy the existing value to prevent // erasure — the private API has no Merge() step to preserve absent optional fields. - if existingSpec.HasIpv4Cidr() && !newSpec.HasIpv4Cidr() { - newSpec.SetIpv4Cidr(existingSpec.GetIpv4Cidr()) - } - if newSpec.HasIpv4Cidr() && newSpec.GetIpv4Cidr() != existingSpec.GetIpv4Cidr() { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'spec.ipv4_cidr' is immutable and cannot be changed from '%s' to '%s'", - existingSpec.GetIpv4Cidr(), newSpec.GetIpv4Cidr()) - } - - // VN-VAL-12: Preserve and check immutable ipv6_cidr field. - // If the request omits ipv6_cidr (HasIpv6Cidr() false), copy the existing value to prevent - // erasure — the private API has no Merge() step to preserve absent optional fields. - if existingSpec.HasIpv6Cidr() && !newSpec.HasIpv6Cidr() { - newSpec.SetIpv6Cidr(existingSpec.GetIpv6Cidr()) - } - if newSpec.HasIpv6Cidr() && newSpec.GetIpv6Cidr() != existingSpec.GetIpv6Cidr() { - return grpcstatus.Errorf(grpccodes.InvalidArgument, - "field 'spec.ipv6_cidr' is immutable and cannot be changed from '%s' to '%s'", - existingSpec.GetIpv6Cidr(), newSpec.GetIpv6Cidr()) + for _, field := range []immutableCIDRField{ + { + fieldName: "spec.ipv4_cidr", + ipVersion: cidrIPv4, + existingHasCIDR: existingSpec.HasIpv4Cidr, + existingCIDR: existingSpec.GetIpv4Cidr, + newHasCIDR: newSpec.HasIpv4Cidr, + newCIDR: newSpec.GetIpv4Cidr, + setNewCIDR: newSpec.SetIpv4Cidr, + }, + { + fieldName: "spec.ipv6_cidr", + ipVersion: cidrIPv6, + existingHasCIDR: existingSpec.HasIpv6Cidr, + existingCIDR: existingSpec.GetIpv6Cidr, + newHasCIDR: newSpec.HasIpv6Cidr, + newCIDR: newSpec.GetIpv6Cidr, + setNewCIDR: newSpec.SetIpv6Cidr, + }, + } { + if err := field.preserveAndValidate(); err != nil { + return err + } } return nil diff --git a/internal/servers/private_virtual_networks_server_test.go b/internal/servers/private_virtual_networks_server_test.go index bbefee118..c3c7c8b2e 100644 --- a/internal/servers/private_virtual_networks_server_test.go +++ b/internal/servers/private_virtual_networks_server_test.go @@ -276,6 +276,86 @@ var _ = Describe("Private virtual networks server", func() { }) }) + Context("CIDR canonicalization", func() { + It("canonicalizes non-canonical IPv4 and IPv6 CIDRs on Create", func() { + nc := createNetworkClass(ctx, privatev1.NetworkClassState_NETWORK_CLASS_STATE_READY) + + ipv4VN := 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, ipv4VN, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(ipv4VN.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + + ipv6VN := 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, ipv6VN, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(ipv6VN.GetSpec().GetIpv6Cidr()).To(Equal("2001:db8::/32")) + }) + + It("stores canonical IPv4 CIDR on Create round-trip", 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")) + }) + + It("canonicalizes on Update including preserved legacy CIDR when omitted", 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() + + equivalentRewrite := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Region: "us-west-1", + NetworkClass: nc.GetId(), + Ipv4Cidr: new("10.0.1.0/24"), + }.Build(), + }.Build() + _, err := server.validateVirtualNetwork(ctx, equivalentRewrite, existing) + Expect(err).ToNot(HaveOccurred()) + Expect(equivalentRewrite.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + + omittedField := privatev1.VirtualNetwork_builder{ + Spec: privatev1.VirtualNetworkSpec_builder{ + Region: "us-west-1", + NetworkClass: nc.GetId(), + }.Build(), + }.Build() + _, err = server.validateVirtualNetwork(ctx, omittedField, existing) + Expect(err).ToNot(HaveOccurred()) + Expect(omittedField.GetSpec().GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + }) + Context("VN-VAL-03: At least one CIDR required", func() { It("rejects empty IPv4 and IPv6 CIDRs", func() { vn := privatev1.VirtualNetwork_builder{ diff --git a/internal/utils/cidr.go b/internal/utils/cidr.go new file mode 100644 index 000000000..d93783182 --- /dev/null +++ b/internal/utils/cidr.go @@ -0,0 +1,44 @@ +/* +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 utils + +import ( + "net/netip" + + grpccodes "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" +) + +// CanonicalizeCIDR returns canonical CIDR notation for cidrStr. +func CanonicalizeCIDR(cidrStr string) (string, error) { + prefix, err := netip.ParsePrefix(cidrStr) + if err != nil { + return "", err + } + return prefix.Masked().String(), nil +} + +// CanonicalizeCIDRField is like CanonicalizeCIDR but returns a gRPC InvalidArgument error +// naming fieldName when parsing fails. +func CanonicalizeCIDRField(cidrStr, fieldName string) (string, error) { + canonical, err := CanonicalizeCIDR(cidrStr) + if err != nil { + return "", grpcstatus.Errorf( + grpccodes.InvalidArgument, + "invalid %s %q: %v", + fieldName, cidrStr, err, + ) + } + return canonical, nil +} diff --git a/internal/utils/cluster_spec_defaults.go b/internal/utils/cluster_spec_defaults.go index d1b5aa6e2..23429030d 100644 --- a/internal/utils/cluster_spec_defaults.go +++ b/internal/utils/cluster_spec_defaults.go @@ -14,10 +14,6 @@ language governing permissions and limitations under the License. package utils import ( - "net/netip" - - grpccodes "google.golang.org/grpc/codes" - grpcstatus "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" privatev1 "github.com/osac-project/fulfillment-service/internal/api/osac/private/v1" @@ -83,22 +79,18 @@ func validateClusterNetwork(network *privatev1.ClusterNetwork) error { return nil } if network.HasPodCidr() { - if _, err := netip.ParsePrefix(network.GetPodCidr()); err != nil { - return grpcstatus.Errorf( - grpccodes.InvalidArgument, - "invalid pod_cidr %q: %v", - network.GetPodCidr(), err, - ) + canonical, err := CanonicalizeCIDRField(network.GetPodCidr(), "pod_cidr") + if err != nil { + return err } + network.SetPodCidr(canonical) } if network.HasServiceCidr() { - if _, err := netip.ParsePrefix(network.GetServiceCidr()); err != nil { - return grpcstatus.Errorf( - grpccodes.InvalidArgument, - "invalid service_cidr %q: %v", - network.GetServiceCidr(), err, - ) + canonical, err := CanonicalizeCIDRField(network.GetServiceCidr(), "service_cidr") + if err != nil { + return err } + network.SetServiceCidr(canonical) } return nil } diff --git a/internal/utils/cluster_spec_defaults_test.go b/internal/utils/cluster_spec_defaults_test.go index b4a53f58f..d7f94d216 100644 --- a/internal/utils/cluster_spec_defaults_test.go +++ b/internal/utils/cluster_spec_defaults_test.go @@ -149,6 +149,21 @@ var _ = Describe("ValidateClusterSpecFields", func() { Expect(err).ToNot(HaveOccurred()) }) + It("canonicalizes non-canonical pod and service CIDRs", func() { + podCidr := "10.128.0.5/14" + serviceCidr := "172.30.1.0/16" + spec := privatev1.ClusterSpec_builder{ + Network: privatev1.ClusterNetwork_builder{ + PodCidr: &podCidr, + ServiceCidr: &serviceCidr, + }.Build(), + }.Build() + err := ValidateClusterSpecFields(spec) + Expect(err).ToNot(HaveOccurred()) + Expect(spec.GetNetwork().GetPodCidr()).To(Equal("10.128.0.0/14")) + Expect(spec.GetNetwork().GetServiceCidr()).To(Equal("172.30.0.0/16")) + }) + It("Returns error for invalid pod_cidr", func() { podCidr := "invalid-cidr" spec := privatev1.ClusterSpec_builder{ From 5d485b142111ca50a18e88eafc02837f7b8b7e93 Mon Sep 17 00:00:00 2001 From: Tommy Hughes Date: Wed, 17 Jun 2026 10:30:13 -0500 Subject: [PATCH 2/2] OSAC-365: add SG and cluster Update canonicalization server tests Assisted-by: Claude Code --- .../servers/private_clusters_server_test.go | 43 +++++++++++ .../servers/security_groups_server_test.go | 77 +++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/internal/servers/private_clusters_server_test.go b/internal/servers/private_clusters_server_test.go index fa0181a08..81896c9a0 100644 --- a/internal/servers/private_clusters_server_test.go +++ b/internal/servers/private_clusters_server_test.go @@ -576,6 +576,49 @@ var _ = Describe("Private clusters server", func() { Expect(proto.Equal(createResponse.GetObject(), getResponse.GetObject())).To(BeTrue()) }) + It("Canonicalizes network CIDRs on Update", func() { + createResponse, err := server.Create(ctx, privatev1.ClustersCreateRequest_builder{ + Object: privatev1.Cluster_builder{ + Spec: privatev1.ClusterSpec_builder{ + Template: "my-template-id", + }.Build(), + Status: privatev1.ClusterStatus_builder{ + Hub: "my-hub-id", + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + object := createResponse.GetObject() + + updateResponse, err := server.Update(ctx, privatev1.ClustersUpdateRequest_builder{ + Object: privatev1.Cluster_builder{ + Id: object.GetId(), + Spec: privatev1.ClusterSpec_builder{ + Template: object.GetSpec().GetTemplate(), + Network: privatev1.ClusterNetwork_builder{ + PodCidr: new("10.128.0.5/14"), + ServiceCidr: new("172.30.1.0/16"), + }.Build(), + }.Build(), + }.Build(), + UpdateMask: &fieldmaskpb.FieldMask{ + Paths: []string{"spec.network.pod_cidr", "spec.network.service_cidr"}, + }, + }.Build()) + Expect(err).ToNot(HaveOccurred()) + network := updateResponse.GetObject().GetSpec().GetNetwork() + Expect(network.GetPodCidr()).To(Equal("10.128.0.0/14")) + Expect(network.GetServiceCidr()).To(Equal("172.30.0.0/16")) + + getResponse, err := server.Get(ctx, privatev1.ClustersGetRequest_builder{ + Id: object.GetId(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + network = getResponse.GetObject().GetSpec().GetNetwork() + Expect(network.GetPodCidr()).To(Equal("10.128.0.0/14")) + Expect(network.GetServiceCidr()).To(Equal("172.30.0.0/16")) + }) + It("Update object", func() { // Create the object: createResponse, err := server.Create(ctx, privatev1.ClustersCreateRequest_builder{ diff --git a/internal/servers/security_groups_server_test.go b/internal/servers/security_groups_server_test.go index 92d27b431..6038fcdc4 100644 --- a/internal/servers/security_groups_server_test.go +++ b/internal/servers/security_groups_server_test.go @@ -301,6 +301,83 @@ var _ = Describe("SecurityGroups server", func() { Expect(proto.Equal(createResponse.GetObject(), getResponse.GetObject())).To(BeTrue()) }) + It("Canonicalizes non-canonical rule CIDRs on Create", func() { + response, err := server.Create(ctx, publicv1.SecurityGroupsCreateRequest_builder{ + Object: publicv1.SecurityGroup_builder{ + Spec: publicv1.SecurityGroupSpec_builder{ + VirtualNetwork: virtualNetworkID, + Ingress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_TCP, + PortFrom: new(int32(80)), + PortTo: new(int32(80)), + Ipv4Cidr: new("10.0.1.5/24"), + }, + }, + Egress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_ALL, + Ipv4Cidr: new("0.0.0.0/0"), + }, + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(response.GetObject().GetSpec().GetIngress()[0].GetIpv4Cidr()).To(Equal("10.0.1.0/24")) + }) + + It("Canonicalizes rule CIDRs on Update", func() { + createResponse, err := server.Create(ctx, publicv1.SecurityGroupsCreateRequest_builder{ + Object: publicv1.SecurityGroup_builder{ + Spec: publicv1.SecurityGroupSpec_builder{ + VirtualNetwork: virtualNetworkID, + Ingress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_TCP, + PortFrom: new(int32(443)), + PortTo: new(int32(443)), + Ipv4Cidr: new("0.0.0.0/0"), + }, + }, + Egress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_ALL, + Ipv4Cidr: new("0.0.0.0/0"), + }, + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + object := createResponse.GetObject() + + updateResponse, err := server.Update(ctx, publicv1.SecurityGroupsUpdateRequest_builder{ + Object: publicv1.SecurityGroup_builder{ + Id: object.GetId(), + Spec: publicv1.SecurityGroupSpec_builder{ + VirtualNetwork: virtualNetworkID, + Ingress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_TCP, + PortFrom: new(int32(8080)), + PortTo: new(int32(8080)), + Ipv4Cidr: new("10.0.2.5/24"), + }, + }, + Egress: []*publicv1.SecurityRule{ + { + Protocol: publicv1.Protocol_PROTOCOL_ALL, + Ipv4Cidr: new("0.0.0.0/0"), + }, + }, + }.Build(), + }.Build(), + }.Build()) + Expect(err).ToNot(HaveOccurred()) + Expect(updateResponse.GetObject().GetSpec().GetIngress()[0].GetIpv4Cidr()).To(Equal("10.0.2.0/24")) + }) + It("Update object", func() { // Create the object: createResponse, err := server.Create(ctx, publicv1.SecurityGroupsCreateRequest_builder{