Skip to content

Commit 14ecbda

Browse files
MaysaMacedoMaysaMacedo
committed
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.
1 parent 05571b9 commit 14ecbda

File tree

5 files changed

+160
-82
lines changed

5 files changed

+160
-82
lines changed

‎controllers/openstackcluster_controller.go

Lines changed: 85 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ import (
2323
"reflect"
2424
"time"
2525

26+
"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
27+
"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
2628
corev1 "k8s.io/api/core/v1"
2729
apierrors "k8s.io/apimachinery/pkg/api/errors"
2830
"k8s.io/client-go/tools/record"
@@ -496,36 +498,41 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o
496498
if openStackCluster.Spec.NodeCIDR == "" {
497499
scope.Logger().V(4).Info("No need to reconcile network, searching network and subnet instead")
498500

499-
netOpts := openStackCluster.Spec.Network.ToListOpt()
500-
networkList, err := networkingService.GetNetworksByFilter(&netOpts)
501-
if err != nil {
502-
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find network: %w", err))
503-
return fmt.Errorf("failed to find network: %w", err)
504-
}
505-
if len(networkList) == 0 {
506-
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find any network"))
507-
return fmt.Errorf("failed to find any network")
508-
}
509-
if len(networkList) > 1 {
510-
handleUpdateOSCError(openStackCluster, fmt.Errorf("found multiple networks (result: %v)", networkList))
511-
return fmt.Errorf("found multiple networks (result: %v)", networkList)
512-
}
513501
if openStackCluster.Status.Network == nil {
514502
openStackCluster.Status.Network = &infrav1.NetworkStatusWithSubnets{}
515503
}
516-
openStackCluster.Status.Network.ID = networkList[0].ID
517-
openStackCluster.Status.Network.Name = networkList[0].Name
518-
openStackCluster.Status.Network.Tags = networkList[0].Tags
519504

520-
subnets, err := filterSubnets(networkingService, openStackCluster)
505+
err := getCAPONetwork(openStackCluster, networkingService)
521506
if err != nil {
522507
return err
523508
}
524509

510+
filteredSubnets, err := filterSubnets(networkingService, openStackCluster)
511+
if err != nil {
512+
return err
513+
}
514+
515+
var subnets []infrav1.Subnet
516+
for subnet := range filteredSubnets {
517+
filterSubnet := &filteredSubnets[subnet]
518+
subnets = append(subnets, infrav1.Subnet{
519+
ID: filterSubnet.ID,
520+
Name: filterSubnet.Name,
521+
CIDR: filterSubnet.CIDR,
522+
Tags: filterSubnet.Tags,
523+
})
524+
}
525+
525526
if err := utils.ValidateSubnets(subnets); err != nil {
526527
return err
527528
}
528529
openStackCluster.Status.Network.Subnets = subnets
530+
531+
// If network is not yet populated on the Status, use networkID defined in the filtered subnets to get the Network.
532+
err = populateCAPONetworkFromSubnet(networkingService, filteredSubnets, openStackCluster)
533+
if err != nil {
534+
return err
535+
}
529536
} else {
530537
err := networkingService.ReconcileNetwork(openStackCluster, clusterName)
531538
if err != nil {
@@ -674,18 +681,23 @@ func handleUpdateOSCError(openstackCluster *infrav1.OpenStackCluster, message er
674681
}
675682

676683
// filterSubnets retrieves the subnets based on the Subnet filters specified on OpenstackCluster.
677-
func filterSubnets(networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster) ([]infrav1.Subnet, error) {
678-
var subnets []infrav1.Subnet
684+
func filterSubnets(networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster) ([]subnets.Subnet, error) {
685+
var filteredSubnets []subnets.Subnet
686+
var err error
679687
openStackClusterSubnets := openStackCluster.Spec.Subnets
680-
if openStackCluster.Status.Network == nil {
681-
return nil, nil
688+
networkID := ""
689+
if openStackCluster.Status.Network != nil {
690+
networkID = openStackCluster.Status.Network.ID
682691
}
683-
networkID := openStackCluster.Status.Network.ID
692+
684693
if len(openStackClusterSubnets) == 0 {
694+
if networkID == "" {
695+
return nil, nil
696+
}
685697
empty := &infrav1.SubnetFilter{}
686698
listOpt := empty.ToListOpt()
687699
listOpt.NetworkID = networkID
688-
filteredSubnets, err := networkingService.GetSubnetsByFilter(listOpt)
700+
filteredSubnets, err = networkingService.GetSubnetsByFilter(listOpt)
689701
if err != nil {
690702
err = fmt.Errorf("failed to find subnets: %w", err)
691703
if errors.Is(err, networking.ErrFilterMatch) {
@@ -696,9 +708,6 @@ func filterSubnets(networkingService *networking.Service, openStackCluster *infr
696708
if len(filteredSubnets) > 2 {
697709
return nil, fmt.Errorf("more than two subnets found in the Network. Specify the subnets in the OpenStackCluster.Spec instead")
698710
}
699-
for subnet := range filteredSubnets {
700-
subnets = networkingService.ConvertOpenStackSubnetToCAPOSubnet(subnets, &filteredSubnets[subnet])
701-
}
702711
} else {
703712
for subnet := range openStackClusterSubnets {
704713
filteredSubnet, err := networkingService.GetNetworkSubnetByFilter(networkID, &openStackClusterSubnets[subnet])
@@ -709,8 +718,55 @@ func filterSubnets(networkingService *networking.Service, openStackCluster *infr
709718
}
710719
return nil, err
711720
}
712-
subnets = networkingService.ConvertOpenStackSubnetToCAPOSubnet(subnets, filteredSubnet)
721+
filteredSubnets = append(filteredSubnets, *filteredSubnet)
713722
}
714723
}
715-
return subnets, nil
724+
return filteredSubnets, nil
725+
}
726+
727+
// convertOpenStackNetworkToCAPONetwork converts an OpenStack network to a capo network.
728+
// It returns the converted subnet.
729+
func convertOpenStackNetworkToCAPONetwork(openStackCluster *infrav1.OpenStackCluster, network *networks.Network) {
730+
openStackCluster.Status.Network.ID = network.ID
731+
openStackCluster.Status.Network.Name = network.Name
732+
openStackCluster.Status.Network.Tags = network.Tags
733+
}
734+
735+
// populateCAPONetworkFromSubnet gets a network based on the networkID of the subnets and converts it to the CAPO format.
736+
// It returns an error in case it failed to retrieve the network.
737+
func populateCAPONetworkFromSubnet(networkingService *networking.Service, subnets []subnets.Subnet, openStackCluster *infrav1.OpenStackCluster) error {
738+
if openStackCluster.Status.Network.ID == "" && len(subnets) > 0 {
739+
if len(subnets) > 1 && subnets[0].NetworkID != subnets[1].NetworkID {
740+
return fmt.Errorf("unable to identify the Network to use. NetworkID from Subnets differ: %s, %s", subnets[0].NetworkID, subnets[1].NetworkID)
741+
}
742+
743+
network, err := networkingService.GetNetworkByID(subnets[0].NetworkID)
744+
if err != nil {
745+
return err
746+
}
747+
convertOpenStackNetworkToCAPONetwork(openStackCluster, network)
748+
}
749+
return nil
750+
}
751+
752+
// getCAPONetwork gets a network based on a filter.
753+
// It returns an error in case it failed to retrieve the network.
754+
func getCAPONetwork(openStackCluster *infrav1.OpenStackCluster, networkingService *networking.Service) error {
755+
emptyNetwork := infrav1.NetworkFilter{}
756+
if openStackCluster.Spec.Network != emptyNetwork {
757+
netOpts := openStackCluster.Spec.Network.ToListOpt()
758+
networkList, err := networkingService.GetNetworksByFilter(&netOpts)
759+
if err != nil {
760+
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find network: %w", err))
761+
return fmt.Errorf("failed to find network: %w", err)
762+
}
763+
if len(networkList) == 0 {
764+
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find any network"))
765+
return fmt.Errorf("failed to find any network")
766+
}
767+
if len(networkList) == 1 {
768+
convertOpenStackNetworkToCAPONetwork(openStackCluster, &networkList[0])
769+
}
770+
}
771+
return nil
716772
}

‎controllers/openstackcluster_controller_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ package controllers
1919
import (
2020
"context"
2121
"fmt"
22+
"reflect"
23+
"testing"
2224

2325
"github.com/go-logr/logr"
2426
"github.com/golang/mock/gomock"
@@ -518,6 +520,45 @@ var _ = Describe("OpenStackCluster controller", func() {
518520
Expect(err).To(BeNil())
519521
Expect(len(testCluster.Status.Network.Subnets)).To(Equal(2))
520522
})
523+
524+
It("should allow fetch network by subnet", func() {
525+
const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
526+
const clusterSubnetID = "cad5a91a-36de-4388-823b-b0cc82cadfdc"
527+
528+
testCluster.SetName("subnet-filtering")
529+
testCluster.Spec = infrav1.OpenStackClusterSpec{
530+
DisableAPIServerFloatingIP: true,
531+
APIServerFixedIP: "10.0.0.1",
532+
DisableExternalNetwork: true,
533+
Subnets: []infrav1.SubnetFilter{
534+
{ID: clusterSubnetID},
535+
},
536+
}
537+
err := k8sClient.Create(ctx, testCluster)
538+
Expect(err).To(BeNil())
539+
err = k8sClient.Create(ctx, capiCluster)
540+
Expect(err).To(BeNil())
541+
scope, err := mockScopeFactory.NewClientScopeFromCluster(ctx, k8sClient, testCluster, nil, logr.Discard())
542+
Expect(err).To(BeNil())
543+
544+
networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()
545+
546+
// Fetching cluster subnets should be filtered by cluster network id
547+
networkClientRecorder.GetSubnet(clusterSubnetID).Return(&subnets.Subnet{
548+
ID: clusterSubnetID,
549+
CIDR: "192.168.0.0/24",
550+
NetworkID: clusterNetworkID,
551+
}, nil)
552+
553+
// Fetch cluster network using the NetworkID from the filtered Subnets
554+
networkClientRecorder.GetNetwork(clusterNetworkID).Return(&networks.Network{
555+
ID: clusterNetworkID,
556+
}, nil)
557+
558+
err = reconcileNetworkComponents(scope, capiCluster, testCluster)
559+
Expect(err).To(BeNil())
560+
Expect(testCluster.Status.Network.ID).To(Equal(clusterNetworkID))
561+
})
521562
})
522563

523564
func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reconcile.Request {
@@ -528,3 +569,25 @@ func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reco
528569
},
529570
}
530571
}
572+
573+
func Test_ConvertOpenStackNetworkToCAPONetwork(t *testing.T) {
574+
openStackCluster := &infrav1.OpenStackCluster{}
575+
openStackCluster.Status.Network = &infrav1.NetworkStatusWithSubnets{}
576+
577+
filterednetwork := &networks.Network{
578+
ID: "network1",
579+
Name: "network1",
580+
Tags: []string{"tag1", "tag2"},
581+
}
582+
583+
convertOpenStackNetworkToCAPONetwork(openStackCluster, filterednetwork)
584+
expected := infrav1.NetworkStatus{
585+
ID: "network1",
586+
Name: "network1",
587+
Tags: []string{"tag1", "tag2"},
588+
}
589+
590+
if !reflect.DeepEqual(openStackCluster.Status.Network.NetworkStatus, expected) {
591+
t.Errorf("ConvertOpenStackNetworkToCAPONetwork() = %v, want %v", openStackCluster.Status.Network.NetworkStatus, expected)
592+
}
593+
}

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,4 +176,8 @@ In v1alpha8, this will be automatically converted to:
176176

177177
`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`.
178178

179-
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.
179+
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.
180+
181+
#### ⚠️ Change to network
182+
183+
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

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -384,14 +384,11 @@ func getNetworkName(clusterName string) string {
384384
return fmt.Sprintf("%s-cluster-%s", networkPrefix, clusterName)
385385
}
386386

387-
// ConvertOpenStackSubnetToCAPOSubnet converts an OpenStack subnet to a capo subnet and adds to a slice.
388-
// It returns the slice with the converted subnet.
389-
func (s *Service) ConvertOpenStackSubnetToCAPOSubnet(subnets []infrav1.Subnet, filteredSubnet *subnets.Subnet) []infrav1.Subnet {
390-
subnets = append(subnets, infrav1.Subnet{
391-
ID: filteredSubnet.ID,
392-
Name: filteredSubnet.Name,
393-
CIDR: filteredSubnet.CIDR,
394-
Tags: filteredSubnet.Tags,
395-
})
396-
return subnets
387+
// GetNetworkByID retrieves network by the ID.
388+
func (s *Service) GetNetworkByID(networkID string) (*networks.Network, error) {
389+
network, err := s.client.GetNetwork(networkID)
390+
if err != nil {
391+
return &networks.Network{}, err
392+
}
393+
return network, nil
397394
}

‎pkg/cloud/services/networking/network_test.go

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,13 @@ limitations under the License.
1717
package networking
1818

1919
import (
20-
"reflect"
2120
"testing"
2221

2322
"github.com/go-logr/logr"
2423
"github.com/golang/mock/gomock"
2524
"github.com/gophercloud/gophercloud"
2625
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/external"
2726
"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
28-
"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
2927
. "github.com/onsi/gomega"
3028
"k8s.io/utils/pointer"
3129

@@ -422,43 +420,3 @@ func Test_ReconcileExternalNetwork(t *testing.T) {
422420
})
423421
}
424422
}
425-
426-
func Test_ConvertOpenStackSubnetToCAPOSubnet(t *testing.T) {
427-
caposubnets := []infrav1.Subnet{
428-
{
429-
ID: "subnet1",
430-
Name: "subnet1",
431-
CIDR: "10.0.0.0/24",
432-
Tags: []string{"tag1", "tag2"},
433-
},
434-
}
435-
436-
filteredSubnet := &subnets.Subnet{
437-
ID: "subnet2",
438-
Name: "subnet2",
439-
CIDR: "192.168.0.0/24",
440-
Tags: []string{"tag3", "tag4"},
441-
}
442-
443-
s := Service{}
444-
result := s.ConvertOpenStackSubnetToCAPOSubnet(caposubnets, filteredSubnet)
445-
446-
expected := []infrav1.Subnet{
447-
{
448-
ID: "subnet1",
449-
Name: "subnet1",
450-
CIDR: "10.0.0.0/24",
451-
Tags: []string{"tag1", "tag2"},
452-
},
453-
{
454-
ID: "subnet2",
455-
Name: "subnet2",
456-
CIDR: "192.168.0.0/24",
457-
Tags: []string{"tag3", "tag4"},
458-
},
459-
}
460-
461-
if !reflect.DeepEqual(result, expected) {
462-
t.Errorf("ConvertOpenStackSubnetToCAPOSubnet() = %v, want %v", result, expected)
463-
}
464-
}

0 commit comments

Comments
 (0)