diff --git a/vpcrouter/__init__.py b/vpcrouter/__init__.py index 695aeae..53f1715 100644 --- a/vpcrouter/__init__.py +++ b/vpcrouter/__init__.py @@ -15,4 +15,4 @@ """ -__version__ = "1.8.6" +__version__ = "1.8.9" diff --git a/vpcrouter/tests/test_vpc.py b/vpcrouter/tests/test_vpc.py index 0b374cf..159dd72 100644 --- a/vpcrouter/tests/test_vpc.py +++ b/vpcrouter/tests/test_vpc.py @@ -152,7 +152,8 @@ def test_connect(self): self.assertEqual( sorted(['subnets', 'route_tables', 'instance_by_id', - 'instances', 'subnet_rt_lookup', 'zones', 'vpc']), + 'ip_subnet_lookup', 'instances', 'rt_subnet_lookup', + 'zones', 'vpc']), sorted(d.keys())) self.assertEqual(self.new_vpc.id, d['vpc'].id) @@ -178,11 +179,18 @@ def _prepare_mock_env(self): d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + rt_id = d['route_tables'][0].id + + con.associate_route_table(route_table_id=rt_id, + subnet_id=self.new_subnet_a.id) + con.associate_route_table(route_table_id=rt_id, + subnet_id=self.new_subnet_b.id) + + d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + 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 - return con, d, i1, eni1, i2, eni2, rt_id @mock_ec2_deprecated @@ -193,6 +201,8 @@ def test_process_route_spec_config(self): u"10.1.0.0/16" : [self.i1ip, self.i2ip] } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) # Process a simple route spec, a route should have been added self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [], []) @@ -208,6 +218,8 @@ def test_process_route_spec_config(self): # One of the two IPs questionable, switch over d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [], [self.i1ip]) self.lc.check( @@ -225,6 +237,8 @@ def test_process_route_spec_config(self): # Now switch back d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [], [self.i2ip]) self.lc.check( @@ -243,6 +257,8 @@ def test_process_route_spec_config(self): # One of the two IPs failed, switch over d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [self.i1ip], []) self.lc.check( @@ -281,6 +297,8 @@ def test_process_route_spec_config(self): } d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [], []) self.lc.check( @@ -305,6 +323,8 @@ def test_process_route_spec_config(self): } d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc.process_route_spec_config(con, d, route_spec, [], []) # See in the logs that 10.2.0.0/16 wasn't deleted, even though it's not @@ -319,6 +339,11 @@ def test_process_route_spec_config(self): @mock_ec2_deprecated def test_add_new_route(self): con, d, i1, eni1, i2, eni2, rt_id = self._prepare_mock_env() + route_spec = { + "10.9.0.0/16" : [self.i1ip] + } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) self.lc.clear() vpc._add_new_route("10.9.0.0/16", self.i1ip, d, con, rt_id) @@ -341,9 +366,20 @@ def test_add_new_route(self): def test_update_route(self): con, d, i1, eni1, i2, eni2, rt_id = self._prepare_mock_env() + route_spec = { + "10.9.0.0/16" : [self.i1ip] + } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) + vpc._add_new_route("10.9.0.0/16", self.i1ip, d, con, rt_id) self.lc.clear() + route_spec = { + "10.9.0.0/16" : [self.i2ip] + } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) vpc._update_route("10.9.0.0/16", self.i2ip, self.i1ip, d, con, rt_id, "foobar") self.lc.check( @@ -438,11 +474,14 @@ def test_get_host_for_route(self): def test_update_existing_routes(self): con, d, i1, eni1, i2, eni2, rt_id = self._prepare_mock_env() - vpc._add_new_route("10.0.0.0/16", self.i1ip, d, con, rt_id) - route_spec = { u"10.0.0.0/16" : [self.i1ip] } + + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) + vpc._add_new_route("10.0.0.0/16", self.i1ip, d, con, rt_id) + routes_in_rts = {} # Test that a protected route doesn't get updated @@ -475,11 +514,11 @@ def test_update_existing_routes(self): self.assertEqual(rt.id, rt_id) route = rt.routes[0] - # Moto doesn't maintain intance or interface ID in the routes + # Moto doesn't maintain instance or interface ID in the routes # correctly, so need to set this one manually. This time the route spec # won't contain eligible hosts. - route.instance_id = i1.id - route.interface_id = eni1.id + route.instance_id = i1.id + route.interface_id = eni1.id self.lc.clear() route_spec = { u"10.0.0.0/16" : [] @@ -495,23 +534,29 @@ def test_update_existing_routes(self): # Get a refresh, since deleting via Boto interface doesn't update the # cached vpc-info d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) # There shouldn't be any routes left now rt = d['route_tables'][0] self.assertFalse(rt.routes) # Now try again, but with proper route spec. First we need to create # the route again and manually... + route_spec = { + u"10.0.0.0/16" : [self.i2ip] + } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) vpc._add_new_route("10.0.0.0/16", self.i1ip, d, con, rt_id) # ... and update our cached vpc info d = vpc.get_vpc_overview(con, self.new_vpc.id, "ap-southeast-2") + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) rt = d['route_tables'][0] route = rt.routes[0] route.instance_id = i1.id route.interface_id = eni1.id - route_spec = { - u"10.0.0.0/16" : [self.i2ip] - } # Only IP for spec is in failed IPs, can't do anything self.lc.clear() vpc._update_existing_routes(route_spec, [self.i2ip], [], @@ -562,6 +607,8 @@ def test_add_missing_routes(self): self.lc.check() self.lc.clear() + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) vpc._add_missing_routes(route_spec, [], [], {}, d, con, routes_in_rts) self.lc.check( ('root', 'INFO', @@ -614,12 +661,14 @@ def test_multi_address(self): private_ip_address="10.9.9.9", primary=False) eni1.private_ip_addresses.append(priv) + vpc._make_ip_subnet_lookup(d) self.lc.clear() route_spec = { "10.0.0.0/16" : ["10.9.9.9"] } - self.lc.clear() + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) vpc._add_missing_routes(route_spec, [], [], {}, d, con, {rt_id : []}) self.lc.check( @@ -636,13 +685,19 @@ 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") + route_spec = { + u"10.2.0.0/16" : [self.i1ip] + } + d['cluster_node_subnets'] = \ + vpc.make_cluster_node_subnet_list(d, route_spec) i, eni = vpc.find_instance_and_eni_by_ip(d, self.i1ip) rt_id = d['route_tables'][0].id - route_spec = { - u"10.2.0.0/16" : [self.i1ip] - } + con.associate_route_table(route_table_id=rt_id, + subnet_id=self.new_subnet_a.id) + con.associate_route_table(route_table_id=rt_id, + subnet_id=self.new_subnet_b.id) # Test handle_spec vid = self.new_vpc.id diff --git a/vpcrouter/vpc/__init__.py b/vpcrouter/vpc/__init__.py index dfb82b2..40206a5 100644 --- a/vpcrouter/vpc/__init__.py +++ b/vpcrouter/vpc/__init__.py @@ -68,13 +68,36 @@ def connect_to_region(region_name): return con +def _make_ip_subnet_lookup(vpc_info): + """ + Updates the vpc-info object with a lookup for IP -> subnet. + + """ + # We create a reverse lookup from the instances private IP addresses to the + # subnets they are associated with. This is used later on in order to + # determine whether routes should be set in an RT: Is the RT's subnet + # associated with ANY of the IP addresses in the route spec? To make this + # easy, we collect the IPs and subnets of all EC2 instances in the VPC. + # Once we get a route spec, we create a list of subnets for only the + # cluster nodes. The assumption is that not all EC2 instances in the VPC + # necessarily belong to the cluster. We really want to narrow it down to + # the cluster nodes only. See make_cluster_node_subnet_list(). + vpc_info['ip_subnet_lookup'] = {} + for instance in vpc_info['instances']: + for interface in instance.interfaces: + subnet_id = interface.subnet_id + for priv_addr in interface.private_ip_addresses: + vpc_info['ip_subnet_lookup'][priv_addr.private_ip_address] = \ + subnet_id + + def get_vpc_overview(con, vpc_id, region_name): """ Retrieve information for the specified VPC. If no VPC ID was specified then just pick the first VPC we find. - Returns a dict with the VPC's zones, subnets and route tables and + Returns a dict with the VPC's zones, subnets, route tables and instances. """ @@ -110,22 +133,30 @@ def get_vpc_overview(con, vpc_id, region_name): d['route_tables'] = con.get_all_route_tables(filters=vpc_filter) # Route tables are associated with subnets. Maintain a lookup table from - # subnet to (list of) route table(s). This is necessary later on, because - # we only want to set a route in an RT if the ENI to which we set the route - # is associated with the same subnet as the RT. That way, we don't have to - # set the route in all tables all the time. - d['subnet_rt_lookup'] = {} + # RT ID to (list of) subnets. This is necessary later on, because + # we only want to set a route in an RT if at least one of the ENIs we are + # dealing with is associated with the RT. That way, we don't have to set + # the route in all tables all the time. + d['rt_subnet_lookup'] = {} for rt in d['route_tables']: for assoc in rt.associations: if hasattr(assoc, 'subnet_id'): subnet_id = assoc.subnet_id - d['subnet_rt_lookup'].setdefault(subnet_id, []).append(rt.id) - - reservations = con.get_all_reservations(filters=vpc_filter) - d['instances'] = [] + if subnet_id: + d['rt_subnet_lookup'].setdefault(rt.id, []). \ + append(subnet_id) + + # Get all the cluster instances (all EC2 instances associated with this + # VPC). + reservations = con.get_all_reservations(filters=vpc_filter) + d['instances'] = [] for r in reservations: # a reservation may have multiple instances d['instances'].extend(r.instances) + # Add reverse lookup from the instances private IP addresses to the + # subnets they are associated with. More info in the doc for that function. + _make_ip_subnet_lookup(d) + # Maintain a quick instance lookup for convenience d['instance_by_id'] = {} for i in d['instances']: @@ -238,15 +269,20 @@ def _update_route(dcidr, router_ip, old_router_ip, instance = eni = None try: instance, eni = find_instance_and_eni_by_ip(vpc_info, router_ip) - # Only set the route if the ENI is associated with the same subnet as - # the route table - rts_for_subnet = vpc_info['subnet_rt_lookup'].get(eni.subnet_id) - if rts_for_subnet is not None and route_table_id not in rts_for_subnet: + # Only set the route if the RT is associated with any of the subnets + # used for the cluster. + rt_subnets = \ + set(vpc_info['rt_subnet_lookup'].get(route_table_id, [])) + cluster_node_subnets = \ + set(vpc_info['cluster_node_subnets']) + if not rt_subnets or not rt_subnets.intersection(cluster_node_subnets): logging.debug("--- skipping updating route in RT '%s' " - "%s -> %s (%s, %s) since RT not associated " - "with same subnet as ENI (ENI subnet: %s, RTs: %s)" % + "%s -> %s (%s, %s) since RT's subnets (%s) are not " + "part of the cluster (%s)." % (route_table_id, dcidr, router_ip, instance.id, - eni.id, eni.subnet_id, rts_for_subnet)) + eni.id, + ", ".join(rt_subnets) if rt_subnets else "none", + ", ".join(cluster_node_subnets))) return logging.info("--- updating existing route in RT '%s' " @@ -283,15 +319,20 @@ def _add_new_route(dcidr, router_ip, vpc_info, con, route_table_id): """ try: instance, eni = find_instance_and_eni_by_ip(vpc_info, router_ip) - # Only set the route if the ENI is associated with the same subnet as - # the route table - rts_for_subnet = vpc_info['subnet_rt_lookup'].get(eni.subnet_id) - if rts_for_subnet is not None and route_table_id not in rts_for_subnet: + # Only set the route if the RT is associated with any of the subnets + # used for the cluster. + rt_subnets = \ + set(vpc_info['rt_subnet_lookup'].get(route_table_id, [])) + cluster_node_subnets = \ + set(vpc_info['cluster_node_subnets']) + if not rt_subnets or not rt_subnets.intersection(cluster_node_subnets): logging.debug("--- skipping adding route in RT '%s' " - "%s -> %s (%s, %s) since RT not associated " - "with same subnet as ENI (ENI subnet: %s, RTs: %s)" % + "%s -> %s (%s, %s) since RT's subnets (%s) are not " + "part of the cluster (%s)." % (route_table_id, dcidr, router_ip, instance.id, - eni.id, eni.subnet_id, rts_for_subnet)) + eni.id, + ", ".join(rt_subnets) if rt_subnets else "none", + ", ".join(cluster_node_subnets))) return logging.info("--- adding route in RT '%s' " @@ -653,6 +694,36 @@ def process_route_spec_config(con, vpc_info, route_spec, vpc_info, con, routes_in_rts) +def make_cluster_node_subnet_list(vpc_info, route_spec): + """ + Create a list of subnets, which are associated with nodes in the current + route spec (cluster). + + This is needed when later on we set routes, but only want to set routes in + those route tables, which are associated with a subnet that in turn is + associated with a cluster node (to avoid having to set the routes in all + RTs). We can check whether the RT's subnet is in this list here. + + Returns list of subnet IDs. + + """ + # VPC-info already contains a lookup between the EC2 instances of the VPC + # and their subnets ('ip_subnet_lookup'). This may also contain some + # non-cluster nodes, but we can use this to look up the subnet for each IP + # address of cluster nodes. + subnets = set() + for cidr_ignore, hosts in route_spec.items(): + for host_addr in hosts: + subnet_id = vpc_info['ip_subnet_lookup'].get(host_addr) + if not subnet_id: + logging.warning("Did not find subnet associated with " + "host address '%s'!" % host_addr) + else: + subnets.add(subnet_id) + + return list(subnets) + + def handle_spec(region_name, vpc_id, route_spec, failed_ips, questionable_ips): """ Connect to region and update routes according to route spec. @@ -671,6 +742,8 @@ def handle_spec(region_name, vpc_id, route_spec, failed_ips, questionable_ips): try: con = connect_to_region(region_name) vpc_info = get_vpc_overview(con, vpc_id, region_name) + vpc_info['cluster_node_subnets'] = \ + make_cluster_node_subnet_list(vpc_info, route_spec) process_route_spec_config(con, vpc_info, route_spec, failed_ips, questionable_ips) con.close()