Adding more unit tests for remove-clusters

app/kubemci/pkg/gcp/backendservice/fake_backendservicesyncer.go

@@ -68,21 +68,33 @@ func (h *FakeBackendServiceSyncer) RemoveFromClusters(ports []ingressbe.ServiceP
 	for _, v := range ports {
 		affectedPorts[v.NodePort] = true
 	}
-	removeLinks := make(map[string]bool, len(removeIGLinks))
-	for _, v := range removeIGLinks {
-		removeLinks[v] = true
-	}
-	for _, v := range h.EnsuredBackendServices {
+	for i, v := range h.EnsuredBackendServices {
 		if _, has := affectedPorts[v.Port.NodePort]; !has {
 			continue
 		}
+		// We remove the given instance group links from each backend service.
+		// For a given backend service, we remove an instance group link only once.
+		// This is because we use duplicate ig links in our tests.
+		removeLinksEachBe := sliceToMap(removeIGLinks)
 		newIGLinks := []string{}
 		for _, ig := range v.IGLinks {
-			if _, has := removeLinks[ig]; !has {
+			if !removeLinksEachBe[ig] {
 				newIGLinks = append(newIGLinks, ig)
+			} else {
+				// Mark the link as removed.
+				// This is to handle duplicate ig links in our tests.
+				removeLinksEachBe[ig] = false
 			}
 		}
-		v.IGLinks = newIGLinks
+		h.EnsuredBackendServices[i].IGLinks = newIGLinks
 	}
 	return nil
 }
+
+func sliceToMap(slice []string) map[string]bool {
+	desiredMap := make(map[string]bool, len(slice))
+	for _, v := range slice {
+		desiredMap[v] = true
+	}
+	return desiredMap
+}

app/kubemci/pkg/gcp/firewallrule/fake_firewallrulesyncer.go

@@ -52,7 +52,7 @@ func (h *FakeFirewallRuleSyncer) DeleteFirewallRules() error {
 }
 
 func (h *FakeFirewallRuleSyncer) RemoveFromClusters(lbName string, removeIGLinks map[string][]string) error {
-	for _, v := range h.EnsuredFirewallRules {
+	for i, v := range h.EnsuredFirewallRules {
 		if v.LBName != lbName {
 			continue
 		}
@@ -62,7 +62,7 @@ func (h *FakeFirewallRuleSyncer) RemoveFromClusters(lbName string, removeIGLinks
 				newIGLinks[clusterName] = igValues
 			}
 		}
-		v.IGLinks = newIGLinks
+		h.EnsuredFirewallRules[i].IGLinks = newIGLinks
 	}
 	return nil
 }

app/kubemci/pkg/gcp/forwardingrule/fake_forwardingrulesyncer.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 
 	"github.com/GoogleCloudPlatform/k8s-multicluster-ingress/app/kubemci/pkg/gcp/status"
+	"github.com/GoogleCloudPlatform/k8s-multicluster-ingress/app/kubemci/pkg/goutils"
 )
 
 type FakeForwardingRule struct {
@@ -88,17 +89,14 @@ func (f *FakeForwardingRuleSyncer) ListLoadBalancerStatuses() ([]status.LoadBala
 }
 
 func (f *FakeForwardingRuleSyncer) RemoveClustersFromStatus(clusters []string) error {
-	removeClusters := make(map[string]bool, len(clusters))
-	for _, c := range clusters {
-		removeClusters[c] = true
-	}
+	clustersToRemove := goutils.MapFromSlice(clusters)
 	for i, fr := range f.EnsuredForwardingRules {
 		if fr.Status == nil {
 			continue
 		}
 		newClusters := []string{}
 		for _, c := range fr.Status.Clusters {
-			if _, has := removeClusters[c]; !has {
+			if _, has := clustersToRemove[c]; !has {
 				newClusters = append(newClusters, c)
 			}
 		}

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

@@ -569,8 +569,8 @@ func addStatus(frs *ForwardingRuleSyncer, lbName, name, ipAddr string, clusters
 	return nil
 }
 
-// Tests that the Load Balancer status contains the expected data (mci metadata).
-func TestRemoveClustersFromStatus(t *testing.T) {
+// Tests RemoveClustersFromStatus assuming that the forwarding rule has the status.
+func TestRemoveClustersFromStatusWithStatus(t *testing.T) {
 	lbName := "lb-name"
 	ipAddr := "1.2.3.4"
 	tpLink := "fakeLink"
@@ -618,8 +618,6 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 			}
 			name := typedFRS.namer.HttpForwardingRuleName()
 			// Add status to the forwarding rule to simulate old forwarding rule which has status.
-			// TODO: This should not be required once lbc.RemoveFromClusters can update url map status:
-			// https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/pull/151
 			if err := addStatus(typedFRS, lbName, name, ipAddr, clusters); err != nil {
 				t.Errorf("%s", err)
 			}
@@ -662,6 +660,78 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 	}
 }
 
+// Tests RemoveClustersFromStatus when forwarding rule may or may not have the status.
+func TestRemoveClustersFromStatus(t *testing.T) {
+	lbName := "lb-name"
+	ipAddr := "1.2.3.4"
+	tpLink := "fakeLink"
+	clusters := []string{"cluster1", "cluster2"}
+	// Should create the forwarding rule as expected.
+	frp := ingresslb.NewFakeLoadBalancers("" /*name*/, nil /*namer*/)
+	namer := utilsnamer.NewNamer("mci1", lbName)
+	frs := NewForwardingRuleSyncer(namer, frp)
+	testCases := []struct {
+		ruleExists bool
+		hasStatus  bool
+		// is RemoveClustersFromStatus expected to return an error.
+		shouldErr bool
+	}{
+		{
+			ruleExists: false,
+			hasStatus:  false,
+			// Should return a NotFound error.
+			shouldErr: true,
+		},
+		{
+			ruleExists: true,
+			hasStatus:  false,
+			shouldErr:  false,
+		},
+		{
+			ruleExists: true,
+			hasStatus:  true,
+			shouldErr:  false,
+		},
+	}
+	for i, c := range testCases {
+		glog.Infof("===============testing case: %d=================", i)
+		typedFRS := frs.(*ForwardingRuleSyncer)
+
+		if c.ruleExists {
+			// Ensure http forwarding rule.
+			if err := frs.EnsureHttpForwardingRule(lbName, ipAddr, tpLink, false /*force*/); err != nil {
+				t.Errorf("expected no error in ensuring http forwarding rule, actual: %v", err)
+			}
+		}
+		if c.hasStatus {
+			name := typedFRS.namer.HttpForwardingRuleName()
+			// Add status to the forwarding rule to simulate old forwarding rule which has status.
+			if err := addStatus(typedFRS, lbName, name, ipAddr, clusters); err != nil {
+				t.Errorf("%s", err)
+			}
+			if err := verifyClusters(lbName, frs, c.shouldErr, clusters); err != nil {
+				t.Errorf("%s", err)
+			}
+		}
+		// Update status to remove one cluster.
+		err := frs.RemoveClustersFromStatus([]string{"cluster1"})
+		if c.shouldErr != (err != nil) {
+			t.Errorf("unexpected error in updating status to remove clusters, expected err != nil: %v, actual err != nil: %v, err: %s", c.shouldErr, err != nil, err)
+		}
+		if c.hasStatus {
+			// Verify that status description has only one cluster now.
+			if err := verifyClusters(lbName, frs, c.shouldErr, []string{"cluster2"}); err != nil {
+				t.Errorf("%s", err)
+			}
+		}
+
+		// Cleanup
+		if err := frs.DeleteForwardingRules(); err != nil {
+			t.Errorf("unexpeted error in deleting forwarding rules: %s", err)
+		}
+	}
+}
+
 // verifyClusters verifies that the given load balancer has the expected clusters in status.
 // Returns error otherwise.
 func verifyClusters(lbName string, frs ForwardingRuleSyncerInterface, shouldErr bool, expectedClusters []string) error {

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

@@ -345,27 +345,36 @@ func (l *LoadBalancerSyncer) removeClustersFromStatus(lbName string, clustersToR
 
 // PrintStatus prints the current status of the load balancer.
 func (l *LoadBalancerSyncer) PrintStatus() (string, error) {
+	status, err := l.getLoadBalancerStatus()
+	if err != nil {
+		return "", err
+	}
+	return formatLoadBalancerStatus(l.lbName, *status), nil
+}
+
+// getLoadBalancerStatus returns the current status of the load balancer.
+func (l *LoadBalancerSyncer) getLoadBalancerStatus() (*status.LoadBalancerStatus, error) {
 	// First try to fetch the status from url map.
 	// If that fails, then we fetch it from forwarding rule.
 	// This is because we first used to store the status on forwarding rules and then migrated to url maps.
 	// https://github.com/GoogleCloudPlatform/k8s-multicluster-ingress/issues/145 has more details.
-	sd, umErr := l.ums.GetLoadBalancerStatus(l.lbName)
-	if umErr == nil {
-		return formatLoadBalancerStatus(l.lbName, *sd), nil
+	umStatus, umErr := l.ums.GetLoadBalancerStatus(l.lbName)
+	if umStatus != nil && umErr == nil {
+		return umStatus, nil
 	}
 	if ingressutils.IsHTTPErrorCode(umErr, http.StatusNotFound) {
-		return "", fmt.Errorf("Load balancer %s does not exist", l.lbName)
+		return nil, fmt.Errorf("Load balancer %s does not exist", l.lbName)
 	}
 	// Try forwarding rule.
-	sd, frErr := l.frs.GetLoadBalancerStatus(l.lbName)
-	if frErr == nil {
-		return formatLoadBalancerStatus(l.lbName, *sd), nil
+	frStatus, frErr := l.frs.GetLoadBalancerStatus(l.lbName)
+	if frStatus != nil && frErr == nil {
+		return frStatus, nil
 	}
 	if ingressutils.IsHTTPErrorCode(frErr, http.StatusNotFound) {
-		return "", fmt.Errorf("Load balancer %s does not exist", l.lbName)
+		return nil, fmt.Errorf("Load balancer %s does not exist", l.lbName)
 	}
 	// Failed to get status from both url map and forwarding rule.
-	return "", fmt.Errorf("failed to get status from both url map and forwarding rule. Error in extracting status from url map: %s. Error in extracting status from forwarding rule: %s", umErr, frErr)
+	return nil, fmt.Errorf("failed to get status from both url map and forwarding rule. Error in extracting status from url map: %s. Error in extracting status from forwarding rule: %s", umErr, frErr)
 }
 
 // formatLoadBalancerStatus formats the given status to be printed for get-status output.