Change return type of AggregatedListNetworkEndpointGroup in cloudprovideradapter.

pkg/neg/manager.go

@@ -22,7 +22,7 @@ import (
 	"sync"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
-	v1 "k8s.io/api/core/v1"
+	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -255,17 +255,23 @@ func (manager *syncerManager) garbageCollectSyncer() {
 func (manager *syncerManager) garbageCollectNEG() error {
 	// Retrieve aggregated NEG list from cloud
 	// Compare against svcPortMap and Remove unintended NEGs by best effort
-	zoneNEGList, err := manager.cloud.AggregatedListNetworkEndpointGroup(meta.VersionGA)
+	negList, err := manager.cloud.AggregatedListNetworkEndpointGroup(meta.VersionGA)
 	if err != nil {
 		return fmt.Errorf("failed to retrieve aggregated NEG list: %v", err)
 	}
 
-	negNames := sets.String{}
-	for _, list := range zoneNEGList {
-		for _, neg := range list {
-			if manager.namer.IsNEG(neg.Name) {
-				negNames.Insert(neg.Name)
+	deleteCandidates := map[string][]string{}
+	for key, neg := range negList {
+		if key.Type() != meta.Zonal {
+			// covers the case when key.Zone is not populated
+			klog.V(4).Infof("Ignoring key %v as it is not zonal", key)
+			continue
+		}
+		if manager.namer.IsNEG(neg.Name) {
+			if _, ok := deleteCandidates[neg.Name]; !ok {
+				deleteCandidates[neg.Name] = []string{}
 			}
+			deleteCandidates[neg.Name] = append(deleteCandidates[neg.Name], key.Zone)
 		}
 	}
 
@@ -274,7 +280,7 @@ func (manager *syncerManager) garbageCollectNEG() error {
 		defer manager.mu.Unlock()
 		for _, portInfoMap := range manager.svcPortMap {
 			for _, portInfo := range portInfoMap {
-				negNames.Delete(portInfo.NegName)
+				delete(deleteCandidates, portInfo.NegName)
 			}
 		}
 	}()
@@ -283,8 +289,8 @@ func (manager *syncerManager) garbageCollectNEG() error {
 	// The worst outcome of the race condition is that neg is deleted in the end but user actually specifies a neg.
 	// This would be resolved (sync neg) when the next endpoint update or resync arrives.
 	// TODO: avoid race condition here
-	for zone := range zoneNEGList {
-		for _, name := range negNames.List() {
+	for name, zones := range deleteCandidates {
+		for _, zone := range zones {
 			if err := manager.ensureDeleteNetworkEndpointGroup(name, zone); err != nil {
 				return fmt.Errorf("failed to delete NEG %q in %q: %v", name, zone, err)
 			}

pkg/neg/manager_test.go

@@ -356,15 +356,21 @@ func TestGarbageCollectionNEG(t *testing.T) {
 			Name:                negName,
 			NetworkEndpointType: string(networkEndpointType),
 		}, negtypes.TestZone1)
+		manager.cloud.CreateNetworkEndpointGroup(&composite.NetworkEndpointGroup{
+			Version:             meta.VersionGA,
+			Name:                negName,
+			NetworkEndpointType: string(networkEndpointType),
+		}, negtypes.TestZone2)
 
 		if err := manager.GC(); err != nil {
 			t.Fatalf("Failed to GC: %v", err)
 		}
-
-		negs, _ := manager.cloud.ListNetworkEndpointGroup(negtypes.TestZone1, meta.VersionGA)
-		for _, neg := range negs {
-			if neg.Name == negName {
-				t.Errorf("Expect NEG %q to be GCed.", negName)
+		for _, zone := range []string{negtypes.TestZone1, negtypes.TestZone2} {
+			negs, _ := manager.cloud.ListNetworkEndpointGroup(zone, meta.VersionGA)
+			for _, neg := range negs {
+				if neg.Name == negName {
+					t.Errorf("Expect NEG %q in zone %q to be GCed.", negName, zone)
+				}
 			}
 		}
 	}

pkg/neg/syncers/transaction_test.go

@@ -151,19 +151,21 @@ func TestTransactionSyncNetworkEndpoints(t *testing.T) {
 	// Verify the NEGs are created as expected
 	ret, _ := transactionSyncer.cloud.AggregatedListNetworkEndpointGroup(meta.VersionGA)
 	expectZones := []string{testZone1, testZone2}
+	retZones := sets.NewString()
+
+	for key, _ := range ret {
+		retZones.Insert(key.Zone)
+	}
 	for _, zone := range expectZones {
-		negs, ok := ret[zone]
+		_, ok := retZones[zone]
 		if !ok {
 			t.Errorf("Failed to find zone %q from ret %v", zone, ret)
 			continue
 		}
-
-		if len(negs) != 1 {
-			t.Errorf("Unexpected negs %v", negs)
-		} else {
-			if negs[0].Name != testNegName {
-				t.Errorf("Unexpected neg %q", negs[0].Name)
-			}
+	}
+	for _, neg := range ret {
+		if neg.Name != testNegName {
+			t.Errorf("Unexpected neg %q", neg.Name)
 		}
 	}
 

pkg/neg/types/cloudprovideradapter.go

@@ -19,7 +19,6 @@ package types
 import (
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
 	"k8s.io/ingress-gce/pkg/composite"
-	"k8s.io/klog"
 	"k8s.io/legacy-cloud-providers/gce"
 )
 
@@ -59,30 +58,9 @@ func (a *cloudProviderAdapter) ListNetworkEndpointGroup(zone string, version met
 }
 
 // AggregatedListNetworkEndpointGroup returns a map of zone -> endpoint group.
-func (a *cloudProviderAdapter) AggregatedListNetworkEndpointGroup(version meta.Version) (map[string][]*composite.NetworkEndpointGroup, error) {
+func (a *cloudProviderAdapter) AggregatedListNetworkEndpointGroup(version meta.Version) (map[*meta.Key]*composite.NetworkEndpointGroup, error) {
 	// TODO: filter for the region the cluster is in.
-	all, err := composite.AggregatedListNetworkEndpointGroup(a.c, version)
-	if err != nil {
-		return nil, err
-	}
-	ret := map[string][]*composite.NetworkEndpointGroup{}
-	for key, obj := range all {
-		// key is scope
-		// zonal key is "zones/<zone name>"
-		// regional key is "regions/<region name>"
-		// global key is "global"
-		// TODO: use cloud provider meta.KeyType and scope name as key
-		if key.Type() == meta.Global {
-			klog.V(4).Infof("Ignoring key %v as it is global", key)
-			continue
-		}
-		if key.Zone == "" {
-			klog.Warningf("Key %v does not have zone populated, ignoring", key)
-			continue
-		}
-		ret[key.Zone] = append(ret[key.Zone], obj)
-	}
-	return ret, nil
+	return composite.AggregatedListNetworkEndpointGroup(a.c, version)
 }
 
 // CreateNetworkEndpointGroup implements NetworkEndpointGroupCloud.

pkg/neg/types/cloudprovideradapter_test.go

@@ -77,36 +77,25 @@ func validateAggregatedList(t *testing.T, adapter NetworkEndpointGroupCloud, exp
 	if err != nil {
 		t.Errorf("Expect AggregatedListNetworkEndpointGroup to return nil error, but got %v", err)
 	}
-	if len(ret) != expectZoneNum {
-		t.Errorf("Expect len(ret) == %v, got %v", expectZoneNum, len(ret))
-	}
 
 	zoneNames := sets.NewString()
 	expectZoneNames := sets.NewString()
-	for key := range expectZoneNegs {
-		expectZoneNames.Insert(key)
-	}
-	for zone, negs := range ret {
-		zoneNames.Insert(zone)
-		negNames := sets.NewString()
-		expectNegNames := sets.NewString()
-
-		for _, neg := range negs {
-			negNames.Insert(neg.Name)
-		}
-
-		expectNegs, ok := expectZoneNegs[zone]
-		if !ok {
-			t.Errorf("Zone %v from return is not expected", zone)
-			continue
-		}
+	negNames := sets.NewString()
+	expectNegNames := sets.NewString()
 
+	for zone, expectNegs := range expectZoneNegs {
+		expectZoneNames.Insert(zone)
 		for _, neg := range expectNegs {
 			expectNegNames.Insert(neg)
 		}
-		if !negNames.Equal(expectNegNames) {
-			t.Errorf("Expect NEG names %v, but got %v", expectNegNames.List(), negNames.List())
-		}
+	}
+	for key, neg := range ret {
+		zoneNames.Insert(key.Zone)
+		negNames.Insert(neg.Name)
+	}
+
+	if !negNames.Equal(expectNegNames) {
+		t.Errorf("Expect NEG names %v, but got %v", expectNegNames.List(), negNames.List())
 	}
 
 	if !zoneNames.Equal(expectZoneNames) {

pkg/neg/types/fakes.go

@@ -108,10 +108,16 @@ func (f *FakeNetworkEndpointGroupCloud) ListNetworkEndpointGroup(zone string, ve
 	return f.NetworkEndpointGroups[zone], nil
 }
 
-func (f *FakeNetworkEndpointGroupCloud) AggregatedListNetworkEndpointGroup(version meta.Version) (map[string][]*composite.NetworkEndpointGroup, error) {
+func (f *FakeNetworkEndpointGroupCloud) AggregatedListNetworkEndpointGroup(version meta.Version) (map[*meta.Key]*composite.NetworkEndpointGroup, error) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
-	return f.NetworkEndpointGroups, nil
+	result := make(map[*meta.Key]*composite.NetworkEndpointGroup)
+	for zone, negs := range f.NetworkEndpointGroups {
+		for _, neg := range negs {
+			result[&meta.Key{Zone: zone}] = neg
+		}
+	}
+	return result, nil
 }
 
 func (f *FakeNetworkEndpointGroupCloud) CreateNetworkEndpointGroup(neg *composite.NetworkEndpointGroup, zone string) error {

pkg/neg/types/interfaces.go

@@ -31,7 +31,7 @@ type ZoneGetter interface {
 type NetworkEndpointGroupCloud interface {
 	GetNetworkEndpointGroup(name string, zone string, version meta.Version) (*composite.NetworkEndpointGroup, error)
 	ListNetworkEndpointGroup(zone string, version meta.Version) ([]*composite.NetworkEndpointGroup, error)
-	AggregatedListNetworkEndpointGroup(version meta.Version) (map[string][]*composite.NetworkEndpointGroup, error)
+	AggregatedListNetworkEndpointGroup(version meta.Version) (map[*meta.Key]*composite.NetworkEndpointGroup, error)
 	CreateNetworkEndpointGroup(neg *composite.NetworkEndpointGroup, zone string) error
 	DeleteNetworkEndpointGroup(name string, zone string, version meta.Version) error
 	AttachNetworkEndpoints(name, zone string, endpoints []*composite.NetworkEndpoint, version meta.Version) error