From de103c56272e2629574a979ba9ecab122f2d6866 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Wed, 26 Jul 2023 13:35:23 +0200 Subject: [PATCH 1/2] Fix race condition causing invalid CS API calls when zone id is not resolved yet Signed-off-by: Hans Rakers --- controllers/cloudstackisolatednetwork_controller.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/cloudstackisolatednetwork_controller.go b/controllers/cloudstackisolatednetwork_controller.go index 1fddffe6..eeb33d36 100644 --- a/controllers/cloudstackisolatednetwork_controller.go +++ b/controllers/cloudstackisolatednetwork_controller.go @@ -75,6 +75,9 @@ func (r *CloudStackIsoNetReconciliationRunner) Reconcile() (retRes ctrl.Result, if err != nil { return r.ReturnWrappedError(retErr, "setting up CloudStackCluster patcher") } + if r.FailureDomain.Spec.Zone.ID == "" { + return r.RequeueWithMessage("Zone ID not resolved yet.") + } if err := r.CSUser.GetOrCreateIsolatedNetwork(r.FailureDomain, r.ReconciliationSubject, r.CSCluster); err != nil { return ctrl.Result{}, err } From b78d279e47a28d196a63b26e226fff86f1bf9f45 Mon Sep 17 00:00:00 2001 From: Hans Rakers Date: Wed, 30 Aug 2023 17:10:30 +0200 Subject: [PATCH 2/2] Add unittest for empty zone ID, fix existing test Signed-off-by: Hans Rakers --- ...oudstackisolatednetwork_controller_test.go | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/controllers/cloudstackisolatednetwork_controller_test.go b/controllers/cloudstackisolatednetwork_controller_test.go index a38866fa..34c61875 100644 --- a/controllers/cloudstackisolatednetwork_controller_test.go +++ b/controllers/cloudstackisolatednetwork_controller_test.go @@ -20,15 +20,18 @@ import ( g "github.com/golang/mock/gomock" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" infrav1 "sigs.k8s.io/cluster-api-provider-cloudstack/api/v1beta3" dummies "sigs.k8s.io/cluster-api-provider-cloudstack/test/dummies/v1beta3" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" ) var _ = Describe("CloudStackIsolatedNetworkReconciler", func() { Context("With k8s like test environment.", func() { BeforeEach(func() { - SetupTestEnvironment() // Must happen before setting up managers/reconcilers. + SetupTestEnvironment() // Must happen before setting up managers/reconcilers. + dummies.SetDummyVars() Ω(IsoNetReconciler.SetupWithManager(k8sManager)).Should(Succeed()) // Register CloudStack IsoNetReconciler. }) @@ -36,15 +39,39 @@ var _ = Describe("CloudStackIsolatedNetworkReconciler", func() { mockCloudClient.EXPECT().GetOrCreateIsolatedNetwork(g.Any(), g.Any(), g.Any()).AnyTimes() mockCloudClient.EXPECT().AddClusterTag(g.Any(), g.Any(), g.Any()).AnyTimes() + // We use CSFailureDomain2 here because CSFailureDomain1 has an empty Spec.Zone.ID + dummies.CSISONet1.Spec.FailureDomainName = dummies.CSFailureDomain2.Spec.Name + Ω(k8sClient.Create(ctx, dummies.CSFailureDomain2)).Should(Succeed()) Ω(k8sClient.Create(ctx, dummies.CSISONet1)).Should(Succeed()) + Eventually(func() bool { tempIsoNet := &infrav1.CloudStackIsolatedNetwork{} key := client.ObjectKeyFromObject(dummies.CSISONet1) if err := k8sClient.Get(ctx, key, tempIsoNet); err == nil { - return true + if tempIsoNet.Status.Ready == true { + return true + } } return false }, timeout).WithPolling(pollInterval).Should(BeTrue()) }) }) + + Context("With a fake ctrlRuntimeClient and no test Env at all.", func() { + BeforeEach(func() { + setupFakeTestClient() + }) + + It("Should requeue in case the zone ID is not resolved yet.", func() { + // We use CSFailureDomain1 here because it has an empty Spec.Zone.ID + dummies.CSISONet1.Spec.FailureDomainName = dummies.CSFailureDomain1.Spec.Name + Ω(fakeCtrlClient.Create(ctx, dummies.CSFailureDomain1)).Should(Succeed()) + Ω(fakeCtrlClient.Create(ctx, dummies.CSISONet1)).Should(Succeed()) + + requestNamespacedName := types.NamespacedName{Namespace: dummies.ClusterNameSpace, Name: dummies.CSISONet1.Name} + res, err := IsoNetReconciler.Reconcile(ctx, ctrl.Request{NamespacedName: requestNamespacedName}) + Ω(err).ShouldNot(HaveOccurred()) + Ω(res.RequeueAfter).ShouldNot(BeZero()) + }) + }) })