diff --git a/pkg/backends/ig_linker.go b/pkg/backends/ig_linker.go index 54244ae518..cd7b527c29 100644 --- a/pkg/backends/ig_linker.go +++ b/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 } diff --git a/pkg/backends/ig_linker_test.go b/pkg/backends/ig_linker_test.go index 73c4504d98..d5119a49b1 100644 --- a/pkg/backends/ig_linker_test.go +++ b/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) + } + }) + } + +} diff --git a/pkg/backends/regional_ig_linker.go b/pkg/backends/regional_ig_linker.go index f04f62aeb2..8fb23e0741 100644 --- a/pkg/backends/regional_ig_linker.go +++ b/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 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") }