fix: Ensure IsoNet is created for new Cloudstack Cluster

internal/controllers/cloudstackfailuredomain_controller.go

@@ -197,9 +197,13 @@ func (r *CloudStackFailureDomainReconciler) reconcileNormal(ctx context.Context,
 	if err := scope.ResolveZone(); err != nil {
 		return ctrl.Result{}, errors.Wrap(err, "resolving CloudStack zone information")
 	}
-	if err := scope.ResolveNetwork(); err != nil &&
-		!strings.Contains(strings.ToLower(err.Error()), "no match") {
-		return ctrl.Result{}, errors.Wrap(err, "resolving Cloudstack network information")
+	if err := scope.ResolveNetwork(); err != nil {
+		// If the network ID was pre-populated and resolution still failed, that's a real error.
+		// If no ID was set, the network simply doesn't exist yet and we'll create an isolated one.
+		if scope.NetworkID() != "" {
+			return ctrl.Result{}, errors.Wrap(err, "resolving Cloudstack network information")
+		}
+		scope.Info("Network not found in CloudStack, will create isolated network", "error", err.Error())
 	}
 
 	if scope.NetworkID() == "" || scope.NetworkType() == infrav1.NetworkTypeIsolated {

internal/controllers/cloudstackfailuredomain_controller_test.go

@@ -19,8 +19,11 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
+	pkgerrors "github.com/pkg/errors"
+
 	"github.com/go-logr/logr"
 	. "github.com/onsi/gomega"
 	"go.uber.org/mock/gomock"
@@ -227,4 +230,132 @@ func TestCloudStackFailureDomainReconcilerIntegrationTests(t *testing.T) {
 			return false
 		}, timeout).Should(BeTrue())
 	})
+
+	t.Run("Should create CloudStackIsolatedNetwork when network is not found", func(t *testing.T) {
+		g := NewWithT(t)
+
+		setup(t)
+		defer teardown()
+
+		expectClient := func(m *mocks.MockClientMockRecorder) {
+			m.ResolveZone(gomock.Any()).MinTimes(1)
+			// Simulate a network that doesn't exist yet in CloudStack:
+			// return an error WITHOUT modifying Network.ID or Network.Type.
+			m.ResolveNetworkForZone(gomock.Any()).Return(
+				pkgerrors.New("No match found for test-network"),
+			).MinTimes(1)
+		}
+		expectClient(mockCSClient.EXPECT())
+
+		ns, err := testEnv.CreateNamespace(ctx, fmt.Sprintf("integ-test-%s", util.RandomString(5)))
+		g.Expect(err).ToNot(HaveOccurred())
+		dummies.SetDummyVars(ns.Name)
+
+		// Ensure the failure domain has an empty network ID and type
+		// to simulate a new isolated network that doesn't exist yet.
+		dummies.CSFailureDomain1.Spec.Zone.Network.ID = ""
+		dummies.CSFailureDomain1.Spec.Zone.Network.Type = ""
+
+		// Create test objects
+		g.Expect(testEnv.Create(ctx, dummies.CAPICluster)).To(Succeed())
+		dummies.CSCluster.OwnerReferences = append(dummies.CSCluster.OwnerReferences, metav1.OwnerReference{
+			Kind:       "Cluster",
+			APIVersion: clusterv1.GroupVersion.String(),
+			Name:       dummies.CAPICluster.Name,
+			UID:        types.UID("cluster-uid"),
+		})
+		g.Expect(testEnv.Create(ctx, dummies.CSCluster)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, dummies.ACSEndpointSecret1)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, dummies.CSFailureDomain1)).To(Succeed())
+
+		defer func() {
+			g.Expect(testEnv.Cleanup(dummies.CAPICluster, dummies.CSCluster, dummies.ACSEndpointSecret1, dummies.CSFailureDomain1, ns)).To(Succeed())
+		}()
+
+		// Wait for the failure domain to be available.
+		fdKey := client.ObjectKey{Namespace: ns.Name, Name: dummies.CSFailureDomain1.Name}
+		fd := &infrav1.CloudStackFailureDomain{}
+		g.Eventually(func() bool {
+			return testEnv.Get(ctx, fdKey, fd) == nil
+		}, timeout).Should(BeTrue())
+
+		result, err := reconciler.Reconcile(ctx, ctrl.Request{
+			NamespacedName: types.NamespacedName{
+				Namespace: ns.Name,
+				Name:      dummies.CSFailureDomain1.Name,
+			},
+		})
+		g.Expect(err).ToNot(HaveOccurred())
+		// Should requeue because the IsolatedNetwork won't be ready yet.
+		g.Expect(result.RequeueAfter).ToNot(BeZero())
+
+		// Verify that a CloudStackIsolatedNetwork was created.
+		netName := dummies.CAPICluster.Name + "-" + strings.ToLower(dummies.CSFailureDomain1.Spec.Zone.Network.Name)
+		isoNetKey := client.ObjectKey{Namespace: ns.Name, Name: netName}
+		isoNet := &infrav1.CloudStackIsolatedNetwork{}
+		g.Eventually(func() bool {
+			return testEnv.Get(ctx, isoNetKey, isoNet) == nil
+		}, timeout).Should(BeTrue(), "CloudStackIsolatedNetwork should have been created")
+
+		// Verify the IsolatedNetwork has the correct owner reference and spec.
+		g.Expect(isoNet.Spec.FailureDomainName).To(Equal(dummies.CSFailureDomain1.Spec.Name))
+		g.Expect(isoNet.Labels[clusterv1.ClusterNameLabel]).To(Equal(dummies.CAPICluster.Name))
+	})
+
+	t.Run("Should skip isolated network creation for shared network", func(t *testing.T) {
+		g := NewWithT(t)
+
+		setup(t)
+		defer teardown()
+
+		expectClient := func(m *mocks.MockClientMockRecorder) {
+			m.ResolveZone(gomock.Any()).MinTimes(1)
+			m.ResolveNetworkForZone(gomock.Any()).AnyTimes().Do(
+				func(arg1 interface{}) {
+					arg1.(*infrav1.CloudStackZoneSpec).Network.ID = "SomeSharedNetID"
+					arg1.(*infrav1.CloudStackZoneSpec).Network.Type = cloud.NetworkTypeShared
+				}).MinTimes(1)
+		}
+		expectClient(mockCSClient.EXPECT())
+
+		ns, err := testEnv.CreateNamespace(ctx, fmt.Sprintf("integ-test-%s", util.RandomString(5)))
+		g.Expect(err).ToNot(HaveOccurred())
+		dummies.SetDummyVars(ns.Name)
+
+		// Create test objects
+		g.Expect(testEnv.Create(ctx, dummies.CAPICluster)).To(Succeed())
+		dummies.CSCluster.OwnerReferences = append(dummies.CSCluster.OwnerReferences, metav1.OwnerReference{
+			Kind:       "Cluster",
+			APIVersion: clusterv1.GroupVersion.String(),
+			Name:       dummies.CAPICluster.Name,
+			UID:        types.UID("cluster-uid"),
+		})
+		g.Expect(testEnv.Create(ctx, dummies.CSCluster)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, dummies.ACSEndpointSecret1)).To(Succeed())
+		g.Expect(testEnv.Create(ctx, dummies.CSFailureDomain1)).To(Succeed())
+
+		defer func() {
+			g.Expect(testEnv.Cleanup(dummies.CAPICluster, dummies.CSCluster, dummies.ACSEndpointSecret1, dummies.CSFailureDomain1, ns)).To(Succeed())
+		}()
+
+		fdKey := client.ObjectKey{Namespace: ns.Name, Name: dummies.CSFailureDomain1.Name}
+		fd := &infrav1.CloudStackFailureDomain{}
+		g.Eventually(func() bool {
+			return testEnv.Get(ctx, fdKey, fd) == nil
+		}, timeout).Should(BeTrue())
+
+		result, err := reconciler.Reconcile(ctx, ctrl.Request{
+			NamespacedName: types.NamespacedName{
+				Namespace: ns.Name,
+				Name:      dummies.CSFailureDomain1.Name,
+			},
+		})
+		g.Expect(err).ToNot(HaveOccurred())
+		g.Expect(result.RequeueAfter).To(BeZero())
+
+		// Verify that NO CloudStackIsolatedNetwork was created.
+		isoNetList := &infrav1.CloudStackIsolatedNetworkList{}
+		g.Expect(testEnv.List(ctx, isoNetList, client.InNamespace(ns.Name))).To(Succeed())
+		g.Expect(isoNetList.Items).To(BeEmpty(), "No CloudStackIsolatedNetwork should be created for shared networks")
+	})
 }

pkg/cloud/network.go

@@ -59,6 +59,12 @@ func (c *client) ResolveNetwork(net *infrav1.Network) (retErr error) {
 		return nil
 	}
 
+	// Only fall back to ID-based lookup if an ID was actually provided.
+	// With an empty ID, the CloudStack API may return an unrelated network.
+	if net.ID == "" {
+		return retErr
+	}
+
 	// Now get network details.
 	netDetails, count, err = c.cs.Network.GetNetworkByID(net.ID, cloudstack.WithProject(c.user.Project.ID))
 	if err != nil {

pkg/cloud/network_test.go

@@ -73,6 +73,18 @@ var _ = Describe("Network", func() {
 			Ω(err).ShouldNot(Succeed())
 			Ω(err.Error()).Should(ContainSubstring(fmt.Sprintf("expected 1 Network with name %s, but got %d", dummies.ISONet1.Name, 2)))
 		})
+
+		It("returns error without ID fallback when name not found and ID is empty", func() {
+			dummies.SetDummyIsoNetToNameOnly()
+			ns.EXPECT().GetNetworkByName(dummies.ISONet1.Name, gomock.Any()).Return(nil, -1, errors.New("No match found"))
+			// GetNetworkByID must NOT be called since the ID is empty.
+
+			err := client.ResolveNetwork(&dummies.ISONet1)
+			Ω(err).ShouldNot(Succeed())
+			Ω(err.Error()).Should(ContainSubstring("could not get Network ID from"))
+			Ω(dummies.ISONet1.ID).Should(BeEmpty())
+			Ω(dummies.ISONet1.Type).Should(BeEmpty())
+		})
 	})
 
 	Context("Remove cluster tag from network", func() {

pkg/cloud/zone.go

@@ -72,6 +72,12 @@ func (c *client) ResolveNetworkForZone(zSpec *infrav1.CloudStackZoneSpec) (retEr
 		return nil
 	}
 
+	// Only fall back to ID-based lookup if an ID was actually provided.
+	// With an empty ID, the CloudStack API may return an unrelated network.
+	if zSpec.Network.ID == "" {
+		return retErr
+	}
+
 	// Now get network details.
 	netDetails, count, err = c.cs.Network.GetNetworkByID(zSpec.Network.ID, cloudstack.WithProject(c.user.Project.ID))
 	if err != nil {

pkg/cloud/zone_test.go

@@ -112,5 +112,17 @@ var _ = Describe("Zone", func() {
 
 			Ω(client.ResolveNetworkForZone(&dummies.CSFailureDomain2.Spec.Zone).Error()).Should(ContainSubstring("could not get Network by ID " + dummies.Zone2.Network.ID))
 		})
+
+		It("returns error without ID fallback when name not found and ID is empty", func() {
+			dummies.SetDummyIsoNetToNameOnly()
+			ns.EXPECT().GetNetworkByName(dummies.Zone1.Network.Name, gomock.Any()).Return(nil, -1, fakeError)
+			// GetNetworkByID must NOT be called since the ID is empty.
+
+			err := client.ResolveNetworkForZone(&dummies.Zone1)
+			Ω(err).ShouldNot(Succeed())
+			Ω(err.Error()).Should(ContainSubstring("could not get Network ID from"))
+			Ω(dummies.Zone1.Network.ID).Should(BeEmpty())
+			Ω(dummies.Zone1.Network.Type).Should(BeEmpty())
+		})
 	})
 })