From 28b94bc1968c994a40d377b5136e2df4b175ba33 Mon Sep 17 00:00:00 2001
From: Tanmay Satam <tsatam@redhat.com>
Date: Mon, 13 Nov 2023 11:46:41 -0500
Subject: [PATCH] Updates subnet reconcile E2E test in order to work

This test has been dependent on the bad behavior fixed in this PR, and has expected
the subnet controller to reconcile frequently without initiating any requests on its own.

The following changes have been implemented in order to ensure this test works and is less flaky:
- Do not replace the NSG on the master-subnet, as this prevents the ability for the E2E test to be run locally with operator out-of-cluster (apiserver becomes inaccessible)
- Always clean up subnet NSGs after test completion, rather than expecting the test to succeed and to have done so
- Perform an explicit change to the ARO cluster resource in order to ensure the reconcile actually happens
---
 test/e2e/operator.go | 51 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/test/e2e/operator.go b/test/e2e/operator.go
index 299e41d95c8..9e7cfbfb661 100644
--- a/test/e2e/operator.go
+++ b/test/e2e/operator.go
@@ -309,13 +309,10 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		Expect(err).NotTo(HaveOccurred())
 		location = *oc.Location
 
-		vnet, masterSubnet, err := apisubnet.Split(*oc.OpenShiftClusterProperties.MasterProfile.SubnetID)
-		Expect(err).NotTo(HaveOccurred())
-		_, workerSubnet, err := apisubnet.Split((*(*oc.OpenShiftClusterProperties.WorkerProfiles)[0].SubnetID))
+		vnet, workerSubnet, err := apisubnet.Split((*(*oc.OpenShiftClusterProperties.WorkerProfiles)[0].SubnetID))
 		Expect(err).NotTo(HaveOccurred())
 
 		subnetsToReconcile = map[string]*string{
-			masterSubnet: to.StringPtr(""),
 			workerSubnet: to.StringPtr(""),
 		}
 
@@ -323,6 +320,32 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		Expect(err).NotTo(HaveOccurred())
 		resourceGroup = r.ResourceGroup
 		vnetName = r.ResourceName
+
+		// Store the existing NSGs for the cluster before the test runs, in order to ensure we clean up
+		// after the test finishes, success or failure.
+		// This is expensive but will prevent flakes.
+		By("gathering existing subnet NSGs")
+		for subnet := range subnetsToReconcile {
+			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
+			Expect(err).NotTo(HaveOccurred())
+
+			subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID
+		}
+	}
+
+	cleanUpSubnetNSGs := func(ctx context.Context) {
+		By("cleaning up subnet NSGs")
+		for subnet := range subnetsToReconcile {
+			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
+			Expect(err).NotTo(HaveOccurred())
+
+			if subnetObject.NetworkSecurityGroup.ID != subnetsToReconcile[subnet] {
+				subnetObject.NetworkSecurityGroup.ID = subnetsToReconcile[subnet]
+
+				err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject)
+				Expect(err).NotTo(HaveOccurred())
+			}
+		}
 	}
 
 	createE2ENSG := func(ctx context.Context) {
@@ -355,6 +378,8 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 		createE2ENSG(ctx)
 	})
 	AfterEach(func(ctx context.Context) {
+		cleanUpSubnetNSGs(ctx)
+
 		By("deleting test NSG")
 		err := clients.NetworkSecurityGroups.DeleteAndWait(ctx, resourceGroup, nsg)
 		if err != nil {
@@ -367,13 +392,27 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 			// Gets current subnet NSG and then updates it to testnsg.
 			subnetObject, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, "")
 			Expect(err).NotTo(HaveOccurred())
-			// Updates the value to the original NSG in our subnetsToReconcile map
-			subnetsToReconcile[subnet] = subnetObject.NetworkSecurityGroup.ID
+
 			subnetObject.NetworkSecurityGroup = &testnsg
+
 			err = clients.Subnet.CreateOrUpdateAndWait(ctx, resourceGroup, vnetName, subnet, subnetObject)
 			Expect(err).NotTo(HaveOccurred())
 		}
 
+		By("updating the ARO cluster resource to ensure a reconcile")
+		Eventually(func(g Gomega, ctx context.Context) {
+			co, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{})
+			g.Expect(err).NotTo(HaveOccurred())
+
+			if co.ObjectMeta.Annotations == nil {
+				co.ObjectMeta.Annotations = map[string]string{}
+			}
+			co.ObjectMeta.Annotations["aro.openshift.io/test-e2e-force-reconcile"] = time.Now().Format(time.RFC3339)
+
+			_, err = clients.AROClusters.AroV1alpha1().Clusters().Update(ctx, co, metav1.UpdateOptions{})
+			g.Expect(err).NotTo(HaveOccurred())
+		}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed())
+
 		for subnet, correctNSG := range subnetsToReconcile {
 			By(fmt.Sprintf("waiting for the subnet %q to be reconciled so it includes the original cluster NSG", subnet))
 			Eventually(func(g Gomega, ctx context.Context) {