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
This commit is contained in:
Brian Haley 2012-08-13 14:58:34 -04:00
parent 40a3454719
commit d141e64de9
5 changed files with 163 additions and 67 deletions

View File

@ -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

View File

@ -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,))

View File

@ -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")

View File

@ -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."

View File

@ -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)