From 1085adf25321ad16201743f7a846c832c58e62c0 Mon Sep 17 00:00:00 2001
From: Katarzyna Lach <katarzynalach@google.com>
Date: Mon, 9 May 2022 10:46:31 +0000
Subject: [PATCH] Fix for backend service update

We should update backend service with ig llinks
only when ig links changed.
---
 pkg/backends/regional_ig_linker.go      | 29 +++++++++-----------
 pkg/backends/regional_ig_linker_test.go | 36 +++++++++++++++++++++++++
 pkg/test/gcehooks.go                    |  4 +++
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/pkg/backends/regional_ig_linker.go b/pkg/backends/regional_ig_linker.go
index f04f62aeb2..25e12ae435 100644
--- a/pkg/backends/regional_ig_linker.go
+++ b/pkg/backends/regional_ig_linker.go
@@ -18,7 +18,6 @@ import (
 
 	cloudprovider "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/ingress-gce/pkg/composite"
 	"k8s.io/ingress-gce/pkg/instances"
 	"k8s.io/ingress-gce/pkg/utils"
@@ -46,33 +45,29 @@ func (linker *RegionalInstanceGroupLinker) Link(sp utils.ServicePort, projectID
 		igSelfLink := cloudprovider.SelfLink(meta.VersionGA, projectID, "instanceGroups", key)
 		igLinks = append(igLinks, igSelfLink)
 	}
-
-	addIGs := sets.String{}
-	for _, igLink := range igLinks {
-		path, err := utils.RelativeResourceName(igLink)
-		if err != nil {
-			return fmt.Errorf("failed to parse instance group %s: %w", igLink, err)
-		}
-		addIGs.Insert(path)
+	bs, err := linker.backendPool.Get(sp.BackendName(), meta.VersionGA, meta.Regional)
+	if err != nil {
+		return err
+	}
+	addIGs, err := getInstanceGroupsToAdd(bs, igLinks)
+	if err != nil {
+		return err
 	}
 	if len(addIGs) == 0 {
 		return nil
 	}
-	// TODO(kl52752) Check for existing links and add only new one
+
 	var newBackends []*composite.Backend
-	for _, igLink := range addIGs.List() {
+	for _, igLink := range addIGs {
 		b := &composite.Backend{
 			Group: igLink,
 		}
 		newBackends = append(newBackends, b)
 	}
-	be, err := linker.backendPool.Get(sp.BackendName(), meta.VersionGA, meta.Regional)
-	if err != nil {
-		return err
-	}
-	be.Backends = newBackends
 
-	if err := linker.backendPool.Update(be); err != nil {
+	bs.Backends = newBackends
+
+	if err := linker.backendPool.Update(bs); err != nil {
 		return fmt.Errorf("updating backend service %s for IG failed, err:%w", sp.BackendName(), err)
 	}
 	return nil
diff --git a/pkg/backends/regional_ig_linker_test.go b/pkg/backends/regional_ig_linker_test.go
index 22d6ac22a1..6466b7a313 100644
--- a/pkg/backends/regional_ig_linker_test.go
+++ b/pkg/backends/regional_ig_linker_test.go
@@ -90,6 +90,42 @@ func TestRegionalLink(t *testing.T) {
 	}
 }
 
+func TestRegionalUpdateLink(t *testing.T) {
+	t.Parallel()
+	fakeGCE := gce.NewFakeGCECloud(linkerTestClusterValues())
+	clusterID, _ := fakeGCE.ClusterID.GetID()
+	l4Namer := namer.NewL4Namer("uid1", namer.NewNamer(clusterID, ""))
+	sp := utils.ServicePort{NodePort: 8080, BackendNamer: l4Namer}
+	fakeBackendPool := NewPool(fakeGCE, l4Namer)
+	linker := newTestRegionalIgLinker(fakeGCE, fakeBackendPool, l4Namer)
+	if _, err := linker.instancePool.EnsureInstanceGroupsAndPorts(l4Namer.InstanceGroup(), []int64{sp.NodePort}); err != nil {
+		t.Fatalf("Unexpected error when ensuring IG for ServicePort %+v: %v", sp, err)
+	}
+	createBackendService(t, sp, fakeBackendPool)
+
+	if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err != nil {
+		t.Fatalf("Unexpected error in Link(_). Error: %v", err)
+	}
+	// Add error hook to check that Link function will not call Backend Service Update when no IG was added
+	(fakeGCE.Compute().(*cloud.MockGCE)).MockRegionBackendServices.UpdateHook = test.UpdateRegionBackendServiceWithErrorHookUpdate
+
+	if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err != nil {
+		t.Fatalf("Unexpected error in Link(_). Error: %v", err)
+	}
+	be, err := fakeGCE.GetRegionBackendService(sp.BackendName(), fakeGCE.Region())
+	if err != nil {
+		t.Fatalf("Get Regional Backend Service failed %v", err)
+	}
+	if len(be.Backends) == 0 {
+		t.Fatalf("Expected Backends to be created")
+	}
+	ig, _ := linker.instancePool.Get(sp.IGName(), uscentralzone)
+	expectedLink, _ := utils.RelativeResourceName(ig.SelfLink)
+	if be.Backends[0].Group != expectedLink {
+		t.Fatalf("Expected Backend Group: %s received: %s", expectedLink, be.Backends[0].Group)
+	}
+}
+
 func createBackendService(t *testing.T, sp utils.ServicePort, backendPool *Backends) {
 	t.Helper()
 	namespacedName := types.NamespacedName{Name: "service.Name", Namespace: "service.Namespace"}
diff --git a/pkg/test/gcehooks.go b/pkg/test/gcehooks.go
index cad974aebc..37fb033239 100644
--- a/pkg/test/gcehooks.go
+++ b/pkg/test/gcehooks.go
@@ -78,6 +78,10 @@ func DeleteBackendServicesErrorHook(ctx context.Context, key *meta.Key, m *cloud
 	return true, fmt.Errorf("DeleteBackendServicesErrorHook")
 }
 
+func UpdateRegionBackendServiceWithErrorHookUpdate(context.Context, *meta.Key, *compute.BackendService, *cloud.MockRegionBackendServices) error {
+	return fmt.Errorf("Undefined error")
+}
+
 func DeleteHealthCheckErrorHook(ctx context.Context, key *meta.Key, m *cloud.MockRegionHealthChecks) (bool, error) {
 	return true, fmt.Errorf("DeleteHealthCheckErrorHook")
 }