Add comments to iptables rules to help debugging

Adds comments to some of the iptables rules generated
by neutron to assist with debugging.

DocImpact

Partial-Bug: #1265493
Change-Id: I7d9e37b9a43589dd7b142869b86fa15cb3fb329c

diff --git a/etc/neutron.conf b/etc/neutron.conf
index 0836626424af65b9dd7dc65ec5b683fffe6c9535..fadbeb3a089c33811e80f1bad7a6736ea60f4998 100644 (file)
--- a/etc/neutron.conf
+++ b/etc/neutron.conf
@@ -545,6 +545,10 @@ lock_path = $state_path/lock
 # Change to "sudo" to skip the filtering and just run the comand directly
 # root_helper = sudo
 
+# Set to true to add comments to generated iptables rules that describe
+# each rule's purpose. (System must support the iptables comments module.)
+# comment_iptables_rules = True
+
 # =========== items for agent management extension =============
 # seconds between nodes reporting state to server; should be less than
 # agent_down_time, best if it is half or less than agent_down_time

diff --git a/neutron/agent/common/config.py b/neutron/agent/common/config.py
index d9395609038d7b5db3151fc0f055201b1050cb06..3ee41dde851f91a2a3575238c14c76e55c16f444 100644 (file)
--- a/neutron/agent/common/config.py
+++ b/neutron/agent/common/config.py
@@ -46,6 +46,11 @@ USE_NAMESPACES_OPTS = [
                 help=_("Allow overlapping IP.")),
 ]
 
+IPTABLES_OPTS = [
+    cfg.BoolOpt('comment_iptables_rules', default=True,
+                help=_("Add comments to iptables rules.")),
+]
+
 
 def get_log_args(conf, log_file_name):
     cmd_args = []
@@ -92,6 +97,10 @@ def register_use_namespaces_opts_helper(conf):
     conf.register_opts(USE_NAMESPACES_OPTS)
 
 
+def register_iptables_opts(conf):
+    conf.register_opts(IPTABLES_OPTS, 'AGENT')
+
+
 def get_root_helper(conf):
     root_helper = conf.AGENT.root_helper
     if root_helper != 'sudo':

diff --git a/neutron/agent/linux/iptables_comments.py b/neutron/agent/linux/iptables_comments.py
new file mode 100644 (file)
index 0000000..641b2ef
--- /dev/null
+++ b/neutron/agent/linux/iptables_comments.py
@@ -0,0 +1,34 @@
+#    Copyright 2014 OpenStack Foundation
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""iptables comments"""
+
+# Do not translate these comments. These comments cannot contain a quote or
+# an escape character because they will end up in a call to iptables and
+# could interfere with other parameters.
+
+SNAT_OUT = 'Perform source NAT on outgoing traffic.'
+UNMATCH_DROP = 'Default drop rule for unmatched traffic.'
+VM_INT_SG = 'Direct traffic from the VM interface to the security group chain.'
+SG_TO_VM_SG = 'Jump to the VM specific chain.'
+INPUT_TO_SG = 'Direct incoming traffic from VM to the security group chain.'
+PAIR_ALLOW = 'Allow traffic from defined IP/MAC pairs.'
+PAIR_DROP = 'Drop traffic without an IP/MAC allow rule.'
+DHCP_CLIENT = 'Allow DHCP client traffic.'
+DHCP_SPOOF = 'Prevent DHCP Spoofing by VM.'
+UNMATCHED = 'Send unmatched traffic to the fallback chain.'
+STATELESS_DROP = 'Drop packets that are not associated with a state.'
+ALLOW_ASSOC = ('Direct packets associated with a known session to the RETURN '
+               'chain.')
+IPV6_RA_ALLOW = 'Allow IPv6 ICMP traffic to allow RA packets.'

diff --git a/neutron/agent/linux/iptables_firewall.py b/neutron/agent/linux/iptables_firewall.py
index 5f050b91aa33a48523850815c0730e8abc83154a..54b39afbbce228c7a3616f124f1a459c979e1da4 100644 (file)
--- a/neutron/agent/linux/iptables_firewall.py
+++ b/neutron/agent/linux/iptables_firewall.py
@@ -18,6 +18,7 @@ from oslo.config import cfg
 
 from neutron.agent import firewall
 from neutron.agent.linux import ipset_manager
+from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import iptables_manager
 from neutron.common import constants
 from neutron.common import ipv6_utils
@@ -40,6 +41,7 @@ LINUX_DEV_LEN = 14
 IPSET_CHAIN_LEN = 20
 IPSET_CHANGE_BULK_THRESHOLD = 10
 IPSET_ADD_BULK_THRESHOLD = 5
+comment_rule = iptables_manager.comment_rule
 
 
 class IptablesFirewallDriver(firewall.FirewallDriver):
@@ -146,9 +148,11 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
 
     def _add_fallback_chain_v4v6(self):
         self.iptables.ipv4['filter'].add_chain('sg-fallback')
-        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP')
+        self.iptables.ipv4['filter'].add_rule('sg-fallback', '-j DROP',
+                                              comment=ic.UNMATCH_DROP)
         self.iptables.ipv6['filter'].add_chain('sg-fallback')
-        self.iptables.ipv6['filter'].add_rule('sg-fallback', '-j DROP')
+        self.iptables.ipv6['filter'].add_rule('sg-fallback', '-j DROP',
+                                              comment=ic.UNMATCH_DROP)
 
     def _add_chain_by_name_v4v6(self, chain_name):
         self.iptables.ipv6['filter'].add_chain(chain_name)
@@ -158,12 +162,15 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
         self.iptables.ipv4['filter'].ensure_remove_chain(chain_name)
         self.iptables.ipv6['filter'].ensure_remove_chain(chain_name)
 
-    def _add_rule_to_chain_v4v6(self, chain_name, ipv4_rules, ipv6_rules):
+    def _add_rule_to_chain_v4v6(self, chain_name, ipv4_rules, ipv6_rules,
+                                comment=None):
         for rule in ipv4_rules:
-            self.iptables.ipv4['filter'].add_rule(chain_name, rule)
+            self.iptables.ipv4['filter'].add_rule(chain_name, rule,
+                                                  comment=comment)
 
         for rule in ipv6_rules:
-            self.iptables.ipv6['filter'].add_rule(chain_name, rule)
+            self.iptables.ipv6['filter'].add_rule(chain_name, rule,
+                                                  comment=comment)
 
     def _get_device_name(self, port):
         return port['device']
@@ -183,17 +190,20 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
                      '-j $%s' % (self.IPTABLES_DIRECTION[direction],
                                  device,
                                  SG_CHAIN)]
-        self._add_rule_to_chain_v4v6('FORWARD', jump_rule, jump_rule)
+        self._add_rule_to_chain_v4v6('FORWARD', jump_rule, jump_rule,
+                                     comment=ic.VM_INT_SG)
 
         # jump to the chain based on the device
         jump_rule = ['-m physdev --%s %s --physdev-is-bridged '
                      '-j $%s' % (self.IPTABLES_DIRECTION[direction],
                                  device,
                                  chain_name)]
-        self._add_rule_to_chain_v4v6(SG_CHAIN, jump_rule, jump_rule)
+        self._add_rule_to_chain_v4v6(SG_CHAIN, jump_rule, jump_rule,
+                                     comment=ic.SG_TO_VM_SG)
 
         if direction == EGRESS_DIRECTION:
-            self._add_rule_to_chain_v4v6('INPUT', jump_rule, jump_rule)
+            self._add_rule_to_chain_v4v6('INPUT', jump_rule, jump_rule,
+                                         comment=ic.INPUT_TO_SG)
 
     def _split_sgr_by_ethertype(self, security_group_rules):
         ipv4_sg_rules = []
@@ -222,12 +232,12 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
                     # of the list after the allowed_address_pair rules.
                     table.add_rule(chain_name,
                                    '-m mac --mac-source %s -j RETURN'
-                                   % mac)
+                                   % mac, comment=ic.PAIR_ALLOW)
                 else:
                     table.add_rule(chain_name,
                                    '-m mac --mac-source %s -s %s -j RETURN'
-                                   % (mac, ip))
-            table.add_rule(chain_name, '-j DROP')
+                                   % (mac, ip), comment=ic.PAIR_ALLOW)
+            table.add_rule(chain_name, '-j DROP', comment=ic.PAIR_DROP)
             rules.append('-j $%s' % chain_name)
 
     def _build_ipv4v6_mac_ip_list(self, mac, ip_address, mac_ipv4_pairs,
@@ -239,9 +249,12 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
 
     def _spoofing_rule(self, port, ipv4_rules, ipv6_rules):
         #Note(nati) allow dhcp or RA packet
-        ipv4_rules += ['-p udp -m udp --sport 68 --dport 67 -j RETURN']
-        ipv6_rules += ['-p icmpv6 -j RETURN']
-        ipv6_rules += ['-p udp -m udp --sport 546 --dport 547 -j RETURN']
+        ipv4_rules += [comment_rule('-p udp -m udp --sport 68 --dport 67 '
+                                    '-j RETURN', comment=ic.DHCP_CLIENT)]
+        ipv6_rules += [comment_rule('-p icmpv6 -j RETURN',
+                                    comment=ic.IPV6_RA_ALLOW)]
+        ipv6_rules += [comment_rule('-p udp -m udp --sport 546 --dport 547 '
+                                    '-j RETURN', comment=None)]
         mac_ipv4_pairs = []
         mac_ipv6_pairs = []
 
@@ -266,8 +279,10 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
 
     def _drop_dhcp_rule(self, ipv4_rules, ipv6_rules):
         #Note(nati) Drop dhcp packet from VM
-        ipv4_rules += ['-p udp -m udp --sport 67 --dport 68 -j DROP']
-        ipv6_rules += ['-p udp -m udp --sport 547 --dport 546 -j DROP']
+        ipv4_rules += [comment_rule('-p udp -m udp --sport 67 --dport 68 '
+                                    '-j DROP', comment=ic.DHCP_SPOOF)]
+        ipv6_rules += [comment_rule('-p udp -m udp --sport 547 --dport 546 '
+                                    '-j DROP', comment=None)]
 
     def _accept_inbound_icmpv6(self):
         # Allow multicast listener, neighbor solicitation and
@@ -454,18 +469,22 @@ class IptablesFirewallDriver(firewall.FirewallDriver):
             args += ['-j RETURN']
             iptables_rules += [' '.join(args)]
 
-        iptables_rules += ['-j $sg-fallback']
+        iptables_rules += [comment_rule('-j $sg-fallback',
+                                        comment=ic.UNMATCHED)]
 
         return iptables_rules
 
     def _drop_invalid_packets(self, iptables_rules):
         # Always drop invalid packets
-        iptables_rules += ['-m state --state ' 'INVALID -j DROP']
+        iptables_rules += [comment_rule('-m state --state ' 'INVALID -j DROP',
+                                        comment=ic.STATELESS_DROP)]
         return iptables_rules
 
     def _allow_established(self, iptables_rules):
         # Allow established connections
-        iptables_rules += ['-m state --state RELATED,ESTABLISHED -j RETURN']
+        iptables_rules += [comment_rule(
+            '-m state --state RELATED,ESTABLISHED -j RETURN',
+            comment=ic.ALLOW_ASSOC)]
         return iptables_rules
 
     def _protocol_arg(self, protocol):

diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 613a929cab61bfa1c666b27a45283dc7f6fe351e..6af69ad32eda9ad40a4c0cf06cd80fec102315aa 100644 (file)
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -22,6 +22,10 @@ import inspect
 import os
 import re
 
+from oslo.config import cfg
+
+from neutron.agent.common import config
+from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import utils as linux_utils
 from neutron.common import utils
 from neutron.openstack.common import excutils
@@ -51,6 +55,12 @@ MAX_CHAIN_LEN_NOWRAP = 28
 IPTABLES_ERROR_LINES_OF_CONTEXT = 5
 
 
+def comment_rule(rule, comment):
+    if not cfg.CONF.AGENT.comment_iptables_rules or not comment:
+        return rule
+    return '%s -m comment --comment "%s"' % (rule, comment)
+
+
 def get_chain_name(chain_name, wrap=True):
     if wrap:
         return chain_name[:MAX_CHAIN_LEN_WRAP]
@@ -67,13 +77,14 @@ class IptablesRule(object):
     """
 
     def __init__(self, chain, rule, wrap=True, top=False,
-                 binary_name=binary_name, tag=None):
+                 binary_name=binary_name, tag=None, comment=None):
         self.chain = get_chain_name(chain, wrap)
         self.rule = rule
         self.wrap = wrap
         self.top = top
         self.wrap_name = binary_name[:16]
         self.tag = tag
+        self.comment = comment
 
     def __eq__(self, other):
         return ((self.chain == other.chain) and
@@ -89,7 +100,7 @@ class IptablesRule(object):
             chain = '%s-%s' % (self.wrap_name, self.chain)
         else:
             chain = self.chain
-        return '-A %s %s' % (chain, self.rule)
+        return comment_rule('-A %s %s' % (chain, self.rule), self.comment)
 
 
 class IptablesTable(object):
@@ -182,7 +193,8 @@ class IptablesTable(object):
         self.rules = [r for r in self.rules
                       if jump_snippet not in r.rule]
 
-    def add_rule(self, chain, rule, wrap=True, top=False, tag=None):
+    def add_rule(self, chain, rule, wrap=True, top=False, tag=None,
+                 comment=None):
         """Add a rule to the table.
 
         This is just like what you'd feed to iptables, just without
@@ -202,7 +214,7 @@ class IptablesTable(object):
                 self._wrap_target_chain(e, wrap) for e in rule.split(' '))
 
         self.rules.append(IptablesRule(chain, rule, wrap, top, self.wrap_name,
-                                       tag))
+                                       tag, comment))
 
     def _wrap_target_chain(self, s, wrap):
         if s.startswith('$'):
@@ -210,7 +222,7 @@ class IptablesTable(object):
 
         return s
 
-    def remove_rule(self, chain, rule, wrap=True, top=False):
+    def remove_rule(self, chain, rule, wrap=True, top=False, comment=None):
         """Remove a rule from a chain.
 
         Note: The rule must be exactly identical to the one that was added.
@@ -225,10 +237,12 @@ class IptablesTable(object):
                     self._wrap_target_chain(e, wrap) for e in rule.split(' '))
 
             self.rules.remove(IptablesRule(chain, rule, wrap, top,
-                                           self.wrap_name))
+                                           self.wrap_name,
+                                           comment=comment))
             if not wrap:
                 self.remove_rules.append(IptablesRule(chain, rule, wrap, top,
-                                                      self.wrap_name))
+                                                      self.wrap_name,
+                                                      comment=comment))
         except ValueError:
             LOG.warn(_('Tried to remove rule that was not there:'
                        ' %(chain)r %(rule)r %(wrap)r %(top)r'),
@@ -288,6 +302,7 @@ class IptablesManager(object):
         else:
             self.execute = linux_utils.execute
 
+        config.register_iptables_opts(cfg.CONF)
         self.use_ipv6 = use_ipv6
         self.root_helper = root_helper
         self.namespace = namespace
@@ -351,7 +366,8 @@ class IptablesManager(object):
             # chain so that it's applied last.
             self.ipv4['nat'].add_chain('snat')
             self.ipv4['nat'].add_rule('neutron-postrouting-bottom',
-                                      '-j $snat', wrap=False)
+                                      '-j $snat', wrap=False,
+                                      comment=ic.SNAT_OUT)
 
             # And then we add a float-snat chain and jump to first thing in
             # the snat chain.

diff --git a/neutron/tests/unit/test_iptables_firewall.py b/neutron/tests/unit/test_iptables_firewall.py
index d94da9aac830bcde19fe95c8928da01aa9a398c5..c4c0cafb250751deee909dc48dca35bd4ac4cda4 100644 (file)
--- a/neutron/tests/unit/test_iptables_firewall.py
+++ b/neutron/tests/unit/test_iptables_firewall.py
@@ -19,6 +19,7 @@ import mock
 from oslo.config import cfg
 
 from neutron.agent.common import config as a_cfg
+from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import iptables_firewall
 from neutron.agent import securitygroups_rpc as sg_cfg
 from neutron.common import constants
@@ -42,6 +43,8 @@ class BaseIptablesFirewallTestCase(base.BaseTestCase):
         super(BaseIptablesFirewallTestCase, self).setUp()
         cfg.CONF.register_opts(a_cfg.ROOT_HELPER_OPTS, 'AGENT')
         cfg.CONF.register_opts(sg_cfg.security_group_opts, 'SECURITYGROUP')
+        cfg.CONF.register_opts(a_cfg.IPTABLES_OPTS, 'AGENT')
+        cfg.CONF.set_override('comment_iptables_rules', False, 'AGENT')
         self.utils_exec_p = mock.patch(
             'neutron.agent.linux.utils.execute')
         self.utils_exec = self.utils_exec_p.start()
@@ -71,55 +74,74 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         port = self._fake_port()
         self.firewall.prepare_port_filter(port)
         calls = [mock.call.add_chain('sg-fallback'),
-                 mock.call.add_rule('sg-fallback', '-j DROP'),
+                 mock.call.add_rule(
+                     'sg-fallback', '-j DROP',
+                     comment=ic.UNMATCH_DROP),
                  mock.call.ensure_remove_chain('sg-chain'),
                  mock.call.add_chain('sg-chain'),
                  mock.call.add_chain('ifake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $ifake_dev'),
+                                    '-j $ifake_dev',
+                                    comment=ic.SG_TO_VM_SG),
+                 mock.call.add_rule(
+                     'ifake_dev',
+                     '-m state --state INVALID -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ifake_dev', '-m state --state INVALID -j DROP'),
+                     'ifake_dev',
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+                     '-j $sg-fallback', comment=None),
                  mock.call.add_chain('ofake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-in tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule('INPUT',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev', '-m mac --mac-source ff:ff:ff:ff:ff:ff '
-                     '-s 10.0.0.1 -j RETURN'),
-                 mock.call.add_rule('sfake_dev', '-j DROP'),
+                     '-s 10.0.0.1 -j RETURN',
+                     comment=ic.PAIR_ALLOW),
+                 mock.call.add_rule(
+                     'sfake_dev', '-j DROP',
+                     comment=ic.PAIR_DROP),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 68 --dport 67 -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sfake_dev'),
+                     '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                    comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 67 --dport 68 -j DROP'),
+                     '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ofake_dev', '-m state --state INVALID -j DROP'),
+                     'ofake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule(
+                     'ofake_dev',
+                     '-j $sg-fallback',
+                     comment=None),
                  mock.call.add_rule('sg-chain', '-j ACCEPT')]
 
         self.v4filter_inst.assert_has_calls(calls)
@@ -127,7 +149,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
     def test_filter_ipv4_ingress(self):
         rule = {'ethertype': 'IPv4',
                 'direction': 'ingress'}
-        ingress = mock.call.add_rule('ifake_dev', '-j RETURN')
+        ingress = mock.call.add_rule('ifake_dev', '-j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -136,7 +159,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'ingress',
                 'source_ip_prefix': prefix}
-        ingress = mock.call.add_rule('ifake_dev', '-s %s -j RETURN' % prefix)
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-s %s -j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -144,7 +168,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'ingress',
                 'protocol': 'tcp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p tcp -m tcp -j RETURN')
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-p tcp -m tcp -j RETURN', comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -155,7 +180,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'tcp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-s %s -p tcp -m tcp -j RETURN' % prefix)
+                                     '-s %s -p tcp -m tcp -j RETURN' % prefix,
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -163,7 +189,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'ingress',
                 'protocol': 'icmp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p icmp -j RETURN')
+        ingress = mock.call.add_rule('ifake_dev', '-p icmp -j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -174,7 +201,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'icmp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule(
-            'ifake_dev', '-s %s -p icmp -j RETURN' % prefix)
+            'ifake_dev', '-s %s -p icmp -j RETURN' % prefix,
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -185,7 +213,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-p tcp -m tcp --dport 10 -j RETURN')
+                                     '-p tcp -m tcp --dport 10 -j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -197,7 +226,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         ingress = mock.call.add_rule(
             'ifake_dev',
-            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN')
+            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -212,7 +242,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         ingress = mock.call.add_rule(
             'ifake_dev',
             '-s %s -p tcp -m tcp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -220,7 +250,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'ingress',
                 'protocol': 'udp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p udp -m udp -j RETURN')
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-p udp -m udp -j RETURN', comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -231,7 +262,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'udp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-s %s -p udp -m udp -j RETURN' % prefix)
+                                     '-s %s -p udp -m udp -j RETURN' % prefix,
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -242,7 +274,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-p udp -m udp --dport 10 -j RETURN')
+                                     '-p udp -m udp --dport 10 -j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -254,7 +287,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         ingress = mock.call.add_rule(
             'ifake_dev',
-            '-p udp -m udp -m multiport --dports 10:100 -j RETURN')
+            '-p udp -m udp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -269,14 +303,14 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         ingress = mock.call.add_rule(
             'ifake_dev',
             '-s %s -p udp -m udp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
     def test_filter_ipv4_egress(self):
         rule = {'ethertype': 'IPv4',
                 'direction': 'egress'}
-        egress = mock.call.add_rule('ofake_dev', '-j RETURN')
+        egress = mock.call.add_rule('ofake_dev', '-j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -285,7 +319,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'egress',
                 'source_ip_prefix': prefix}
-        egress = mock.call.add_rule('ofake_dev', '-s %s -j RETURN' % prefix)
+        egress = mock.call.add_rule(
+            'ofake_dev', '-s %s -j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -293,7 +328,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'egress',
                 'protocol': 'tcp'}
-        egress = mock.call.add_rule('ofake_dev', '-p tcp -m tcp -j RETURN')
+        egress = mock.call.add_rule(
+            'ofake_dev', '-p tcp -m tcp -j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -304,7 +340,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'tcp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-s %s -p tcp -m tcp -j RETURN' % prefix)
+                                    '-s %s -p tcp -m tcp -j RETURN' % prefix,
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -312,7 +349,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'egress',
                 'protocol': 'icmp'}
-        egress = mock.call.add_rule('ofake_dev', '-p icmp -j RETURN')
+        egress = mock.call.add_rule('ofake_dev', '-p icmp -j RETURN',
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -323,7 +361,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'icmp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
-            'ofake_dev', '-s %s -p icmp -j RETURN' % prefix)
+            'ofake_dev', '-s %s -p icmp -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -336,7 +375,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmp --icmp-type 8 -j RETURN' % prefix)
+            '-s %s -p icmp --icmp-type 8 -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -349,7 +389,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmp --icmp-type echo-request -j RETURN' % prefix)
+            '-s %s -p icmp --icmp-type echo-request -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -363,7 +404,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmp --icmp-type 8/0 -j RETURN' % prefix)
+            '-s %s -p icmp --icmp-type 8/0 -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -374,7 +416,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-p tcp -m tcp --dport 10 -j RETURN')
+                                    '-p tcp -m tcp --dport 10 -j RETURN',
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -386,7 +429,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN')
+            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -401,7 +445,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         egress = mock.call.add_rule(
             'ofake_dev',
             '-s %s -p tcp -m tcp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -409,7 +453,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv4',
                 'direction': 'egress',
                 'protocol': 'udp'}
-        egress = mock.call.add_rule('ofake_dev', '-p udp -m udp -j RETURN')
+        egress = mock.call.add_rule(
+            'ofake_dev', '-p udp -m udp -j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -420,7 +465,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'udp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-s %s -p udp -m udp -j RETURN' % prefix)
+                                    '-s %s -p udp -m udp -j RETURN' % prefix,
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -431,7 +477,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-p udp -m udp --dport 10 -j RETURN')
+                                    '-p udp -m udp --dport 10 -j RETURN',
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -443,7 +490,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-p udp -m udp -m multiport --dports 10:100 -j RETURN')
+            '-p udp -m udp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -458,14 +506,15 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         egress = mock.call.add_rule(
             'ofake_dev',
             '-s %s -p udp -m udp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
     def test_filter_ipv6_ingress(self):
         rule = {'ethertype': 'IPv6',
                 'direction': 'ingress'}
-        ingress = mock.call.add_rule('ifake_dev', '-j RETURN')
+        ingress = mock.call.add_rule('ifake_dev', '-j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -474,7 +523,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'ingress',
                 'source_ip_prefix': prefix}
-        ingress = mock.call.add_rule('ifake_dev', '-s %s -j RETURN' % prefix)
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-s %s -j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -482,7 +532,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'ingress',
                 'protocol': 'tcp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p tcp -m tcp -j RETURN')
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-p tcp -m tcp -j RETURN', comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -493,7 +544,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'tcp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-s %s -p tcp -m tcp -j RETURN' % prefix)
+                                     '-s %s -p tcp -m tcp -j RETURN' % prefix,
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -504,7 +556,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-p tcp -m tcp --dport 10 -j RETURN')
+                                     '-p tcp -m tcp --dport 10 -j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -512,7 +565,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'ingress',
                 'protocol': 'icmp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p icmpv6 -j RETURN')
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-p icmpv6 -j RETURN', comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -523,7 +577,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'icmp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule(
-            'ifake_dev', '-s %s -p icmpv6 -j RETURN' % prefix)
+            'ifake_dev', '-s %s -p icmpv6 -j RETURN' % prefix,
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -535,7 +590,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         ingress = mock.call.add_rule(
             'ifake_dev',
-            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN')
+            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -550,7 +606,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         ingress = mock.call.add_rule(
             'ifake_dev',
             '-s %s -p tcp -m tcp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -558,7 +614,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'ingress',
                 'protocol': 'udp'}
-        ingress = mock.call.add_rule('ifake_dev', '-p udp -m udp -j RETURN')
+        ingress = mock.call.add_rule(
+            'ifake_dev', '-p udp -m udp -j RETURN', comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -569,7 +626,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'udp',
                 'source_ip_prefix': prefix}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-s %s -p udp -m udp -j RETURN' % prefix)
+                                     '-s %s -p udp -m udp -j RETURN' % prefix,
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -580,7 +638,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         ingress = mock.call.add_rule('ifake_dev',
-                                     '-p udp -m udp --dport 10 -j RETURN')
+                                     '-p udp -m udp --dport 10 -j RETURN',
+                                     comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -592,7 +651,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         ingress = mock.call.add_rule(
             'ifake_dev',
-            '-p udp -m udp -m multiport --dports 10:100 -j RETURN')
+            '-p udp -m udp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -607,14 +667,14 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         ingress = mock.call.add_rule(
             'ifake_dev',
             '-s %s -p udp -m udp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         egress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
     def test_filter_ipv6_egress(self):
         rule = {'ethertype': 'IPv6',
                 'direction': 'egress'}
-        egress = mock.call.add_rule('ofake_dev', '-j RETURN')
+        egress = mock.call.add_rule('ofake_dev', '-j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -623,7 +683,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'egress',
                 'source_ip_prefix': prefix}
-        egress = mock.call.add_rule('ofake_dev', '-s %s -j RETURN' % prefix)
+        egress = mock.call.add_rule(
+            'ofake_dev', '-s %s -j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -631,7 +692,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'egress',
                 'protocol': 'tcp'}
-        egress = mock.call.add_rule('ofake_dev', '-p tcp -m tcp -j RETURN')
+        egress = mock.call.add_rule(
+            'ofake_dev', '-p tcp -m tcp -j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -642,7 +704,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'tcp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-s %s -p tcp -m tcp -j RETURN' % prefix)
+                                    '-s %s -p tcp -m tcp -j RETURN' % prefix,
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -650,7 +713,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'egress',
                 'protocol': 'icmp'}
-        egress = mock.call.add_rule('ofake_dev', '-p icmpv6 -j RETURN')
+        egress = mock.call.add_rule(
+            'ofake_dev', '-p icmpv6 -j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -661,7 +725,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'icmp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
-            'ofake_dev', '-s %s -p icmpv6 -j RETURN' % prefix)
+            'ofake_dev', '-s %s -p icmpv6 -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -674,7 +739,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmpv6 --icmpv6-type 8 -j RETURN' % prefix)
+            '-s %s -p icmpv6 --icmpv6-type 8 -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -687,7 +753,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmpv6 --icmpv6-type echo-request -j RETURN' % prefix)
+            '-s %s -p icmpv6 --icmpv6-type echo-request -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -701,7 +768,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-s %s -p icmpv6 --icmpv6-type 8/0 -j RETURN' % prefix)
+            '-s %s -p icmpv6 --icmpv6-type 8/0 -j RETURN' % prefix,
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -712,7 +780,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-p tcp -m tcp --dport 10 -j RETURN')
+                                    '-p tcp -m tcp --dport 10 -j RETURN',
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -724,7 +793,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN')
+            '-p tcp -m tcp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -739,7 +809,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         egress = mock.call.add_rule(
             'ofake_dev',
             '-s %s -p tcp -m tcp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -747,7 +817,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         rule = {'ethertype': 'IPv6',
                 'direction': 'egress',
                 'protocol': 'udp'}
-        egress = mock.call.add_rule('ofake_dev', '-p udp -m udp -j RETURN')
+        egress = mock.call.add_rule(
+            'ofake_dev', '-p udp -m udp -j RETURN', comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -758,7 +829,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'protocol': 'udp',
                 'source_ip_prefix': prefix}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-s %s -p udp -m udp -j RETURN' % prefix)
+                                    '-s %s -p udp -m udp -j RETURN' % prefix,
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -769,7 +841,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_min': 10,
                 'port_range_max': 10}
         egress = mock.call.add_rule('ofake_dev',
-                                    '-p udp -m udp --dport 10 -j RETURN')
+                                    '-p udp -m udp --dport 10 -j RETURN',
+                                    comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -781,7 +854,8 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'port_range_max': 100}
         egress = mock.call.add_rule(
             'ofake_dev',
-            '-p udp -m udp -m multiport --dports 10:100 -j RETURN')
+            '-p udp -m udp -m multiport --dports 10:100 -j RETURN',
+            comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -796,7 +870,7 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         egress = mock.call.add_rule(
             'ofake_dev',
             '-s %s -p udp -m udp -m multiport --dports 10:100 '
-            '-j RETURN' % prefix)
+            '-j RETURN' % prefix, comment=None)
         ingress = None
         self._test_prepare_port_filter(rule, ingress, egress)
 
@@ -810,89 +884,113 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         filter_inst = self.v4filter_inst
         dhcp_rule = [mock.call.add_rule(
             'ofake_dev',
-            '-p udp -m udp --sport 68 --dport 67 -j RETURN')]
+            '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+            comment=None)]
 
         if ethertype == 'IPv6':
             filter_inst = self.v6filter_inst
 
             dhcp_rule = [mock.call.add_rule('ofake_dev',
-                                            '-p icmpv6 -j RETURN'),
+                                            '-p icmpv6 -j RETURN',
+                                            comment=None),
                          mock.call.add_rule('ofake_dev', '-p udp -m udp '
                                             '--sport 546 --dport 547 '
-                                            '-j RETURN')]
+                                            '-j RETURN', comment=None)]
         sg = [rule]
         port['security_group_rules'] = sg
         self.firewall.prepare_port_filter(port)
         calls = [mock.call.add_chain('sg-fallback'),
-                 mock.call.add_rule('sg-fallback', '-j DROP'),
+                 mock.call.add_rule(
+                     'sg-fallback',
+                     '-j DROP',
+                     comment=ic.UNMATCH_DROP),
                  mock.call.ensure_remove_chain('sg-chain'),
                  mock.call.add_chain('sg-chain'),
                  mock.call.add_chain('ifake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $ifake_dev'),
+                                    '-j $ifake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  ]
         if ethertype == 'IPv6':
             for icmp6_type in constants.ICMPV6_ALLOWED_TYPES:
                 calls.append(
                     mock.call.add_rule('ifake_dev',
                                        '-p icmpv6 --icmpv6-type %s -j RETURN' %
-                                       icmp6_type))
-        calls += [mock.call.add_rule('ifake_dev',
-                                     '-m state --state INVALID -j DROP'),
-                  mock.call.add_rule(
-                      'ifake_dev',
-                      '-m state --state RELATED,ESTABLISHED -j RETURN')]
+                                       icmp6_type, comment=None))
+        calls += [
+            mock.call.add_rule(
+                'ifake_dev',
+                '-m state --state INVALID -j DROP', comment=None
+            ),
+            mock.call.add_rule(
+                'ifake_dev',
+                '-m state --state RELATED,ESTABLISHED -j RETURN',
+                comment=None
+            )
+        ]
 
         if ingress_expected_call:
             calls.append(ingress_expected_call)
 
-        calls += [mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+        calls += [mock.call.add_rule('ifake_dev',
+                                     '-j $sg-fallback', comment=None),
                   mock.call.add_chain('ofake_dev'),
                   mock.call.add_rule('FORWARD',
                                      '-m physdev --physdev-in tapfake_dev '
                                      '--physdev-is-bridged '
-                                     '-j $sg-chain'),
+                                     '-j $sg-chain', comment=ic.VM_INT_SG),
                   mock.call.add_rule('sg-chain',
                                      '-m physdev --physdev-in tapfake_dev '
-                                     '--physdev-is-bridged '
-                                     '-j $ofake_dev'),
+                                     '--physdev-is-bridged -j $ofake_dev',
+                                     comment=ic.SG_TO_VM_SG),
                   mock.call.add_rule('INPUT',
                                      '-m physdev --physdev-in tapfake_dev '
-                                     '--physdev-is-bridged '
-                                     '-j $ofake_dev'),
+                                     '--physdev-is-bridged -j $ofake_dev',
+                                     comment=ic.INPUT_TO_SG),
                   mock.call.add_chain('sfake_dev'),
                   mock.call.add_rule(
                       'sfake_dev',
                       '-m mac --mac-source ff:ff:ff:ff:ff:ff -s %s -j RETURN'
-                      % prefix),
-                  mock.call.add_rule('sfake_dev', '-j DROP')]
+                      % prefix,
+                      comment=ic.PAIR_ALLOW),
+                  mock.call.add_rule(
+                      'sfake_dev', '-j DROP',
+                      comment=ic.PAIR_DROP)]
         calls += dhcp_rule
-        calls.append(mock.call.add_rule('ofake_dev', '-j $sfake_dev'))
+        calls.append(mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                        comment=None))
         if ethertype == 'IPv4':
             calls.append(mock.call.add_rule(
                 'ofake_dev',
-                '-p udp -m udp --sport 67 --dport 68 -j DROP'))
+                '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                comment=None))
         if ethertype == 'IPv6':
             calls.append(mock.call.add_rule(
                 'ofake_dev',
-                '-p udp -m udp --sport 547 --dport 546 -j DROP'))
+                '-p udp -m udp --sport 547 --dport 546 -j DROP',
+                comment=None))
 
-        calls += [mock.call.add_rule(
-                  'ofake_dev', '-m state --state INVALID -j DROP'),
-                  mock.call.add_rule(
-                      'ofake_dev',
-                      '-m state --state RELATED,ESTABLISHED -j RETURN')]
+        calls += [
+            mock.call.add_rule(
+                'ofake_dev',
+                '-m state --state INVALID -j DROP', comment=None),
+            mock.call.add_rule(
+                'ofake_dev',
+                '-m state --state RELATED,ESTABLISHED -j RETURN',
+                comment=None),
+        ]
 
         if egress_expected_call:
             calls.append(egress_expected_call)
 
-        calls += [mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+        calls += [mock.call.add_rule('ofake_dev',
+                                     '-j $sg-fallback', comment=None),
                   mock.call.add_rule('sg-chain', '-j ACCEPT')]
 
         filter_inst.assert_has_calls(calls)
@@ -909,57 +1007,80 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
         self.firewall.remove_port_filter(port)
         self.firewall.remove_port_filter({'device': 'no-exist-device'})
         calls = [mock.call.add_chain('sg-fallback'),
-                 mock.call.add_rule('sg-fallback', '-j DROP'),
+                 mock.call.add_rule(
+                     'sg-fallback',
+                     '-j DROP',
+                     comment=ic.UNMATCH_DROP),
                  mock.call.ensure_remove_chain('sg-chain'),
                  mock.call.add_chain('sg-chain'),
                  mock.call.add_chain('ifake_dev'),
                  mock.call.add_rule(
                      'FORWARD',
                      '-m physdev --physdev-out tapfake_dev '
-                     '--physdev-is-bridged -j $sg-chain'),
+                     '--physdev-is-bridged -j $sg-chain',
+                     comment=ic.VM_INT_SG),
                  mock.call.add_rule(
                      'sg-chain',
                      '-m physdev --physdev-out tapfake_dev '
-                     '--physdev-is-bridged -j $ifake_dev'),
+                     '--physdev-is-bridged -j $ifake_dev',
+                     comment=ic.SG_TO_VM_SG),
+                 mock.call.add_rule(
+                     'ifake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
-                     'ifake_dev', '-m state --state INVALID -j DROP'),
+                     'ifake_dev',
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ifake_dev', '-j RETURN',
+                                    comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+                     '-j $sg-fallback', comment=None),
                  mock.call.add_chain('ofake_dev'),
                  mock.call.add_rule(
                      'FORWARD',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $sg-chain'),
+                     '--physdev-is-bridged -j $sg-chain',
+                     comment=ic.VM_INT_SG),
                  mock.call.add_rule(
                      'sg-chain',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $ofake_dev'),
+                     '--physdev-is-bridged -j $ofake_dev',
+                     comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule(
                      'INPUT',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $ofake_dev'),
+                     '--physdev-is-bridged -j $ofake_dev',
+                     comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
                      '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
-                     '-j RETURN'),
-                 mock.call.add_rule('sfake_dev', '-j DROP'),
+                     '-j RETURN',
+                     comment=ic.PAIR_ALLOW),
+                 mock.call.add_rule(
+                     'sfake_dev', '-j DROP',
+                     comment=ic.PAIR_DROP),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 68 --dport 67 -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sfake_dev'),
+                     '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                    comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 67 --dport 68 -j DROP'),
+                     '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ofake_dev', '-m state --state INVALID -j DROP'),
+                     'ofake_dev', '-m state --state INVALID -j DROP',
+                     comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule(
+                     'ofake_dev',
+                     '-j $sg-fallback', comment=None),
                  mock.call.add_rule('sg-chain', '-j ACCEPT'),
                  mock.call.ensure_remove_chain('ifake_dev'),
                  mock.call.ensure_remove_chain('ofake_dev'),
@@ -970,51 +1091,70 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                  mock.call.add_rule(
                      'FORWARD',
                      '-m physdev --physdev-out tapfake_dev '
-                     '--physdev-is-bridged -j $sg-chain'),
+                     '--physdev-is-bridged -j $sg-chain',
+                     comment=ic.VM_INT_SG),
                  mock.call.add_rule(
                      'sg-chain',
                      '-m physdev --physdev-out tapfake_dev '
-                     '--physdev-is-bridged -j $ifake_dev'),
+                     '--physdev-is-bridged -j $ifake_dev',
+                     comment=ic.SG_TO_VM_SG),
+                 mock.call.add_rule(
+                     'ifake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state INVALID -j DROP'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+                     '-j $sg-fallback', comment=None),
                  mock.call.add_chain('ofake_dev'),
                  mock.call.add_rule(
                      'FORWARD',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $sg-chain'),
+                     '--physdev-is-bridged -j $sg-chain',
+                     comment=ic.VM_INT_SG),
                  mock.call.add_rule(
                      'sg-chain',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $ofake_dev'),
+                     '--physdev-is-bridged -j $ofake_dev',
+                     comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule(
                      'INPUT',
                      '-m physdev --physdev-in tapfake_dev '
-                     '--physdev-is-bridged -j $ofake_dev'),
+                     '--physdev-is-bridged -j $ofake_dev',
+                     comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
                      '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
-                     '-j RETURN'),
-                 mock.call.add_rule('sfake_dev', '-j DROP'),
+                     '-j RETURN',
+                     comment=ic.PAIR_ALLOW),
+                 mock.call.add_rule(
+                     'sfake_dev', '-j DROP',
+                     comment=ic.PAIR_DROP),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 68 --dport 67 -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sfake_dev'),
+                     '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                    comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 67 --dport 68 -j DROP'),
+                     '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ofake_dev', '-m state --state INVALID -j DROP'),
+                     'ofake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j RETURN',
+                                    comment=None),
+                 mock.call.add_rule('ofake_dev',
+                                    '-j $sg-fallback',
+                                    comment=None),
                  mock.call.add_rule('sg-chain', '-j ACCEPT'),
                  mock.call.ensure_remove_chain('ifake_dev'),
                  mock.call.ensure_remove_chain('ofake_dev'),
@@ -1116,60 +1256,76 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'fixed_ips': ['10.0.0.1', 'fe80::1', '10.0.0.2']}
         self.firewall.prepare_port_filter(port)
         calls = [mock.call.add_chain('sg-fallback'),
-                 mock.call.add_rule('sg-fallback', '-j DROP'),
+                 mock.call.add_rule(
+                     'sg-fallback', '-j DROP',
+                     comment=ic.UNMATCH_DROP),
                  mock.call.ensure_remove_chain('sg-chain'),
                  mock.call.add_chain('sg-chain'),
                  mock.call.add_chain('ifake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $ifake_dev'),
+                                    '-j $ifake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule(
-                     'ifake_dev', '-m state --state INVALID -j DROP'),
+                     'ifake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ifake_dev',
+                                    '-j $sg-fallback', comment=None),
                  mock.call.add_chain('ofake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-in tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule('INPUT',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
                      '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.1 '
-                     '-j RETURN'),
+                     '-j RETURN',
+                     comment=ic.PAIR_ALLOW),
                  mock.call.add_rule(
                      'sfake_dev',
                      '-m mac --mac-source ff:ff:ff:ff:ff:ff -s 10.0.0.2 '
-                     '-j RETURN'),
-                 mock.call.add_rule('sfake_dev', '-j DROP'),
+                     '-j RETURN',
+                     comment=ic.PAIR_ALLOW),
+                 mock.call.add_rule(
+                     'sfake_dev', '-j DROP',
+                     comment=ic.PAIR_DROP),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 68 --dport 67 -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sfake_dev'),
+                     '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                    comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 67 --dport 68 -j DROP'),
+                     '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ofake_dev', '-m state --state INVALID -j DROP'),
+                     'ofake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev',
+                                    '-j $sg-fallback', comment=None),
                  mock.call.add_rule('sg-chain', '-j ACCEPT')]
         self.v4filter_inst.assert_has_calls(calls)
 
@@ -1179,55 +1335,71 @@ class IptablesFirewallTestCase(BaseIptablesFirewallTestCase):
                 'fixed_ips': []}
         self.firewall.prepare_port_filter(port)
         calls = [mock.call.add_chain('sg-fallback'),
-                 mock.call.add_rule('sg-fallback', '-j DROP'),
+                 mock.call.add_rule(
+                     'sg-fallback', '-j DROP',
+                     comment=ic.UNMATCH_DROP),
                  mock.call.ensure_remove_chain('sg-chain'),
                  mock.call.add_chain('sg-chain'),
                  mock.call.add_chain('ifake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-out tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $ifake_dev'),
+                                    '-j $ifake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule(
-                     'ifake_dev', '-m state --state INVALID -j DROP'),
+                     'ifake_dev',
+                     '-m state --state INVALID -j DROP', comment=None),
                  mock.call.add_rule(
                      'ifake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ifake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ifake_dev', '-j $sg-fallback',
+                                    comment=None),
                  mock.call.add_chain('ofake_dev'),
                  mock.call.add_rule('FORWARD',
                                     '-m physdev --physdev-in tapfake_dev '
                                     '--physdev-is-bridged '
-                                    '-j $sg-chain'),
+                                    '-j $sg-chain', comment=ic.VM_INT_SG),
                  mock.call.add_rule('sg-chain',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.SG_TO_VM_SG),
                  mock.call.add_rule('INPUT',
                                     '-m physdev --physdev-in tapfake_dev '
-                                    '--physdev-is-bridged '
-                                    '-j $ofake_dev'),
+                                    '--physdev-is-bridged -j $ofake_dev',
+                                    comment=ic.INPUT_TO_SG),
                  mock.call.add_chain('sfake_dev'),
                  mock.call.add_rule(
                      'sfake_dev',
-                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -j RETURN'),
-                 mock.call.add_rule('sfake_dev', '-j DROP'),
+                     '-m mac --mac-source ff:ff:ff:ff:ff:ff -j RETURN',
+                     comment=ic.PAIR_ALLOW),
+                 mock.call.add_rule(
+                     'sfake_dev', '-j DROP',
+                     comment=ic.PAIR_DROP),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 68 --dport 67 -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sfake_dev'),
+                     '-p udp -m udp --sport 68 --dport 67 -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sfake_dev',
+                                    comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-p udp -m udp --sport 67 --dport 68 -j DROP'),
+                     '-p udp -m udp --sport 67 --dport 68 -j DROP',
+                     comment=None),
                  mock.call.add_rule(
-                     'ofake_dev', '-m state --state INVALID -j DROP'),
+                     'ofake_dev',
+                     '-m state --state INVALID -j DROP',
+                     comment=None),
                  mock.call.add_rule(
                      'ofake_dev',
-                     '-m state --state RELATED,ESTABLISHED -j RETURN'),
-                 mock.call.add_rule('ofake_dev', '-j $sg-fallback'),
+                     '-m state --state RELATED,ESTABLISHED -j RETURN',
+                     comment=None),
+                 mock.call.add_rule('ofake_dev', '-j $sg-fallback',
+                                    comment=None),
                  mock.call.add_rule('sg-chain', '-j ACCEPT')]
         self.v4filter_inst.assert_has_calls(calls)
 

diff --git a/neutron/tests/unit/test_iptables_manager.py b/neutron/tests/unit/test_iptables_manager.py
index ac0f9ad8c0f51a77068729469749479ae839d472..54380401d748e74e2a45a6a0a069d02db0b110cf 100644 (file)
--- a/neutron/tests/unit/test_iptables_manager.py
+++ b/neutron/tests/unit/test_iptables_manager.py
@@ -17,13 +17,17 @@ import inspect
 import os
 
 import mock
+from oslo.config import cfg
 
+from neutron.agent.common import config as a_cfg
+from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import iptables_manager
 from neutron.tests import base
 from neutron.tests import tools
 
 
-IPTABLES_ARG = {'bn': iptables_manager.binary_name}
+IPTABLES_ARG = {'bn': iptables_manager.binary_name,
+                'snat_out_comment': ic.SNAT_OUT}
 
 NAT_DUMP = ('# Generated by iptables_manager\n'
             '*nat\n'
@@ -59,6 +63,107 @@ FILTER_DUMP = ('# Generated by iptables_manager\n'
                'COMMIT\n'
                '# Completed by iptables_manager\n' % IPTABLES_ARG)
 
+COMMENTED_NAT_DUMP = (
+    '# Generated by iptables_manager\n'
+    '*nat\n'
+    ':neutron-postrouting-bottom - [0:0]\n'
+    ':%(bn)s-OUTPUT - [0:0]\n'
+    ':%(bn)s-POSTROUTING - [0:0]\n'
+    ':%(bn)s-PREROUTING - [0:0]\n'
+    ':%(bn)s-float-snat - [0:0]\n'
+    ':%(bn)s-snat - [0:0]\n'
+    '[0:0] -A PREROUTING -j %(bn)s-PREROUTING\n'
+    '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
+    '[0:0] -A POSTROUTING -j %(bn)s-POSTROUTING\n'
+    '[0:0] -A POSTROUTING -j neutron-postrouting-bottom\n'
+    '[0:0] -A neutron-postrouting-bottom -j %(bn)s-snat '
+    '-m comment --comment "%(snat_out_comment)s"\n'
+    '[0:0] -A %(bn)s-snat -j '
+    '%(bn)s-float-snat\n'
+    'COMMIT\n'
+    '# Completed by iptables_manager\n' % IPTABLES_ARG)
+
+
+class IptablesCommentsTestCase(base.BaseTestCase):
+
+    def setUp(self):
+        super(IptablesCommentsTestCase, self).setUp()
+        cfg.CONF.register_opts(a_cfg.IPTABLES_OPTS, 'AGENT')
+        cfg.CONF.set_override('comment_iptables_rules', True, 'AGENT')
+        self.root_helper = 'sudo'
+        self.iptables = (iptables_manager.
+                         IptablesManager(root_helper=self.root_helper))
+        self.execute = mock.patch.object(self.iptables, "execute").start()
+
+    def test_comments_short_enough(self):
+        for attr in dir(ic):
+            if not attr.startswith('__') and len(getattr(ic, attr)) > 255:
+                self.fail("Iptables comment %s is longer than 255 characters."
+                          % attr)
+
+    def test_add_filter_rule(self):
+        filter_dump_mod = ('# Generated by iptables_manager\n'
+                           '*filter\n'
+                           ':neutron-filter-top - [0:0]\n'
+                           ':%(bn)s-FORWARD - [0:0]\n'
+                           ':%(bn)s-INPUT - [0:0]\n'
+                           ':%(bn)s-OUTPUT - [0:0]\n'
+                           ':%(bn)s-filter - [0:0]\n'
+                           ':%(bn)s-local - [0:0]\n'
+                           '[0:0] -A FORWARD -j neutron-filter-top\n'
+                           '[0:0] -A OUTPUT -j neutron-filter-top\n'
+                           '[0:0] -A neutron-filter-top -j %(bn)s-local\n'
+                           '[0:0] -A INPUT -j %(bn)s-INPUT\n'
+                           '[0:0] -A OUTPUT -j %(bn)s-OUTPUT\n'
+                           '[0:0] -A FORWARD -j %(bn)s-FORWARD\n'
+                           '[0:0] -A %(bn)s-filter -j DROP\n'
+                           '[0:0] -A %(bn)s-INPUT -s 0/0 -d 192.168.0.2 -j '
+                           '%(bn)s-filter\n'
+                           'COMMIT\n'
+                           '# Completed by iptables_manager\n'
+                           % IPTABLES_ARG)
+
+        raw_dump = _generate_raw_dump(IPTABLES_ARG)
+
+        expected_calls_and_values = [
+            (mock.call(['iptables-save', '-c'],
+                       root_helper=self.root_helper),
+             ''),
+            (mock.call(['iptables-restore', '-c'],
+                       process_input=(
+                           raw_dump + COMMENTED_NAT_DUMP + filter_dump_mod),
+                       root_helper=self.root_helper),
+             None),
+            (mock.call(['iptables-save', '-c'],
+                       root_helper=self.root_helper),
+             ''),
+            (mock.call(['iptables-restore', '-c'],
+                       process_input=(
+                           raw_dump + COMMENTED_NAT_DUMP + FILTER_DUMP),
+                       root_helper=self.root_helper
+                       ),
+             None),
+        ]
+        tools.setup_mock_calls(self.execute, expected_calls_and_values)
+
+        self.iptables.ipv4['filter'].add_chain('filter')
+        self.iptables.ipv4['filter'].add_rule('filter', '-j DROP')
+        self.iptables.ipv4['filter'].add_rule('INPUT',
+                                              '-s 0/0 -d 192.168.0.2 -j'
+                                              ' %(bn)s-filter' % IPTABLES_ARG)
+        self.iptables.apply()
+
+        self.iptables.ipv4['filter'].remove_rule('filter', '-j DROP')
+        self.iptables.ipv4['filter'].remove_rule('INPUT',
+                                                 '-s 0/0 -d 192.168.0.2 -j'
+                                                 ' %(bn)s-filter'
+                                                 % IPTABLES_ARG)
+        self.iptables.ipv4['filter'].remove_chain('filter')
+
+        self.iptables.apply()
+
+        tools.verify_mock_calls(self.execute, expected_calls_and_values)
+
 
 def _generate_raw_dump(iptables_args):
     return ('# Generated by iptables_manager\n'
@@ -77,6 +182,8 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
 
     def setUp(self):
         super(IptablesManagerStateFulTestCase, self).setUp()
+        cfg.CONF.register_opts(a_cfg.IPTABLES_OPTS, 'AGENT')
+        cfg.CONF.set_override('comment_iptables_rules', False, 'AGENT')
         self.root_helper = 'sudo'
         self.iptables = iptables_manager.IptablesManager(
             root_helper=self.root_helper)
@@ -936,6 +1043,8 @@ class IptablesManagerStateLessTestCase(base.BaseTestCase):
 
     def setUp(self):
         super(IptablesManagerStateLessTestCase, self).setUp()
+        cfg.CONF.register_opts(a_cfg.IPTABLES_OPTS, 'AGENT')
+        cfg.CONF.set_override('comment_iptables_rules', False, 'AGENT')
         self.iptables = (iptables_manager.IptablesManager(state_less=True))
 
     def test_nat_not_found(self):

diff --git a/neutron/tests/unit/test_security_groups_rpc.py b/neutron/tests/unit/test_security_groups_rpc.py
index 5a1d5dc97edad1146ef4b3efaf7336bee98f9b2a..af5ec53861a868a2b84b1da6d335397a206b025f 100644 (file)
--- a/neutron/tests/unit/test_security_groups_rpc.py
+++ b/neutron/tests/unit/test_security_groups_rpc.py
@@ -2254,11 +2254,13 @@ class TestSecurityGroupAgentWithIptables(base.BaseTestCase):
     def setUp(self, defer_refresh_firewall=False, test_rpc_v1_1=True):
         super(TestSecurityGroupAgentWithIptables, self).setUp()
         config.register_root_helper(cfg.CONF)
+        config.register_iptables_opts(cfg.CONF)
         cfg.CONF.set_override(
             'lock_path',
             '$state_path/lock')
         set_firewall_driver(self.FIREWALL_DRIVER)
         cfg.CONF.set_override('enable_ipset', False, group='SECURITYGROUP')
+        cfg.CONF.set_override('comment_iptables_rules', False, group='AGENT')
 
         self.agent = sg_rpc.SecurityGroupAgentRpcMixin()
         self.agent.context = None