From d141e64de98f4e7eb0493d8f0a631f071b6e6dc1 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Mon, 13 Aug 2012 14:58:34 -0400 Subject: Change IPtablesManager to preserve packet:byte counts. Modified IPtablesManager.apply() method to save/restore chain and rule packet:byte counts by using the '-c' flag with iptables-save and iptables-restore calls. Currently they are zeroed every time we change something in the table. This will allow users to better analyze usage for instances over an extended period of time, for example, for billing purposes. Change all applicable iptables, libvirt and Xen tests to account for the changes made to support the packet:byte counts. This work uncovered two bugs in the existing implementation found during my testing, specifically: 1. Fix IptablesManager to clean-up non-wrapped chains correctly, instead of leaving them in the kernel's table. We now keep a list of chains and rules we need to remove, and double-check in apply() that they are filtered-out. 2. Fix IptablesManager to honor "top=True" iptables rules by only adding non-top rules after we've gone through all the top rules first. Implements first work item of blueprint libvirt-network-usage. Fixes bug 1037127 and bug 1037137. Change-Id: Ia5a11aabbfb45b6c16c8d94757eeaa2041785b60 --- nova/network/linux_net.py | 95 ++++++++++++++++++++++++++++++++++--- nova/tests/test_iptables_network.py | 60 ++++++++++++----------- nova/tests/test_libvirt.py | 38 ++++++++------- nova/tests/test_xenapi.py | 27 ++++++----- nova/tests/xenapi/stubs.py | 10 ++-- 5 files changed, 163 insertions(+), 67 deletions(-) (limited to 'nova') diff --git a/nova/network/linux_net.py b/nova/network/linux_net.py index f0ffa8b86..444e45e97 100644 --- a/nova/network/linux_net.py +++ b/nova/network/linux_net.py @@ -125,7 +125,8 @@ class IptablesRule(object): chain = '%s-%s' % (binary_name, self.chain) else: chain = self.chain - return '-A %s %s' % (chain, self.rule) + # new rules should have a zero [packet: byte] count + return '[0:0] -A %s %s' % (chain, self.rule) class IptablesTable(object): @@ -133,8 +134,10 @@ class IptablesTable(object): def __init__(self): self.rules = [] + self.remove_rules = [] self.chains = set() self.unwrapped_chains = set() + self.remove_chains = set() def add_chain(self, name, wrap=True): """Adds a named chain to the table. @@ -172,7 +175,13 @@ class IptablesTable(object): name) return + # non-wrapped chains and rules need to be dealt with specially, + # so we keep a list of them to be iterated over in apply() + if not wrap: + self.remove_chains.add(name) chain_set.remove(name) + if not wrap: + self.remove_rules += filter(lambda r: r.chain == name, self.rules) self.rules = filter(lambda r: r.chain != name, self.rules) if wrap: @@ -180,6 +189,9 @@ class IptablesTable(object): else: jump_snippet = '-j %s' % (name,) + if not wrap: + self.remove_rules += filter(lambda r: jump_snippet in r.rule, + self.rules) self.rules = filter(lambda r: jump_snippet not in r.rule, self.rules) def add_rule(self, chain, rule, wrap=True, top=False): @@ -216,6 +228,8 @@ class IptablesTable(object): """ try: self.rules.remove(IptablesRule(chain, rule, wrap, top)) + if not wrap: + self.remove_rules.append(IptablesRule(chain, rule, wrap, top)) except ValueError: LOG.warn(_('Tried to remove rule that was not there:' ' %(chain)r %(rule)r %(wrap)r %(top)r'), @@ -342,14 +356,14 @@ class IptablesManager(object): for cmd, tables in s: for table in tables: - current_table, _err = self.execute('%s-save' % (cmd,), + current_table, _err = self.execute('%s-save' % (cmd,), '-c', '-t', '%s' % (table,), run_as_root=True, attempts=5) current_lines = current_table.split('\n') new_filter = self._modify_rules(current_lines, tables[table]) - self.execute('%s-restore' % (cmd,), run_as_root=True, + self.execute('%s-restore' % (cmd,), '-c', run_as_root=True, process_input='\n'.join(new_filter), attempts=5) LOG.debug(_("IPTablesManager.apply completed with success")) @@ -357,7 +371,9 @@ class IptablesManager(object): def _modify_rules(self, current_lines, table, binary=None): unwrapped_chains = table.unwrapped_chains chains = table.chains + remove_chains = table.remove_chains rules = table.rules + remove_rules = table.remove_rules # Remove any trace of our rules new_filter = filter(lambda line: binary_name not in line, @@ -374,15 +390,42 @@ class IptablesManager(object): break our_rules = [] + bot_rules = [] for rule in rules: rule_str = str(rule) if rule.top: # rule.top == True means we want this rule to be at the top. # Further down, we weed out duplicates from the bottom of the # list, so here we remove the dupes ahead of time. - new_filter = filter(lambda s: s.strip() != rule_str.strip(), + + # We don't want to remove an entry if it has non-zero + # [packet:byte] counts and replace it with [0:0], so let's + # go look for a duplicate, and over-ride our table rule if + # found. + + # ignore [packet:byte] counts at beginning of line + if rule_str.startswith('['): + rule_str = rule_str.split(']', 1)[1] + dup_filter = filter(lambda s: rule_str.strip() in s.strip(), new_filter) - our_rules += [rule_str] + + new_filter = filter(lambda s: + rule_str.strip() not in s.strip(), + new_filter) + # if no duplicates, use original rule + if dup_filter: + # grab the last entry, if there is one + dup = dup_filter[-1] + rule_str = str(dup) + else: + rule_str = str(rule) + rule_str.strip() + + our_rules += [rule_str] + else: + bot_rules += [rule_str] + + our_rules += bot_rules new_filter[rules_index:rules_index] = our_rules @@ -395,6 +438,9 @@ class IptablesManager(object): seen_lines = set() def _weed_out_duplicates(line): + # ignore [packet:byte] counts at beginning of lines + if line.startswith('['): + line = line.split(']', 1)[1] line = line.strip() if line in seen_lines: return False @@ -402,11 +448,48 @@ class IptablesManager(object): seen_lines.add(line) return True + def _weed_out_removes(line): + # We need to find exact matches here + if line.startswith(':'): + # it's a chain, for example, ":nova-billing - [0:0]" + # strip off everything except the chain name + line = line.split(':')[1] + line = line.split('- [')[0] + line = line.strip() + for chain in remove_chains: + if chain == line: + remove_chains.remove(chain) + return False + elif line.startswith('['): + # it's a rule + # ignore [packet:byte] counts at beginning of lines + line = line.split(']', 1)[1] + line = line.strip() + for rule in remove_rules: + # ignore [packet:byte] counts at beginning of rules + rule_str = str(rule) + rule_str = rule_str.split(' ', 1)[1] + rule_str = rule_str.strip() + if rule_str == line: + remove_rules.remove(rule) + return False + + # Leave it alone + return True + # We filter duplicates, letting the *last* occurrence take - # precedence. + # precendence. We also filter out anything in the "remove" + # lists. new_filter.reverse() new_filter = filter(_weed_out_duplicates, new_filter) + new_filter = filter(_weed_out_removes, new_filter) new_filter.reverse() + + # flush lists, just in case we didn't find something + remove_chains.clear() + for rule in remove_rules: + remove_rules.remove(rule) + return new_filter diff --git a/nova/tests/test_iptables_network.py b/nova/tests/test_iptables_network.py index 0d7e54723..166e28a5c 100644 --- a/nova/tests/test_iptables_network.py +++ b/nova/tests/test_iptables_network.py @@ -35,21 +35,26 @@ class IptablesManagerTestCase(test.TestCase): ':nova-compute-local - [0:0]', ':nova-compute-OUTPUT - [0:0]', ':nova-filter-top - [0:0]', - '-A FORWARD -j nova-filter-top ', - '-A OUTPUT -j nova-filter-top ', - '-A nova-filter-top -j nova-compute-local ', - '-A INPUT -j nova-compute-INPUT ', - '-A OUTPUT -j nova-compute-OUTPUT ', - '-A FORWARD -j nova-compute-FORWARD ', - '-A INPUT -i virbr0 -p udp -m udp --dport 53 -j ACCEPT ', - '-A INPUT -i virbr0 -p tcp -m tcp --dport 53 -j ACCEPT ', - '-A INPUT -i virbr0 -p udp -m udp --dport 67 -j ACCEPT ', - '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ', - '-A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ', - '-A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', - '-A FORWARD -o virbr0 -j REJECT --reject-with ' + '[0:0] -A FORWARD -j nova-filter-top ', + '[0:0] -A OUTPUT -j nova-filter-top ', + '[0:0] -A nova-filter-top -j nova-compute-local ', + '[0:0] -A INPUT -j nova-compute-INPUT ', + '[0:0] -A OUTPUT -j nova-compute-OUTPUT ', + '[0:0] -A FORWARD -j nova-compute-FORWARD ', + '[0:0] -A INPUT -i virbr0 -p udp -m udp --dport 53 ' + '-j ACCEPT ', + '[0:0] -A INPUT -i virbr0 -p tcp -m tcp --dport 53 ' + '-j ACCEPT ', + '[0:0] -A INPUT -i virbr0 -p udp -m udp --dport 67 ' + '-j ACCEPT ', + '[0:0] -A INPUT -i virbr0 -p tcp -m tcp --dport 67 ' + '-j ACCEPT ', + '[0:0] -A FORWARD -s 192.168.122.0/24 -i virbr0 ' + '-j ACCEPT ', + '[0:0] -A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', + '[0:0] -A FORWARD -o virbr0 -j REJECT --reject-with ' 'icmp-port-unreachable ', - '-A FORWARD -i virbr0 -j REJECT --reject-with ' + '[0:0] -A FORWARD -i virbr0 -j REJECT --reject-with ' 'icmp-port-unreachable ', 'COMMIT', '# Completed on Fri Feb 18 15:17:05 2011'] @@ -66,12 +71,13 @@ class IptablesManagerTestCase(test.TestCase): ':nova-compute-PREROUTING - [0:0]', ':nova-compute-POSTROUTING - [0:0]', ':nova-postrouting-bottom - [0:0]', - '-A PREROUTING -j nova-compute-PREROUTING ', - '-A OUTPUT -j nova-compute-OUTPUT ', - '-A POSTROUTING -j nova-compute-POSTROUTING ', - '-A POSTROUTING -j nova-postrouting-bottom ', - '-A nova-postrouting-bottom -j nova-compute-SNATTING ', - '-A nova-compute-SNATTING -j nova-compute-floating-ip-snat ', + '[0:0] -A PREROUTING -j nova-compute-PREROUTING ', + '[0:0] -A OUTPUT -j nova-compute-OUTPUT ', + '[0:0] -A POSTROUTING -j nova-compute-POSTROUTING ', + '[0:0] -A POSTROUTING -j nova-postrouting-bottom ', + '[0:0] -A nova-postrouting-bottom -j nova-compute-SNATTING ', + '[0:0] -A nova-compute-SNATTING ' + '-j nova-compute-floating-ip-snat ', 'COMMIT', '# Completed on Fri Feb 18 15:17:05 2011'] @@ -85,12 +91,12 @@ class IptablesManagerTestCase(test.TestCase): table = self.manager.ipv4['filter'] table.add_rule('FORWARD', '-s 1.2.3.4/5 -j DROP') new_lines = self.manager._modify_rules(current_lines, table) - self.assertTrue('-A %s-FORWARD ' + self.assertTrue('[0:0] -A %s-FORWARD ' '-s 1.2.3.4/5 -j DROP' % self.binary_name in new_lines) table.remove_rule('FORWARD', '-s 1.2.3.4/5 -j DROP') new_lines = self.manager._modify_rules(current_lines, table) - self.assertTrue('-A %s-FORWARD ' + self.assertTrue('[0:0] -A %s-FORWARD ' '-s 1.2.3.4/5 -j DROP' % self.binary_name \ not in new_lines) @@ -117,7 +123,7 @@ class IptablesManagerTestCase(test.TestCase): last_postrouting_line = '' for line in new_lines: - if line.startswith('-A POSTROUTING'): + if line.startswith('[0:0] -A POSTROUTING'): last_postrouting_line = line self.assertTrue('-j nova-postrouting-bottom' in last_postrouting_line, @@ -125,7 +131,7 @@ class IptablesManagerTestCase(test.TestCase): "nova-postouting-bottom: %s" % last_postrouting_line) for chain in ['POSTROUTING', 'PREROUTING', 'OUTPUT']: - self.assertTrue('-A %s -j %s-%s' % + self.assertTrue('[0:0] -A %s -j %s-%s' % (chain, self.binary_name, chain) in new_lines, "Built-in chain %s not wrapped" % (chain,)) @@ -150,17 +156,17 @@ class IptablesManagerTestCase(test.TestCase): for chain in ['FORWARD', 'OUTPUT']: for line in new_lines: - if line.startswith('-A %s' % chain): + if line.startswith('[0:0] -A %s' % chain): self.assertTrue('-j nova-filter-top' in line, "First %s rule does not " "jump to nova-filter-top" % chain) break - self.assertTrue('-A nova-filter-top ' + self.assertTrue('[0:0] -A nova-filter-top ' '-j %s-local' % self.binary_name in new_lines, "nova-filter-top does not jump to wrapped local chain") for chain in ['INPUT', 'OUTPUT', 'FORWARD']: - self.assertTrue('-A %s -j %s-%s' % + self.assertTrue('[0:0] -A %s -j %s-%s' % (chain, self.binary_name, chain) in new_lines, "Built-in chain %s not wrapped" % (chain,)) diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py index b45780912..c30a6c1b8 100644 --- a/nova/tests/test_libvirt.py +++ b/nova/tests/test_libvirt.py @@ -2787,13 +2787,15 @@ class IptablesFirewallTestCase(test.TestCase): ':FORWARD ACCEPT [0:0]', ':OUTPUT ACCEPT [915599:63811649]', ':nova-block-ipv4 - [0:0]', - '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ', - '-A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED' + '[0:0] -A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ', + '[0:0] -A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED' ',ESTABLISHED -j ACCEPT ', - '-A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ', - '-A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', - '-A FORWARD -o virbr0 -j REJECT --reject-with icmp-port-unreachable ', - '-A FORWARD -i virbr0 -j REJECT --reject-with icmp-port-unreachable ', + '[0:0] -A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ', + '[0:0] -A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', + '[0:0] -A FORWARD -o virbr0 -j REJECT ' + '--reject-with icmp-port-unreachable ', + '[0:0] -A FORWARD -i virbr0 -j REJECT ' + '--reject-with icmp-port-unreachable ', 'COMMIT', '# Completed on Mon Dec 6 11:54:13 2010', ] @@ -2873,18 +2875,18 @@ class IptablesFirewallTestCase(test.TestCase): # self.fw.add_instance(instance_ref) def fake_iptables_execute(*cmd, **kwargs): process_input = kwargs.get('process_input', None) - if cmd == ('ip6tables-save', '-t', 'filter'): + if cmd == ('ip6tables-save', '-c', '-t', 'filter'): return '\n'.join(self.in6_filter_rules), None - if cmd == ('iptables-save', '-t', 'filter'): + if cmd == ('iptables-save', '-c', '-t', 'filter'): return '\n'.join(self.in_filter_rules), None - if cmd == ('iptables-save', '-t', 'nat'): + if cmd == ('iptables-save', '-c', '-t', 'nat'): return '\n'.join(self.in_nat_rules), None - if cmd == ('iptables-restore',): + if cmd == ('iptables-restore', '-c',): lines = process_input.split('\n') if '*filter' in lines: self.out_rules = lines return '', '' - if cmd == ('ip6tables-restore',): + if cmd == ('ip6tables-restore', '-c',): lines = process_input.split('\n') if '*filter' in lines: self.out6_rules = lines @@ -2927,27 +2929,29 @@ class IptablesFirewallTestCase(test.TestCase): self.assertTrue(security_group_chain, "The security group chain wasn't added") - regex = re.compile('-A .* -j ACCEPT -p icmp -s 192.168.11.0/24') + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p icmp ' + '-s 192.168.11.0/24') self.assertTrue(len(filter(regex.match, self.out_rules)) > 0, "ICMP acceptance rule wasn't added") - regex = re.compile('-A .* -j ACCEPT -p icmp -m icmp --icmp-type 8' - ' -s 192.168.11.0/24') + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p icmp -m icmp ' + '--icmp-type 8 -s 192.168.11.0/24') self.assertTrue(len(filter(regex.match, self.out_rules)) > 0, "ICMP Echo Request acceptance rule wasn't added") for ip in network_model.fixed_ips(): if ip['version'] != 4: continue - regex = re.compile('-A .* -j ACCEPT -p tcp -m multiport ' + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p tcp -m multiport ' '--dports 80:81 -s %s' % ip['address']) self.assertTrue(len(filter(regex.match, self.out_rules)) > 0, "TCP port 80/81 acceptance rule wasn't added") - regex = re.compile('-A .* -j ACCEPT -s %s' % ip['address']) + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -s ' + '%s' % ip['address']) self.assertTrue(len(filter(regex.match, self.out_rules)) > 0, "Protocol/port-less acceptance rule wasn't added") - regex = re.compile('-A .* -j ACCEPT -p tcp ' + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p tcp ' '-m multiport --dports 80:81 -s 192.168.10.0/24') self.assertTrue(len(filter(regex.match, self.out_rules)) > 0, "TCP port 80/81 acceptance rule wasn't added") diff --git a/nova/tests/test_xenapi.py b/nova/tests/test_xenapi.py index 04049ae70..c84a924e3 100644 --- a/nova/tests/test_xenapi.py +++ b/nova/tests/test_xenapi.py @@ -1499,13 +1499,15 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase): ':FORWARD ACCEPT [0:0]', ':OUTPUT ACCEPT [915599:63811649]', ':nova-block-ipv4 - [0:0]', - '-A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ', - '-A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED' + '[0:0] -A INPUT -i virbr0 -p tcp -m tcp --dport 67 -j ACCEPT ', + '[0:0] -A FORWARD -d 192.168.122.0/24 -o virbr0 -m state --state RELATED' ',ESTABLISHED -j ACCEPT ', - '-A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ', - '-A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', - '-A FORWARD -o virbr0 -j REJECT --reject-with icmp-port-unreachable ', - '-A FORWARD -i virbr0 -j REJECT --reject-with icmp-port-unreachable ', + '[0:0] -A FORWARD -s 192.168.122.0/24 -i virbr0 -j ACCEPT ', + '[0:0] -A FORWARD -i virbr0 -o virbr0 -j ACCEPT ', + '[0:0] -A FORWARD -o virbr0 -j REJECT ' + '--reject-with icmp-port-unreachable ', + '[0:0] -A FORWARD -i virbr0 -j REJECT ' + '--reject-with icmp-port-unreachable ', 'COMMIT', '# Completed on Mon Dec 6 11:54:13 2010', ] @@ -1598,16 +1600,17 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase): self.assertTrue(security_group_chain, "The security group chain wasn't added") - regex = re.compile('-A .* -j ACCEPT -p icmp -s 192.168.11.0/24') + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p icmp' + ' -s 192.168.11.0/24') self.assertTrue(len(filter(regex.match, self._out_rules)) > 0, "ICMP acceptance rule wasn't added") - regex = re.compile('-A .* -j ACCEPT -p icmp -m icmp --icmp-type 8' - ' -s 192.168.11.0/24') + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p icmp -m icmp' + ' --icmp-type 8 -s 192.168.11.0/24') self.assertTrue(len(filter(regex.match, self._out_rules)) > 0, "ICMP Echo Request acceptance rule wasn't added") - regex = re.compile('-A .* -j ACCEPT -p tcp --dport 80:81' + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p tcp --dport 80:81' ' -s 192.168.10.0/24') self.assertTrue(len(filter(regex.match, self._out_rules)) > 0, "TCP port 80/81 acceptance rule wasn't added") @@ -1652,7 +1655,7 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase): for ip in network_model.fixed_ips(): if ip['version'] != 4: continue - regex = re.compile('-A .* -j ACCEPT -p tcp' + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p tcp' ' --dport 80:81 -s %s' % ip['address']) self.assertTrue(len(filter(regex.match, self._out_rules)) > 0, "TCP port 80/81 acceptance rule wasn't added") @@ -1717,7 +1720,7 @@ class XenAPIDom0IptablesFirewallTestCase(stubs.XenAPITestBase): 'cidr': '192.168.99.0/24'}) #validate the extra rule self.fw.refresh_security_group_rules(secgroup) - regex = re.compile('-A .* -j ACCEPT -p udp --dport 200:299' + regex = re.compile('\[0\:0\] -A .* -j ACCEPT -p udp --dport 200:299' ' -s 192.168.99.0/24') self.assertTrue(len(filter(regex.match, self._out_rules)) > 0, "Rules were not updated properly." diff --git a/nova/tests/xenapi/stubs.py b/nova/tests/xenapi/stubs.py index d9c1d510d..bb31a5327 100644 --- a/nova/tests/xenapi/stubs.py +++ b/nova/tests/xenapi/stubs.py @@ -244,19 +244,19 @@ class FakeSessionForFirewallTests(FakeSessionForVMTests): else: output = '' process_input = args.get('process_input', None) - if cmd == ['ip6tables-save', '-t', 'filter']: + if cmd == ['ip6tables-save', '-c', '-t', 'filter']: output = '\n'.join(self._in6_filter_rules) - if cmd == ['iptables-save', '-t', 'filter']: + if cmd == ['iptables-save', '-c', '-t', 'filter']: output = '\n'.join(self._in_filter_rules) - if cmd == ['iptables-save', '-t', 'nat']: + if cmd == ['iptables-save', '-c', '-t', 'nat']: output = '\n'.join(self._in_nat_rules) - if cmd == ['iptables-restore', ]: + if cmd == ['iptables-restore', '-c', ]: lines = process_input.split('\n') if '*filter' in lines: if self._test_case is not None: self._test_case._out_rules = lines output = '\n'.join(lines) - if cmd == ['ip6tables-restore', ]: + if cmd == ['ip6tables-restore', '-c', ]: lines = process_input.split('\n') if '*filter' in lines: output = '\n'.join(lines) -- cgit