diff --git a/internal/controllers/cloudstackfailuredomain_controller.go b/internal/controllers/cloudstackfailuredomain_controller.go index 208ae41d..fdf56cd7 100644 --- a/internal/controllers/cloudstackfailuredomain_controller.go +++ b/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 { diff --git a/internal/controllers/cloudstackfailuredomain_controller_test.go b/internal/controllers/cloudstackfailuredomain_controller_test.go index b0a9c988..cffb7e59 100644 --- a/internal/controllers/cloudstackfailuredomain_controller_test.go +++ b/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") + }) } diff --git a/pkg/cloud/network.go b/pkg/cloud/network.go index de31f121..4807147b 100644 --- a/pkg/cloud/network.go +++ b/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 { diff --git a/pkg/cloud/network_test.go b/pkg/cloud/network_test.go index 3a114bef..2bc18f8a 100644 --- a/pkg/cloud/network_test.go +++ b/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() { diff --git a/pkg/cloud/zone.go b/pkg/cloud/zone.go index f6f2cb5a..3dc04263 100644 --- a/pkg/cloud/zone.go +++ b/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 { diff --git a/pkg/cloud/zone_test.go b/pkg/cloud/zone_test.go index a4464394..92f48a90 100644 --- a/pkg/cloud/zone_test.go +++ b/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()) + }) }) })