| Message ID | 20181018183307.1552-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [iptables] iptables-test: add -N option to exercise netns removal path | expand |
Hi, On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: [...] > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > command = IPTABLES_SAVE > elif splitted[0] == IP6TABLES: > command = IP6TABLES_SAVE > + > + if netns: > + path = "/sbin/ip" > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > + else: > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > + In netns case, doesn't this lead to calling xtables-*-multi from $PATH instead of the local one we want to test? Cheers, Phil
On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote: > Hi, > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: > [...] > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > > command = IPTABLES_SAVE > > elif splitted[0] == IP6TABLES: > > command = IP6TABLES_SAVE > > + > > + if netns: > > + path = "/sbin/ip" > > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > > + else: > > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > > + > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH > instead of the local one we want to test? Hm, right, will fix this.
On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote: > > Hi, > > > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: > > [...] > > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > > > command = IPTABLES_SAVE > > > elif splitted[0] == IP6TABLES: > > > command = IP6TABLES_SAVE > > > + > > > + if netns: > > > + path = "/sbin/ip" > > > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > > > + else: > > > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > > > + > > > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH > > instead of the local one we want to test? > > Hm, right, will fix this. I had another look: In main(), PATH is extended to include $PWD/iptables as first component. So actually this shouldn't matter, but maybe better to have it explicit. Cheers, Phil
On Fri, Oct 19, 2018 at 03:38:58PM +0200, Phil Sutter wrote: > On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote: > > On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote: > > > Hi, > > > > > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: > > > [...] > > > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > > > > command = IPTABLES_SAVE > > > > elif splitted[0] == IP6TABLES: > > > > command = IP6TABLES_SAVE > > > > + > > > > + if netns: > > > > + path = "/sbin/ip" > > > > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > > > > + else: > > > > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > > > > + > > > > > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH > > > instead of the local one we want to test? > > > > Hm, right, will fix this. > > I had another look: In main(), PATH is extended to include $PWD/iptables > as first component. So actually this shouldn't matter, but maybe better > to have it explicit. You mean, we could remove lines that are updating PATH and have them explicit everywhere, right? If so, that's fine with it. I can have a look in a follow up patch, or may this affect this patch in some way I'm overlooking? Thanks.
Hi, On Sat, Oct 20, 2018 at 11:21:42AM +0200, Pablo Neira Ayuso wrote: > On Fri, Oct 19, 2018 at 03:38:58PM +0200, Phil Sutter wrote: > > On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote: > > > On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote: > > > > Hi, > > > > > > > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: > > > > [...] > > > > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > > > > > command = IPTABLES_SAVE > > > > > elif splitted[0] == IP6TABLES: > > > > > command = IP6TABLES_SAVE > > > > > + > > > > > + if netns: > > > > > + path = "/sbin/ip" > > > > > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > > > > > + else: > > > > > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > > > > > + > > > > > > > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH > > > > instead of the local one we want to test? > > > > > > Hm, right, will fix this. > > > > I had another look: In main(), PATH is extended to include $PWD/iptables > > as first component. So actually this shouldn't matter, but maybe better > > to have it explicit. > > You mean, we could remove lines that are updating PATH and have them > explicit everywhere, right? If so, that's fine with it. Yes, that's what I meant. > I can have a look in a follow up patch, or may this affect this patch > in some way I'm overlooking? No, no secret insights I didn't tell you about. :D Thanks, Phil
On Sat, Oct 20, 2018 at 11:39:17AM +0200, Phil Sutter wrote: > Hi, > > On Sat, Oct 20, 2018 at 11:21:42AM +0200, Pablo Neira Ayuso wrote: > > On Fri, Oct 19, 2018 at 03:38:58PM +0200, Phil Sutter wrote: > > > On Fri, Oct 19, 2018 at 11:55:07AM +0200, Pablo Neira Ayuso wrote: > > > > On Fri, Oct 19, 2018 at 11:04:42AM +0200, Phil Sutter wrote: > > > > > Hi, > > > > > > > > > > On Thu, Oct 18, 2018 at 08:33:07PM +0200, Pablo Neira Ayuso wrote: > > > > > [...] > > > > > > @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): > > > > > > command = IPTABLES_SAVE > > > > > > elif splitted[0] == IP6TABLES: > > > > > > command = IP6TABLES_SAVE > > > > > > + > > > > > > + if netns: > > > > > > + path = "/sbin/ip" > > > > > > + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command > > > > > > + else: > > > > > > + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE > > > > > > + > > > > > > > > > > In netns case, doesn't this lead to calling xtables-*-multi from $PATH > > > > > instead of the local one we want to test? > > > > > > > > Hm, right, will fix this. > > > > > > I had another look: In main(), PATH is extended to include $PWD/iptables > > > as first component. So actually this shouldn't matter, but maybe better > > > to have it explicit. > > > > You mean, we could remove lines that are updating PATH and have them > > explicit everywhere, right? If so, that's fine with it. > > Yes, that's what I meant. OK, will send a follow up patch to remove the PATH variable. Thanks!
diff --git a/iptables-test.py b/iptables-test.py index 9bfb8086aa0a..11b6c05a2b91 100755 --- a/iptables-test.py +++ b/iptables-test.py @@ -61,7 +61,7 @@ def delete_rule(iptables, rule, filename, lineno): return 0 -def run_test(iptables, rule, rule_save, res, filename, lineno): +def run_test(iptables, rule, rule_save, res, filename, lineno, netns): ''' Executes an unit test. Returns the output of delete_rule(). @@ -76,6 +76,9 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): ret = 0 cmd = iptables + " -A " + rule + if netns: + cmd = "ip netns exec ____vm-iptable-test " + EXECUTEABLE + " " + cmd + ret = execute_cmd(cmd, filename, lineno) # @@ -108,8 +111,15 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): command = IPTABLES_SAVE elif splitted[0] == IP6TABLES: command = IP6TABLES_SAVE + + if netns: + path = "/sbin/ip" + command = "netns exec ____vm-iptable-test " + EXECUTEABLE + " " + command + else: + path = os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE + args = splitted[1:] - proc = subprocess.Popen((os.path.abspath(os.path.curdir) + "/iptables/" + EXECUTEABLE, command), + proc = subprocess.Popen(path + " " + command, shell=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE) out, err = proc.communicate() @@ -131,8 +141,17 @@ def run_test(iptables, rule, rule_save, res, filename, lineno): delete_rule(iptables, rule, filename, lineno) return -1 + # Test "ip netns del NETNS" path with rules in place + if netns: + return 0 + return delete_rule(iptables, rule, filename, lineno) +def run_test_netns(iptables, rule, rule_save, res, filename, lineno): + execute_cmd("ip netns add ____vm-iptable-test", filename, lineno) + ret = run_test(iptables, rule, rule_save, res, filename, lineno, True) + execute_cmd("ip netns del ____vm-iptable-test", filename, lineno) + return ret def execute_cmd(cmd, filename, lineno): ''' @@ -159,7 +178,7 @@ def execute_cmd(cmd, filename, lineno): return ret -def run_test_file(filename): +def run_test_file(filename, netns): ''' Runs a test file @@ -227,12 +246,20 @@ def run_test_file(filename): res = item[2].rstrip() - ret = run_test(iptables, rule, rule_save, - res, filename, lineno + 1) - if ret < 0: - test_passed = False - total_test_passed = False - break + if netns: + ret = run_test_netns(iptables, rule, rule_save, + res, filename, lineno + 1) + if ret < 0: + test_passed = False + total_test_passed = False + break + else: + ret = run_test(iptables, rule, rule_save, + res, filename, lineno + 1, False) + if ret < 0: + test_passed = False + total_test_passed = False + break if test_passed: passed += 1 @@ -275,6 +302,8 @@ def main(): help='Check for missing tests') parser.add_argument('-n', '--nftables', action='store_true', help='Test iptables-over-nftables') + parser.add_argument('-N', '--netns', action='store_true', + help='Test netnamespace path') args = parser.parse_args() # @@ -289,6 +318,11 @@ def main(): if args.nftables: EXECUTEABLE = "xtables-nft-multi" + if args.netns: + netns = True + else: + netns = False + if os.getuid() != 0: print "You need to be root to run this, sorry" return @@ -313,7 +347,7 @@ def main(): if args.filename: file_list = [args.filename] for filename in file_list: - file_tests, file_passed = run_test_file(filename) + file_tests, file_passed = run_test_file(filename, netns) if file_tests: tests += file_tests passed += file_passed
We are getting bug reports lately from the netns path, add a new option to exercise this path. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- This is crashing the kernel in a few spots, will retest with recent fixes to see if we are address all existing problems. iptables-test.py | 54 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 44 insertions(+), 10 deletions(-)