Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ feat: reconcile pre-existing router #2338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+168 −4
Open

✨ feat: reconcile pre-existing router #2338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05c3f17
feat: reconcile pre-existing router
simonostendorf Dec 18, 2024
e400aad
fix: comments on network reconcile
simonostendorf Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions controllers/openstackcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,10 +690,10 @@ func reconcileNetworkComponents(scope *scope.WithLogger, cluster *clusterv1.Clus
}

// reconcilePreExistingNetworkComponents reconciles the cluster network status when the cluster is
// using pre-existing networks and subnets which are not provisioned by the
// using pre-existing networks, subnets and router which are not provisioned by the
EmilienM marked this conversation as resolved.
Show resolved Hide resolved
// cluster controller.
func reconcilePreExistingNetworkComponents(scope *scope.WithLogger, networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster) error {
scope.Logger().V(4).Info("No need to reconcile network, searching network and subnet instead")
scope.Logger().V(4).Info("No need to reconcile network, searching network, subnet and router instead")

if openStackCluster.Status.Network == nil {
openStackCluster.Status.Network = &infrav1.NetworkStatusWithSubnets{}
Expand Down Expand Up @@ -740,9 +740,37 @@ func reconcilePreExistingNetworkComponents(scope *scope.WithLogger, networkingSe
setClusterNetwork(openStackCluster, network)
}

if openStackCluster.Status.Router == nil {
openStackCluster.Status.Router = &infrav1.Router{}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if we don't do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont remember why I added this. But when I read this now I think it doesnt make sense because it forces the user to provide a router reference otherwise they would get logs that the router couldnt be found over and over again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this block doesn't make sense to me.


if openStackCluster.Spec.Router != nil {
router, err := networkingService.GetRouterByParam(openStackCluster.Spec.Router)
if err != nil {
handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find router: %w", err), false)
return fmt.Errorf("error fetching cluster router: %w", err)
}

scope.Logger().V(4).Info("Found pre-existing router", "id", router.ID, "name", router.Name)

routerIPs := []string{}
for _, ip := range router.GatewayInfo.ExternalFixedIPs {
routerIPs = append(routerIPs, ip.IPAddress)
}

openStackCluster.Status.Router = &infrav1.Router{
Name: router.Name,
ID: router.ID,
Tags: router.Tags,
IPs: routerIPs,
}
}

EmilienM marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

// reconcileProvisionedNetworkComponents reconciles the cluster network status when the cluster is
// using networks, subnets and router provisioned by the cluster controller.
func reconcileProvisionedNetworkComponents(networkingService *networking.Service, openStackCluster *infrav1.OpenStackCluster, clusterResourceName string) error {
err := networkingService.ReconcileNetwork(openStackCluster, clusterResourceName)
if err != nil {
Expand Down
136 changes: 136 additions & 0 deletions controllers/openstackcluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"reflect"
"testing"

"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/layer3/routers"
"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/networks"
"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/subnets"
. "github.com/onsi/ginkgo/v2" //nolint:revive
Expand All @@ -38,6 +39,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

infrav1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/networking"
"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
)

Expand Down Expand Up @@ -431,6 +433,140 @@ var _ = Describe("OpenStackCluster controller", func() {
Expect(err).To(BeNil())
Expect(testCluster.Status.Network.ID).To(Equal(clusterNetworkID))
})

It("reconcile pre-existing network components by id", func() {
const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
const clusterSubnetID = "cad5a91a-36de-4388-823b-b0cc82cadfdc"
const clusterRouterID = "e2407c18-c4e7-4d3d-befa-8eec5d8756f2"

testCluster.SetName("pre-existing-network-components-by-id")
testCluster.Spec = infrav1.OpenStackClusterSpec{
Network: &infrav1.NetworkParam{
ID: ptr.To(clusterNetworkID),
},
Subnets: []infrav1.SubnetParam{
{
ID: ptr.To(clusterSubnetID),
},
},
ManagedSubnets: []infrav1.SubnetSpec{},
Router: &infrav1.RouterParam{
ID: ptr.To(clusterRouterID),
},
}
err := k8sClient.Create(ctx, testCluster)
Expect(err).To(BeNil())
err = k8sClient.Create(ctx, capiCluster)
Expect(err).To(BeNil())

log := GinkgoLogr
clientScope, err := mockScopeFactory.NewClientScopeFromObject(ctx, k8sClient, nil, log, testCluster)
Expect(err).To(BeNil())
scope := scope.NewWithLogger(clientScope, log)

networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()

networkClientRecorder.GetSubnet(clusterSubnetID).Return(&subnets.Subnet{
ID: clusterSubnetID,
CIDR: "192.168.0.0/24",
NetworkID: clusterNetworkID,
}, nil)

networkClientRecorder.GetNetwork(clusterNetworkID).Return(&networks.Network{
ID: clusterNetworkID,
}, nil)

networkClientRecorder.GetRouter(clusterRouterID).Return(&routers.Router{
ID: clusterRouterID,
}, nil)

networkingService, err := networking.NewService(scope)
Expect(err).To(BeNil())

err = reconcilePreExistingNetworkComponents(scope, networkingService, testCluster)
Expect(err).To(BeNil())
Expect(testCluster.Status.Network.ID).To(Equal(clusterNetworkID))
Expect(testCluster.Status.Network.Subnets[0].ID).To(Equal(clusterSubnetID))
Expect(testCluster.Status.Router.ID).To(Equal(clusterRouterID))
})

It("reconcile pre-existing network components by name", func() {
const clusterNetworkID = "6c90b532-7ba0-418a-a276-5ae55060b5b0"
const clusterNetworkName = "capo"
const clusterSubnetID = "cad5a91a-36de-4388-823b-b0cc82cadfdc"
const clusterSubnetName = "capo"
const clusterRouterID = "e2407c18-c4e7-4d3d-befa-8eec5d8756f2"
const clusterRouterName = "capo"

testCluster.SetName("pre-existing-network-components-by-id")
testCluster.Spec = infrav1.OpenStackClusterSpec{
Network: &infrav1.NetworkParam{
Filter: &infrav1.NetworkFilter{
Name: clusterNetworkName,
},
},
Subnets: []infrav1.SubnetParam{
{
Filter: &infrav1.SubnetFilter{
Name: clusterSubnetName,
},
},
},
ManagedSubnets: []infrav1.SubnetSpec{},
Router: &infrav1.RouterParam{
Filter: &infrav1.RouterFilter{
Name: clusterRouterName,
},
},
}
err := k8sClient.Create(ctx, testCluster)
Expect(err).To(BeNil())
err = k8sClient.Create(ctx, capiCluster)
Expect(err).To(BeNil())

log := GinkgoLogr
clientScope, err := mockScopeFactory.NewClientScopeFromObject(ctx, k8sClient, nil, log, testCluster)
Expect(err).To(BeNil())
scope := scope.NewWithLogger(clientScope, log)

networkClientRecorder := mockScopeFactory.NetworkClient.EXPECT()

networkClientRecorder.ListNetwork(networks.ListOpts{
Name: clusterNetworkName,
}).Return([]networks.Network{
{
ID: clusterNetworkID,
},
}, nil)

networkClientRecorder.ListSubnet(subnets.ListOpts{
Name: clusterSubnetName,
NetworkID: clusterNetworkID,
}).Return([]subnets.Subnet{
{
ID: clusterSubnetID,
CIDR: "192.168.0.0/24",
NetworkID: clusterNetworkID,
},
}, nil)

networkClientRecorder.ListRouter(routers.ListOpts{
Name: clusterRouterName,
}).Return([]routers.Router{
{
ID: clusterRouterID,
},
}, nil)

networkingService, err := networking.NewService(scope)
Expect(err).To(BeNil())

err = reconcilePreExistingNetworkComponents(scope, networkingService, testCluster)
Expect(err).To(BeNil())
Expect(testCluster.Status.Network.ID).To(Equal(clusterNetworkID))
Expect(testCluster.Status.Network.Subnets[0].ID).To(Equal(clusterSubnetID))
Expect(testCluster.Status.Router.ID).To(Equal(clusterRouterID))
})
})

func createRequestFromOSCluster(openStackCluster *infrav1.OpenStackCluster) reconcile.Request {
Expand Down
4 changes: 2 additions & 2 deletions pkg/cloud/services/networking/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,10 +157,10 @@ func (s *Service) getExternallyManagedRouter(openStackCluster *infrav1.OpenStack
if openStackCluster.Status.Router != nil {
return s.client.GetRouter(openStackCluster.Status.Router.ID)
}
return s.getRouterByParam(openStackCluster.Spec.Router)
return s.GetRouterByParam(openStackCluster.Spec.Router)
}

func (s *Service) getRouterByParam(routerParam *infrav1.RouterParam) (*routers.Router, error) {
func (s *Service) GetRouterByParam(routerParam *infrav1.RouterParam) (*routers.Router, error) {
if routerParam.ID != nil {
return s.client.GetRouter(*routerParam.ID)
}
Expand Down