fix: [NPM] [Linux] race condition when editing NetPol with "except" C…

…IDR (#2841) * fix: syntax error when deleting nomatch CIDR ipset Signed-off-by: Hunter Gregory <[email protected]> * test: ut members with nomatch Signed-off-by: Hunter Gregory <[email protected]> --------- Signed-off-by: Hunter Gregory <[email protected]>
Azure · Jul 24, 2024 · 2688c59 · 2688c59
1 parent 0bdb3ab
commit 2688c59
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 0 deletions.
diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go
@@ -774,6 +774,17 @@ func (iMgr *IPSetManager) deleteMemberForApply(creator *ioutil.FileCreator, set
 			},
 		},
 	}
+
+	splitMember := strings.Split(member, space)
+	if len(splitMember) == 2 && splitMember[1] == util.IpsetNomatch {
+		// Remove "nomatch" from the member.
+		// A "nomatch" CIDR must be deleted like so:
+		// ipset -D 10.0.0.1/32
+		// The following command would cause a syntax failure:
+		// ipset -D 10.0.0.1/32 nomatch
+		member = splitMember[0]
+	}
+
 	creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) // delete member
 }
 

diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go
@@ -992,6 +992,45 @@ func TestDeleteMembers(t *testing.T) {
 	require.False(t, wasFileAltered, "file should not be altered")
 }
 
+func TestCidrExceptMembers(t *testing.T) {
+	calls := []testutils.TestCmd{
+		fakeRestoreSuccessCommand,
+	}
+	ioshim := common.NewMockIOShim(calls)
+	defer ioshim.VerifyCalls(t, calls)
+	iMgr := NewIPSetManager(applyAlwaysCfg, ioshim)
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "1.1.1.1 nomatch", "a"))
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "2.2.2.2", "b"))
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "3.3.3.3 nomatch", "c"))
+	// will not delete this member
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "4.4.4.4 nomatch", "d"))
+	// clear dirty cache, otherwise a set deletion will be a no-op
+	iMgr.clearDirtyCache()
+
+	// remove members
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "1.1.1.1 nomatch", "a"))
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "2.2.2.2", "b"))
+	require.NoError(t, iMgr.RemoveFromSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "3.3.3.3 nomatch", "c"))
+	// add one more nomatch member
+	require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{TestCIDRSet.Metadata}, "5.5.5.5 nomatch", "e"))
+
+	expectedLines := []string{
+		fmt.Sprintf("-N %s --exist nethash maxelem 4294967295", TestCIDRSet.HashedName),
+		fmt.Sprintf("-D %s 1.1.1.1", TestCIDRSet.HashedName),
+		fmt.Sprintf("-D %s 2.2.2.2", TestCIDRSet.HashedName),
+		fmt.Sprintf("-D %s 3.3.3.3", TestCIDRSet.HashedName),
+		fmt.Sprintf("-A %s 5.5.5.5 nomatch", TestCIDRSet.HashedName),
+		"",
+	}
+	sortedExpectedLines := testAndSortRestoreFileLines(t, expectedLines)
+	creator := iMgr.fileCreatorForApply(len(calls))
+	actualLines := testAndSortRestoreFileString(t, creator.ToString())
+	dptestutils.AssertEqualLines(t, sortedExpectedLines, actualLines)
+	wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore")
+	require.NoError(t, err, "ipset restore should be successful")
+	require.False(t, wasFileAltered, "file should not be altered")
+}
+
 func TestUpdateWithIdenticalSaveFile(t *testing.T) {
 	calls := []testutils.TestCmd{fakeRestoreSuccessCommand}
 	ioshim := common.NewMockIOShim(calls)