Skip to content

fix: [NPM] eliminate chance of ipset race between pod delete and netpol create #1257

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

Merged
merged 1 commit into from
Feb 25, 2022
Merged

fix: [NPM] eliminate chance of ipset race between pod delete and netpol create #1257

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
23 changes: 16 additions & 7 deletions npm/ipsm/ipsm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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.
Expand All @@ -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]
Expand Down Expand Up @@ -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
}
}
Expand Down
24 changes: 12 additions & 12 deletions npm/ipsm/ipsm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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")
}

Expand All @@ -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)
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down
16 changes: 9 additions & 7 deletions npm/pkg/controlplane/controllers/v1/networkPolicyController.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,25 +368,27 @@ 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)
}
}

// 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)
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
}
Expand Down