Look for correct neg when using multi-networking

Multi-networking and multi subnet clusters are currently incompatible
features and approach neg naming differently. Therefore, ensure that the
neg syncer understands which naming scheme to use and how to detect
multi-networking versus multi subnet clusters correctly

pkg/neg/syncers/transaction.go

@@ -256,7 +256,7 @@ func (s *transactionSyncer) syncInternal() error {
 
 func (s *transactionSyncer) syncInternalImpl() error {
 	if s.syncer.IsStopped() || s.syncer.IsShuttingDown() {
-		s.logger.V(3).Info("Skip syncing NEG", "negSyncerKey", s.NegSyncerKey.String())
+		s.logger.Info("Skip syncing NEG", "negSyncerKey", s.NegSyncerKey.String(), "syncerStopper", s.syncer.IsStopped(), "syncerShuttingDown", s.syncer.IsShuttingDown())
 		return nil
 	}
 	if s.needInit || s.isZoneChange() {
@@ -267,28 +267,13 @@ func (s *transactionSyncer) syncInternalImpl() error {
 	}
 	s.logger.V(2).Info("Sync NEG", "negSyncerKey", s.NegSyncerKey.String(), "endpointsCalculatorMode", s.endpointsCalculator.Mode())
 
-	defaultSubnet, err := utils.KeyName(s.networkInfo.SubnetworkURL)
+	subnetConfigs := s.zoneGetter.ListSubnets(s.logger)
+	subnetToNegMapping, err := s.generateSubnetToNegNameMap(subnetConfigs)
 	if err != nil {
-		s.logger.Error(err, "Errored getting default subnet from NetworkInfo when retrieving existing endpoints")
+		s.logger.Error(err, "failed to generate subnet to neg name mapping")
 		return err
 	}
 
-	subnetToNegMapping := map[string]string{}
-	subnetConfigs := s.zoneGetter.ListSubnets(s.logger)
-	for _, subnetConfig := range subnetConfigs {
-		// negs in default subnet have a different naming scheme from other subnets
-		if subnetConfig.Name == defaultSubnet {
-			subnetToNegMapping[defaultSubnet] = s.NegSyncerKey.NegName
-			continue
-		}
-		nonDefaultNegName, err := s.getNonDefaultSubnetNEGName(subnetConfig.Name)
-		if err != nil {
-			s.logger.Error(err, "Errored when getting NEG name from non-default subnets when retrieving existing endpoints")
-			return err
-		}
-		subnetToNegMapping[subnetConfig.Name] = nonDefaultNegName
-	}
-
 	currentMap, currentPodLabelMap, err := retrieveExistingZoneNetworkEndpointMap(subnetToNegMapping, s.zoneGetter, s.cloud, s.NegSyncerKey.GetAPIVersion(), s.endpointsCalculator.Mode(), s.enableDualStackNEG, s.logger)
 	if err != nil {
 		return fmt.Errorf("%w: %w", negtypes.ErrCurrentNegEPNotFound, err)
@@ -406,6 +391,41 @@ func (s *transactionSyncer) syncInternalImpl() error {
 	return s.syncNetworkEndpoints(addEndpoints, removeEndpoints, endpointPodLabelMap, migrationZone)
 }
 
+func (s *transactionSyncer) generateSubnetToNegNameMap(subnetConfigs []nodetopologyv1.SubnetConfig) (map[string]string, error) {
+	defaultSubnet, err := utils.KeyName(s.networkInfo.SubnetworkURL)
+	if err != nil {
+		s.logger.Error(err, "Errored getting default subnet from NetworkInfo when retrieving existing endpoints")
+		return nil, err
+	}
+
+	subnetToNegMapping := make(map[string]string)
+	// If networkInfo is not on the default subnet, then this service is using
+	// multi-networking which cannot be used with multi subnet clusters. Even though
+	// multi-networking subnet is using a non default subnet name, we use the default
+	// neg naming which differs from how multi subnet cluster non default NEG names are
+	// handled.
+	if !s.networkInfo.IsDefault {
+		subnetToNegMapping[defaultSubnet] = s.NegSyncerKey.NegName
+		return subnetToNegMapping, nil
+	}
+
+	for _, subnetConfig := range subnetConfigs {
+		// negs in default subnet have a different naming scheme from other subnets
+		if subnetConfig.Name == defaultSubnet {
+			subnetToNegMapping[defaultSubnet] = s.NegSyncerKey.NegName
+			continue
+		}
+		nonDefaultNegName, err := s.getNonDefaultSubnetNEGName(subnetConfig.Name)
+		if err != nil {
+			s.logger.Error(err, "Errored when getting NEG name from non-default subnets when retrieving existing endpoints")
+			return nil, err
+		}
+		subnetToNegMapping[subnetConfig.Name] = nonDefaultNegName
+	}
+
+	return subnetToNegMapping, nil
+}
+
 func (s *transactionSyncer) getEndpointsCalculation(
 	endpointsData []negtypes.EndpointsData,
 	currentMap map[negtypes.NEGLocation]negtypes.NetworkEndpointSet,

pkg/neg/syncers/transaction_test.go

@@ -507,6 +507,68 @@ func TestTransactionSyncNetworkEndpointsMSC(t *testing.T) {
 	}
 }
 
+func TestNegNameMultiNetworking(t *testing.T) {
+	t.Parallel()
+
+	fakeGCE := gce.NewFakeGCECloud(test.DefaultTestClusterValues())
+	negtypes.MockNetworkEndpointAPIs(fakeGCE)
+	fakeCloud := negtypes.NewAdapter(fakeGCE)
+	nonDefaultSubnetURL := "projects/mock-project/regions/test-region/subnetworks/non-default"
+	netInfo := network.NetworkInfo{IsDefault: false, NetworkURL: fakeGCE.NetworkURL(), SubnetworkURL: nonDefaultSubnetURL}
+
+	_, transactionSyncer, err := newTestTransactionSyncerWithNetInfo(fakeCloud, negtypes.VmIpEndpointType, false, netInfo)
+	if err != nil {
+		t.Fatalf("failed to initialize transaction syncer: %v", err)
+	}
+
+	// Start syncer without starting syncer goroutine
+	(transactionSyncer.syncer.(*syncer)).stopped = false
+	if err := transactionSyncer.ensureNetworkEndpointGroups(); err != nil {
+		t.Errorf("Expect error == nil, but got %v", err)
+	}
+	// var targetPort string
+	// if testNegType == negtypes.VmIpPortEndpointType {
+	// 	targetPort = "8080"
+	// }
+
+	// Verify the NEGs are created as expected
+	ret, _ := transactionSyncer.cloud.AggregatedListNetworkEndpointGroup(transactionSyncer.NegSyncerKey.GetAPIVersion(), klog.TODO())
+	// Though the test cases below only add instances in zone1 and zone2, NEGs will be created in zone3 or zone4 as well since fakeZoneGetter includes those zones.
+	expectZones := []string{negtypes.TestZone1, negtypes.TestZone2, negtypes.TestZone3}
+	retZones := sets.NewString()
+
+	for key := range ret {
+		retZones.Insert(key.Zone)
+	}
+	for _, zone := range expectZones {
+		_, ok := retZones[zone]
+		if !ok {
+			t.Errorf("Failed to find zone %q from ret %v for negType %v", zone, ret, negtypes.VmIpEndpointType)
+			continue
+		}
+	}
+
+	err = transactionSyncer.syncInternal()
+	if err != nil {
+		t.Errorf("unexpected error when syncing: %s", err)
+	}
+
+	for _, neg := range ret {
+		if neg.Name != transactionSyncer.NegName {
+			t.Errorf("Unexpected neg %q, expected %q", neg.Name, transactionSyncer.NegName)
+		}
+		if neg.NetworkEndpointType != string(negtypes.VmIpEndpointType) {
+			t.Errorf("Unexpected neg type %q, expected %q", neg.NetworkEndpointType, negtypes.VmIpEndpointType)
+		}
+		if neg.Description == "" {
+			t.Errorf("Neg Description should be populated when NEG CRD is enabled")
+		}
+		if neg.Subnetwork != nonDefaultSubnetURL {
+			t.Errorf("Neg subnetwork URL is incorrect. Got %s, Expected %s", neg.Subnetwork, nonDefaultSubnetURL)
+		}
+	}
+}
+
 func TestSyncNetworkEndpointLabel(t *testing.T) {
 
 	var (
@@ -3340,10 +3402,16 @@ func TestGetNonDefaultSubnetNEGName(t *testing.T) {
 }
 
 func newTestTransactionSyncer(fakeGCE negtypes.NetworkEndpointGroupCloud, negType negtypes.NetworkEndpointType, customName bool) (negtypes.NegSyncer, *transactionSyncer, error) {
-	return newTestTransactionSyncerWithTopologyInformer(fakeGCE, negType, customName, zonegetter.FakeNodeTopologyInformer())
+	netInfo := network.NetworkInfo{IsDefault: true, NetworkURL: fakeGCE.NetworkURL(), SubnetworkURL: fakeGCE.SubnetworkURL()}
+	return newCustomTestTransactionSyncer(fakeGCE, negType, customName, zonegetter.FakeNodeTopologyInformer(), netInfo)
 }
 
 func newTestTransactionSyncerWithTopologyInformer(fakeGCE negtypes.NetworkEndpointGroupCloud, negType negtypes.NetworkEndpointType, customName bool, nodeTopologyInformer cache.SharedIndexInformer) (negtypes.NegSyncer, *transactionSyncer, error) {
+	netInfo := network.NetworkInfo{IsDefault: true, NetworkURL: fakeGCE.NetworkURL(), SubnetworkURL: fakeGCE.SubnetworkURL()}
+	return newCustomTestTransactionSyncer(fakeGCE, negType, customName, nodeTopologyInformer, netInfo)
+}
+
+func newCustomTestTransactionSyncer(fakeGCE negtypes.NetworkEndpointGroupCloud, negType negtypes.NetworkEndpointType, customName bool, nodeTopologyInformer cache.SharedIndexInformer, netInfo network.NetworkInfo) (negtypes.NegSyncer, *transactionSyncer, error) {
 	testContext := negtypes.NewTestContext()
 	svcPort := negtypes.NegSyncerKey{
 		Namespace: testServiceNamespace,
@@ -3398,7 +3466,7 @@ func newTestTransactionSyncerWithTopologyInformer(fakeGCE negtypes.NetworkEndpoi
 		klog.TODO(),
 		labels.PodLabelPropagationConfig{},
 		testContext.EnableDualStackNEG,
-		network.NetworkInfo{IsDefault: true, NetworkURL: fakeGCE.NetworkURL(), SubnetworkURL: fakeGCE.SubnetworkURL()},
+		netInfo,
 		negNamer,
 	)
 	transactionSyncer := negsyncer.(*syncer).core.(*transactionSyncer)