Skip to content

Commit 5d8c2ae

Browse files
huntergregoryhuntergregory
authored
lock ipsm while adding/updating netpol (#1257)
1 parent 569c946 commit 5d8c2ae

File tree

3 files changed

+37
-26
lines changed

3 files changed

+37
-26
lines changed

‎npm/ipsm/ipsm.go‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,8 @@ func (ipsMgr *IpsetManager) run(entry *ipsEntry) (int, error) {
210210
return 0, nil
211211
}
212212

213-
func (ipsMgr *IpsetManager) createList(listName string) error {
213+
// CreateListNoLock is identical to CreateList except it does not lock the ipsMgr.
214+
func (ipsMgr *IpsetManager) CreateListNoLock(listName string) error {
214215

215216
if _, exists := ipsMgr.listMap[listName]; exists {
216217
return nil
@@ -238,8 +239,8 @@ func (ipsMgr *IpsetManager) createList(listName string) error {
238239
return nil
239240
}
240241

241-
// createSet creates an ipset.
242-
func (ipsMgr *IpsetManager) createSet(setName string, spec []string) error {
242+
// CreateSetNoLock is identical to CreateSet except it does not lock the ipsMgr.
243+
func (ipsMgr *IpsetManager) CreateSetNoLock(setName string, spec []string) error {
243244
// This timer measures execution time to run this function regardless of success or failure cases
244245
prometheusTimer := metrics.StartNewTimer()
245246

@@ -304,14 +305,18 @@ func (ipsMgr *IpsetManager) deleteSet(setName string) error {
304305
func (ipsMgr *IpsetManager) CreateList(listName string) error {
305306
ipsMgr.Lock()
306307
defer ipsMgr.Unlock()
307-
return ipsMgr.createList(listName)
308+
return ipsMgr.CreateListNoLock(listName)
308309
}
309310

310311
// AddToList inserts an ipset to an ipset list.
311312
func (ipsMgr *IpsetManager) AddToList(listName string, setName string) error {
312313
ipsMgr.Lock()
313314
defer ipsMgr.Unlock()
315+
return ipsMgr.AddToListNoLock(listName, setName)
316+
}
314317

318+
// AddToListNoLock is identical to AddToList except it does not lock the ipsMgr.
319+
func (ipsMgr *IpsetManager) AddToListNoLock(listName, setName string) error {
315320
if listName == setName {
316321
return nil
317322
}
@@ -332,7 +337,7 @@ func (ipsMgr *IpsetManager) AddToList(listName string, setName string) error {
332337
return fmt.Errorf("Failed to add set [%s] to list [%s], but list is of type [%s]", setName, listName, listtype)
333338
} else if !exists {
334339
// if the list doesn't exist, create it
335-
if err := ipsMgr.createList(listName); err != nil {
340+
if err := ipsMgr.CreateListNoLock(listName); err != nil {
336341
return err
337342
}
338343
}
@@ -426,7 +431,7 @@ func (ipsMgr *IpsetManager) DeleteFromList(listName string, setName string) erro
426431
func (ipsMgr *IpsetManager) CreateSet(setName string, spec []string) error {
427432
ipsMgr.Lock()
428433
defer ipsMgr.Unlock()
429-
return ipsMgr.createSet(setName, spec)
434+
return ipsMgr.CreateSetNoLock(setName, spec)
430435
}
431436

432437
// DeleteSet removes a set from ipset.
@@ -440,7 +445,11 @@ func (ipsMgr *IpsetManager) DeleteSet(setName string) error {
440445
func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error {
441446
ipsMgr.Lock()
442447
defer ipsMgr.Unlock()
448+
return ipsMgr.AddToSetNoLock(setName, ip, spec, podKey)
449+
}
443450

451+
// AddToSetNoLock is identical to AddToSet except it does not lock the ipsMgr.
452+
func (ipsMgr *IpsetManager) AddToSetNoLock(setName, ip, spec, podKey string) error {
444453
if ipsMgr.exists(setName, ip, spec) {
445454
// make sure we have updated the podKey in case it gets changed
446455
cachedPodKey := ipsMgr.setMap[setName].elements[ip]
@@ -468,7 +477,7 @@ func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error {
468477
exists, _ := ipsMgr.setExists(setName)
469478

470479
if !exists {
471-
if err := ipsMgr.createSet(setName, []string{spec}); err != nil {
480+
if err := ipsMgr.CreateSetNoLock(setName, []string{spec}); err != nil {
472481
return err
473482
}
474483
}

‎npm/ipsm/ipsm_test.go‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ func TestCreateList(t *testing.T) {
3737
execCount := resetPrometheusAndGetExecCount(t)
3838
defer testPrometheusMetrics(t, 1, execCount+1, 0, expectedSetInfo{0, testListName})
3939

40-
err := ipsMgr.createList(testListName)
40+
err := ipsMgr.CreateListNoLock(testListName)
4141
require.NoError(t, err)
4242
}
4343

@@ -90,7 +90,7 @@ func TestDeleteList(t *testing.T) {
9090
execCount := resetPrometheusAndGetExecCount(t)
9191
defer testPrometheusMetrics(t, 0, execCount+1, 0, expectedSetInfo{0, testListName})
9292

93-
err := ipsMgr.createList(testListName)
93+
err := ipsMgr.CreateListNoLock(testListName)
9494
require.NoError(t, err)
9595

9696
err = ipsMgr.deleteList(testListName)
@@ -111,7 +111,7 @@ func TestAddToList(t *testing.T) {
111111
execCount := resetPrometheusAndGetExecCount(t)
112112
defer testPrometheusMetrics(t, 2, execCount+2, 1, expectedSetInfo{1, testListName})
113113

114-
err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag})
114+
err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag})
115115
require.NoError(t, err)
116116

117117
err = ipsMgr.AddToList(testListName, testSetName)
@@ -144,7 +144,7 @@ func TestDeleteFromList(t *testing.T) {
144144
defer testPrometheusMetrics(t, 0, execCount+2, 0, expectedSets...)
145145

146146
// Create set and validate set is created.
147-
err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag})
147+
err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag})
148148
require.NoError(t, err)
149149

150150
entry := &ipsEntry{
@@ -235,16 +235,16 @@ func TestCreateSet(t *testing.T) {
235235
expectedSets := []expectedSetInfo{{0, testSet1Name}, {0, testSet2Name}, {1, testSet3Name}}
236236
defer testPrometheusMetrics(t, 3, execCount+3, 1, expectedSets...)
237237

238-
err := ipsMgr.createSet(testSet1Name, []string{util.IpsetNetHashFlag})
238+
err := ipsMgr.CreateSetNoLock(testSet1Name, []string{util.IpsetNetHashFlag})
239239
require.NoError(t, err)
240240

241241
spec := []string{util.IpsetNetHashFlag, util.IpsetMaxelemName, util.IpsetMaxelemNum}
242-
if err := ipsMgr.createSet(testSet2Name, spec); err != nil {
242+
if err = ipsMgr.CreateSetNoLock(testSet2Name, spec); err != nil {
243243
t.Errorf("TestCreateSet failed @ ipsMgr.CreateSet when set maxelem")
244244
}
245245

246246
spec = []string{util.IpsetIPPortHashFlag}
247-
if err := ipsMgr.createSet(testSet3Name, spec); err != nil {
247+
if err = ipsMgr.CreateSetNoLock(testSet3Name, spec); err != nil {
248248
t.Errorf("TestCreateSet failed @ ipsMgr.CreateSet when creating port set")
249249
}
250250

@@ -270,7 +270,7 @@ func TestDeleteSet(t *testing.T) {
270270
execCount := resetPrometheusAndGetExecCount(t)
271271
defer testPrometheusMetrics(t, 0, execCount+1, 0, expectedSetInfo{0, testSetName})
272272

273-
err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag})
273+
err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag})
274274
require.NoError(t, err)
275275

276276
err = ipsMgr.deleteSet(testSetName)
@@ -544,13 +544,13 @@ func TestDestroyNpmIpsets(t *testing.T) {
544544
expectedSets := []expectedSetInfo{{0, testSet1Name}, {0, testSet1Name}}
545545
defer testPrometheusMetrics(t, 0, execCount+2, 0, expectedSets...)
546546

547-
err := ipsMgr.createSet(testSet1Name, []string{"nethash"})
547+
err := ipsMgr.CreateSetNoLock(testSet1Name, []string{"nethash"})
548548
if err != nil {
549549
t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.createSet")
550550
t.Errorf(err.Error())
551551
}
552552

553-
err = ipsMgr.createSet(testSet2Name, []string{"nethash"})
553+
err = ipsMgr.CreateSetNoLock(testSet2Name, []string{"nethash"})
554554
if err != nil {
555555
t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.createSet")
556556
t.Errorf(err.Error())
@@ -580,7 +580,7 @@ func TestMarshalListMapJSON(t *testing.T) {
580580
ipsMgr := NewIpsetManager(fexec)
581581
defer testutils.VerifyCalls(t, fexec, calls)
582582

583-
err := ipsMgr.createList(testListSet)
583+
err := ipsMgr.CreateListNoLock(testListSet)
584584
require.NoError(t, err)
585585

586586
listMapRaw, err := ipsMgr.MarshalListMapJSON()
@@ -603,7 +603,7 @@ func TestMarshalSetMapJSON(t *testing.T) {
603603
ipsMgr := NewIpsetManager(fexec)
604604
defer testutils.VerifyCalls(t, fexec, calls)
605605

606-
err := ipsMgr.createSet(testSet, []string{util.IpsetNetHashFlag})
606+
err := ipsMgr.CreateSetNoLock(testSet, []string{util.IpsetNetHashFlag})
607607
require.NoError(t, err)
608608

609609
setMapRaw, err := ipsMgr.MarshalSetMapJSON()

‎npm/pkg/controlplane/controllers/v1/networkPolicyController.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -368,25 +368,27 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1
368368
// the key is re-queued in workqueue and process this function again, which eventually meets desired states of network policy
369369
c.rawNpMap[netpolKey] = netPolObj
370370
metrics.IncNumPolicies()
371+
c.ipsMgr.Lock()
372+
defer c.ipsMgr.Unlock()
371373

372374
sets, namedPorts, lists, ingressIPCidrs, egressIPCidrs, iptEntries := translatePolicy(netPolObj)
373375
for _, set := range sets {
374376
klog.Infof("Creating set: %v, hashedSet: %v", set, util.GetHashedName(set))
375-
if err = c.ipsMgr.CreateSet(set, []string{util.IpsetNetHashFlag}); err != nil {
377+
if err = c.ipsMgr.CreateSetNoLock(set, []string{util.IpsetNetHashFlag}); err != nil {
376378
return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset %s with err: %w", set, err)
377379
}
378380
}
379381
for _, set := range namedPorts {
380382
klog.Infof("Creating set: %v, hashedSet: %v", set, util.GetHashedName(set))
381-
if err = c.ipsMgr.CreateSet(set, []string{util.IpsetIPPortHashFlag}); err != nil {
383+
if err = c.ipsMgr.CreateSetNoLock(set, []string{util.IpsetIPPortHashFlag}); err != nil {
382384
return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset named port %s with err: %w", set, err)
383385
}
384386
}
385387

386388
// lists is a map with list name and members as value
387389
// NPM will create the list first and increments the refer count
388390
for listKey := range lists {
389-
if err = c.ipsMgr.CreateList(listKey); err != nil {
391+
if err = c.ipsMgr.CreateListNoLock(listKey); err != nil {
390392
return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %w", listKey, err)
391393
}
392394
c.ipsMgr.IpSetReferIncOrDec(listKey, util.IpsetSetListFlag, ipsm.IncrementOp)
@@ -395,7 +397,7 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1
395397
// to lists before they are created.
396398
for listKey, listLabelsMembers := range lists {
397399
for _, listMember := range listLabelsMembers {
398-
if err = c.ipsMgr.AddToList(listKey, listMember); err != nil {
400+
if err = c.ipsMgr.AddToListNoLock(listKey, listMember); err != nil {
399401
return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: Adding ipset member %s to ipset list %s with err: %w", listMember, listKey, err)
400402
}
401403
}
@@ -489,7 +491,7 @@ func (c *NetworkPolicyController) createCidrsRule(direction, policyName, ns stri
489491
}
490492
setName := policyName + "-in-ns-" + ns + "-" + strconv.Itoa(i) + direction
491493
klog.Infof("Creating set: %v, hashedSet: %v", setName, util.GetHashedName(setName))
492-
if err := c.ipsMgr.CreateSet(setName, spec); err != nil {
494+
if err := c.ipsMgr.CreateSetNoLock(setName, spec); err != nil {
493495
return fmt.Errorf("[createCidrsRule] Error: creating ipset %s with err: %w", ipCidrSet, err)
494496
}
495497
for _, ipCidrEntry := range util.DropEmptyFields(ipCidrSet) {
@@ -498,12 +500,12 @@ func (c *NetworkPolicyController) createCidrsRule(direction, policyName, ns stri
498500
if ipCidrEntry == "0.0.0.0/0" {
499501
splitEntry := [2]string{"0.0.0.0/1", "128.0.0.0/1"}
500502
for _, entry := range splitEntry {
501-
if err := c.ipsMgr.AddToSet(setName, entry, util.IpsetNetHashFlag, ""); err != nil {
503+
if err := c.ipsMgr.AddToSetNoLock(setName, entry, util.IpsetNetHashFlag, ""); err != nil {
502504
return fmt.Errorf("[createCidrsRule] adding ip cidrs %s into ipset %s with err: %w", entry, ipCidrSet, err)
503505
}
504506
}
505507
} else {
506-
if err := c.ipsMgr.AddToSet(setName, ipCidrEntry, util.IpsetNetHashFlag, ""); err != nil {
508+
if err := c.ipsMgr.AddToSetNoLock(setName, ipCidrEntry, util.IpsetNetHashFlag, ""); err != nil {
507509
return fmt.Errorf("[createCidrsRule] adding ip cidrs %s into ipset %s with err: %w", ipCidrEntry, ipCidrSet, err)
508510
}
509511
}

0 commit comments

Comments
 (0)