diff --git a/controllers/openstackcluster_controller.go b/controllers/openstackcluster_controller.go index 5246ba218d..914e662fa4 100644 --- a/controllers/openstackcluster_controller.go +++ b/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" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -507,36 +509,41 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o if len(openStackCluster.Spec.ManagedSubnets) == 0 { 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 if len(openStackCluster.Spec.ManagedSubnets) == 1 { err := networkingService.ReconcileNetwork(openStackCluster, clusterName) if err != nil { @@ -687,18 +694,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) { @@ -709,9 +721,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]) @@ -722,8 +731,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 } diff --git a/controllers/openstackcluster_controller_test.go b/controllers/openstackcluster_controller_test.go index b512ac62f4..3a51ad6413 100644 --- a/controllers/openstackcluster_controller_test.go +++ b/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" @@ -544,6 +546,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 { @@ -554,3 +595,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) + } +} diff --git a/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md b/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md index be1b3b63c8..67f338ea49 100644 --- a/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md +++ b/docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md @@ -243,3 +243,7 @@ Now the user needs to request creation of the security group rules by using the Note that when upgrading from a previous version, the Calico CNI security group rules will be added automatically to allow backwards compatibility if `allowAllInClusterTraffic` is set to false. + +#### ⚠️ Change to network + +In v1alpha8, when the `OpenStackCluster.Spec.Network` is not defined, the `Subnets` are now used to identify the `Network`. diff --git a/pkg/cloud/services/networking/network.go b/pkg/cloud/services/networking/network.go index 540dce22f4..7629abdee8 100644 --- a/pkg/cloud/services/networking/network.go +++ b/pkg/cloud/services/networking/network.go @@ -386,14 +386,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 } diff --git a/pkg/cloud/services/networking/network_test.go b/pkg/cloud/services/networking/network_test.go index 512cc53146..47ff27783a 100644 --- a/pkg/cloud/services/networking/network_test.go +++ b/pkg/cloud/services/networking/network_test.go @@ -17,7 +17,6 @@ limitations under the License. package networking import ( - "reflect" "testing" "github.com/go-logr/logr" @@ -25,7 +24,6 @@ import ( "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) - } -}