Packets getting lost during SNAT with too many connections

We have a problem with SNAT with too many connections using the
same source and destination on the network nodes.

In addition we can see in the conntrack table that the who
"instert_failed" increases.

This might be a generic problem with conntrack and linux.
We suspect that we encounter the following "limitation / bug"
in the kernel.

There seems to be a workaround to alleviate this behavior by
setting the -random-fully flag in iptables for port consumption.

This patch fixes the problem by adding the --random-fully to
the SNAT rules.

Conflicts:
    neutron/agent/linux/iptables_manager.py
    neutron/common/constants.py
    neutron/tests/unit/agent/l3/test_agent.py

Change-Id: I246c1f56df889bad9c7e140b56c3614124d80a19
Closes-Bug: #1814002
(cherry picked from commit 30f35e08f92e5262e7a9108684da048d11402b07)

9 files changed, 121 insertions, 30 deletions

diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py
index d015af3941..43002c1572 100644
--- a/neutron/agent/l3/dvr_edge_router.py
+++ b/neutron/agent/l3/dvr_edge_router.py
@@ -343,12 +343,14 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         return lib_constants.FLOATINGIP_STATUS_ACTIVE
 
     def _centralized_floating_forward_rules(self, floating_ip, fixed_ip):
+        to_source = '-s %s/32 -j SNAT --to-source %s' % (fixed_ip, floating_ip)
+        if self.snat_iptables_manager.random_fully:
+            to_source += ' --random-fully'
         return [('PREROUTING', '-d %s/32 -j DNAT --to-destination %s' %
                  (floating_ip, fixed_ip)),
                 ('OUTPUT', '-d %s/32 -j DNAT --to-destination %s' %
                  (floating_ip, fixed_ip)),
-                ('float-snat', '-s %s/32 -j SNAT --to-source %s' %
-                 (fixed_ip, floating_ip))]
+                ('float-snat', to_source)]
 
     def _set_floating_ip_nat_rules_for_centralized_floatingip(self, fip):
         if fip.get(n_const.DVR_SNAT_BOUND):
diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py
index 4178506689..0f367d5569 100644
--- a/neutron/agent/l3/dvr_local_router.py
+++ b/neutron/agent/l3/dvr_local_router.py
@@ -75,9 +75,10 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
         dnat_from_floatingip_to_fixedip = (
             'PREROUTING', '-d %s/32 -i %s -j DNAT --to-destination %s' % (
                 floating_ip, rtr_2_fip_name, fixed_ip))
-        snat_from_fixedip_to_floatingip = (
-            'float-snat', '-s %s/32 -j SNAT --to-source %s' % (
-                fixed_ip, floating_ip))
+        to_source = '-s %s/32 -j SNAT --to-source %s' % (fixed_ip, floating_ip)
+        if self.iptables_manager.random_fully:
+            to_source += ' --random-fully'
+        snat_from_fixedip_to_floatingip = ('float-snat', to_source)
         return [dnat_from_floatingip_to_fixedip,
                 snat_from_fixedip_to_floatingip]
 
diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index 82a7351f36..1c6418a704 100644
--- a/neutron/agent/l3/router_info.py
+++ b/neutron/agent/l3/router_info.py
@@ -166,12 +166,14 @@ class RouterInfo(object):
     def floating_forward_rules(self, fip):
         fixed_ip = fip['fixed_ip_address']
         floating_ip = fip['floating_ip_address']
+        to_source = '-s %s/32 -j SNAT --to-source %s' % (fixed_ip, floating_ip)
+        if self.iptables_manager.random_fully:
+            to_source += ' --random-fully'
         return [('PREROUTING', '-d %s/32 -j DNAT --to-destination %s' %
                  (floating_ip, fixed_ip)),
                 ('OUTPUT', '-d %s/32 -j DNAT --to-destination %s' %
                  (floating_ip, fixed_ip)),
-                ('float-snat', '-s %s/32 -j SNAT --to-source %s' %
-                 (fixed_ip, floating_ip))]
+                ('float-snat', to_source)]
 
     def floating_mangle_rules(self, floating_ip, fixed_ip, internal_mark):
         mark_traffic_to_floating_ip = (
@@ -846,19 +848,21 @@ class RouterInfo(object):
             self._prevent_snat_for_internal_traffic_rule(interface_name))
         # Makes replies come back through the router to reverse DNAT
         ext_in_mark = self.agent_conf.external_ingress_mark
-        snat_internal_traffic_to_floating_ip = (
-            'snat', '-m mark ! --mark %s/%s '
-                    '-m conntrack --ctstate DNAT '
-                    '-j SNAT --to-source %s'
-                    % (ext_in_mark, n_const.ROUTER_MARK_MASK, ex_gw_ip))
+        to_source = ('-m mark ! --mark %s/%s '
+                     '-m conntrack --ctstate DNAT '
+                     '-j SNAT --to-source %s'
+                     % (ext_in_mark, n_const.ROUTER_MARK_MASK, ex_gw_ip))
+        if self.iptables_manager.random_fully:
+            to_source += ' --random-fully'
+        snat_internal_traffic_to_floating_ip = ('snat', to_source)
         return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip,
                 snat_internal_traffic_to_floating_ip]
 
     def external_gateway_nat_snat_rules(self, ex_gw_ip, interface_name):
-        snat_normal_external_traffic = (
-            'snat', '-o %s -j SNAT --to-source %s' %
-                    (interface_name, ex_gw_ip))
-        return [snat_normal_external_traffic]
+        to_source = '-o %s -j SNAT --to-source %s' % (interface_name, ex_gw_ip)
+        if self.iptables_manager.random_fully:
+            to_source += ' --random-fully'
+        return [('snat', to_source)]
 
     def external_gateway_mangle_rules(self, interface_name):
         mark = self.agent_conf.external_ingress_mark
diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 1f953ca954..ab37dd382e 100644
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -34,6 +34,7 @@ from neutron._i18n import _
 from neutron.agent.linux import ip_lib
 from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import utils as linux_utils
+from neutron.common import constants
 from neutron.common import exceptions as n_exc
 from neutron.common import utils
 from neutron.conf.agent import common as config
@@ -304,6 +305,9 @@ class IptablesManager(object):
     # run iptables-restore without it.
     use_table_lock = False
 
+    # Flag to denote iptables supports --random-fully argument
+    _random_fully = None
+
     def __init__(self, _execute=None, state_less=False, use_ipv6=False,
                  namespace=None, binary_name=binary_name):
         if _execute:
@@ -475,6 +479,23 @@ class IptablesManager(object):
             args = ['ip', 'netns', 'exec', self.namespace] + args
         return self.execute(args, run_as_root=True).split('\n')
 
+    def _get_version(self):
+        # Output example is "iptables v1.6.2"
+        args = ['iptables', '--version']
+        version = str(self.execute(args, run_as_root=True).split()[1][1:])
+        LOG.debug("IPTables version installed: %s", version)
+        return version
+
+    @property
+    def random_fully(self):
+        if self._random_fully is not None:
+            return self._random_fully
+
+        version = self._get_version()
+        self.__class__._random_fully = utils.is_version_greater_equal(
+            version, constants.IPTABLES_RANDOM_FULLY_VERSION)
+        return self._random_fully
+
     @property
     def xlock_wait_time(self):
         # give agent some time to report back to server
diff --git a/neutron/common/constants.py b/neutron/common/constants.py
index ef66fe9ec6..9c2115e8e7 100644
--- a/neutron/common/constants.py
+++ b/neutron/common/constants.py
@@ -262,3 +262,7 @@ AGENT_RES_PROCESSING_STEP = 100
 # Number of resources for neutron to divide the large RPC
 # call data sets.
 RPC_RES_PROCESSING_STEP = 20
+
+# IPtables version to support --random-fully option.
+# Do not move this constant to neutron-lib, since it is temporary
+IPTABLES_RANDOM_FULLY_VERSION = '1.6.2'
diff --git a/neutron/common/utils.py b/neutron/common/utils.py
index 7f72787bcf..09c11a8993 100644
--- a/neutron/common/utils.py
+++ b/neutron/common/utils.py
@@ -45,6 +45,7 @@ from oslo_log import log as logging
 from oslo_utils import excutils
 from oslo_utils import fileutils
 from oslo_utils import importutils
+import pkg_resources
 import six
 from stevedore import driver
 
@@ -289,6 +290,12 @@ def ip_version_from_int(ip_version_int):
     raise ValueError(_('Illegal IP version number'))
 
 
+def is_version_greater_equal(version1, version2):
+    """Returns True if version1 is greater or equal than version2 else False"""
+    return (pkg_resources.parse_version(version1) >=
+            pkg_resources.parse_version(version2))
+
+
 class DelayedStringRenderer(object):
     """Takes a callable and its args and calls when __str__ is called
 
diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index 67aeafeb91..7f78b66eb7 100644
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -18,6 +18,7 @@ from itertools import chain as iter_chain
 from itertools import combinations as iter_combinations
 
 import eventlet
+import fixtures
 import mock
 import netaddr
 from neutron_lib.api.definitions import portbindings
@@ -187,10 +188,28 @@ class BasicRouterOperationsFramework(base.BaseTestCase):
         ri.process()
 
 
+class IptablesFixture(fixtures.Fixture):
+    def _setUp(self):
+        # We MUST save and restore random_fully because it is a class
+        # attribute and could change state in some tests, which can cause
+        # the other router test cases to randomly fail due to race conditions.
+        self.random_fully = iptables_manager.IptablesManager.random_fully
+        iptables_manager.IptablesManager.random_fully = True
+        self.addCleanup(self._reset)
+
+    def _reset(self):
+        iptables_manager.IptablesManager.random_fully = self.random_fully
+
+
 class TestBasicRouterOperations(BasicRouterOperationsFramework):
+    def setUp(self):
+        super(TestBasicRouterOperations, self).setUp()
+        self.useFixture(IptablesFixture())
+
     def test_request_id_changes(self):
         a = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.assertNotEqual(a.context.request_id, a.context.request_id)
+        self.useFixture(IptablesFixture())
 
     def test_init_ha_conf(self):
         with mock.patch('os.path.dirname', return_value='/etc/ha/'):
@@ -1009,7 +1028,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         self._test_external_gateway_action('remove', router, dual_stack=True)
 
     def _verify_snat_mangle_rules(self, nat_rules, mangle_rules, router,
-                                  negate=False):
+                                  random_fully, negate=False):
         interfaces = router[lib_constants.INTERFACE_KEY]
         source_cidrs = []
         for iface in interfaces:
@@ -1020,13 +1039,18 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 source_cidrs.append(source_cidr)
         source_nat_ip = router['gw_port']['fixed_ips'][0]['ip_address']
         interface_name = ('qg-%s' % router['gw_port']['id'])[:14]
+        mask_rule = ('-m mark ! --mark 0x2/%s -m conntrack --ctstate DNAT '
+                     '-j SNAT --to-source %s' %
+                     (n_const.ROUTER_MARK_MASK, source_nat_ip))
+        snat_rule = ('-o %s -j SNAT --to-source %s' %
+                     (interface_name, source_nat_ip))
+        if random_fully:
+            mask_rule += ' --random-fully'
+            snat_rule += ' --random-fully'
         expected_rules = [
             '! -i %s ! -o %s -m conntrack ! --ctstate DNAT -j ACCEPT' %
             (interface_name, interface_name),
-            '-o %s -j SNAT --to-source %s' % (interface_name, source_nat_ip),
-            '-m mark ! --mark 0x2/%s -m conntrack --ctstate DNAT '
-            '-j SNAT --to-source %s' %
-            (n_const.ROUTER_MARK_MASK, source_nat_ip)]
+            mask_rule, snat_rule]
         for r in nat_rules:
             if negate:
                 self.assertNotIn(r.rule, expected_rules)
@@ -1607,7 +1631,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.get_external_device_name = mock.Mock(return_value='exgw')
         self._test_process_floating_ip_addresses_add(ri, agent)
 
-    def test_process_router_snat_disabled(self):
+    def _test_process_router_snat_disabled(self, random_fully):
+        iptables_manager.IptablesManager.random_fully = random_fully
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         router = l3_test_common.prepare_router_data(enable_snat=True)
         ri = l3router.RouterInfo(agent, router['id'], router, **self.ri_kwargs)
@@ -1631,10 +1656,17 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             if r not in ri.iptables_manager.ipv4['mangle'].rules]
         self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
-                                       router)
+                                       router, random_fully)
         self.assertEqual(1, self.send_adv_notif.call_count)
 
-    def test_process_router_snat_enabled(self):
+    def test_process_router_snat_disabled_random_fully(self):
+        self._test_process_router_snat_disabled(True)
+
+    def test_process_router_snat_disabled_random_fully_false(self):
+        self._test_process_router_snat_disabled(False)
+
+    def _test_process_router_snat_enabled(self, random_fully):
+        iptables_manager.IptablesManager.random_fully = random_fully
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         router = l3_test_common.prepare_router_data(enable_snat=False)
         ri = l3router.RouterInfo(agent, router['id'], router, **self.ri_kwargs)
@@ -1658,9 +1690,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             if r not in orig_mangle_rules]
         self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
-                                       router)
+                                       router, random_fully)
         self.assertEqual(1, self.send_adv_notif.call_count)
 
+    def test_process_router_snat_enabled_random_fully(self):
+        self._test_process_router_snat_enabled(True)
+
+    def test_process_router_snat_enabled_random_fully_false(self):
+        self._test_process_router_snat_enabled(False)
+
     def _test_update_routing_table(self, is_snat_host=True):
         router = l3_test_common.prepare_router_data()
         uuid = router['id']
@@ -2262,11 +2300,12 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
 
         jump_float_rule = "-A %s-snat -j %s-float-snat" % (wrap_name,
                                                            wrap_name)
-        snat_rule1 = ("-A %s-snat -o iface -j SNAT --to-source %s") % (
+        snat_rule1 = ("-A %s-snat -o iface -j SNAT --to-source %s "
+                      "--random-fully") % (
             wrap_name, ex_gw_port['fixed_ips'][0]['ip_address'])
         snat_rule2 = ("-A %s-snat -m mark ! --mark 0x2/%s "
                       "-m conntrack --ctstate DNAT "
-                      "-j SNAT --to-source %s") % (
+                      "-j SNAT --to-source %s --random-fully") % (
             wrap_name, n_const.ROUTER_MARK_MASK,
             ex_gw_port['fixed_ips'][0]['ip_address'])
 
diff --git a/neutron/tests/unit/agent/l3/test_dvr_local_router.py b/neutron/tests/unit/agent/l3/test_dvr_local_router.py
index 957c12ea3a..8fe15a477c 100644
--- a/neutron/tests/unit/agent/l3/test_dvr_local_router.py
+++ b/neutron/tests/unit/agent/l3/test_dvr_local_router.py
@@ -262,9 +262,11 @@ class TestDvrRouterOperations(base.BaseTestCase):
         dnat_from_floatingip_to_fixedip = (
             'PREROUTING', '-d %s/32 -i %s -j DNAT --to-destination %s' % (
                 floating_ip, rtr_2_fip_name, fixed_ip))
-        snat_from_fixedip_to_floatingip = (
-            'float-snat', '-s %s/32 -j SNAT --to-source %s' % (
-                fixed_ip, floating_ip))
+        to_source = '-s %s/32 -j SNAT --to-source %s' % (fixed_ip, floating_ip)
+
+        if ri.iptables_manager.random_fully:
+            to_source += ' --random-fully'
+        snat_from_fixedip_to_floatingip = ('float-snat', to_source)
         actual = ri.floating_forward_rules(fip)
         expected = [dnat_from_floatingip_to_fixedip,
                     snat_from_fixedip_to_floatingip]
diff --git a/neutron/tests/unit/common/test_utils.py b/neutron/tests/unit/common/test_utils.py
index b609d96a9c..9fd075afa0 100644
--- a/neutron/tests/unit/common/test_utils.py
+++ b/neutron/tests/unit/common/test_utils.py
@@ -567,6 +567,17 @@ class TestIpVersionFromInt(base.BaseTestCase):
                           8)
 
 
+class TestIsVersionGreaterEqual(base.BaseTestCase):
+    def test_is_version_greater_equal_greater(self):
+        self.assertTrue(utils.is_version_greater_equal('1.6.2', '1.6.0'))
+
+    def test_is_version_greater_equal_equal(self):
+        self.assertTrue(utils.is_version_greater_equal('1.6.2', '1.6.2'))
+
+    def test_is_version_greater_equal_less(self):
+        self.assertFalse(utils.is_version_greater_equal('1.6.0', '1.6.2'))
+
+
 class TestDelayedStringRenderer(base.BaseTestCase):
     def test_call_deferred_until_str(self):
         my_func = mock.MagicMock(return_value='Brie cheese!')