Merge pull request #41 from romana/issue-40-eni-check

Check for black-hole routes by comparing ENIs.

vpcrouter/__init__.py

@@ -15,4 +15,4 @@
 
 """
 
-__version__ = "1.6.0"
+__version__ = "1.6.1"

vpcrouter/tests/test_vpc.py

@@ -128,9 +128,9 @@ def test_connect(self):
         self.assertTrue(d['instance_by_id'][self.i1.id].id == self.i1.id)
         self.assertTrue(d['instance_by_id'][self.i2.id].id == self.i2.id)
 
-        self.assertTrue(vpc.find_instance_and_emi_by_ip(d, self.i1ip)[0].id ==
+        self.assertTrue(vpc.find_instance_and_eni_by_ip(d, self.i1ip)[0].id ==
                         self.i1.id)
-        self.assertTrue(vpc.find_instance_and_emi_by_ip(d, self.i2ip)[0].id ==
+        self.assertTrue(vpc.find_instance_and_eni_by_ip(d, self.i2ip)[0].id ==
                         self.i2.id)
 
     @mock_ec2_deprecated
@@ -141,8 +141,8 @@ def test_process_route_spec_config(self):
 
         d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2")
 
-        i1, eni1 = vpc.find_instance_and_emi_by_ip(d, self.i1ip)
-        i2, eni2 = vpc.find_instance_and_emi_by_ip(d, self.i2ip)
+        i1, eni1 = vpc.find_instance_and_eni_by_ip(d, self.i1ip)
+        i2, eni2 = vpc.find_instance_and_eni_by_ip(d, self.i2ip)
 
         rt_id = d['route_tables'][0].id
 
@@ -216,7 +216,7 @@ def test_handle_spec(self):
         # output later on
         con = vpc.connect_to_region("ap-southeast-2")
         d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2")
-        i, eni = vpc.find_instance_and_emi_by_ip(d, self.i1ip)
+        i, eni = vpc.find_instance_and_eni_by_ip(d, self.i1ip)
         rt_id = d['route_tables'][0].id
 
         route_spec = {
@@ -230,12 +230,41 @@ def test_handle_spec(self):
         self.lc.check(
             ('root', 'DEBUG', 'Handle route spec'),
             ('root', 'DEBUG', "Connecting to AWS region 'ap-southeast-2'"),
-            ('root', 'DEBUG', u"Retrieving information for VPC '%s'" % vid),
+            ('root', 'DEBUG', "Retrieving information for VPC '%s'" % vid),
             ('root', 'DEBUG', 'Route spec processing. No failed IPs.'),
             ('root', 'INFO',
              "--- adding route in RT '%s' 10.2.0.0/16 -> %s (%s, %s)" %
              (rt_id, self.i1ip, self.i1.id, eni.id)))
 
+        # mock the get_instance_private_ip_from_route() function in vpc. Reason
+        # being: The boto mocking library (moto) doesn't handle ENIs in routes
+        # correctly. Therefore, a match against the information we get from the
+        # routes will never work. So, we provide a wrapper, which fills the
+        # instance's ENI information into the route. This means that this
+        # function now will always match. It's good for testing the 'match'
+        # part of the code.
+        old_func = vpc.get_instance_private_ip_from_route
+
+        def my_get_instance_private_ip_from_route(instance, route):
+            route.interface_id = instance.interfaces[0].id
+            return old_func(instance, route)
+
+        vpc.get_instance_private_ip_from_route = \
+                                my_get_instance_private_ip_from_route
+        self.lc.clear()
+        vpc.handle_spec("ap-southeast-2", vid, route_spec, [])
+
+        vpc.get_instance_private_ip_from_route = old_func
+
+        self.lc.check(
+            ('root', 'DEBUG', 'Handle route spec'),
+            ('root', 'DEBUG', "Connecting to AWS region 'ap-southeast-2'"),
+            ('root', 'DEBUG', "Retrieving information for VPC '%s'" % vid),
+            ('root', 'DEBUG', 'Route spec processing. No failed IPs.'),
+            ('root', 'INFO',
+             "--- route exists already in RT '%s': 10.2.0.0/16 -> "
+             "%s (%s, %s)" % (rt_id, self.i1ip, self.i1.id, eni.id)))
+
 
 if __name__ == '__main__':
     unittest.main()

vpcrouter/vpc/__init__.py

@@ -119,7 +119,7 @@ def get_vpc_overview(con, vpc_id, region_name):
     return d
 
 
-def find_instance_and_emi_by_ip(vpc_info, ip):
+def find_instance_and_eni_by_ip(vpc_info, ip):
     """
     Given a specific IP address, find the EC2 instance and ENI.
 
@@ -132,7 +132,7 @@ def find_instance_and_emi_by_ip(vpc_info, ip):
         for eni in instance.interfaces:
             if eni.private_ip_address == ip:
                 return instance, eni
-    raise VpcRouteSetError("Could not find instance/emi for '%s' "
+    raise VpcRouteSetError("Could not find instance/eni for '%s' "
                            "in VPC '%s'." % (ip, vpc_info['vpc'].id))
 
 
@@ -193,7 +193,7 @@ def _update_route(dcidr, router_ip, old_router_ip,
 
     """
     try:
-        instance, eni = find_instance_and_emi_by_ip(vpc_info, router_ip)
+        instance, eni = find_instance_and_eni_by_ip(vpc_info, router_ip)
 
         logging.info("--- updating existing route in RT '%s' "
                      "%s -> %s (%s, %s) (old IP: %s, reason: %s)" %
@@ -220,7 +220,7 @@ def _add_new_route(dcidr, router_ip, vpc_info, con, route_table_id):
 
     """
     try:
-        instance, eni = find_instance_and_emi_by_ip(vpc_info, router_ip)
+        instance, eni = find_instance_and_eni_by_ip(vpc_info, router_ip)
 
         logging.info("--- adding route in RT '%s' "
                      "%s -> %s (%s, %s)" %
@@ -237,6 +237,28 @@ def _add_new_route(dcidr, router_ip, vpc_info, con, route_table_id):
                       (route_table_id, dcidr, router_ip, e.message))
 
 
+def _get_real_instance_if_mismatch(vpc_info, ipaddr, instance, eni):
+    """
+    Return the real instance for the given IP address, if that instance is
+    different than the passed in instance or has a different eni.
+
+    If the ipaddr belongs to the same instance and eni that was passed in then
+    this returns None.
+
+    """
+    # Careful! A route may be a black-hole route, which still has instance and
+    # eni information for an instance that doesn't exist anymore. If a host was
+    # terminated and a new host got the same IP then this route won't be
+    # updated and will keep pointing to a non-existing node.  So we find the
+    # instance by IP and check that the route really points to this instance.
+    if ipaddr:
+        real_instance, real_eni = \
+                        find_instance_and_eni_by_ip(vpc_info, ipaddr)
+        if real_instance.id != instance.id  or real_eni.id != eni.id:
+            return real_instance
+    return None
+
+
 def _update_existing_routes(route_spec, failed_ips,
                             vpc_info, con, routes_in_rts):
     """
@@ -269,10 +291,24 @@ def _update_existing_routes(route_spec, failed_ips,
 
             hosts = route_spec.get(dcidr)       # eligible routers for CIDR
 
-            # Current router host for this CIDR/route
+            # Current router host for this CIDR/route.
             instance    = vpc_info['instance_by_id'][r.instance_id]
             ipaddr, eni = get_instance_private_ip_from_route(instance, r)
 
+            # If route points to outdated instance, set ipaddr and eni to None
+            # to signal that route needs to be updated
+            real_instance = _get_real_instance_if_mismatch(vpc_info, ipaddr,
+                                                           instance, eni)
+            if real_instance:
+                logging.info("--- obsoleted route in RT '%s' "
+                             "%s -> %s (%s, %s) (new instance with same "
+                             "IP address should be used: %s)" %
+                             (rt.id, dcidr, ipaddr, instance.id, eni.id,
+                              real_instance.id))
+                # Setting the ipaddr and eni to None signals code further down
+                # that the route must be updated.
+                ipaddr = eni = None
+
             if not hosts:
                 # The route isn't in the spec anymore and should be deleted.
                 logging.info("--- route not in spec, deleting in RT '%s': "
@@ -359,7 +395,7 @@ def _add_missing_routes(route_spec, failed_ips, chosen_routers,
     """
     Iterate over route spec and add all the routes we haven't set yet.
 
-    This relies on the being told what routes we HAVE already. This is passed
+    This relies on being told what routes we HAVE already. This is passed
     in via the routes_in_rts dict.
 
     Furthermore, some routes may be set in some RTs, but not in others. In that
@@ -388,7 +424,7 @@ def _add_missing_routes(route_spec, failed_ips, chosen_routers,
 
 def process_route_spec_config(con, vpc_info, route_spec, failed_ips):
     """
-    Looks through the route spec and updates routes accordingly.
+    Look through the route spec and update routes accordingly.
 
     Idea: Make sure we have a route for each CIDR.
 
@@ -438,7 +474,8 @@ def handle_spec(region_name, vpc_id, route_spec, failed_ips):
         con.close()
     except boto.exception.StandardError as e:
         logging.warning("vpc-router could not set route: %s - %s" %
-                        e.message, e.args)
+                        (e.message, e.args))
+        raise
 
     except boto.exception.NoAuthHandlerFound:
         logging.error("vpc-router could not authenticate")