diff --git a/pkg/neg/syncers/transaction.go b/pkg/neg/syncers/transaction.go index d49a0badf8..d9f0ea4136 100644 --- a/pkg/neg/syncers/transaction.go +++ b/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, diff --git a/pkg/neg/syncers/transaction_test.go b/pkg/neg/syncers/transaction_test.go index 9d7870fd1b..c634518a15 100644 --- a/pkg/neg/syncers/transaction_test.go +++ b/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,20 @@ 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 newTestTransactionSyncerWithNetInfo(fakeGCE negtypes.NetworkEndpointGroupCloud, negType negtypes.NetworkEndpointType, customName bool, netInfo network.NetworkInfo) (negtypes.NegSyncer, *transactionSyncer, error) { + 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 +3470,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)