diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index cf3446b62c..e7f7e8abd4 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -210,7 +210,8 @@ func (ipsMgr *IpsetManager) run(entry *ipsEntry) (int, error) { return 0, nil } -func (ipsMgr *IpsetManager) createList(listName string) error { +// CreateListNoLock is identical to CreateList except it does not lock the ipsMgr. +func (ipsMgr *IpsetManager) CreateListNoLock(listName string) error { if _, exists := ipsMgr.listMap[listName]; exists { return nil @@ -238,8 +239,8 @@ func (ipsMgr *IpsetManager) createList(listName string) error { return nil } -// createSet creates an ipset. -func (ipsMgr *IpsetManager) createSet(setName string, spec []string) error { +// CreateSetNoLock is identical to CreateSet except it does not lock the ipsMgr. +func (ipsMgr *IpsetManager) CreateSetNoLock(setName string, spec []string) error { // This timer measures execution time to run this function regardless of success or failure cases prometheusTimer := metrics.StartNewTimer() @@ -304,14 +305,18 @@ func (ipsMgr *IpsetManager) deleteSet(setName string) error { func (ipsMgr *IpsetManager) CreateList(listName string) error { ipsMgr.Lock() defer ipsMgr.Unlock() - return ipsMgr.createList(listName) + return ipsMgr.CreateListNoLock(listName) } // AddToList inserts an ipset to an ipset list. func (ipsMgr *IpsetManager) AddToList(listName string, setName string) error { ipsMgr.Lock() defer ipsMgr.Unlock() + return ipsMgr.AddToListNoLock(listName, setName) +} +// AddToListNoLock is identical to AddToList except it does not lock the ipsMgr. +func (ipsMgr *IpsetManager) AddToListNoLock(listName, setName string) error { if listName == setName { return nil } @@ -332,7 +337,7 @@ func (ipsMgr *IpsetManager) AddToList(listName string, setName string) error { return fmt.Errorf("Failed to add set [%s] to list [%s], but list is of type [%s]", setName, listName, listtype) } else if !exists { // if the list doesn't exist, create it - if err := ipsMgr.createList(listName); err != nil { + if err := ipsMgr.CreateListNoLock(listName); err != nil { return err } } @@ -426,7 +431,7 @@ func (ipsMgr *IpsetManager) DeleteFromList(listName string, setName string) erro func (ipsMgr *IpsetManager) CreateSet(setName string, spec []string) error { ipsMgr.Lock() defer ipsMgr.Unlock() - return ipsMgr.createSet(setName, spec) + return ipsMgr.CreateSetNoLock(setName, spec) } // DeleteSet removes a set from ipset. @@ -440,7 +445,11 @@ func (ipsMgr *IpsetManager) DeleteSet(setName string) error { func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error { ipsMgr.Lock() defer ipsMgr.Unlock() + return ipsMgr.AddToSetNoLock(setName, ip, spec, podKey) +} +// AddToSetNoLock is identical to AddToSet except it does not lock the ipsMgr. +func (ipsMgr *IpsetManager) AddToSetNoLock(setName, ip, spec, podKey string) error { if ipsMgr.exists(setName, ip, spec) { // make sure we have updated the podKey in case it gets changed cachedPodKey := ipsMgr.setMap[setName].elements[ip] @@ -468,7 +477,7 @@ func (ipsMgr *IpsetManager) AddToSet(setName, ip, spec, podKey string) error { exists, _ := ipsMgr.setExists(setName) if !exists { - if err := ipsMgr.createSet(setName, []string{spec}); err != nil { + if err := ipsMgr.CreateSetNoLock(setName, []string{spec}); err != nil { return err } } diff --git a/npm/ipsm/ipsm_test.go b/npm/ipsm/ipsm_test.go index 2b3f1e9e2c..8d470db51e 100644 --- a/npm/ipsm/ipsm_test.go +++ b/npm/ipsm/ipsm_test.go @@ -37,7 +37,7 @@ func TestCreateList(t *testing.T) { execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 1, execCount+1, 0, expectedSetInfo{0, testListName}) - err := ipsMgr.createList(testListName) + err := ipsMgr.CreateListNoLock(testListName) require.NoError(t, err) } @@ -90,7 +90,7 @@ func TestDeleteList(t *testing.T) { execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 0, execCount+1, 0, expectedSetInfo{0, testListName}) - err := ipsMgr.createList(testListName) + err := ipsMgr.CreateListNoLock(testListName) require.NoError(t, err) err = ipsMgr.deleteList(testListName) @@ -111,7 +111,7 @@ func TestAddToList(t *testing.T) { execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 2, execCount+2, 1, expectedSetInfo{1, testListName}) - err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag}) + err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag}) require.NoError(t, err) err = ipsMgr.AddToList(testListName, testSetName) @@ -144,7 +144,7 @@ func TestDeleteFromList(t *testing.T) { defer testPrometheusMetrics(t, 0, execCount+2, 0, expectedSets...) // Create set and validate set is created. - err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag}) + err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag}) require.NoError(t, err) entry := &ipsEntry{ @@ -235,16 +235,16 @@ func TestCreateSet(t *testing.T) { expectedSets := []expectedSetInfo{{0, testSet1Name}, {0, testSet2Name}, {1, testSet3Name}} defer testPrometheusMetrics(t, 3, execCount+3, 1, expectedSets...) - err := ipsMgr.createSet(testSet1Name, []string{util.IpsetNetHashFlag}) + err := ipsMgr.CreateSetNoLock(testSet1Name, []string{util.IpsetNetHashFlag}) require.NoError(t, err) spec := []string{util.IpsetNetHashFlag, util.IpsetMaxelemName, util.IpsetMaxelemNum} - if err := ipsMgr.createSet(testSet2Name, spec); err != nil { + if err = ipsMgr.CreateSetNoLock(testSet2Name, spec); err != nil { t.Errorf("TestCreateSet failed @ ipsMgr.CreateSet when set maxelem") } spec = []string{util.IpsetIPPortHashFlag} - if err := ipsMgr.createSet(testSet3Name, spec); err != nil { + if err = ipsMgr.CreateSetNoLock(testSet3Name, spec); err != nil { t.Errorf("TestCreateSet failed @ ipsMgr.CreateSet when creating port set") } @@ -270,7 +270,7 @@ func TestDeleteSet(t *testing.T) { execCount := resetPrometheusAndGetExecCount(t) defer testPrometheusMetrics(t, 0, execCount+1, 0, expectedSetInfo{0, testSetName}) - err := ipsMgr.createSet(testSetName, []string{util.IpsetNetHashFlag}) + err := ipsMgr.CreateSetNoLock(testSetName, []string{util.IpsetNetHashFlag}) require.NoError(t, err) err = ipsMgr.deleteSet(testSetName) @@ -544,13 +544,13 @@ func TestDestroyNpmIpsets(t *testing.T) { expectedSets := []expectedSetInfo{{0, testSet1Name}, {0, testSet1Name}} defer testPrometheusMetrics(t, 0, execCount+2, 0, expectedSets...) - err := ipsMgr.createSet(testSet1Name, []string{"nethash"}) + err := ipsMgr.CreateSetNoLock(testSet1Name, []string{"nethash"}) if err != nil { t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.createSet") t.Errorf(err.Error()) } - err = ipsMgr.createSet(testSet2Name, []string{"nethash"}) + err = ipsMgr.CreateSetNoLock(testSet2Name, []string{"nethash"}) if err != nil { t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.createSet") t.Errorf(err.Error()) @@ -580,7 +580,7 @@ func TestMarshalListMapJSON(t *testing.T) { ipsMgr := NewIpsetManager(fexec) defer testutils.VerifyCalls(t, fexec, calls) - err := ipsMgr.createList(testListSet) + err := ipsMgr.CreateListNoLock(testListSet) require.NoError(t, err) listMapRaw, err := ipsMgr.MarshalListMapJSON() @@ -603,7 +603,7 @@ func TestMarshalSetMapJSON(t *testing.T) { ipsMgr := NewIpsetManager(fexec) defer testutils.VerifyCalls(t, fexec, calls) - err := ipsMgr.createSet(testSet, []string{util.IpsetNetHashFlag}) + err := ipsMgr.CreateSetNoLock(testSet, []string{util.IpsetNetHashFlag}) require.NoError(t, err) setMapRaw, err := ipsMgr.MarshalSetMapJSON() diff --git a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go index 7b274ca5dd..628d66db01 100644 --- a/npm/pkg/controlplane/controllers/v1/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v1/networkPolicyController.go @@ -368,17 +368,19 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // the key is re-queued in workqueue and process this function again, which eventually meets desired states of network policy c.rawNpMap[netpolKey] = netPolObj metrics.IncNumPolicies() + c.ipsMgr.Lock() + defer c.ipsMgr.Unlock() sets, namedPorts, lists, ingressIPCidrs, egressIPCidrs, iptEntries := translatePolicy(netPolObj) for _, set := range sets { klog.Infof("Creating set: %v, hashedSet: %v", set, util.GetHashedName(set)) - if err = c.ipsMgr.CreateSet(set, []string{util.IpsetNetHashFlag}); err != nil { + if err = c.ipsMgr.CreateSetNoLock(set, []string{util.IpsetNetHashFlag}); err != nil { return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset %s with err: %w", set, err) } } for _, set := range namedPorts { klog.Infof("Creating set: %v, hashedSet: %v", set, util.GetHashedName(set)) - if err = c.ipsMgr.CreateSet(set, []string{util.IpsetIPPortHashFlag}); err != nil { + if err = c.ipsMgr.CreateSetNoLock(set, []string{util.IpsetIPPortHashFlag}); err != nil { return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset named port %s with err: %w", set, err) } } @@ -386,7 +388,7 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // lists is a map with list name and members as value // NPM will create the list first and increments the refer count for listKey := range lists { - if err = c.ipsMgr.CreateList(listKey); err != nil { + if err = c.ipsMgr.CreateListNoLock(listKey); err != nil { return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %w", listKey, err) } c.ipsMgr.IpSetReferIncOrDec(listKey, util.IpsetSetListFlag, ipsm.IncrementOp) @@ -395,7 +397,7 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 // to lists before they are created. for listKey, listLabelsMembers := range lists { for _, listMember := range listLabelsMembers { - if err = c.ipsMgr.AddToList(listKey, listMember); err != nil { + if err = c.ipsMgr.AddToListNoLock(listKey, listMember); err != nil { return operationKind, fmt.Errorf("[syncAddAndUpdateNetPol] Error: Adding ipset member %s to ipset list %s with err: %w", listMember, listKey, err) } } @@ -489,7 +491,7 @@ func (c *NetworkPolicyController) createCidrsRule(direction, policyName, ns stri } setName := policyName + "-in-ns-" + ns + "-" + strconv.Itoa(i) + direction klog.Infof("Creating set: %v, hashedSet: %v", setName, util.GetHashedName(setName)) - if err := c.ipsMgr.CreateSet(setName, spec); err != nil { + if err := c.ipsMgr.CreateSetNoLock(setName, spec); err != nil { return fmt.Errorf("[createCidrsRule] Error: creating ipset %s with err: %w", ipCidrSet, err) } for _, ipCidrEntry := range util.DropEmptyFields(ipCidrSet) { @@ -498,12 +500,12 @@ func (c *NetworkPolicyController) createCidrsRule(direction, policyName, ns stri if ipCidrEntry == "0.0.0.0/0" { splitEntry := [2]string{"0.0.0.0/1", "128.0.0.0/1"} for _, entry := range splitEntry { - if err := c.ipsMgr.AddToSet(setName, entry, util.IpsetNetHashFlag, ""); err != nil { + if err := c.ipsMgr.AddToSetNoLock(setName, entry, util.IpsetNetHashFlag, ""); err != nil { return fmt.Errorf("[createCidrsRule] adding ip cidrs %s into ipset %s with err: %w", entry, ipCidrSet, err) } } } else { - if err := c.ipsMgr.AddToSet(setName, ipCidrEntry, util.IpsetNetHashFlag, ""); err != nil { + if err := c.ipsMgr.AddToSetNoLock(setName, ipCidrEntry, util.IpsetNetHashFlag, ""); err != nil { return fmt.Errorf("[createCidrsRule] adding ip cidrs %s into ipset %s with err: %w", ipCidrEntry, ipCidrSet, err) } }