Adding more unit tests for remove-clusters

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

@@ -72,17 +72,21 @@ func (h *FakeBackendServiceSyncer) RemoveFromClusters(ports []ingressbe.ServiceP
 	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
 		}
 		newIGLinks := []string{}
 		for _, ig := range v.IGLinks {
-			if _, has := removeLinks[ig]; !has {
+			if !removeLinks[ig] {
 				newIGLinks = append(newIGLinks, ig)
+			} else {
+				// Mark the link as removed.
+				// This is to handle duplicate ig links in our tests.
+				removeLinks[ig] = false
 			}
 		}
-		v.IGLinks = newIGLinks
+		h.EnsuredBackendServices[i].IGLinks = newIGLinks
 	}
 	return nil
 }

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.

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

@@ -367,6 +367,10 @@ func TestDeleteLoadBalancer(t *testing.T) {
 	if err != nil {
 		t.Fatalf("unexpected error in test setup: %s", err)
 	}
+	// Verify that trying to delete when no load balancer exists does not return any error.
+	if err := lbc.DeleteLoadBalancer(ing); err != nil {
+		t.Fatalf("unexpected error while deleting load balancer when none exist: %s", err)
+	}
 	if err := lbc.CreateLoadBalancer(ing, true /*forceUpdate*/, true /*validate*/, clusters); err != nil {
 		t.Fatalf("unexpected error %s", err)
 	}
@@ -424,6 +428,7 @@ func TestRemoveFromClusters(t *testing.T) {
 	igName := "my-fake-ig"
 	igZone := "my-fake-zone"
 	zoneLink := fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/fake-project/zones/%s", igZone)
+	igLink := fmt.Sprintf("%s/instanceGroups/%s", zoneLink, igName)
 	clusters := []string{"cluster1", "cluster2"}
 	lbc := newLoadBalancerSyncer(lbName)
 	ipAddress := &compute.Address{
@@ -439,38 +444,30 @@ func TestRemoveFromClusters(t *testing.T) {
 	if err := lbc.CreateLoadBalancer(ing, true /*forceUpdate*/, true /*validate*/, clusters); err != nil {
 		t.Fatalf("unexpected error %s", err)
 	}
-	fhc := lbc.hcs.(*healthcheck.FakeHealthCheckSyncer)
-	if len(fhc.EnsuredHealthChecks) == 0 {
-		t.Errorf("unexpected health checks not created")
-	}
+	// Verify that backend service, firewall rule and status are as expected.
 	fbs := lbc.bss.(*backendservice.FakeBackendServiceSyncer)
-	if len(fbs.EnsuredBackendServices) == 0 {
-		t.Errorf("unexpected backend services not created")
+	if len(fbs.EnsuredBackendServices) != 2 {
+		t.Errorf("unexpected backend services, expected 2 backend services, got: %v", fbs.EnsuredBackendServices)
 	}
-	fum := lbc.ums.(*urlmap.FakeURLMapSyncer)
-	if len(fum.EnsuredURLMaps) == 0 {
-		t.Errorf("unexpected url maps not created")
+	be := fbs.EnsuredBackendServices[0]
+	expectedIGlinks := []string{igLink, igLink}
+	if !reflect.DeepEqual(be.IGLinks, expectedIGlinks) {
+		t.Errorf("unexpected IG links on backend service. expected: %v, got: %v", expectedIGlinks, be.IGLinks)
 	}
-	ftp := lbc.tps.(*targetproxy.FakeTargetProxySyncer)
-	if len(ftp.EnsuredTargetProxies) == 0 {
-		t.Errorf("unexpected target proxies not created")
+	ffw := lbc.fws.(*firewallrule.FakeFirewallRuleSyncer)
+	if len(ffw.EnsuredFirewallRules) != 1 {
+		t.Errorf("unexpected firewall rules, expected 1, got: %v", ffw.EnsuredFirewallRules)
 	}
-	ffr := lbc.frs.(*forwardingrule.FakeForwardingRuleSyncer)
-	if len(ffr.EnsuredForwardingRules) == 0 {
-		t.Errorf("unexpected forwarding rules not created")
+	fw := ffw.EnsuredFirewallRules[0]
+	expectedFWIGLinks := map[string][]string{
+		"cluster1": []string{igLink},
+		"cluster2": []string{igLink},
 	}
-	ffw := lbc.fws.(*firewallrule.FakeFirewallRuleSyncer)
-	if len(ffw.EnsuredFirewallRules) == 0 {
-		t.Errorf("unexpected firewall rules not created")
+	if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) {
+		t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks)
 	}
-	// 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
-	typedFRS := lbc.frs.(*forwardingrule.FakeForwardingRuleSyncer)
-	typedFRS.AddStatus(lbName, &status.LoadBalancerStatus{Clusters: clusters})
-
 	// Verify that the load balancer is spread to both clusters.
-	if err := verifyClusters(lbName, lbc.frs, clusters); err != nil {
+	if err := verifyClusters(lbc, clusters); err != nil {
 		t.Errorf("%s", err)
 	}
 
@@ -481,7 +478,27 @@ func TestRemoveFromClusters(t *testing.T) {
 	if err := lbc.RemoveFromClusters(ing, removeClients, false /*forceUpdate*/); err != nil {
 		t.Errorf("unexpected error in removing existing load balancer from clusters: %s", err)
 	}
-	if err := verifyClusters(lbName, lbc.frs, []string{"cluster2"}); err != nil {
+
+	// Verify that the backend service, firewall rule and status are updated as expected.
+	if len(fbs.EnsuredBackendServices) != 2 {
+		t.Errorf("unexpected backend services, expected 2 backend services, got: %v", fbs.EnsuredBackendServices)
+	}
+	be = fbs.EnsuredBackendServices[0]
+	expectedIGlinks = []string{igLink}
+	if !reflect.DeepEqual(be.IGLinks, expectedIGlinks) {
+		t.Errorf("unexpected IG links on backend service. expected: %v, got: %v", expectedIGlinks, be.IGLinks)
+	}
+	if len(ffw.EnsuredFirewallRules) != 1 {
+		t.Errorf("unexpected firewall rules, expected 1, got: %v", ffw.EnsuredFirewallRules)
+	}
+	fw = ffw.EnsuredFirewallRules[0]
+	expectedFWIGLinks = map[string][]string{
+		"cluster2": []string{igLink},
+	}
+	if !reflect.DeepEqual(fw.IGLinks, expectedFWIGLinks) {
+		t.Errorf("unexpected IG links on firewall rule, expected: %v, got: %v", expectedFWIGLinks, fw.IGLinks)
+	}
+	if err := verifyClusters(lbc, []string{"cluster2"}); err != nil {
 		t.Errorf("%s", err)
 	}
 
@@ -516,14 +533,15 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 	}{
 		{
 			// Default case.
-			true,
 			false,
+			true,
 			false,
 		},
 		{
-			false,
+			// Old setup - forwarding rule has the status, url map does not.
 			true,
-			true, // TODO: Update this to false when GetLoadBalancerStatus can read from url map.
+			false,
+			false,
 		},
 		{
 			true,
@@ -541,40 +559,35 @@ func TestRemoveClustersFromStatus(t *testing.T) {
 			t.Fatalf("unexpected error %s", err)
 		}
 		if c.frHasStatus {
+			// Explicitly add status, since forwarding rule will not have status by default.
 			lbc.frs.(*forwardingrule.FakeForwardingRuleSyncer).AddStatus(lbName, &status.LoadBalancerStatus{Clusters: clusters})
 		}
 		if !c.umHasStatus {
+			// Explicitly remove status, since url map will have status by default.
 			lbc.ums.(*urlmap.FakeURLMapSyncer).RemoveStatus(lbName)
 		}
 		// Verify that the load balancer is spread to both clusters.
-		// TODO: Remove this if once we update GetLoadBalancerStatus to read from urlmap as well.
-		if c.frHasStatus {
-
-			if err := verifyClusters(lbName, lbc.frs, clusters); c.errorInGetStatus != (err != nil) {
-				t.Errorf("expected error in verifying status: %v, got err: %s", c.errorInGetStatus, err)
-			}
+		if err := verifyClusters(lbc, clusters); c.errorInGetStatus != (err != nil) {
+			t.Errorf("expected error in verifying status: %v, got err: %s", c.errorInGetStatus, err)
 		}
 
 		// Remove the load balancer from the first cluster and verify that the load balancer status is updated appropriately.
 		clustersToRemove := []string{"cluster1"}
 		if err := lbc.removeClustersFromStatus(lbName, clustersToRemove); err != nil {
 			t.Errorf("unexpected error in removing existing load balancer from clusters: %s", err)
 		}
-		// TODO: Remove this if once we update GetLoadBalancerStatus to read from urlmap as well.
-		if c.frHasStatus {
-			if err := verifyClusters(lbName, lbc.frs, []string{"cluster2"}); c.errorInGetStatus != (err != nil) {
+		if err := verifyClusters(lbc, []string{"cluster2"}); c.errorInGetStatus != (err != nil) {
 
-				t.Errorf("expected error in verifying status: %v, got err: %s", c.errorInGetStatus, err)
-			}
+			t.Errorf("expected error in verifying status: %v, got err: %s", c.errorInGetStatus, err)
 		}
 		if err := lbc.DeleteLoadBalancer(ing); err != nil {
 			t.Fatalf("unexpected error %s", err)
 		}
 	}
 }
 
-func verifyClusters(lbName string, frs forwardingrule.ForwardingRuleSyncerInterface, expectedClusters []string) error {
-	status, err := frs.GetLoadBalancerStatus(lbName)
+func verifyClusters(lbc *LoadBalancerSyncer, expectedClusters []string) error {
+	status, err := lbc.getLoadBalancerStatus()
 	if status == nil || err != nil {
 		return fmt.Errorf("unexpected error in getting load balancer status: %s", err)
 	}