Skip to content

[NPM] Remove unused codes to save and restore ipset and iptables #952

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 4 commits into from
Aug 3, 2021
Merged

[NPM] Remove unused codes to save and restore ipset and iptables #952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4810d63
Fix save and restore function to work properly with real exec
JungukCho Jul 23, 2021
3304779
Delete unused save and restore codes
JungukCho Jul 23, 2021
28bb016
Clean-up deadcodes (i.e., save and restore) for ipset
JungukCho Jul 23, 2021
a0d173e
Remove an unused variable
JungukCho Aug 2, 2021
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
48 changes: 0 additions & 48 deletions npm/ipsm/ipsm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package ipsm

import (
"fmt"
"os"
"os/exec"
"regexp"
"strings"
Expand Down Expand Up @@ -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
Expand Down
73 changes: 21 additions & 52 deletions npm/ipsm/ipsm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ package ipsm

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/Azure/azure-container-networking/npm/metrics"
Expand All @@ -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"}},
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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"
Expand Down
88 changes: 0 additions & 88 deletions npm/iptm/iptm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
38 changes: 0 additions & 38 deletions npm/iptm/iptm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
32 changes: 0 additions & 32 deletions npm/nameSpaceController_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand Down
Loading