diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index ddca757425..a6740f832f 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -5,7 +5,6 @@ package ipsm import ( "fmt" - "os" "os/exec" "regexp" "strings" @@ -539,53 +538,6 @@ func (ipsMgr *IpsetManager) Run(entry *ipsEntry) (int, error) { return 0, nil } -// Save saves ipset to file. -func (ipsMgr *IpsetManager) Save(configFile string) error { - if len(configFile) == 0 { - configFile = util.IpsetConfigFile - } - - cmd := ipsMgr.exec.Command(util.Ipset, util.IpsetSaveFlag, util.IpsetFileFlag, configFile) - output, err := cmd.CombinedOutput() - if err != nil { - metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to save ipset: [%s] Stderr: [%v, %s]", cmd, err, strings.TrimSuffix(string(output), "\n")) - return err - } - cmd.Wait() - - return nil -} - -// Restore restores ipset from file. -func (ipsMgr *IpsetManager) Restore(configFile string) error { - if len(configFile) == 0 { - configFile = util.IpsetConfigFile - } - - f, err := os.Stat(configFile) - if err != nil { - metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to get file %s stat from ipsm.Restore", configFile) - return err - } - - if f.Size() == 0 { - if err := ipsMgr.Destroy(); err != nil { - return err - } - } - - cmd := ipsMgr.exec.Command(util.Ipset, util.IpsetRestoreFlag, util.IpsetFileFlag, configFile) - output, err := cmd.CombinedOutput() - if err != nil { - metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to to restore ipset from file: [%s] Stderr: [%v, %s]", cmd, err, strings.TrimSuffix(string(output), "\n")) - return err - } - - //TODO based on the set name and number of entries in the config file, update IPSetInventory - - return nil -} - // DestroyNpmIpsets destroys only ipsets created by NPM func (ipsMgr *IpsetManager) DestroyNpmIpsets() error { cmdName := util.Ipset diff --git a/npm/ipsm/ipsm_test.go b/npm/ipsm/ipsm_test.go index dd8a3c8a97..282fdad603 100644 --- a/npm/ipsm/ipsm_test.go +++ b/npm/ipsm/ipsm_test.go @@ -4,9 +4,7 @@ package ipsm import ( "fmt" - "io/ioutil" "os" - "path/filepath" "testing" "github.com/Azure/azure-container-networking/npm/metrics" @@ -16,38 +14,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestSave(t *testing.T) { - var calls = []testutils.TestCmd{ - {Cmd: []string{"ipset", "save", "-file", "ipset.conf"}}, - } - - fexec := testutils.GetFakeExecWithScripts(calls) - ipsMgr := NewIpsetManager(fexec) - defer testutils.VerifyCalls(t, fexec, calls) - err := ipsMgr.Save("ipset.conf") - require.NoError(t, err) -} - -func TestRestore(t *testing.T) { - // create temporary ipset config file to use - tmpFile, err := ioutil.TempFile(os.TempDir(), filepath.Base(util.IpsetTestConfigFile)) - require.NoError(t, err) - defer os.Remove(tmpFile.Name()) - - var calls = []testutils.TestCmd{ - {Cmd: []string{"ipset", "-F", "-exist"}}, - {Cmd: []string{"ipset", "-X", "-exist"}}, - {Cmd: []string{"ipset", "restore", "-file", tmpFile.Name()}}, - } - - fexec := testutils.GetFakeExecWithScripts(calls) - ipsMgr := NewIpsetManager(fexec) - defer testutils.VerifyCalls(t, fexec, calls) - - err = ipsMgr.Restore(tmpFile.Name()) - require.NoError(t, err) -} - func TestCreateList(t *testing.T) { var calls = []testutils.TestCmd{ {Cmd: []string{"ipset", "-N", "-exist", util.GetHashedName("test-list"), "setlist"}}, @@ -228,7 +194,7 @@ func TestCreateSet(t *testing.T) { if err := ipsMgr.CreateSet(testSet3Name, spec); err != nil { t.Errorf("TestCreateSet failed @ ipsMgr.CreateSet when creating port set") } - + err = ipsMgr.AddToSet(testSet3Name, fmt.Sprintf("%s,%s%d", "1.1.1.1", "tcp", 8080), util.IpsetIPPortHashFlag, "0") require.Error(t, err) @@ -478,23 +444,26 @@ func TestDeleteFromSetWithPodCache(t *testing.T) { } } -func TestClean(t *testing.T) { - var calls = []testutils.TestCmd{ - {Cmd: []string{"ipset", "save", "-file", "/var/log/ipset-test.conf"}}, - } - - fexec := testutils.GetFakeExecWithScripts(calls) - ipsMgr := NewIpsetManager(fexec) - defer testutils.VerifyCalls(t, fexec, calls) - - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestClean failed @ ipsMgr.Save") - } - - if err := ipsMgr.Clean(); err != nil { - t.Errorf("TestClean failed @ ipsMgr.Clean") - } -} +// (TODO): it looks this UT is not valid to test Clean function It tests "ipset save". +// I am not sure when Clean function is used and how Clean function changes. +// When someone wants to use Clean function, please update this UT function properly. +// func TestClean(t *testing.T) { +// var calls = []testutils.TestCmd{ +// {Cmd: []string{"ipset", "save", "-file", "/var/log/ipset-test.conf"}}, +// } + +// fexec := testutils.GetFakeExecWithScripts(calls) +// ipsMgr := NewIpsetManager(fexec) +// defer testutils.VerifyCalls(t, fexec, calls) + +// if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { +// t.Errorf("TestClean failed @ ipsMgr.Save") +// } + +// if err := ipsMgr.Clean(); err != nil { +// t.Errorf("TestClean failed @ ipsMgr.Clean") +// } +// } func TestDestroy(t *testing.T) { setName := "test-destroy" diff --git a/npm/iptm/iptm.go b/npm/iptm/iptm.go index 1d9665bd08..0224569baa 100644 --- a/npm/iptm/iptm.go +++ b/npm/iptm/iptm.go @@ -447,94 +447,6 @@ func (iptMgr *IptablesManager) Run(entry *IptEntry) (int, error) { return 0, nil } -// Save saves current iptables configuration to /var/log/iptables.conf -func (iptMgr *IptablesManager) Save(configFile string) error { - if len(configFile) == 0 { - configFile = util.IptablesConfigFile - } - - log.Printf("Saving iptables...") - - err := iptMgr.io.lockIptables() - if err != nil { - return err - } - - defer func() { - er := iptMgr.io.unlockIptables() - if er != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to unlock iptables with err %v", er) - } - }() - - // create the config file for writing - f, err := iptMgr.io.createConfigFile(configFile) - if err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to open file: %s.", configFile) - return err - } - defer func() { - er := iptMgr.io.closeConfigFile() - if er != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to close file: %s with err %v", configFile, er) - } - }() - - cmd := iptMgr.exec.Command(util.IptablesSave) - cmd.SetStdout(f) - output, err := cmd.CombinedOutput() - if err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to run iptables-save: err %v, output %v", err, output) - return err - } - - return nil -} - -// Restore restores iptables configuration from /var/log/iptables.conf -func (iptMgr *IptablesManager) Restore(configFile string) error { - if len(configFile) == 0 { - configFile = util.IptablesConfigFile - } - - log.Printf("Restoring iptables...") - - err := iptMgr.io.lockIptables() - if err != nil { - return err - } - - defer func() { - er := iptMgr.io.unlockIptables() - if er != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to unlock iptables with err %v", er) - } - }() - - // open the config file for reading - f, err := iptMgr.io.openConfigFile(configFile) - if err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to open file: %s with err %v", configFile, err) - return err - } - - defer func() { - if er := iptMgr.io.closeConfigFile(); err != nil { - log.Printf("Failed to close config file with err %v", er) - } - }() - - cmd := iptMgr.exec.Command(util.IptablesRestore) - cmd.SetStdin(f) - output, err := cmd.CombinedOutput() - if err := cmd.Start(); err != nil { - metrics.SendErrorLogAndMetric(util.IptmID, "Error: failed to run iptables-restore with err: %v, output: %v", output, err) - return err - } - - return nil -} - // TO-DO :- Use iptables-restore to update iptables. // func SyncIptables(entries []*IptEntry) error { // // Ensure main chains and rules are installed. diff --git a/npm/iptm/iptm_test.go b/npm/iptm/iptm_test.go index 4b7c819b96..a834b4f5c2 100644 --- a/npm/iptm/iptm_test.go +++ b/npm/iptm/iptm_test.go @@ -89,44 +89,6 @@ var ( } ) -func TestSave(t *testing.T) { - saveoutput := `# Generated by iptables-save v1.8.4 on Fri Jun 18 11:14:36 2021 - *security - :INPUT ACCEPT [6619221:10395032991] - :FORWARD ACCEPT [0:0] - :OUTPUT ACCEPT [6827371:1204418063] - COMMIT - # Completed on Fri Jun 18 11:14:36 2021 - # Generated by iptables-save v1.8.4 on Fri Jun 18 11:14:36 2021 - *raw - :PREROUTING ACCEPT [6626479:10396460496]` - - var calls = []testutils.TestCmd{ - {Cmd: []string{"iptables-save"}, ExitCode: 0, Stdout: saveoutput}, - } - - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) - - if err := iptMgr.Save(testFileName); err != nil { - t.Errorf("TestSave failed @ iptMgr.Save") - } -} - -func TestRestore(t *testing.T) { - var calls = []testutils.TestCmd{ - {Cmd: []string{"iptables-restore"}}, - } - - fexec := testutils.GetFakeExecWithScripts(calls) - defer testutils.VerifyCalls(t, fexec, calls) - iptMgr := NewIptablesManager(fexec, NewFakeIptOperationShim()) - - err := iptMgr.Restore(testFileName) - require.NoError(t, err) -} - func TestInitNpmChains(t *testing.T) { var calls = initCalls diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index de63643858..cff8b6aa70 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -75,22 +75,6 @@ func (f *nameSpaceFixture) newNsController(stopCh chan struct{}) { //f.kubeInformer.Start() } -func (f *nameSpaceFixture) ipSetSave(ipsetConfigFile string) { - // call /sbin/ipset save -file /var/log/ipset-test.conf - f.t.Logf("Start storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Save(ipsetConfigFile); err != nil { - f.t.Errorf("TestAddPod failed @ ipsMgr.Save") - } -} - -func (f *nameSpaceFixture) ipSetRestore(ipsetConfigFile string) { - // call /sbin/ipset restore -file /var/log/ipset-test.conf - f.t.Logf("Start re-storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Restore(ipsetConfigFile); err != nil { - f.t.Errorf("TestAddPod failed @ ipsMgr.Restore") - } -} - func newNameSpace(name, rv string, labels map[string]string) *corev1.Namespace { return &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ @@ -162,8 +146,6 @@ func TestNewNs(t *testing.T) { func TestAddNamespace(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) nsObj := newNameSpace( "test-namespace", @@ -194,8 +176,6 @@ func TestAddNamespace(t *testing.T) { func TestUpdateNamespace(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) oldNsObj := newNameSpace( "test-namespace", @@ -240,8 +220,6 @@ func TestUpdateNamespace(t *testing.T) { func TestAddNamespaceLabel(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) oldNsObj := newNameSpace( "test-namespace", @@ -286,8 +264,6 @@ func TestAddNamespaceLabel(t *testing.T) { func TestAddNamespaceLabelSameRv(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) oldNsObj := newNameSpace( "test-namespace", @@ -333,8 +309,6 @@ func TestAddNamespaceLabelSameRv(t *testing.T) { func TestDeleteandUpdateNamespaceLabel(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) oldNsObj := newNameSpace( "test-namespace", @@ -385,8 +359,6 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { func TestNewNameSpaceUpdate(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) oldNsObj := newNameSpace( "test-namespace", @@ -436,8 +408,6 @@ func TestNewNameSpaceUpdate(t *testing.T) { func TestDeleteNamespace(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) nsObj := newNameSpace( "test-namespace", @@ -467,8 +437,6 @@ func TestDeleteNamespace(t *testing.T) { func TestDeleteNamespaceWithTombstone(t *testing.T) { fexec := exec.New() f := newNsFixture(t, fexec) - f.ipSetSave(util.IpsetTestConfigFile) - defer f.ipSetRestore(util.IpsetTestConfigFile) stopCh := make(chan struct{}) defer close(stopCh) f.newNsController(stopCh) diff --git a/npm/networkPolicyController_test.go b/npm/networkPolicyController_test.go index c4b2cafad5..211b7e2b54 100644 --- a/npm/networkPolicyController_test.go +++ b/npm/networkPolicyController_test.go @@ -83,33 +83,6 @@ func (f *netPolFixture) newNetPolController(stopCh chan struct{}) { //f.kubeInformer.Start(stopCh) } -func (f *netPolFixture) saveIpTables(iptablesConfigFile string) { - if err := f.iptMgr.Save(iptablesConfigFile); err != nil { - f.t.Errorf("Failed to save iptables rules") - } -} - -func (f *netPolFixture) restoreIpTables(iptablesConfigFile string) { - if err := f.iptMgr.Restore(iptablesConfigFile); err != nil { - f.t.Errorf("Failed to restore iptables rules") - } -} - -func (f *netPolFixture) saveIpSet(ipsetConfigFile string) { - // call /sbin/ipset save -file /var/log/ipset-test.conf - f.t.Logf("Start storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Save(ipsetConfigFile); err != nil { - f.t.Errorf("Failed to save ipsets") - } -} -func (f *netPolFixture) restoreIpSet(ipsetConfigFile string) { - // call /sbin/ipset restore -file /var/log/ipset-test.conf - f.t.Logf("Start re-storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Restore(ipsetConfigFile); err != nil { - f.t.Errorf("failed to restore ipsets") - } -} - // (TODO): make createNetPol flexible func createNetPol() *networkingv1.NetworkPolicy { tcp := corev1.ProtocolTCP diff --git a/npm/npm.go b/npm/npm.go index 8a33e91411..dc43e74e60 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -31,12 +31,8 @@ import ( var aiMetadata string const ( - restoreRetryWaitTimeInSeconds = 5 - restoreMaxRetries = 10 - backupWaitTimeInSeconds = 60 - telemetryRetryTimeInSeconds = 60 - heartbeatIntervalInMinutes = 30 - reconcileChainTimeInMinutes = 5 + heartbeatIntervalInMinutes = 30 + reconcileChainTimeInMinutes = 5 // TODO: consider increasing thread number later when logics are correct threadness = 1 ) @@ -141,40 +137,6 @@ func (npMgr *NetworkPolicyManager) SendClusterMetrics() { } } -// restore restores iptables from backup file -func (npMgr *NetworkPolicyManager) restore() { - iptMgr := iptm.NewIptablesManager(npMgr.Exec, iptm.NewIptOperationShim()) - var err error - for i := 0; i < restoreMaxRetries; i++ { - if err = iptMgr.Restore(util.IptablesConfigFile); err == nil { - return - } - - time.Sleep(restoreRetryWaitTimeInSeconds * time.Second) - } - - metrics.SendErrorLogAndMetric(util.NpmID, "Error: timeout restoring Azure-NPM states") - panic(err.Error) -} - -// backup takes snapshots of iptables filter table and saves it periodically. -func (npMgr *NetworkPolicyManager) backup(stopCh <-chan struct{}) { - iptMgr := iptm.NewIptablesManager(npMgr.Exec, iptm.NewIptOperationShim()) - ticker := time.NewTicker(time.Second * time.Duration(backupWaitTimeInSeconds)) - defer ticker.Stop() - - for { - select { - case <-stopCh: - return - case <-ticker.C: - if err := iptMgr.Save(util.IptablesConfigFile); err != nil { - metrics.SendErrorLogAndMetric(util.NpmID, "Error: failed to back up Azure-NPM states %s", err.Error()) - } - } - } -} - // Start starts shared informers and waits for the shared informer cache to sync. func (npMgr *NetworkPolicyManager) Start(stopCh <-chan struct{}) error { // Starts all informers manufactured by npMgr's informerFactory. @@ -201,7 +163,6 @@ func (npMgr *NetworkPolicyManager) Start(stopCh <-chan struct{}) error { go npMgr.nameSpaceController.Run(threadness, stopCh) go npMgr.netPolController.Run(threadness, stopCh) go npMgr.reconcileChains(stopCh) - go npMgr.backup(stopCh) return nil } diff --git a/npm/podController_test.go b/npm/podController_test.go index 1d2f6acb50..5ca2d305c1 100644 --- a/npm/podController_test.go +++ b/npm/podController_test.go @@ -72,21 +72,6 @@ func (f *podFixture) newPodController(stopCh chan struct{}) { //f.kubeInformer.Start(stopCh) } -func (f *podFixture) ipSetSave(ipsetConfigFile string) { - // call /sbin/ipset save -file /var/log/ipset-test.conf - f.t.Logf("Start storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Save(ipsetConfigFile); err != nil { - f.t.Errorf("TestAddPod failed @ ipsMgr.Save") - } -} -func (f *podFixture) ipSetRestore(ipsetConfigFile string) { - // call /sbin/ipset restore -file /var/log/ipset-test.conf - f.t.Logf("Start re-storing ipset to %s", ipsetConfigFile) - if err := f.ipsMgr.Restore(ipsetConfigFile); err != nil { - f.t.Errorf("TestAddPod failed @ ipsMgr.Restore") - } -} - func createPod(name, ns, rv, podIP string, labels map[string]string, isHostNewtwork bool, podPhase corev1.PodPhase) *corev1.Pod { return &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{