Fix for backend service update

pkg/backends/ig_linker.go

@@ -191,6 +191,5 @@ func getInstanceGroupsToAdd(be *composite.BackendService, igLinks []string) ([]s
 		klog.V(2).Infof("Backend service %q has instance groups %+v, want %+v",
 			be.Name, existingIGs.List(), wantIGs.List())
 	}
-
 	return missingIGs.List(), nil
 }

pkg/backends/ig_linker_test.go

@@ -15,7 +15,10 @@ package backends
 
 import (
 	"context"
+	"fmt"
 	"net/http"
+	"reflect"
+	"sort"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
@@ -165,3 +168,86 @@ func TestBackendsForIG(t *testing.T) {
 		})
 	}
 }
+
+func TestGetIGLinksToAdd(t *testing.T) {
+	url := "https://googleapis.com/v1/compute/projects/my-project/global/backendServices/%s"
+	link := "projects/my-project/global/backendServices/%s"
+	for _, tc := range []struct {
+		name           string
+		igLinks        []string
+		currentIGLinks []string
+		want           []string
+	}{
+		{
+			name:           "empty",
+			igLinks:        []string{},
+			currentIGLinks: []string{},
+			want:           []string{},
+		},
+		{
+			name:           "No IGs to add",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend")},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend")},
+			want:           []string{},
+		},
+		{
+			name:           "same IGs in wrong order",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend")},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
+			want:           []string{},
+		},
+		{
+			name:           "one IG to add",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend3")},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
+			want:           []string{fmt.Sprintf(link, "same-backend3")},
+		},
+		{
+			name:           "IGs in wrong order",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend")},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend3"), fmt.Sprintf(url, "same-backend2")},
+			want:           []string{fmt.Sprintf(link, "same-backend")},
+		},
+		{
+			name:           "different IGs",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend3"), fmt.Sprintf(url, "same-backend4")},
+			want:           []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
+		},
+		{
+			name:           "empty current",
+			igLinks:        []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
+			currentIGLinks: []string{},
+			want:           []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
+		},
+		{
+			name:           "empty IGs and non-empty current",
+			igLinks:        []string{},
+			currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
+			want:           []string{},
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			be := composite.BackendService{}
+			var newBackends []*composite.Backend
+			for _, igLink := range tc.currentIGLinks {
+				b := &composite.Backend{
+					Group: igLink,
+				}
+				newBackends = append(newBackends, b)
+			}
+			be.Backends = newBackends
+			toAdd, err := getInstanceGroupsToAdd(&be, tc.igLinks)
+			if err != nil {
+				t.Fatalf("getInstanceGroupsToAdd(_,_): err:%v ", err)
+			}
+			sort.Slice(toAdd, func(i, j int) bool {
+				return toAdd[i] <= toAdd[j]
+			})
+			if !reflect.DeepEqual(toAdd, tc.want) {
+				t.Fatalf("getInstanceGroupsToAdd(_,_) error. Got:%v, Want:%v", toAdd, tc.want)
+			}
+		})
+	}
+
+}

pkg/backends/regional_ig_linker.go

@@ -18,10 +18,10 @@ 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"
+	"k8s.io/klog"
 )
 
 // RegionalInstanceGroupLinker handles linking backends to InstanceGroups.
@@ -39,40 +39,38 @@ func NewRegionalInstanceGroupLinker(instancePool instances.NodePool, backendPool
 
 // Link performs linking instance groups to regional backend service
 func (linker *RegionalInstanceGroupLinker) Link(sp utils.ServicePort, projectID string, zones []string) error {
-	var igLinks []string
+	klog.V(2).Infof("Link(%v, %q, %v)", sp, projectID, zones)
 
+	var igLinks []string
 	for _, zone := range zones {
 		key := meta.ZonalKey(sp.IGName(), zone)
 		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 {
+		klog.V(3).Infof("No backends to add for %s, skipping update.", sp.BackendName())
 		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
+	klog.V(3).Infof("Update Backend %s, with %d backends.", sp.BackendName(), len(addIGs))
+	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

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"}

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")
 }