From d14fbc849e45e7a959c159d460715ece20013a31 Mon Sep 17 00:00:00 2001 From: Akihiro Motoki Date: Sat, 21 Oct 2017 10:50:48 +0000 Subject: [PATCH] FIP associate: Show only ports from a target server Previously when associating an FIP to a server, neutron ports which do not belong to a target server were also shown in the target list. It is clearer if only neutron ports connected to a target server are shown. This commit changes the logic to lookup FIP targets to achieve this. Closes-Bug: #1630412 Change-Id: I9901b54e125b6bf09df86537c8308b2bfb1499f2 --- openstack_dashboard/api/neutron.py | 33 ++++++++++++++----- .../dashboards/project/floating_ips/tests.py | 11 +++---- .../project/floating_ips/workflows.py | 28 ++++++++++++---- .../test/api_tests/neutron_tests.py | 31 ++++++++++++++++- 4 files changed, 79 insertions(+), 24 deletions(-) diff --git a/openstack_dashboard/api/neutron.py b/openstack_dashboard/api/neutron.py index e526c8210d..6c26d29bea 100644 --- a/openstack_dashboard/api/neutron.py +++ b/openstack_dashboard/api/neutron.py @@ -29,6 +29,7 @@ from django.conf import settings from django.utils.translation import ugettext_lazy as _ from neutronclient.common import exceptions as neutron_exc from neutronclient.v2_0 import client as neutron_client +from novaclient import exceptions as nova_exc import six from horizon import exceptions @@ -625,7 +626,7 @@ class FloatingIpManager(object): self.client.update_floatingip(floating_ip_id, {'floatingip': update_dict}) - def _get_reachable_subnets(self, ports): + def _get_reachable_subnets(self, ports, fetch_router_ports=False): if not is_enabled_by_config('enable_fip_topology_check', True): # All subnets are reachable from external network return set( @@ -637,10 +638,15 @@ class FloatingIpManager(object): if (r.external_gateway_info and r.external_gateway_info.get('network_id') in ext_net_ids)] - reachable_subnets = set([p.fixed_ips[0]['subnet_id'] for p in ports - if ((p.device_owner in - ROUTER_INTERFACE_OWNERS) - and (p.device_id in gw_routers))]) + if fetch_router_ports: + router_ports = port_list(self.request, + device_owner=ROUTER_INTERFACE_OWNERS) + else: + router_ports = [p for p in ports + if p.device_owner in ROUTER_INTERFACE_OWNERS] + reachable_subnets = set([p.fixed_ips[0]['subnet_id'] + for p in router_ports + if p.device_id in gw_routers]) # we have to include any shared subnets as well because we may not # have permission to see the router interface to infer connectivity shared = set([s.id for n in network_list(self.request, shared=True) @@ -729,12 +735,21 @@ class FloatingIpManager(object): if target['instance_id'] == instance_id] else: ports = self._target_ports_by_instance(instance_id) + reachable_subnets = self._get_reachable_subnets( + ports, fetch_router_ports=True) + name = self._get_server_name(instance_id) # TODO(amotoki): Avoid using p.fixed_ips[0]. # Extract all IPv4 addresses instead - # TODO(amotoki): Replace a label with an empty string - # with a real server name. - return [FloatingIpTarget(p, p.fixed_ips[0]['ip_address'], '') - for p in ports] + return [FloatingIpTarget(p, p.fixed_ips[0]['ip_address'], name) + for p in ports + if p.fixed_ips[0]['subnet_id'] in reachable_subnets] + + def _get_server_name(self, server_id): + try: + server = nova.server_get(self.request, server_id) + return server.name + except nova_exc.NotFound: + return '' def is_simple_associate_supported(self): """Returns True if the default floating IP pool is enabled.""" diff --git a/openstack_dashboard/dashboards/project/floating_ips/tests.py b/openstack_dashboard/dashboards/project/floating_ips/tests.py index d800f01919..e7fc4bd276 100644 --- a/openstack_dashboard/dashboards/project/floating_ips/tests.py +++ b/openstack_dashboard/dashboards/project/floating_ips/tests.py @@ -54,17 +54,14 @@ class FloatingIpViewTests(test.TestCase): # Verify that our "associated" floating IP isn't in the choices list. self.assertNotIn(self.floating_ips.first(), choices) - @test.create_stubs({api.neutron: ('floating_ip_target_list', - 'floating_ip_target_get_by_instance', + @test.create_stubs({api.neutron: ('floating_ip_target_list_by_instance', 'tenant_floating_ip_list',)}) def test_associate_with_instance_id(self): targets = self._get_fip_targets() target = targets[0] - api.neutron.floating_ip_target_list(IsA(http.HttpRequest)) \ - .AndReturn(targets) - api.neutron.floating_ip_target_get_by_instance( - IsA(http.HttpRequest), target.instance_id, targets) \ - .AndReturn(target) + api.neutron.floating_ip_target_list_by_instance( + IsA(http.HttpRequest), target.instance_id) \ + .AndReturn([target]) api.neutron.tenant_floating_ip_list(IsA(http.HttpRequest)) \ .AndReturn(self.floating_ips.list()) self.mox.ReplayAll() diff --git a/openstack_dashboard/dashboards/project/floating_ips/workflows.py b/openstack_dashboard/dashboards/project/floating_ips/workflows.py index ce4516dc08..147353cf51 100644 --- a/openstack_dashboard/dashboards/project/floating_ips/workflows.py +++ b/openstack_dashboard/dashboards/project/floating_ips/workflows.py @@ -53,10 +53,15 @@ class AssociateIPAction(workflows.Action): q_instance_id = self.request.GET.get('instance_id') q_port_id = self.request.GET.get('port_id') if q_instance_id: - targets = self._get_target_list() - target = api.neutron.floating_ip_target_get_by_instance( - self.request, q_instance_id, targets) - self.initial['instance_id'] = target.id + targets = self._get_target_list(q_instance_id) + # Setting the initial value here is required to avoid a situation + # where instance_id passed in the URL is used as the initial value + # unexpectedly. (This always happens if the form is invoked from + # the instance table.) + if targets: + self.initial['instance_id'] = targets[0].id + else: + self.initial['instance_id'] = '' elif q_port_id: targets = self._get_target_list() for target in targets: @@ -84,10 +89,14 @@ class AssociateIPAction(workflows.Action): return options @memoized.memoized_method - def _get_target_list(self): + def _get_target_list(self, instance_id=None): targets = [] try: - targets = api.neutron.floating_ip_target_list(self.request) + if instance_id: + targets = api.neutron.floating_ip_target_list_by_instance( + self.request, instance_id) + else: + targets = api.neutron.floating_ip_target_list(self.request) except Exception: redirect = reverse('horizon:project:floating_ips:index') exceptions.handle(self.request, @@ -97,7 +106,12 @@ class AssociateIPAction(workflows.Action): # TODO(amotoki): [drop-nova-network] Rename instance_id to port_id def populate_instance_id_choices(self, request, context): - targets = self._get_target_list() + q_instance_id = self.request.GET.get('instance_id') + # The reason of specifying an empty tuple when q_instance_id is None + # is to make memoized_method _get_target_list work. Two calls of + # _get_target_list from here and __init__ must have a same arguments. + params = (q_instance_id, ) if q_instance_id else () + targets = self._get_target_list(*params) instances = sorted([(target.id, target.name) for target in targets], # Sort FIP targets by server name for easy browsing key=lambda x: x[1]) diff --git a/openstack_dashboard/test/api_tests/neutron_tests.py b/openstack_dashboard/test/api_tests/neutron_tests.py index 95b79185ea..cdde320955 100644 --- a/openstack_dashboard/test/api_tests/neutron_tests.py +++ b/openstack_dashboard/test/api_tests/neutron_tests.py @@ -1246,10 +1246,39 @@ class NeutronApiFloatingIpTests(NeutronApiTestBase): self.assertEqual('1', ret.instance_id) def test_target_floating_ip_port_by_instance(self): + server = self.servers.first() ports = self.api_ports.list() - candidates = [p for p in ports if p['device_id'] == '1'] + # _target_ports_by_instance() + candidates = [p for p in ports if p['device_id'] == server.id] search_opts = {'device_id': '1'} self.qclient.list_ports(**search_opts).AndReturn({'ports': candidates}) + # _get_reachable_subnets() + search_opts = {'router:external': True} + ext_nets = [n for n in self.api_networks.list() + if n['router:external']] + self.qclient.list_networks(**search_opts) \ + .AndReturn({'networks': ext_nets}) + self.qclient.list_routers() \ + .AndReturn({'routers': self.api_routers.list()}) + rinfs = [p for p in ports + if p['device_owner'] in api.neutron.ROUTER_INTERFACE_OWNERS] + filters = {'device_owner': api.neutron.ROUTER_INTERFACE_OWNERS} + self.qclient.list_ports(**filters).AndReturn({'ports': rinfs}) + shared_nets = [n for n in self.api_networks.list() if n['shared']] + self.qclient.list_networks(shared=True) \ + .AndReturn({'networks': shared_nets}) + shared_subnet_ids = [s for n in shared_nets for s in n['subnets']] + shared_subs = [s for s in self.api_subnets.list() + if s['id'] in shared_subnet_ids] + self.qclient.list_subnets().AndReturn({'subnets': shared_subs}) + # _get_server_name() + novaclient = self.stub_novaclient() + novaclient.servers = self.mox.CreateMockAnything() + novaclient.versions = self.mox.CreateMockAnything() + novaclient.versions.get_current().AndReturn("2.45") + search_opts = {'project_id': self.request.user.tenant_id} + novaclient.servers.get(server.id).AndReturn(server) + self.mox.ReplayAll() ret = api.neutron.floating_ip_target_list_by_instance(self.request,