Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look for correct neg when using multi-networking #2809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Look for correct neg when using multi-networking #2809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 39 additions & 19 deletions pkg/neg/syncers/transaction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
76 changes: 74 additions & 2 deletions pkg/neg/syncers/transaction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down