Move subnet options to SubnetSpec

This commit introduces SubnetSpec field onto the OpenStackClusterSpec
that is supposed to hold all options related to subnets created by CAPO.
This means nodeCidr and DNSNameservers are moved into that struct.

Author: dulek

api/v1alpha5/conversion.go

@@ -196,6 +196,11 @@ func Convert_v1alpha8_OpenStackClusterSpec_To_v1alpha5_OpenStackClusterSpec(in *
 		out.ExternalNetworkID = in.ExternalNetwork.ID
 	}
 
+	if len(in.ManagedSubnets) > 0 {
+		out.NodeCIDR = in.ManagedSubnets[0].CIDR
+		out.DNSNameservers = in.ManagedSubnets[0].DNSNameservers
+	}
+
 	if in.Subnets != nil {
 		if len(in.Subnets) >= 1 {
 			if err := Convert_v1alpha8_SubnetFilter_To_v1alpha5_SubnetFilter(&in.Subnets[0], &out.Subnet, s); err != nil {
@@ -228,6 +233,16 @@ func Convert_v1alpha5_OpenStackClusterSpec_To_v1alpha8_OpenStackClusterSpec(in *
 		out.Subnets = []infrav1.SubnetFilter{subnet}
 	}
 
+	if len(in.NodeCIDR) > 0 {
+		out.ManagedSubnets = []infrav1.SubnetSpec{
+			{
+				CIDR:           in.NodeCIDR,
+				DNSNameservers: in.DNSNameservers,
+			},
+		}
+	}
+	// We're dropping DNSNameservers even if these were set as without NodeCIDR it doesn't make sense.
+
 	return nil
 }
 

api/v1alpha5/zz_generated.conversion.go

@@ -127,6 +127,11 @@ func restorev1alpha6ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackCl
 		}
 	}
 
+	// We only restore DNSNameservers when these were lossly converted when NodeCIDR is empty.
+	if len(previous.DNSNameservers) > 0 && dst.NodeCIDR == "" {
+		dst.DNSNameservers = previous.DNSNameservers
+	}
+
 	prevBastion := previous.Bastion
 	dstBastion := dst.Bastion
 	if prevBastion != nil && dstBastion != nil {
@@ -545,6 +550,11 @@ func Convert_v1alpha8_OpenStackClusterSpec_To_v1alpha6_OpenStackClusterSpec(in *
 		}
 	}
 
+	if len(in.ManagedSubnets) > 0 {
+		out.NodeCIDR = in.ManagedSubnets[0].CIDR
+		out.DNSNameservers = in.ManagedSubnets[0].DNSNameservers
+	}
+
 	return nil
 }
 
@@ -569,6 +579,16 @@ func Convert_v1alpha6_OpenStackClusterSpec_To_v1alpha8_OpenStackClusterSpec(in *
 		out.Subnets = []infrav1.SubnetFilter{subnet}
 	}
 
+	// DNSNameservers without NodeCIDR doesn't make sense, so we drop that.
+	if len(in.NodeCIDR) > 0 {
+		out.ManagedSubnets = []infrav1.SubnetSpec{
+			{
+				CIDR:           in.NodeCIDR,
+				DNSNameservers: in.DNSNameservers,
+			},
+		}
+	}
+
 	return nil
 }
 

api/v1alpha6/conversion.go

@@ -132,6 +132,16 @@ func TestFuzzyConversion(t *testing.T) {
 					}
 				}
 			},
+
+			func(spec *infrav1.SubnetSpec, c fuzz.Continue) {
+				c.FuzzNoCustom(spec)
+
+				// CIDR is required and API validates that it's present, so
+				// we force it to always be set.
+				for spec.CIDR == "" {
+					spec.CIDR = c.RandString()
+				}
+			},
 		}
 	}
 
@@ -156,6 +166,7 @@ func TestFuzzyConversion(t *testing.T) {
 		Hub:              &infrav1.OpenStackClusterTemplate{},
 		Spoke:            &OpenStackClusterTemplate{},
 		HubAfterMutation: ignoreDataAnnotation,
+		FuzzerFuncs:      []fuzzer.FuzzerFuncs{fuzzerFuncs},
 	})))
 
 	t.Run("for OpenStackClusterTemplate with mutate", runParallel(testhelpers.FuzzMutateTestFunc(testhelpers.FuzzMutateTestFuncInput{

api/v1alpha6/conversion_test.go

@@ -39,6 +39,24 @@ var v1alpha7OpenStackClusterRestorer = conversion.RestorerFor[*OpenStackCluster]
 		},
 		restorev1alpha7Bastion,
 	),
+	"spec": conversion.HashedFieldRestorer(
+		func(c *OpenStackCluster) *OpenStackClusterSpec {
+			return &c.Spec
+		},
+		restorev1alpha7ClusterSpec,
+
+		// Filter out Bastion, which is restored separately
+		conversion.HashedFilterField[*OpenStackCluster, OpenStackClusterSpec](
+			func(s *OpenStackClusterSpec) *OpenStackClusterSpec {
+				if s.Bastion != nil {
+					f := *s
+					f.Bastion = nil
+					return &f
+				}
+				return s
+			},
+		),
+	),
 }
 
 var v1alpha8OpenStackClusterRestorer = conversion.RestorerFor[*infrav1.OpenStackCluster]{
@@ -111,6 +129,19 @@ func restorev1alpha8Bastion(previous **infrav1.Bastion, dst **infrav1.Bastion) {
 	}
 }
 
+func restorev1alpha7ClusterSpec(previous *OpenStackClusterSpec, dst *OpenStackClusterSpec) {
+	prevBastion := previous.Bastion
+	dstBastion := dst.Bastion
+	if prevBastion != nil && dstBastion != nil {
+		restorev1alpha7MachineSpec(&prevBastion.Instance, &dstBastion.Instance)
+	}
+
+	// We only restore DNSNameservers when these were lossly converted when NodeCIDR is empty.
+	if len(previous.DNSNameservers) > 0 && dst.NodeCIDR == "" {
+		dst.DNSNameservers = previous.DNSNameservers
+	}
+}
+
 func restorev1alpha8ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *infrav1.OpenStackClusterSpec) {
 	prevBastion := previous.Bastion
 	dstBastion := dst.Bastion
@@ -132,6 +163,8 @@ func restorev1alpha8ClusterSpec(previous *infrav1.OpenStackClusterSpec, dst *inf
 	if len(previous.Subnets) > 1 {
 		dst.Subnets = append(dst.Subnets, previous.Subnets[1:]...)
 	}
+
+	dst.ManagedSubnets = previous.ManagedSubnets
 }
 
 func (r *OpenStackCluster) ConvertTo(dstRaw ctrlconversion.Hub) error {
@@ -171,6 +204,7 @@ func (r *OpenStackClusterList) ConvertFrom(srcRaw ctrlconversion.Hub) error {
 var _ ctrlconversion.Convertible = &OpenStackClusterTemplate{}
 
 func restorev1alpha7ClusterTemplateSpec(previous *OpenStackClusterTemplateSpec, dst *OpenStackClusterTemplateSpec) {
+	restorev1alpha7ClusterSpec(&previous.Template.Spec, &dst.Template.Spec)
 	restorev1alpha7Bastion(&previous.Template.Spec.Bastion, &dst.Template.Spec.Bastion)
 }
 
@@ -465,6 +499,16 @@ func Convert_v1alpha7_OpenStackClusterSpec_To_v1alpha8_OpenStackClusterSpec(in *
 		out.Subnets = []infrav1.SubnetFilter{subnet}
 	}
 
+	// DNSNameservers without NodeCIDR doesn't make sense, so we drop that.
+	if len(in.NodeCIDR) > 0 {
+		out.ManagedSubnets = []infrav1.SubnetSpec{
+			{
+				CIDR:           in.NodeCIDR,
+				DNSNameservers: in.DNSNameservers,
+			},
+		}
+	}
+
 	return nil
 }
 
@@ -484,5 +528,10 @@ func Convert_v1alpha8_OpenStackClusterSpec_To_v1alpha7_OpenStackClusterSpec(in *
 		}
 	}
 
+	if len(in.ManagedSubnets) > 0 {
+		out.NodeCIDR = in.ManagedSubnets[0].CIDR
+		out.DNSNameservers = in.ManagedSubnets[0].DNSNameservers
+	}
+
 	return nil
 }

api/v1alpha6/zz_generated.conversion.go

@@ -100,6 +100,16 @@ func TestFuzzyConversion(t *testing.T) {
 					}
 				}
 			},
+
+			func(spec *infrav1.SubnetSpec, c fuzz.Continue) {
+				c.FuzzNoCustom(spec)
+
+				// CIDR is required and API validates that it's present, so
+				// we force it to always be set.
+				for spec.CIDR == "" {
+					spec.CIDR = c.RandString()
+				}
+			},
 		}
 	}
 

api/v1alpha7/conversion.go

@@ -34,10 +34,11 @@ type OpenStackClusterSpec struct {
 	// +optional
 	CloudName string `json:"cloudName"`
 
-	// NodeCIDR is the OpenStack Subnet to be created. Cluster actuator will create a
-	// network, a subnet with NodeCIDR, and a router connected to this subnet.
-	// If you leave this empty, no network will be created.
-	NodeCIDR string `json:"nodeCidr,omitempty"`
+	// ManagedSubnets describe OpenStack Subnets to be created. Cluster actuator will create a network,
+	// subnets with the defined CIDR, and a router connected to these subnets. Currently only one IPv4
+	// subnet is supported. If you leave this empty, no network will be created.
+	// +kubebuilder:validation:MaxItems=1
+	ManagedSubnets []SubnetSpec `json:"managedSubnets,omitempty"`
 
 	// If NodeCIDR is set this option can be used to detect an existing router.
 	// If specified, no new router will be created.
@@ -58,11 +59,6 @@ type OpenStackClusterSpec struct {
 	// +optional
 	NetworkMTU int `json:"networkMtu,omitempty"`
 
-	// DNSNameservers is the list of nameservers for OpenStack Subnet being created.
-	// Set this value when you need create a new network/subnet while the access
-	// through DNS is required.
-	// +listType=set
-	DNSNameservers []string `json:"dnsNameservers,omitempty"`
 	// ExternalRouterIPs is an array of externalIPs on the respective subnets.
 	// This is necessary if the router needs a fixed ip in a specific subnet.
 	ExternalRouterIPs []ExternalRouterIPParam `json:"externalRouterIPs,omitempty"`

api/v1alpha7/conversion_test.go

@@ -87,6 +87,16 @@ type RouterFilter struct {
 	NotTagsAny  string `json:"notTagsAny,omitempty"`
 }
 
+type SubnetSpec struct {
+	// CIDR is representing the IP address range used to create the subnet, e.g. 10.0.0.0/24.
+	// This field is required when defining a subnet.
+	// +required
+	CIDR string `json:"cidr"`
+	// DNSNameservers holds a list of DNS server addresses that will be provided when creating
+	// the subnet. These addresses need to have the same IP version as CIDR.
+	DNSNameservers []string `json:"dnsNameservers,omitempty"`
+}
+
 type PortOpts struct {
 	// Network is a query for an openstack network that the port will be created or discovered on.
 	// This will fail if the query returns more than one network.