From 4380ffebbf8a01c67413bab655b4f20701df140d Mon Sep 17 00:00:00 2001 From: Swetha Repakula Date: Fri, 21 Feb 2025 11:22:07 -0800 Subject: [PATCH] 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 | 58 ++++++++++++++-------- pkg/neg/syncers/transaction_test.go | 76 ++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 21 deletions(-) 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)