Modify OpenStackCluster.Spec.Network API

For the BYO scenario, when the `OpenStackCluster.Spec.Network`
is not specified the query to OpenStack would return all the
Networks available in the cloud and fail the reconciliation.
To avoid this, if any Subnets were specified under
`OpenStackCluster.Spec.Subnets` this can be used to identify
which Network to use.

Author: MaysaMacedo

controllers/openstackcluster_controller.go

@@ -23,6 +23,8 @@ import (
 	"reflect"
 	"time"
 
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/tools/record"
@@ -496,36 +498,41 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o
 	if openStackCluster.Spec.NodeCIDR == "" {
 		scope.Logger().V(4).Info("No need to reconcile network, searching network and subnet instead")
 
-		netOpts := openStackCluster.Spec.Network.ToListOpt()
-		networkList, err := networkingService.GetNetworksByFilter(&netOpts)
-		if err != nil {
-			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find network: %w", err))
-			return fmt.Errorf("failed to find network: %w", err)
-		}
-		if len(networkList) == 0 {
-			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find any network"))
-			return fmt.Errorf("failed to find any network")
-		}
-		if len(networkList) > 1 {
-			handleUpdateOSCError(openStackCluster, fmt.Errorf("found multiple networks (result: %v)", networkList))
-			return fmt.Errorf("found multiple networks (result: %v)", networkList)
-		}
 		if openStackCluster.Status.Network == nil {
 			openStackCluster.Status.Network = &infrav1.NetworkStatusWithSubnets{}
 		}
-		openStackCluster.Status.Network.ID = networkList[0].ID
-		openStackCluster.Status.Network.Name = networkList[0].Name
-		openStackCluster.Status.Network.Tags = networkList[0].Tags
 
-		subnets, err := filterSubnets(networkingService, openStackCluster)
+		err := getCAPONetwork(openStackCluster, networkingService)
 		if err != nil {
 			return err
 		}
 
+		filteredSubnets, err := filterSubnets(networkingService, openStackCluster)
+		if err != nil {
+			return err
+		}
+
+		var subnets []infrav1.Subnet
+		for subnet := range filteredSubnets {
+			filterSubnet := &filteredSubnets[subnet]
+			subnets = append(subnets, infrav1.Subnet{
+				ID:   filterSubnet.ID,
+				Name: filterSubnet.Name,
+				CIDR: filterSubnet.CIDR,
+				Tags: filterSubnet.Tags,
+			})
+		}
+
 		if err := utils.ValidateSubnets(subnets); err != nil {
 			return err
 		}
 		openStackCluster.Status.Network.Subnets = subnets
+
+		// If network is not yet populated on the Status, use networkID defined in the filtered subnets to get the Network.
+		err = populateCAPONetworkFromSubnet(networkingService, filteredSubnets, openStackCluster)
+		if err != nil {
+			return err
+		}
 	} else {
 		err := networkingService.ReconcileNetwork(openStackCluster, clusterName)
 		if err != nil {
@@ -674,18 +681,23 @@ func handleUpdateOSCError(openstackCluster *infrav1.OpenStackCluster, message er
 }
 
 // filterSubnets retrieves the subnets based on the Subnet filters specified on OpenstackCluster.
-func filterSubnets(networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster) ([]infrav1.Subnet, error) {
-	var subnets []infrav1.Subnet
+func filterSubnets(networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster) ([]subnets.Subnet, error) {
+	var filteredSubnets []subnets.Subnet
+	var err error
 	openStackClusterSubnets := openStackCluster.Spec.Subnets
-	if openStackCluster.Status.Network == nil {
-		return nil, nil
+	networkID := ""
+	if openStackCluster.Status.Network != nil {
+		networkID = openStackCluster.Status.Network.ID
 	}
-	networkID := openStackCluster.Status.Network.ID
+
 	if len(openStackClusterSubnets) == 0 {
+		if networkID == "" {
+			return nil, nil
+		}
 		empty := &infrav1.SubnetFilter{}
 		listOpt := empty.ToListOpt()
 		listOpt.NetworkID = networkID
-		filteredSubnets, err := networkingService.GetSubnetsByFilter(listOpt)
+		filteredSubnets, err = networkingService.GetSubnetsByFilter(listOpt)
 		if err != nil {
 			err = fmt.Errorf("failed to find subnets: %w", err)
 			if errors.Is(err, networking.ErrFilterMatch) {
@@ -696,9 +708,6 @@ func filterSubnets(networkingService *networking.Service, openStackCluster *infr
 		if len(filteredSubnets) > 2 {
 			return nil, fmt.Errorf("more than two subnets found in the Network. Specify the subnets in the OpenStackCluster.Spec instead")
 		}
-		for subnet := range filteredSubnets {
-			subnets = networkingService.ConvertOpenStackSubnetToCAPOSubnet(subnets, &filteredSubnets[subnet])
-		}
 	} else {
 		for subnet := range openStackClusterSubnets {
 			filteredSubnet, err := networkingService.GetNetworkSubnetByFilter(networkID, &openStackClusterSubnets[subnet])
@@ -709,8 +718,55 @@ func filterSubnets(networkingService *networking.Service, openStackCluster *infr
 				}
 				return nil, err
 			}
-			subnets = networkingService.ConvertOpenStackSubnetToCAPOSubnet(subnets, filteredSubnet)
+			filteredSubnets = append(filteredSubnets, *filteredSubnet)
 		}
 	}
-	return subnets, nil
+	return filteredSubnets, nil
+}
+
+// convertOpenStackNetworkToCAPONetwork converts an OpenStack network to a capo network.
+// It returns the converted subnet.
+func convertOpenStackNetworkToCAPONetwork(openStackCluster *infrav1.OpenStackCluster, network *networks.Network) {
+	openStackCluster.Status.Network.ID = network.ID
+	openStackCluster.Status.Network.Name = network.Name
+	openStackCluster.Status.Network.Tags = network.Tags
+}
+
+// populateCAPONetworkFromSubnet gets a network based on the networkID of the subnets and converts it to the CAPO format.
+// It returns an error in case it failed to retrieve the network.
+func populateCAPONetworkFromSubnet(networkingService *networking.Service, subnets []subnets.Subnet, openStackCluster *infrav1.OpenStackCluster) error {
+	if openStackCluster.Status.Network.ID == "" && len(subnets) > 0 {
+		if len(subnets) > 1 && subnets[0].NetworkID != subnets[1].NetworkID {
+			return fmt.Errorf("unable to identify the Network to use. NetworkID %s from subnet %s does not match NetworkID %s from subnet %s", subnets[0].NetworkID, subnets[0].ID, subnets[1].NetworkID, subnets[1].ID)
+		}
+
+		network, err := networkingService.GetNetworkByID(subnets[0].NetworkID)
+		if err != nil {
+			return err
+		}
+		convertOpenStackNetworkToCAPONetwork(openStackCluster, network)
+	}
+	return nil
+}
+
+// getCAPONetwork gets a network based on a filter, if defined, and convert the network to the CAPO format.
+// It returns an error in case it failed to retrieve the network.
+func getCAPONetwork(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service) error {
+	emptyNetwork := infrav1.NetworkFilter{}
+	if openStackCluster.Spec.Network != emptyNetwork {
+		netOpts := openStackCluster.Spec.Network.ToListOpt()
+		networkList, err := networkingService.GetNetworksByFilter(&netOpts)
+		if err != nil {
+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find network: %w", err))
+			return fmt.Errorf("failed to find network: %w", err)
+		}
+		if len(networkList) == 0 {
+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find any network"))
+			return fmt.Errorf("failed to find any network")
+		}
+		if len(networkList) == 1 {
+			convertOpenStackNetworkToCAPONetwork(openStackCluster, &networkList[0])
+		}
+	}
+	return nil
 }

controllers/openstackcluster_controller_test.go

@@ -19,6 +19,8 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"reflect"
+	"testing"
 
 	"github.com/go-logr/logr"
 	"github.com/golang/mock/gomock"
@@ -518,6 +520,45 @@ var _ = Describe("OpenStackCluster controller", func() {
 		Expect(err).To(BeNil())
 		Expect(len(testCluster.Status.Network.Subnets)).To(Equal(2))
 	})
+
+	It("should allow fetch network by subnet", func() {
+		const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
+		const clusterSubnetID = "cad5a91a-36de-4388-823b-b0cc82cadfdc"
+
+		testCluster.SetName("subnet-filtering")
+		testCluster.Spec = infrav1.OpenStackClusterSpec{
+			DisableAPIServerFloatingIP: true,
+			APIServerFixedIP:           "10.0.0.1",
+			DisableExternalNetwork:     true,
+			Subnets: []infrav1.SubnetFilter{
+				{ID: clusterSubnetID},
+			},
+		}
+		err := k8sClient.Create(ctx, testCluster)
+		Expect(err).To(BeNil())
+		err = k8sClient.Create(ctx, capiCluster)
+		Expect(err).To(BeNil())
+		scope, err := mockScopeFactory.NewClientScopeFromCluster(ctx, k8sClient, testCluster, nil, logr.Discard())
+		Expect(err).To(BeNil())
+
+		networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
+
+		// Fetching cluster subnets should be filtered by cluster network id
+		networkClientRecorder.GetSubnet(clusterSubnetID).Return(&subnets.Subnet{
+			ID:        clusterSubnetID,
+			CIDR:      "192.168.0.0/24",
+			NetworkID: clusterNetworkID,
+		}, nil)
+
+		// Fetch cluster network using the NetworkID from the filtered Subnets
+		networkClientRecorder.GetNetwork(clusterNetworkID).Return(&networks.Network{
+			ID: clusterNetworkID,
+		}, nil)
+
+		err = reconcileNetworkComponents(scope, capiCluster, testCluster)
+		Expect(err).To(BeNil())
+		Expect(testCluster.Status.Network.ID).To(Equal(clusterNetworkID))
+	})
 })
 
 func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reconcile.Request {
@@ -528,3 +569,25 @@ func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reco
 		},
 	}
 }
+
+func Test_ConvertOpenStackNetworkToCAPONetwork(t *testing.T) {
+	openStackCluster := &infrav1.OpenStackCluster{}
+	openStackCluster.Status.Network = &infrav1.NetworkStatusWithSubnets{}
+
+	filterednetwork := &networks.Network{
+		ID:   "network1",
+		Name: "network1",
+		Tags: []string{"tag1", "tag2"},
+	}
+
+	convertOpenStackNetworkToCAPONetwork(openStackCluster, filterednetwork)
+	expected := infrav1.NetworkStatus{
+		ID:   "network1",
+		Name: "network1",
+		Tags: []string{"tag1", "tag2"},
+	}
+
+	if !reflect.DeepEqual(openStackCluster.Status.Network.NetworkStatus, expected) {
+		t.Errorf("ConvertOpenStackNetworkToCAPONetwork() = %v, want %v", openStackCluster.Status.Network.NetworkStatus, expected)
+	}
+}

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

@@ -176,4 +176,8 @@ In v1alpha8, this will be automatically converted to:
 
 `Subnets` allows specifications of maximum two `SubnetFilter` one being IPv4 and the other IPv6. Both subnets must be on the same network. Any filtered subnets will be added to `OpenStackCluster.Status.Network.Subnets`.
 
-When subnets are not specified on `OpenStackCluster` and only the network is, the network is used to identify the subnets to use. If more than two subnets exist in the network, the user must specify which ones to use by defining the `OpenStackCluster.Spec.Subnets` field.
+When subnets are not specified on `OpenStackCluster` and only the network is, the network is used to identify the subnets to use. If more than two subnets exist in the network, the user must specify which ones to use by defining the `OpenStackCluster.Spec.Subnets` field.
+
+#### ⚠️ Change to network
+
+In v1alpha8, when the `OpenStackCluster.Spec.Network` is not defined, the `Subnets` are now used to identify the `Network`.

pkg/cloud/services/networking/network.go

@@ -384,14 +384,11 @@ func getNetworkName(clusterName string) string {
 	return fmt.Sprintf("%s-cluster-%s", networkPrefix, clusterName)
 }
 
-// ConvertOpenStackSubnetToCAPOSubnet converts an OpenStack subnet to a capo subnet and adds to a slice.
-// It returns the slice with the converted subnet.
-func (s *Service) ConvertOpenStackSubnetToCAPOSubnet(subnets []infrav1.Subnet, filteredSubnet *subnets.Subnet) []infrav1.Subnet {
-	subnets = append(subnets, infrav1.Subnet{
-		ID:   filteredSubnet.ID,
-		Name: filteredSubnet.Name,
-		CIDR: filteredSubnet.CIDR,
-		Tags: filteredSubnet.Tags,
-	})
-	return subnets
+// GetNetworkByID retrieves network by the ID.
+func (s *Service) GetNetworkByID(networkID string) (*networks.Network, error) {
+	network, err := s.client.GetNetwork(networkID)
+	if err != nil {
+		return &networks.Network{}, err
+	}
+	return network, nil
 }

pkg/cloud/services/networking/network_test.go

@@ -17,15 +17,13 @@ limitations under the License.
 package networking
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/go-logr/logr"
 	"github.com/golang/mock/gomock"
 	"github.com/gophercloud/gophercloud"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/external"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
-	"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
 	. "github.com/onsi/gomega"
 	"k8s.io/utils/pointer"
 
@@ -422,43 +420,3 @@ func Test_ReconcileExternalNetwork(t *testing.T) {
 		})
 	}
 }
-
-func Test_ConvertOpenStackSubnetToCAPOSubnet(t *testing.T) {
-	caposubnets := []infrav1.Subnet{
-		{
-			ID:   "subnet1",
-			Name: "subnet1",
-			CIDR: "10.0.0.0/24",
-			Tags: []string{"tag1", "tag2"},
-		},
-	}
-
-	filteredSubnet := &subnets.Subnet{
-		ID:   "subnet2",
-		Name: "subnet2",
-		CIDR: "192.168.0.0/24",
-		Tags: []string{"tag3", "tag4"},
-	}
-
-	s := Service{}
-	result := s.ConvertOpenStackSubnetToCAPOSubnet(caposubnets, filteredSubnet)
-
-	expected := []infrav1.Subnet{
-		{
-			ID:   "subnet1",
-			Name: "subnet1",
-			CIDR: "10.0.0.0/24",
-			Tags: []string{"tag1", "tag2"},
-		},
-		{
-			ID:   "subnet2",
-			Name: "subnet2",
-			CIDR: "192.168.0.0/24",
-			Tags: []string{"tag3", "tag4"},
-		},
-	}
-
-	if !reflect.DeepEqual(result, expected) {
-		t.Errorf("ConvertOpenStackSubnetToCAPOSubnet() = %v, want %v", result, expected)
-	}
-}