Merge pull request #1572 from shiftstack/issue1570

🐛 Always filter cluster subnets by cluster network ID

Author: k8s-ci-robot

controllers/openstackcluster_controller.go

@@ -445,7 +445,7 @@ func reconcileNetworkComponents(scope scope.Scope, cluster *clusterv1.Cluster, o
 		openStackCluster.Status.Network.Name = networkList[0].Name
 		openStackCluster.Status.Network.Tags = networkList[0].Tags
 
-		subnet, err := networkingService.GetSubnetByFilter(&openStackCluster.Spec.Subnet)
+		subnet, err := networkingService.GetNetworkSubnetByFilter(openStackCluster.Status.Network.ID, &openStackCluster.Spec.Subnet)
 		if err != nil {
 			err = fmt.Errorf("failed to find subnet: %w", err)
 

controllers/openstackcluster_controller_test.go

@@ -24,6 +24,8 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
 	"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups"
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
+	"github.com/gophercloud/gophercloud/openstack/networking/v2/subnets"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -207,6 +209,63 @@ var _ = Describe("OpenStackCluster controller", func() {
 		err = deleteBastion(scope, capiCluster, testCluster)
 		Expect(err).To(BeNil())
 	})
+	It("should implicitly filter cluster subnets by cluster network", func() {
+		const externalNetworkID = "a42211a2-4d2c-426f-9413-830e4b4abbbc"
+		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",
+			ExternalNetworkID:          externalNetworkID,
+			Network: infrav1.NetworkFilter{
+				ID: clusterNetworkID,
+			},
+		}
+		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()
+
+		// Fetch external network
+		networkClientRecorder.ListNetwork(networks.ListOpts{
+			ID: externalNetworkID,
+		}).Return([]networks.Network{
+			{
+				ID:   externalNetworkID,
+				Name: "external-network",
+			},
+		}, nil)
+
+		// Fetch cluster network
+		networkClientRecorder.ListNetwork(&networks.ListOpts{
+			ID: clusterNetworkID,
+		}).Return([]networks.Network{
+			{
+				ID:   clusterNetworkID,
+				Name: "cluster-network",
+			},
+		}, nil)
+
+		// Fetching cluster subnets should be filtered by cluster network id
+		networkClientRecorder.ListSubnet(subnets.ListOpts{
+			NetworkID: clusterNetworkID,
+		}).Return([]subnets.Subnet{
+			{
+				ID:   clusterSubnetID,
+				Name: "cluster-subnet",
+				CIDR: "192.168.0.0/24",
+			},
+		}, nil)
+
+		err = reconcileNetworkComponents(scope, capiCluster, testCluster)
+		Expect(err).To(BeNil())
+	})
 })
 
 func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reconcile.Request {

pkg/cloud/services/networking/network.go

@@ -351,16 +351,31 @@ func (s *Service) GetSubnetsByFilter(opts subnets.ListOptsBuilder) ([]subnets.Su
 // GetSubnetByFilter gets a single subnet specified by the given SubnetFilter.
 // It returns an ErrFilterMatch if no or multiple subnets are found.
 func (s *Service) GetSubnetByFilter(filter *infrav1.SubnetFilter) (*subnets.Subnet, error) {
+	return s.getSubnetByFilter(filter.ToListOpt())
+}
+
+// GetNetworkSubnetByFilter gets a single subnet of the given network, specified by the given SubnetFilter.
+// It returns an ErrFilterMatch if no or multiple subnets are found.
+func (s *Service) GetNetworkSubnetByFilter(networkID string, filter *infrav1.SubnetFilter) (*subnets.Subnet, error) {
+	listOpt := filter.ToListOpt()
+	listOpt.NetworkID = networkID
+
+	return s.getSubnetByFilter(listOpt)
+}
+
+// getSubnetByFilter gets a single subnet specified by the given gophercloud ListOpts.
+// It returns an ErrFilterMatch if no or multiple subnets are found.
+func (s *Service) getSubnetByFilter(listOpts subnets.ListOpts) (*subnets.Subnet, error) {
 	// If the ID is set, we can just get the subnet by ID.
-	if filter.ID != "" {
-		subnet, err := s.client.GetSubnet(filter.ID)
+	if listOpts.ID != "" {
+		subnet, err := s.client.GetSubnet(listOpts.ID)
 		if capoerrors.IsNotFound(err) {
 			return nil, ErrNoMatches
 		}
 		return subnet, err
 	}
 
-	subnets, err := s.GetSubnetsByFilter(filter.ToListOpt())
+	subnets, err := s.GetSubnetsByFilter(listOpts)
 	if err != nil {
 		return nil, err
 	}