Skip to content

Commit

Permalink
Change IPtablesManager to preserve packet:byte counts.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Brian Haley committed Aug 15, 2012
1 parent 40a3454 commit d141e64
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 67 deletions.
95 changes: 89 additions & 6 deletions nova/network/linux_net.py
View file
Expand Up @@ -125,16 +125,19 @@ def __str__(self):
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):
"""An iptables table."""

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.
Expand Down Expand Up @@ -172,14 +175,23 @@ def remove_chain(self, name, wrap=True):
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:
jump_snippet = '-j %s-%s' % (binary_name, name)
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):
Expand Down Expand Up @@ -216,6 +228,8 @@ def remove_rule(self, chain, rule, wrap=True, top=False):
"""
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'),
Expand Down Expand Up @@ -342,22 +356,24 @@ def _apply(self):

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

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,
Expand All @@ -374,15 +390,42 @@ def _modify_rules(self, current_lines, table, binary=None):
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

Expand All @@ -395,18 +438,58 @@ def _modify_rules(self, current_lines, table, binary=None):
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
else:
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


Expand Down
60 changes: 33 additions & 27 deletions nova/tests/test_iptables_network.py
View file
Expand Up @@ -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']
Expand All @@ -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']

Expand All @@ -85,12 +91,12 @@ def test_filter_rules_are_wrapped(self):
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)

Expand All @@ -117,15 +123,15 @@ def test_nat_rules(self):
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,
"Last POSTROUTING rule does not jump to "
"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,))

Expand All @@ -150,17 +156,17 @@ def test_filter_rules(self):

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,))
38 changes: 21 additions & 17 deletions nova/tests/test_libvirt.py
View file
Expand Up @@ -2787,13 +2787,15 @@ def nwfilterDefineXML(*args, **kwargs):
':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',
]
Expand Down Expand Up @@ -2873,18 +2875,18 @@ def test_static_filters(self):
# 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
Expand Down Expand Up @@ -2927,27 +2929,29 @@ def fake_iptables_execute(*cmd, **kwargs):
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")
Expand Down

0 comments on commit d141e64

Please sign in to comment.