diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index 6b73e65eb7..112054037d 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -644,6 +644,9 @@ func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { } else { metrics.ResetNumIPSets() } + // NOTE: in v2, we reset ipset entries, but in v1 we only remove entries for ipsets we delete. + // So v2 may underestimate the number of entries if there are destroy failures, + // but v1 may miss removing entries if some sets are in the prometheus metric but not in the kernel. return nil } diff --git a/npm/ipsm/ipsm_test.go b/npm/ipsm/ipsm_test.go index 0788f458af..2b3f1e9e2c 100644 --- a/npm/ipsm/ipsm_test.go +++ b/npm/ipsm/ipsm_test.go @@ -528,6 +528,7 @@ func TestDestroyNpmIpsets(t *testing.T) { calls := []testutils.TestCmd{ {Cmd: []string{"ipset", "-N", "-exist", util.GetHashedName(testSet1Name), "nethash"}}, {Cmd: []string{"ipset", "-N", "-exist", util.GetHashedName(testSet2Name), "nethash"}}, + {Cmd: []string{"ipset", "-A", "-exist", util.GetHashedName(testSet1Name), "1.2.3.4"}}, {Cmd: []string{"ipset", "list"}, Stdout: ipsetListStdout}, {Cmd: []string{"ipset", "-F", "-exist", testSet1Name}}, {Cmd: []string{"ipset", "-F", "-exist", testSet2Name}}, @@ -555,6 +556,13 @@ func TestDestroyNpmIpsets(t *testing.T) { t.Errorf(err.Error()) } + // expect prometheus to add this entry, but remove it when destroying npm sets + err = ipsMgr.AddToSet(testSet1Name, "1.2.3.4", util.IpsetNetHashFlag, "") + if err != nil { + t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.addToSet") + t.Errorf(err.Error()) + } + err = ipsMgr.DestroyNpmIpsets() if err != nil { t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.DestroyNpmIpsets") diff --git a/npm/metrics/ipsets.go b/npm/metrics/ipsets.go index 503388f110..01f685bd4c 100644 --- a/npm/metrics/ipsets.go +++ b/npm/metrics/ipsets.go @@ -81,6 +81,9 @@ func DeleteIPSet(setName string) { // It doesn't ever update the number of IPSets. func ResetIPSetEntries() { numIPSetEntries.Set(0) + for setName := range ipsetInventoryMap { + removeFromIPSetInventory(setName) + } ipsetInventoryMap = make(map[string]int) } diff --git a/npm/metrics/ipsets_test.go b/npm/metrics/ipsets_test.go index 6300ee545d..9c02efd6ca 100644 --- a/npm/metrics/ipsets_test.go +++ b/npm/metrics/ipsets_test.go @@ -94,11 +94,19 @@ func assertNumEntriesAndCounts(t *testing.T, sets ...*testSet) { expectedTotal := 0 for _, set := range sets { val, exists := ipsetInventoryMap[set.name] - if !exists { + if exists && set.entryCount == 0 { + require.FailNowf(t, "", "expected set %s to not exist since we expect an entry count of 0", set.name) + } + if !exists && set.entryCount > 0 { require.FailNowf(t, "", "expected set %s to exist in map for ipset entries", set.name) } if set.entryCount != val { - require.FailNowf(t, "", "set %s has incorrect number of entries. Expected %d, got %d", set.name, set.entryCount, val) + require.FailNowf(t, "", "set %s has incorrect number of entries in the map. Expected %d, got %d", set.name, set.entryCount, val) + } + prometheusVal, err := GetNumEntriesForIPSet(set.name) + require.NoError(t, err, "unexpectedly got an error when getting prometheus num entries for set %s", set.name) + if set.entryCount != prometheusVal { + require.FailNow(t, "", "set %s has incorrect number of entries in the Prometheus metric. Expected %d, got %d", set.name, set.entryCount, prometheusVal) } expectedTotal += val } @@ -110,15 +118,6 @@ func assertNumEntriesAndCounts(t *testing.T, sets ...*testSet) { } } -func assertNotInMap(t *testing.T, setNames ...string) { - for _, setName := range setNames { - _, exists := ipsetInventoryMap[setName] - if exists { - require.FailNowf(t, "", "expected set %s to not exist in map for ipset entries", setName) - } - } -} - func assertMapIsGood(t *testing.T) { assertEqualMapAndMetricElements(t) assertNoZeroEntriesInMap(t) @@ -160,8 +159,7 @@ func TestRemoveAllEntriesFromIPSet(t *testing.T) { AddEntryToIPSet(testName1) AddEntryToIPSet(testName2) RemoveAllEntriesFromIPSet(testName1) - assertNotInMap(t, testName1) - assertNumEntriesAndCounts(t, &testSet{testName2, 1}) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 1}) assertMapIsGood(t) } @@ -175,8 +173,7 @@ func TestDeleteIPSet(t *testing.T) { AddEntryToIPSet(testName2) DeleteIPSet(testName1) assertNumSets(t, 1) - assertNotInMap(t, testName1) - assertNumEntriesAndCounts(t, &testSet{testName2, 1}) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 1}) assertMapIsGood(t) } @@ -193,6 +190,6 @@ func TestResetIPSetEntries(t *testing.T) { AddEntryToIPSet(testName1) AddEntryToIPSet(testName2) ResetIPSetEntries() - assertNotInMap(t, testName1, testName2) + assertNumEntriesAndCounts(t, &testSet{testName1, 0}, &testSet{testName2, 0}) assertMapIsGood(t) } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 5f477a5b82..13b6e1870d 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -37,6 +37,8 @@ type IPSetManagerCfg struct { NetworkName string } +// TODO delegate prometheus metrics logic to OS-specific ones? + func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetManager { return &IPSetManager{ iMgrCfg: iMgrCfg, diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go index c145d9dea5..93a3b2c861 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux.go @@ -4,6 +4,7 @@ import ( "fmt" "regexp" + "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ioutil" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/parse" "github.com/Azure/azure-container-networking/npm/util" @@ -15,6 +16,8 @@ const ( azureNPMPrefix = "azure-npm-" ipsetCommand = "ipset" + ipsetListFlag = "list" + ipsetNameFlag = "--name" ipsetSaveFlag = "save" ipsetRestoreFlag = "restore" ipsetCreateFlag = "-N" @@ -57,14 +60,134 @@ var ( nameForAddRegex = regexp.MustCompile(fmt.Sprintf("%s (%s) ", ipsetAddString, hashedNamePattern)) ) +/* + based on ipset list output with azure-npm- prefix, create an ipset restore file where we flush all sets first, then destroy all sets + + overall error handling: + - if flush fails because the set doesn't exist (should never happen because we're listing sets right before), then ignore it and the destroy + - if flush fails otherwise, then add to destroyFailureCount and continue (aborting the destroy too) + - if destroy fails because the set doesn't exist (should never happen since the flush operation would have worked), then ignore it + - if destroy fails for another reason, then ignore it and add to destroyFailureCount and mark for reconcile (TODO) + + example: + grep output: + azure-npm-123456 + azure-npm-987654 + azure-npm-777777 + + example restore file [flag meanings: -F (flush), -X (destroy)]: + -F azure-npm-123456 + -F azure-npm-987654 + -F azure-npm-777777 + -X azure-npm-123456 + -X azure-npm-987654 + -X azure-npm-777777 + + prometheus metrics: + After this function, NumIPSets should be 0 or the number of NPM IPSets that existed and failed to be destroyed. + When NPM restarts, Prometheus metrics will initialize at 0, but NPM IPSets may exist. + We will reset ipset entry metrics if the restore succeeds whether or not some flushes/destroys failed (NOTE: this is different behavior than v1). + If a flush fails, we could update the num entries for that set, but that would be a lot of overhead. +*/ func (iMgr *IPSetManager) resetIPSets() error { - // called on failure or when NPM is created - // so no ipset cache. need to use ipset list like in ipsm.go + listCommand := iMgr.ioShim.Exec.Command(ipsetCommand, ipsetListFlag, ipsetNameFlag) + grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) + azureIPSets, haveAzureIPSets, commandError := ioutil.PipeCommandToGrep(listCommand, grepCommand) + if commandError != nil { + return npmerrors.SimpleErrorWrapper("failed to run ipset list for resetting IPSets", commandError) + } + if !haveAzureIPSets { + metrics.ResetNumIPSets() + metrics.ResetIPSetEntries() + return nil + } + creator, originalNumAzureSets, destroyFailureCount := iMgr.fileCreatorForReset(azureIPSets) + restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag) + if restoreError != nil { + metrics.SetNumIPSets(originalNumAzureSets) + // NOTE: the num entries for sets may be incorrect if the restore fails + return npmerrors.SimpleErrorWrapper("failed to run ipset restore for resetting IPSets", restoreError) + } + if metrics.NumIPSetsIsPositive() { + metrics.SetNumIPSets(*destroyFailureCount) + } else { + metrics.ResetNumIPSets() + } + metrics.ResetIPSetEntries() // NOTE: the num entries for sets that fail to flush may be incorrect after this + return nil +} - // create restore file that flushes all sets, then deletes all sets - // technically don't need to flush a hashset +// this needs to be a separate function because we need to check creator contents in UTs +func (iMgr *IPSetManager) fileCreatorForReset(ipsetListOutput []byte) (*ioutil.FileCreator, int, *int) { + destroyFailureCount := 0 + creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) + names := make([]string, 0) + readIndex := 0 + var line []byte + // flush all the sets and create a list of the sets so we can destroy them + for readIndex < len(ipsetListOutput) { + line, readIndex = parse.Line(readIndex, ipsetListOutput) + hashedSetName := string(line) + names = append(names, hashedSetName) + // error handlers specific to resetting ipsets + errorHandlers := []*ioutil.LineErrorHandler{ + { + Definition: setDoesntExistDefinition, + Method: ioutil.ContinueAndAbortSection, + Callback: func() { + klog.Infof("[RESET-IPSETS] skipping flush and upcoming destroy for set %s since the set doesn't exist", hashedSetName) + }, + }, + { + Definition: ioutil.AlwaysMatchDefinition, + Method: ioutil.ContinueAndAbortSection, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking flush and upcoming destroy for set %s as a failure due to unknown error", hashedSetName) + destroyFailureCount++ + // TODO mark as a failure + }, + }, + } + sectionID := sectionID(destroySectionPrefix, hashedSetName) + creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedSetName) // flush set + } - return nil + // destroy all the sets + for _, hashedSetName := range names { + hashedSetName := hashedSetName // to appease go lint + errorHandlers := []*ioutil.LineErrorHandler{ + // error handlers specific to resetting ipsets + { + Definition: setInUseByKernelDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure since the set is in use by a kernel component", hashedSetName) + destroyFailureCount++ + // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it + }, + }, + { + Definition: setDoesntExistDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Infof("[RESET-IPSETS] skipping destroy for set %s since the set does not exist", hashedSetName) + }, + }, + { + Definition: ioutil.AlwaysMatchDefinition, + Method: ioutil.Continue, + Callback: func() { + klog.Errorf("[RESET-IPSETS] marking destroy for set %s as a failure due to unknown error", hashedSetName) + destroyFailureCount++ + // TODO mark the set as a failure and reconcile what iptables rule or ipset is referring to it + }, + }, + } + sectionID := sectionID(destroySectionPrefix, hashedSetName) + creator.AddLine(sectionID, errorHandlers, ipsetDestroyFlag, hashedSetName) // destroy set + } + originalNumAzureSets := len(names) + return creator, originalNumAzureSets, &destroyFailureCount } /* @@ -134,7 +257,6 @@ example where every set in add/update cache should have ip 1.2.3.4 and 2.3.4.5: -A new-set-3 2.3.4.5 */ - func (iMgr *IPSetManager) applyIPSets() error { var saveFile []byte var saveError error @@ -144,7 +266,7 @@ func (iMgr *IPSetManager) applyIPSets() error { return npmerrors.SimpleErrorWrapper("ipset save failed when applying ipsets", saveError) } } - creator := iMgr.fileCreator(maxTryCount, saveFile) + creator := iMgr.fileCreatorForApply(maxTryCount, saveFile) restoreError := creator.RunCommandWithFile(ipsetCommand, ipsetRestoreFlag) if restoreError != nil { return npmerrors.SimpleErrorWrapper("ipset restore failed when applying ipsets", restoreError) @@ -155,17 +277,17 @@ func (iMgr *IPSetManager) applyIPSets() error { func (iMgr *IPSetManager) ipsetSave() ([]byte, error) { command := iMgr.ioShim.Exec.Command(ipsetCommand, ipsetSaveFlag) grepCommand := iMgr.ioShim.Exec.Command(ioutil.Grep, azureNPMPrefix) - searchResults, gotMatches, err := ioutil.PipeCommandToGrep(command, grepCommand) + saveFile, haveAzureSets, err := ioutil.PipeCommandToGrep(command, grepCommand) if err != nil { return nil, npmerrors.SimpleErrorWrapper("failed to run ipset save", err) } - if !gotMatches { + if !haveAzureSets { return nil, nil } - return searchResults, nil + return saveFile, nil } -func (iMgr *IPSetManager) fileCreator(maxTryCount int, saveFile []byte) *ioutil.FileCreator { +func (iMgr *IPSetManager) fileCreatorForApply(maxTryCount int, saveFile []byte) *ioutil.FileCreator { creator := ioutil.NewFileCreator(iMgr.ioShim, maxTryCount, ipsetRestoreLineFailurePattern) // TODO make the line failure pattern into a definition constant eventually // flush all sets first so we don't try to delete an ipset referenced by a list we're deleting too @@ -350,15 +472,14 @@ func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedNa Definition: setDoesntExistDefinition, Method: ioutil.ContinueAndAbortSection, Callback: func() { - // no action needed - klog.Infof("skipping flush and upcoming delete for set %s since the set doesn't exist", prefixedName) + klog.Infof("skipping flush and upcoming destroy for set %s since the set doesn't exist", prefixedName) }, }, { Definition: ioutil.AlwaysMatchDefinition, Method: ioutil.ContinueAndAbortSection, Callback: func() { - klog.Errorf("skipping flush and upcoming delete for set %s due to unknown error", prefixedName) + klog.Errorf("skipping flush and upcoming destroy for set %s due to unknown error", prefixedName) // TODO mark as a failure // would this ever happen? }, @@ -366,7 +487,7 @@ func (iMgr *IPSetManager) flushSetInFile(creator *ioutil.FileCreator, prefixedNa } sectionID := sectionID(destroySectionPrefix, prefixedName) hashedName := util.GetHashedName(prefixedName) - creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedName) + creator.AddLine(sectionID, errorHandlers, ipsetFlushFlag, hashedName) // flush set } func (iMgr *IPSetManager) destroySetInFile(creator *ioutil.FileCreator, prefixedName string) { @@ -438,7 +559,7 @@ func (iMgr *IPSetManager) deleteMemberInFile(creator *ioutil.FileCreator, set *I }, }, } - creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) + creator.AddLine(sectionID, errorHandlers, ipsetDeleteFlag, set.HashedName, member) // delete member } func (iMgr *IPSetManager) addMemberInFile(creator *ioutil.FileCreator, set *IPSet, sectionID, member string) { @@ -472,7 +593,7 @@ func (iMgr *IPSetManager) addMemberInFile(creator *ioutil.FileCreator, set *IPSe }, } } - creator.AddLine(sectionID, errorHandlers, ipsetAddFlag, set.HashedName, member) + creator.AddLine(sectionID, errorHandlers, ipsetAddFlag, set.HashedName, member) // add member } func sectionID(prefix, prefixedName string) string { diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go index 8125ed4cb1..8fe803e55e 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_linux_test.go @@ -8,29 +8,295 @@ import ( "testing" "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/metrics/promutil" dptestutils "github.com/Azure/azure-container-networking/npm/pkg/dataplane/testutils" testutils "github.com/Azure/azure-container-networking/test/utils" "github.com/stretchr/testify/require" ) -const saveResult = "create test-list1 list:set size 8\nadd test-list1 test-list2" +const ( + saveResult = "create test-list1 list:set size 8\nadd test-list1 test-list2" -var iMgrApplyAllCfg = &IPSetManagerCfg{ - IPSetMode: ApplyAllIPSets, - NetworkName: "", -} + resetIPSetsListOutputString = `azure-npm-123456 +azure-npm-987654 +azure-npm-777777` + resetIPSetsNumGreppedSets = 3 +) + +var ( + iMgrApplyAllCfg = &IPSetManagerCfg{ + IPSetMode: ApplyAllIPSets, + NetworkName: "", + } + + resetIPSetsListOutput = []byte(resetIPSetsListOutputString) +) // TODO test that a reconcile list is updated for all the TestFailure UTs // TODO same exact TestFailure UTs for unknown errors -func TestDestroyNPMIPSets(t *testing.T) { - // TODO - calls := []testutils.TestCmd{} +func TestDestroyNPMIPSetsCreatorSuccess(t *testing.T) { + calls := []testutils.TestCmd{fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + creator, numSets, destroyFailureCount := iMgr.fileCreatorForReset(resetIPSetsListOutput) + actualLines := strings.Split(creator.ToString(), "\n") + expectedLines := []string{ + "-F azure-npm-123456", + "-F azure-npm-987654", + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-987654", + "-X azure-npm-777777", + "", + } + dptestutils.AssertEqualLines(t, expectedLines, actualLines) + require.Equal(t, resetIPSetsNumGreppedSets, numSets, "got unexpected num sets") + wasModified, err := creator.RunCommandOnceWithFile("ipset", "restore") + require.False(t, wasModified) + require.NoError(t, err) + require.Equal(t, 0, *destroyFailureCount, "got unexpected failure count") +} - require.NoError(t, iMgr.resetIPSets()) +func TestDestroyNPMIPSetsCreatorErrorHandling(t *testing.T) { + tests := []struct { + name string + call testutils.TestCmd + expectedLines []string + expectedFailureCount int + }{ + { + name: "set doesn't exist on flush", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: The set with the given name does not exist", + ExitCode: 1, + }, + expectedLines: []string{ + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 0, + }, + { + name: "some other error on flush", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: for some other error", + ExitCode: 1, + }, + expectedLines: []string{ + "-F azure-npm-777777", + "-X azure-npm-123456", + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 1, + }, + { + name: "set doesn't exist on destroy", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: The set with the given name does not exist", + ExitCode: 1, + }, + expectedLines: []string{ + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 0, + }, + { + name: "some other error on destroy", + call: testutils.TestCmd{ + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: some other error", + ExitCode: 1, + }, + expectedLines: []string{ + "-X azure-npm-777777", + "", + }, + expectedFailureCount: 1, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + calls := []testutils.TestCmd{tt.call} + ioshim := common.NewMockIOShim(calls) + defer ioshim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + creator, numSets, destroyFailureCount := iMgr.fileCreatorForReset(resetIPSetsListOutput) + require.Equal(t, resetIPSetsNumGreppedSets, numSets, "got unexpected num sets") + wasModified, err := creator.RunCommandOnceWithFile("ipset", "restore") + require.True(t, wasModified) + require.Error(t, err) + actualLines := strings.Split(creator.ToString(), "\n") + dptestutils.AssertEqualLines(t, tt.expectedLines, actualLines) + require.Equal(t, tt.expectedFailureCount, *destroyFailureCount, "got unexpected failure count") + }) + break + } +} + +func TestDestroyNPMIPSets(t *testing.T) { + numSetsToStart := 2 + numEntriesToStart := 5 + + tests := []struct { + name string + calls []testutils.TestCmd + wantErr bool + expectedNumSets int + expectedNumEntries int + }{ + { + name: "success with no results from grep", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully delete sets", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "grep error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, HasStartError: true, PipedToCommand: true, ExitCode: 1}, + {Cmd: []string{"grep", "azure-npm-"}}, + }, + wantErr: true, + expectedNumSets: numSetsToStart, + expectedNumEntries: numEntriesToStart, + }, + { + name: "restore error from max tries", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + {Cmd: ipsetRestoreStringSlice, ExitCode: 1}, + }, + wantErr: true, + expectedNumSets: resetIPSetsNumGreppedSets, + expectedNumEntries: numEntriesToStart, + }, + { + name: "successfully restore, but fail to flush/destroy 1 set since the set doesn't exist when flushing", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: The set with the given name does not exist", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to flush/destroy 1 set due to other flush error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 2: for some other error", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 1, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to destroy 1 set since the set doesn't exist when destroying", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: The set with the given name does not exist", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 0, + expectedNumEntries: 0, + }, + { + name: "successfully restore, but fail to destroy 1 set due to other destroy error", + calls: []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, Stdout: resetIPSetsListOutputString}, + { + Cmd: ipsetRestoreStringSlice, + Stdout: "Error in line 5: for some other error", + ExitCode: 1, + }, + fakeRestoreSuccessCommand, + }, + wantErr: false, + expectedNumSets: 1, + expectedNumEntries: 0, + }, + } + + testSet := "set1" + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + ioshim := common.NewMockIOShim(tt.calls) + defer ioshim.VerifyCalls(t, tt.calls) + iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) + metrics.SetNumIPSets(numSetsToStart) + metrics.ResetIPSetEntries() + for i := 0; i < numEntriesToStart; i++ { + metrics.AddEntryToIPSet(testSet) + } + + err := iMgr.resetIPSets() + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + numSets, err := metrics.GetNumIPSets() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumSets, numSets, "got unexpected prometheus metric for num ipsets") + + numEntries, err := metrics.GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumEntries, numEntries, "got unexpected prometheus metric for num ipset entries") + + numEntriesForSet, err := metrics.GetNumEntriesForIPSet(testSet) + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedNumEntries, numEntriesForSet, "got unexpected prometheus metric for num entries for the test set") + }) + } } func TestApplyIPSetsSuccessWithoutSave(t *testing.T) { @@ -48,7 +314,6 @@ func TestApplyIPSetsSuccessWithoutSave(t *testing.T) { } func TestApplyIPSetsSuccessWithSave(t *testing.T) { - // no sets to add/update, so don't call ipset save calls := []testutils.TestCmd{ {Cmd: ipsetSaveStringSlice, PipedToCommand: true}, {Cmd: []string{"grep", "azure-npm-"}}, @@ -160,7 +425,7 @@ func TestCreateForAllSetTypes(t *testing.T) { require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata})) iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -207,7 +472,7 @@ func TestDestroy(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestNestedLabelList.PrefixName) - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -261,7 +526,7 @@ func TestUpdateWithIdenticalSaveFile(t *testing.T) { require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{TestKVNSList.Metadata}, []*IPSetMetadata{TestKVPodSet.Metadata})) iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) lines := []string{ @@ -323,7 +588,7 @@ func TestUpdateWithRealisticSaveFile(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestNestedLabelList.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestNestedLabelList.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) // adding NSSet and KeyPodSet (should be keeping NSSet and deleting NamedportSet) lines := []string{ @@ -423,7 +688,7 @@ func TestUpdateWithBadSaveFile(t *testing.T) { TestKeyNSList.Metadata, TestNamedportSet.Metadata, cidrSet2.Metadata, nsSet2.Metadata, }) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) actualLines := testAndSortRestoreFileString(t, creator.ToString()) expectedLines := []string{ @@ -475,7 +740,7 @@ func TestFailureOnCreateForNewSet(t *testing.T) { iMgr.DeleteIPSet(TestKeyNSList.PrefixName) // get original creator and run it the first time - creator := iMgr.fileCreator(len(calls), nil) + creator := iMgr.fileCreatorForApply(len(calls), nil) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -532,7 +797,7 @@ func TestFailureOnCreateForSetInKernel(t *testing.T) { iMgr.DeleteIPSet(TestKeyNSList.PrefixName) // get original creator and run it the first time - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -563,12 +828,12 @@ func TestFailureOnAddToListInKernel(t *testing.T) { // - update three lists already in the set, each with a delete and add line. the second list to appear will have the failed add // - create a set and add a member to it errorLineNum := 10 - setAlreadyExistsCommand := testutils.TestCmd{ + memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{memberDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -592,7 +857,7 @@ func TestFailureOnAddToListInKernel(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -620,12 +885,12 @@ func TestFailureOnAddToNewList(t *testing.T) { // - update a set already in the kernel with a delete and add line // - create three lists in the set, each with an add line. the second list to appear will have the failed add errorLineNum := 10 - setAlreadyExistsCommand := testutils.TestCmd{ + memberDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set to be added/deleted/tested as element does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{memberDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -644,7 +909,7 @@ func TestFailureOnAddToNewList(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -675,12 +940,12 @@ func TestFailureOnFlush(t *testing.T) { // - update a set by deleting a member // - create a set with a member errorLineNum := 1 - setAlreadyExistsCommand := testutils.TestCmd{ + setDoesNotExistCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: The set with the given name does not exist", errorLineNum), // this error might happen if the cache is out of date with the kernel ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{setDoesNotExistCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -700,7 +965,7 @@ func TestFailureOnFlush(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -728,12 +993,12 @@ func TestFailureOnDestroy(t *testing.T) { // - update a set by deleting a member // - create a set with a member errorLineNum := 3 - setAlreadyExistsCommand := testutils.TestCmd{ + inUseByKernelCommand := testutils.TestCmd{ Cmd: ipsetRestoreStringSlice, Stdout: fmt.Sprintf("Error in line %d: Set cannot be destroyed: it is in use by a kernel component", errorLineNum), ExitCode: 1, } - calls := []testutils.TestCmd{setAlreadyExistsCommand, fakeRestoreSuccessCommand} + calls := []testutils.TestCmd{inUseByKernelCommand, fakeRestoreSuccessCommand} ioshim := common.NewMockIOShim(calls) defer ioshim.VerifyCalls(t, calls) iMgr := NewIPSetManager(iMgrApplyAllCfg, ioshim) @@ -753,7 +1018,7 @@ func TestFailureOnDestroy(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(len(calls), saveFileBytes) + creator := iMgr.fileCreatorForApply(len(calls), saveFileBytes) originalLines := strings.Split(creator.ToString(), "\n") wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") @@ -790,7 +1055,7 @@ func TestFailureOnLastLine(t *testing.T) { iMgr.CreateIPSets([]*IPSetMetadata{TestCIDRSet.Metadata}) // create so we can delete iMgr.DeleteIPSet(TestCIDRSet.PrefixName) - creator := iMgr.fileCreator(2, nil) + creator := iMgr.fileCreatorForApply(2, nil) wasFileAltered, err := creator.RunCommandOnceWithFile("ipset", "restore") require.Error(t, err, "ipset restore should fail") require.True(t, wasFileAltered, "file should be altered") @@ -803,20 +1068,20 @@ func TestFailureOnLastLine(t *testing.T) { require.False(t, wasFileAltered, "file should not be altered") } -// make sure file goes in order of flushes, destroys, creates, then adds/deletes, -// then sort those sections and return the lines in an array func testAndSortRestoreFileString(t *testing.T, multilineString string) []string { return testAndSortRestoreFileLines(t, strings.Split(multilineString, "\n")) } +// make sure file goes in order of flushes, destroys, creates, then adds/deletes, +// then sort those sections and return the lines in an array func testAndSortRestoreFileLines(t *testing.T, lines []string) []string { require.True(t, lines[len(lines)-1] == "", "restore file must end with blank line") lines = lines[:len(lines)-1] // remove the blank line flushIndices := [2]int{0, len(lines)} - destroyIndices := flushIndices - createIndices := flushIndices - addDeleteIndices := flushIndices + destroyIndices := [2]int{-1, len(lines)} // -1 means the file ended with the previous operation + createIndices := [2]int{-1, len(lines)} + addDeleteIndices := [2]int{-1, len(lines)} k := 0 for k < len(lines) { operation := lines[k][0:2] @@ -853,9 +1118,18 @@ func testAndSortRestoreFileLines(t *testing.T, lines []string) []string { k++ } flushLines := lines[flushIndices[0]:flushIndices[1]] - destroyLines := lines[destroyIndices[0]:destroyIndices[1]] - createLines := lines[createIndices[0]:createIndices[1]] - addDeleteLines := lines[addDeleteIndices[0]:addDeleteIndices[1]] + var destroyLines []string + var createLines []string + var addDeleteLines []string + if destroyIndices[0] != -1 { + destroyLines = lines[destroyIndices[0]:destroyIndices[1]] + } + if createIndices[0] != -1 { + createLines = lines[createIndices[0]:createIndices[1]] + } + if addDeleteIndices[0] != -1 { + addDeleteLines = lines[addDeleteIndices[0]:addDeleteIndices[1]] + } sort.Strings(flushLines) sort.Strings(destroyLines) sort.Strings(createLines) diff --git a/npm/pkg/dataplane/ipsets/testutils_linux.go b/npm/pkg/dataplane/ipsets/testutils_linux.go index 4181f75a17..0b063fb215 100644 --- a/npm/pkg/dataplane/ipsets/testutils_linux.go +++ b/npm/pkg/dataplane/ipsets/testutils_linux.go @@ -25,5 +25,8 @@ func GetApplyIPSetsTestCalls(toAddOrUpdateIPSets, toDeleteIPSets []*IPSetMetadat } func GetResetTestCalls() []testutils.TestCmd { - return []testutils.TestCmd{} + return []testutils.TestCmd{ + {Cmd: []string{"ipset", "list", "--name"}, PipedToCommand: true}, + {Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, // grep didn't find anything + } }