From ebc9ee920eab20f19eb3ea353d48a9ace82475a7 Mon Sep 17 00:00:00 2001
From: Marek <marek.lycka@ultimum.io>
Date: Wed, 30 Oct 2019 09:38:29 +0100
Subject: Fixes a series of bugs related to Floating IPs.

- Fixes KeyErrors when accessing 'floatingip' values in usages, which
  broken Floating IP allocation.
- The quota display in the bottom right of the Allocation dialog are
  only displayed if 'enabled_quotas' is True
- Adds security group rule tallying for the usages overview page, which
  fixes a KeyError crash for installations where Horizon 'enable_quotas'
  is set to true, but the 'quota_details' extension is not installed on
  in Neutron
- Adds a policy check to show and hide The plus/add button in
  Instances->Associate Floating IP to match the Allocate IP To Project
  button in Floating IPs
- Fixed the page title not being set for the non-modal version of the
  modal allocation dialog/form
- Added an 'allowed' functionality for network usage overview charts to
  allow for them to be dynamically disabled
- Added tests and mocks for the above
- Added tests for non-legacy quota tallying for networks
- Added test for disabled network quotas in overview

Change-Id: I47f73ff94664d315a2400feb8ce8a25f4e6beced
closes-bug: #1838522
(cherry picked from commit 161b4ae5d4a7a29f9e5c91d9b391dee49e075993)
---
 .../dashboards/project/floating_ips/forms.py       |   3 +-
 .../dashboards/project/floating_ips/tables.py      |   2 +-
 .../templates/floating_ips/_allocate.html          |  40 ++++----
 .../templates/floating_ips/allocate.html           |   2 +
 .../dashboards/project/floating_ips/views.py       |   3 +-
 .../dashboards/project/floating_ips/workflows.py   |  10 +-
 .../dashboards/project/overview/tests.py           |  27 ++++--
 openstack_dashboard/test/unit/usage/test_quotas.py | 106 ++++++++++++++++++---
 openstack_dashboard/usage/quotas.py                |  25 ++++-
 openstack_dashboard/usage/views.py                 |  33 +++++--
 10 files changed, 196 insertions(+), 55 deletions(-)

diff --git a/openstack_dashboard/dashboards/project/floating_ips/forms.py b/openstack_dashboard/dashboards/project/floating_ips/forms.py
index 7b9964583..e838bf71c 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/forms.py
+++ b/openstack_dashboard/dashboards/project/floating_ips/forms.py
@@ -56,7 +56,8 @@ class FloatingIpAllocate(forms.SelfHandlingForm):
             # Prevent allocating more IP than the quota allows
             usages = quotas.tenant_quota_usages(request,
                                                 targets=('floatingip', ))
-            if usages['floatingip']['available'] <= 0:
+            if ('floatingip' in usages and
+                    usages['floatingip']['available'] <= 0):
                 error_message = _('You are already using all of your available'
                                   ' floating IPs.')
                 self.api_error(error_message)
diff --git a/openstack_dashboard/dashboards/project/floating_ips/tables.py b/openstack_dashboard/dashboards/project/floating_ips/tables.py
index fe0984491..de7b0fb58 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/tables.py
+++ b/openstack_dashboard/dashboards/project/floating_ips/tables.py
@@ -49,7 +49,7 @@ class AllocateIP(tables.LinkAction):
     def allowed(self, request, fip=None):
         usages = quotas.tenant_quota_usages(request,
                                             targets=('floatingip', ))
-        if usages['floatingip']['available'] <= 0:
+        if 'floatingip' in usages and usages['floatingip']['available'] <= 0:
             if "disabled" not in self.classes:
                 self.classes = [c for c in self.classes] + ['disabled']
                 self.verbose_name = format_lazy(
diff --git a/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/_allocate.html b/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/_allocate.html
index 6bd9e48ac..8dd180100 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/_allocate.html
+++ b/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/_allocate.html
@@ -5,26 +5,30 @@
 <div class="quota-dynamic">
   <h3>{% trans "Description:" %}</h3>
   <p>{% trans "Allocate a floating IP from a given floating IP pool." %}</p>
+  {% if usages %}
+    <h3>{% trans "Project Quotas" %}</h3>
 
-  <h3>{% trans "Project Quotas" %}</h3>
-  <div class="quota_title">
-    <div class="pull-left">
-      <strong>{% trans "Floating IP" %}</strong>
-    </div>
-    <span class="pull-right">
-        {% blocktrans with used=usages.floatingip.used quota=usages.floatingip.quota|quotainf %}{{ used }} of {{ quota }} Used{% endblocktrans %}
-    </span>
-  </div>
-  <div id="floating_ip_progress"
-       class="quota_bar"
-       data-quota-used="{{ usages.floatingip.used }}"
-       data-quota-limit="{{ usages.floatingip.quota }}">
-  {% widthratio usages.floatingip.used usages.floatingip.quota 100 as ip_percent %}
-  {% widthratio 100 usages.floatingip.quota 1 as single_step %}
-  {% bs_progress_bar ip_percent single_step %}
-  </div>
-</div>
+    {% if usages.floatingip %}
+      <div class="quota_title">
+        <div class="pull-left">
+          <strong>{% trans "Floating IP" %}</strong>
+        </div>
+        <span class="pull-right">
+            {% blocktrans with used=usages.floatingip.used quota=usages.floatingip.quota|quotainf %}{{ used }} of {{ quota }} Used{% endblocktrans %}
+        </span>
+      </div>
+      <div id="floating_ip_progress"
+           class="quota_bar"
+           data-quota-used="{{ usages.floatingip.used }}"
+           data-quota-limit="{{ usages.floatingip.quota }}">
+      {% widthratio usages.floatingip.used usages.floatingip.quota 100 as ip_percent %}
+      {% widthratio 100 usages.floatingip.quota 1 as single_step %}
+      {% bs_progress_bar ip_percent single_step %}
+      </div>
+    {% endif %}
+  {% endif %}
 
+</div>
   <script type="text/javascript" charset="utf-8">
     if(typeof horizon.Quota !== 'undefined') {
       horizon.Quota.init();
diff --git a/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/allocate.html b/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/allocate.html
index 17b7957d4..5f7b3fc13 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/allocate.html
+++ b/openstack_dashboard/dashboards/project/floating_ips/templates/floating_ips/allocate.html
@@ -1,6 +1,8 @@
 {% extends 'base.html' %}
 {% load i18n %}
 
+{% block title %}{{ page_title }}{% endblock %}
+
 {% block main %}
   {% include 'project/floating_ips/_allocate.html' %}
 {% endblock %}
diff --git a/openstack_dashboard/dashboards/project/floating_ips/views.py b/openstack_dashboard/dashboards/project/floating_ips/views.py
index affbe28a3..252fe60c6 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/views.py
+++ b/openstack_dashboard/dashboards/project/floating_ips/views.py
@@ -62,7 +62,8 @@ class AllocateView(forms.ModalFormView):
         context = super(AllocateView, self).get_context_data(**kwargs)
         try:
             context['usages'] = quotas.tenant_quota_usages(
-                self.request, targets=('floatingip', ))
+                self.request, targets=('floatingip', )
+            )
         except Exception:
             exceptions.handle(self.request)
         return context
diff --git a/openstack_dashboard/dashboards/project/floating_ips/workflows.py b/openstack_dashboard/dashboards/project/floating_ips/workflows.py
index 44da77da0..b3a0e0338 100644
--- a/openstack_dashboard/dashboards/project/floating_ips/workflows.py
+++ b/openstack_dashboard/dashboards/project/floating_ips/workflows.py
@@ -23,9 +23,9 @@ from horizon.utils import memoized
 from horizon import workflows
 
 from openstack_dashboard import api
+from openstack_dashboard import policy
 from openstack_dashboard.utils import filters
 
-
 ALLOCATE_URL = "horizon:project:floating_ips:allocate"
 
 
@@ -34,8 +34,7 @@ class AssociateIPAction(workflows.Action):
     ip_id = forms.ThemableDynamicTypedChoiceField(
         label=_("IP Address"),
         coerce=filters.get_int_or_uuid,
-        empty_value=None,
-        add_item_link=ALLOCATE_URL
+        empty_value=None
     )
     instance_id = forms.ThemableChoiceField(
         label=_("Port to be associated")
@@ -56,6 +55,11 @@ class AssociateIPAction(workflows.Action):
         # and set the initial value of instance_id ChoiceField.
         q_instance_id = self.request.GET.get('instance_id')
         q_port_id = self.request.GET.get('port_id')
+
+        if policy.check((("network", "create_floatingip"),),
+                        request=self.request):
+            self.fields['ip_id'].widget.add_item_link = ALLOCATE_URL
+
         if q_instance_id:
             targets = self._get_target_list(q_instance_id)
             # Setting the initial value here is required to avoid a situation
diff --git a/openstack_dashboard/dashboards/project/overview/tests.py b/openstack_dashboard/dashboards/project/overview/tests.py
index d01f13686..adfad843b 100644
--- a/openstack_dashboard/dashboards/project/overview/tests.py
+++ b/openstack_dashboard/dashboards/project/overview/tests.py
@@ -35,13 +35,16 @@ INDEX_URL = reverse('horizon:project:overview:index')
 
 class UsageViewTests(test.TestCase):
 
-    @test.create_mocks({api.nova: (
-        'usage_get',
-        'extension_supported',
-    )})
+    @test.create_mocks({
+        api.nova: ('usage_get', 'extension_supported',),
+        api.neutron: ('is_quotas_extension_supported',)
+    })
     def _stub_api_calls(self, nova_stu_enabled=True,
                         stu_exception=False, overview_days_range=1,
-                        quota_usage_overrides=None):
+                        quota_usage_overrides=None,
+                        quota_extension_support=True):
+        self.mock_is_quotas_extension_supported.return_value = \
+            quota_extension_support
         self.mock_extension_supported.side_effect = [nova_stu_enabled,
                                                      nova_stu_enabled]
         if nova_stu_enabled:
@@ -255,9 +258,11 @@ class UsageViewTests(test.TestCase):
 
         self._check_api_calls(nova_stu_enabled=True)
 
-    def _test_usage_charts(self, quota_usage_overrides=None):
+    def _test_usage_charts(self, quota_usage_overrides=None,
+                           quota_extension_support=True):
         self._stub_api_calls(nova_stu_enabled=False,
-                             quota_usage_overrides=quota_usage_overrides)
+                             quota_usage_overrides=quota_usage_overrides,
+                             quota_extension_support=quota_extension_support)
 
         res = self.client.get(reverse('horizon:project:overview:index'))
 
@@ -301,6 +306,14 @@ class UsageViewTests(test.TestCase):
         self.assertEqual(1234, chart_fip['used'])
         self.assertEqual('1,234', chart_fip['used_display'])
 
+    def test_disallowed_network_chart(self):
+        res = self._test_usage_charts(
+            quota_usage_overrides={'floatingip': {'quota': -1, 'used': 1234}},
+            quota_extension_support=False)
+        charts = res.context['charts']
+        self.assertEqual(['Compute', 'Volume'],
+                         [c['title'] for c in charts])
+
     def test_usage_charts_infinite_quota(self):
         res = self._test_usage_charts(
             quota_usage_overrides={'floatingip': {'quota': -1}})
diff --git a/openstack_dashboard/test/unit/usage/test_quotas.py b/openstack_dashboard/test/unit/usage/test_quotas.py
index c555b6280..91baf2b5b 100644
--- a/openstack_dashboard/test/unit/usage/test_quotas.py
+++ b/openstack_dashboard/test/unit/usage/test_quotas.py
@@ -113,12 +113,13 @@ class QuotaTests(test.APITestCase):
                 usages[item]['quota'] = float('inf')
         return usages
 
-    def assertAvailableQuotasEqual(self, expected_usages, actual_usages):
+    def assertAvailableQuotasEqual(self, expected_usages, actual_usages,
+                                   msg=None):
         expected_available = {key: value['available'] for key, value in
                               expected_usages.items() if 'available' in value}
         actual_available = {key: value['available'] for key, value in
                             actual_usages.items() if 'available' in value}
-        self.assertEqual(expected_available, actual_available)
+        self.assertEqual(expected_available, actual_available, msg=msg)
 
     @test.create_mocks({
         api.nova: (('tenant_absolute_limits', 'nova_tenant_absolute_limits'),),
@@ -370,20 +371,20 @@ class QuotaTests(test.APITestCase):
 
     def test_tenant_quota_usages_with_target_instances(self):
         self._test_tenant_quota_usages_with_target(
-            targets=('instances', ), use_cinder_call=False)
+            targets=('instances',), use_cinder_call=False)
 
     def test_tenant_quota_usages_with_target_ram(self):
         self._test_tenant_quota_usages_with_target(
-            targets=('ram', ), use_flavor_list=True, use_cinder_call=False)
+            targets=('ram',), use_flavor_list=True, use_cinder_call=False)
 
     def test_tenant_quota_usages_with_target_volume(self):
         self._test_tenant_quota_usages_with_target(
-            targets=('volumes', ), use_compute_call=False,
+            targets=('volumes',), use_compute_call=False,
             use_cinder_call=True)
 
     def test_tenant_quota_usages_with_target_compute_volume(self):
         self._test_tenant_quota_usages_with_target(
-            targets=('instances', 'cores', 'ram', 'volumes', ),
+            targets=('instances', 'cores', 'ram', 'volumes',),
             use_flavor_list=True, use_cinder_call=True)
 
     @test.create_mocks({
@@ -435,15 +436,81 @@ class QuotaTests(test.APITestCase):
 
     def test_tenant_quota_usages_neutron_with_target_network_resources(self):
         self._test_tenant_quota_usages_neutron_with_target(
-            targets=('network', 'subnet', 'router', ))
+            targets=('network', 'subnet', 'router',))
 
     def test_tenant_quota_usages_neutron_with_target_security_groups(self):
         self._test_tenant_quota_usages_neutron_with_target(
-            targets=('security_group', ))
+            targets=('security_group',))
 
     def test_tenant_quota_usages_neutron_with_target_floating_ips(self):
         self._test_tenant_quota_usages_neutron_with_target(
-            targets=('floatingip', ))
+            targets=('floatingip',))
+
+    def test_tenant_quota_usages_neutron_with_target_security_group_rule(self):
+        self._test_tenant_quota_usages_neutron_with_target(
+            targets=('security_group_rule',)
+        )
+
+    def _list_security_group_rules(self):
+        security_groups = self.security_groups.list()
+        security_group_rules = []
+        for group in security_groups:
+            security_group_rules += group.security_group_rules
+        return security_group_rules
+
+    # Tests network quota retrieval via neutron.tenant_quota_detail_get
+    # rather than quotas._get_tenant_network_usages_legacy (see
+    # quotas._get_tenant_network_usages)
+    @test.create_mocks({api.base: ('is_service_enabled',),
+                        cinder: ('is_volume_service_enabled',),
+                        api.neutron: ('floating_ip_supported',
+                                      'is_extension_supported',
+                                      'is_quotas_extension_supported',
+                                      'tenant_quota_detail_get')})
+    def test_tenant_quota_usages_non_legacy(self):
+        self._mock_service_enabled(network_enabled=True)
+        self.mock_is_extension_supported.return_value = True
+
+        test_data = [
+            ("network", self.networks.list(), 10),
+            ("subnet", self.subnets.list(), 10),
+            ("port", self.ports.list(), 100),
+            ("router", self.routers.list(), 10),
+            ("floatingip", self.floating_ips.list(), 50),
+            ("security_group", self.security_groups.list(), 20),
+            ("security_group_rule", self._list_security_group_rules(), 100)
+        ]
+
+        for datum in test_data:
+            target = datum[0]
+            used = len(datum[1])
+            limit = datum[2]
+
+            expected = {
+                target: {
+                    'used': used,
+                    'quota': limit,
+                    'available': limit - used
+                }
+            }
+
+            self.mock_tenant_quota_detail_get.return_value = {
+                target: {
+                    'reserved': 0,
+                    'used': used,
+                    'limit': limit
+                }
+            }
+
+            quota_usages = quotas.tenant_quota_usages(self.request,
+                                                      targets=(target,))
+
+            msg = "Test failure for resource: '{}'".format(target)
+            # Compare internal structure of usages to expected.
+            self.assertEqual(expected, quota_usages.usages, msg=msg)
+            # Compare available resources
+            self.assertAvailableQuotasEqual(expected, quota_usages.usages,
+                                            msg=msg)
 
     @test.create_mocks({api.base: ('is_service_enabled',),
                         cinder: ('is_volume_service_enabled',),
@@ -459,7 +526,7 @@ class QuotaTests(test.APITestCase):
                                       'router_list')})
     def _test_tenant_quota_usages_neutron_with_target(self, targets):
         self._mock_service_enabled(network_enabled=True)
-        if 'security_group' in targets:
+        if 'security_group' in targets or 'security_group_rule' in targets:
             self.mock_is_extension_supported.side_effect = [True, False]
         else:
             self.mock_is_extension_supported.side_effect = [False]
@@ -476,7 +543,7 @@ class QuotaTests(test.APITestCase):
         if 'floatingip' in targets:
             self.mock_tenant_floating_ip_list.return_value = \
                 self.floating_ips.list()
-        if 'security_group' in targets:
+        if 'security_group' in targets or 'security_group_rule' in targets:
             self.mock_security_group_list.return_value = \
                 self.security_groups.list()
 
@@ -487,7 +554,14 @@ class QuotaTests(test.APITestCase):
         subnet_used = len(self.subnets.list())
         router_used = len(self.routers.list())
         fip_used = len(self.floating_ips.list())
-        sg_used = len(self.security_groups.list())
+
+        security_groups = self.security_groups.list()
+        sg_used = len(security_groups)
+        sgr_used = sum(map(
+            lambda group: len(group.security_group_rules),
+            security_groups
+        ))
+
         expected = {
             'network': {'used': network_used, 'quota': 10,
                         'available': 10 - network_used},
@@ -497,6 +571,9 @@ class QuotaTests(test.APITestCase):
                        'available': 10 - router_used},
             'security_group': {'used': sg_used, 'quota': 20,
                                'available': 20 - sg_used},
+            'security_group_rule': {
+                'quota': 100, 'used': sgr_used, 'available': 100 - sgr_used
+            },
             'floatingip': {'used': fip_used, 'quota': 50,
                            'available': 50 - fip_used},
         }
@@ -508,7 +585,8 @@ class QuotaTests(test.APITestCase):
         self.assertAvailableQuotasEqual(expected, quota_usages.usages)
 
         self._check_service_enabled({'network': 1})
-        if 'security_group' in targets:
+
+        if 'security_group' in targets or 'security_group_rule' in targets:
             self.mock_is_extension_supported.assert_has_calls([
                 mock.call(test.IsHttpRequest(), 'security-group'),
                 mock.call(test.IsHttpRequest(), 'quota_details'),
@@ -549,7 +627,7 @@ class QuotaTests(test.APITestCase):
                 test.IsHttpRequest())
         else:
             self.mock_tenant_floating_ip_list.assert_not_called()
-        if 'security_group' in targets:
+        if 'security_group' in targets or 'security_group_rule' in targets:
             self.mock_security_group_list.assert_called_once_with(
                 test.IsHttpRequest())
         else:
diff --git a/openstack_dashboard/usage/quotas.py b/openstack_dashboard/usage/quotas.py
index 731ad4ee7..e56328515 100644
--- a/openstack_dashboard/usage/quotas.py
+++ b/openstack_dashboard/usage/quotas.py
@@ -135,6 +135,9 @@ class QuotaUsage(dict):
     def __repr__(self):
         return repr(dict(self.usages))
 
+    def __bool__(self):
+        return bool(self.usages)
+
     def get(self, key, default=None):
         return self.usages.get(key, default)
 
@@ -365,14 +368,12 @@ def _get_tenant_network_usages_legacy(request, usages, disabled_quotas,
     for quota in qs:
         usages.add_quota(quota)
 
-    # TODO(amotoki): Add security_group_rule?
     resource_lister = {
         'network': (neutron.network_list, {'tenant_id': tenant_id}),
         'subnet': (neutron.subnet_list, {'tenant_id': tenant_id}),
         'port': (neutron.port_list, {'tenant_id': tenant_id}),
         'router': (neutron.router_list, {'tenant_id': tenant_id}),
         'floatingip': (neutron.tenant_floating_ip_list, {}),
-        'security_group': (neutron.security_group_list, {}),
     }
 
     for quota_name, lister_info in resource_lister.items():
@@ -385,6 +386,26 @@ def _get_tenant_network_usages_legacy(request, usages, disabled_quotas,
                 resources = []
             usages.tally(quota_name, len(resources))
 
+    # Security groups have to be processed separately so that rules may be
+    # processed in the same api call and in a single pass
+    add_sg = 'security_group' not in disabled_quotas
+    add_sgr = 'security_group_rule' not in disabled_quotas
+
+    if add_sg or add_sgr:
+        try:
+            security_groups = neutron.security_group_list(request)
+            num_rules = sum(len(group['security_group_rules'])
+                            for group in security_groups)
+        except Exception:
+            security_groups = []
+            num_rules = 0
+
+    if add_sg:
+        usages.tally('security_group', len(security_groups))
+
+    if add_sgr:
+        usages.tally('security_group_rule', num_rules)
+
 
 @profiler.trace
 def _get_tenant_volume_usages(request, usages, disabled_quotas, tenant_id):
diff --git a/openstack_dashboard/usage/views.py b/openstack_dashboard/usage/views.py
index 5d1ea8f20..d88b56088 100644
--- a/openstack_dashboard/usage/views.py
+++ b/openstack_dashboard/usage/views.py
@@ -87,6 +87,10 @@ class UsageView(tables.DataTableView):
         return resp
 
 
+def _check_network_allowed(request):
+    return api.neutron.is_quotas_extension_supported(request)
+
+
 ChartDef = collections.namedtuple(
     'ChartDef',
     ('quota_key', 'label', 'used_phrase', 'filters'))
@@ -99,6 +103,10 @@ ChartDef = collections.namedtuple(
 # - filters to be applied to the value
 #   If None is specified, the default filter 'intcomma' will be applied.
 #   if you want to apply no filters, specify an empty tuple or list.
+# - allowed:
+#   An optional argument used to determine if the chart section should be
+#   displayed. Can be a static value or a function, which is called dynamically
+#   with the request as it's first parameter.
 CHART_DEFS = [
     {
         'title': _("Compute"),
@@ -106,7 +114,7 @@ CHART_DEFS = [
             ChartDef("instances", _("Instances"), None, None),
             ChartDef("cores", _("VCPUs"), None, None),
             ChartDef("ram", _("RAM"), None, (sizeformat.mb_float_format,)),
-        ]
+        ],
     },
     {
         'title': _("Volume"),
@@ -115,7 +123,7 @@ CHART_DEFS = [
             ChartDef("snapshots", _("Volume Snapshots"), None, None),
             ChartDef("gigabytes", _("Volume Storage"), None,
                      (sizeformat.diskgbformat,)),
-        ]
+        ],
     },
     {
         'title': _("Network"),
@@ -129,7 +137,8 @@ CHART_DEFS = [
             ChartDef("network", _("Networks"), None, None),
             ChartDef("port", _("Ports"), None, None),
             ChartDef("router", _("Routers"), None, None),
-        ]
+        ],
+        'allowed': _check_network_allowed,
     },
 ]
 
@@ -147,13 +156,21 @@ class ProjectUsageView(UsageView):
     def _get_charts_data(self):
         chart_sections = []
         for section in CHART_DEFS:
-            chart_data = self._process_chart_section(section['charts'])
-            chart_sections.append({
-                'title': section['title'],
-                'charts': chart_data
-            })
+            if self._check_chart_allowed(section):
+                chart_data = self._process_chart_section(section['charts'])
+                chart_sections.append({
+                    'title': section['title'],
+                    'charts': chart_data
+                })
         return chart_sections
 
+    def _check_chart_allowed(self, chart_def):
+        result = True
+        if 'allowed' in chart_def:
+            allowed = chart_def['allowed']
+            result = allowed(self.request) if callable(allowed) else allowed
+        return result
+
     def _process_chart_section(self, chart_defs):
         charts = []
         for t in chart_defs:
-- 
cgit v1.2.1