Fix global stack list in periodic task

The periodic task unneccessarily lists Heat stacks in the
global tenant (across all tenants) which the Magnum service
user may lack permission for. Also, the most restrictive way
to let it use global stack-list is chose a Keystone role and
open that operation to any user in any project holding that
role.

This commit substitutes a direct lookup of all bays' stack_id
attributes for this global stack list. This direct lookup will
yield the same net result. In order to get the neccessary
permissions it will use each bay's stored Keystone trust to
act on behalf of the bay's creating user.

Co-Authored-By: Jiri Suchomel <jiri.suchomel@suse.com>
Closes-Bug: #1589955
Change-Id: I67b176c137c463e37e037970cc4e468d51db30c9

doc/source/install-guide-from-source.rst

@@ -466,17 +466,7 @@ Install and configure components
 
       # su -s /bin/sh -c "/var/lib/magnum/env/bin/magnum-db-manage upgrade" magnum
 
-9. Update heat policy to allow magnum list stacks. Edit your heat policy file,
+9. Set magnum for log rotation:
-   usually ``/etc/heat/policy.json``:
-
-   .. code-block:: ini
-
-      ...
-      stacks:global_index: "role:admin",
-
-   Now restart heat.
-
-10. Set magnum for log rotation:
 
    .. code-block:: console
 

doc/source/userguide.rst

@@ -1025,8 +1025,34 @@ High Availability
 =======
 Scaling
 =======
-*To be filled in*
 
+Performance tuning for periodic task
+------------------------------------
+
+Magnum's periodic task performs a `stack-get` operation on the Heat stack
+underlying each of its bays. If you have a large amount of bays this can create
+considerable load on the Heat API. To reduce that load you can configure Magnum
+to perform one global `stack-list` per periodic task instead instead of one per
+bay. This is disabled by default, both from the Heat and Magnum side since it
+causes a security issue, though: any user in any tenant holding the `admin`
+role can perform a global `stack-list` operation if Heat is configured to allow
+it for Magnum. If you want to enable it nonetheless, proceed as follows:
+
+1. Set `periodic_global_stack_list` in magnum.conf to `True`
+   (`False` by default).
+
+2. Update heat policy to allow magnum list stacks. To this end, edit your heat
+   policy file, usually etc/heat/policy.json``:
+
+   .. code-block:: ini
+
+      ...
+      stacks:global_index: "role:admin",
+
+   Now restart heat.
+
+
+*To be filled in*
 Include auto scaling
 
 =======

install-guide/source/common/configure_4_update_heat_policy.rst

@@ -1,9 +0,0 @@
-4. Update heat policy to allow magnum list stacks. Edit your heat policy file,
-   usually ``/etc/heat/policy.json``:
-
-   .. code-block:: ini
-
-      ...
-      stacks:global_index: "role:admin",
-
-   Now restart heat.

install-guide/source/install-debian-manual.rst

@@ -21,8 +21,6 @@ Install and configure components
 
 .. include:: common/configure_3_populate_database.rst
 
-.. include:: common/configure_4_update_heat_policy.rst
-
 Finalize installation
 ---------------------
 

install-guide/source/install-obs.rst

@@ -22,8 +22,6 @@ Install and configure components
 
 .. include:: common/configure_3_populate_database.rst
 
-.. include:: common/configure_4_update_heat_policy.rst
-
 Finalize installation
 ---------------------
 

install-guide/source/install-rdo.rst

@@ -31,8 +31,6 @@ Install and configure components
 
 .. include:: common/configure_3_populate_database.rst
 
-.. include:: common/configure_4_update_heat_policy.rst
-
 Finalize installation
 ---------------------
 

install-guide/source/install-ubuntu.rst

@@ -21,8 +21,6 @@ Install and configure components
 
 .. include:: common/configure_3_populate_database.rst
 
-.. include:: common/configure_4_update_heat_policy.rst
-
 Finalize installation
 ---------------------
 

magnum/common/context.py

@@ -11,22 +11,30 @@
 # under the License.
 
 from eventlet.green import threading
+from oslo_config import cfg
 from oslo_context import context
 
+CONF = cfg.CONF
+
 
 class RequestContext(context.RequestContext):
     """Extends security contexts from the OpenStack common library."""
 
     def __init__(self, auth_token=None, auth_url=None, domain_id=None,
                  domain_name=None, user_name=None, user_id=None,
+                 user_domain_name=None, user_domain_id=None,
                  project_name=None, project_id=None, roles=None,
                  is_admin=False, read_only=False, show_deleted=False,
                  request_id=None, trust_id=None, auth_token_info=None,
-                 all_tenants=False, **kwargs):
+                 all_tenants=False, password=None, **kwargs):
         """Stores several additional request parameters:
 
         :param domain_id: The ID of the domain.
         :param domain_name: The name of the domain.
+        :param user_domain_id: The ID of the domain to
+                               authenticate a user against.
+        :param user_domain_name: The name of the domain to
+                                 authenticate a user against.
 
         """
         super(RequestContext, self).__init__(auth_token=auth_token,
@@ -43,11 +51,14 @@ class RequestContext(context.RequestContext):
         self.project_id = project_id
         self.domain_id = domain_id
         self.domain_name = domain_name
+        self.user_domain_id = user_domain_id
+        self.user_domain_name = user_domain_name
         self.roles = roles
         self.auth_url = auth_url
         self.auth_token_info = auth_token_info
         self.trust_id = trust_id
         self.all_tenants = all_tenants
+        self.password = password
 
     def to_dict(self):
         value = super(RequestContext, self).to_dict()
@@ -55,6 +66,8 @@ class RequestContext(context.RequestContext):
                       'auth_url': self.auth_url,
                       'domain_id': self.domain_id,
                       'domain_name': self.domain_name,
+                      'user_domain_id': self.user_domain_id,
+                      'user_domain_name': self.user_domain_name,
                       'user_name': self.user_name,
                       'user_id': self.user_id,
                       'project_name': self.project_name,
@@ -66,6 +79,7 @@ class RequestContext(context.RequestContext):
                       'request_id': self.request_id,
                       'trust_id': self.trust_id,
                       'auth_token_info': self.auth_token_info,
+                      'password': self.password,
                       'all_tenants': self.all_tenants})
         return value
 
@@ -91,6 +105,21 @@ def make_admin_context(show_deleted=False, all_tenants=False):
     return context
 
 
+def make_bay_context(bay, show_deleted=False):
+    """Create a user context based on a bay's stored Keystone trust.
+
+    :param bay: the bay supplying the Keystone trust to use
+    :param show_deleted: if True, will show deleted items when query db
+    """
+    context = RequestContext(user_name=bay.trustee_username,
+                             password=bay.trustee_password,
+                             trust_id=bay.trust_id,
+                             show_deleted=show_deleted,
+                             user_domain_id=CONF.trust.trustee_domain_id,
+                             user_domain_name=CONF.trust.trustee_domain_name)
+    return context
+
+
 _CTX_STORE = threading.local()
 _CTX_KEY = 'current_ctx'
 

magnum/common/keystone.py

@@ -113,7 +113,7 @@ class KeystoneClientV3(object):
         return session
 
     def _get_auth(self):
-        if self.context.is_admin or self.context.trust_id:
+        if self.context.is_admin:
             try:
                 auth = ka_loading.load_auth_from_conf_options(CONF, CFG_GROUP)
             except ka_exception.MissingRequiredOptions:
@@ -125,6 +125,18 @@ class KeystoneClientV3(object):
         elif self.context.auth_token:
             auth = ka_v3.Token(auth_url=self.auth_url,
                                token=self.context.auth_token)
+        elif self.context.trust_id:
+            auth_info = {
+                'auth_url': self.auth_url,
+                'username': self.context.user_name,
+                'password': self.context.password,
+                'user_domain_id': self.context.user_domain_id,
+                'user_domain_name': self.context.user_domain_name,
+                'trust_id': self.context.trust_id
+            }
+
+            auth = ka_v3.Password(**auth_info)
+
         else:
             LOG.error(_LE('Keystone API connection failed: no password, '
                           'trust_id or token found.'))

magnum/common/rpc_service.py

@@ -42,6 +42,13 @@ TRANSPORT_ALIASES = {
 }
 
 periodic_opts = [
+    cfg.BoolOpt('periodic_global_stack_list',
+                default=False,
+                help="List Heat stacks globally when syncing bays. "
+                     "Default is to do retrieve each bay's stack  "
+                     "individually. Reduces number of requests against "
+                     "Heat API if enabled but requires changes to Heat's "
+                     "policy.json."),
     cfg.BoolOpt('periodic_enable',
                 default=True,
                 help='Enable periodic tasks.'),

magnum/service/periodic.py

@@ -15,6 +15,8 @@
 
 import functools
 
+from heatclient import exc as heat_exc
+from oslo_config import cfg
 from oslo_log import log
 from oslo_service import periodic_task
 import six
@@ -31,6 +33,7 @@ from magnum import objects
 from magnum.objects.fields import BayStatus as bay_status
 
 
+CONF = cfg.CONF
 LOG = log.getLogger(__name__)
 
 
@@ -80,8 +83,14 @@ class MagnumPeriodicTasks(periodic_task.PeriodicTasks):
             sid_to_bay_mapping = {bay.stack_id: bay for bay in bays}
             bay_stack_ids = sid_to_bay_mapping.keys()
 
-            stacks = osc.heat().stacks.list(global_tenant=True,
+            if CONF.periodic_global_stack_list:
-                                            filters={'id': bay_stack_ids})
+                stacks = osc.heat().stacks.list(global_tenant=True,
+                                                filters={'id': bay_stack_ids})
+            else:
+                ret = self._get_bay_stacks(bays, sid_to_bay_mapping,
+                                           bay_stack_ids)
+                [stacks, bays, bay_stack_ids, sid_to_bay_mapping] = ret
+
             sid_to_stack_mapping = {s.id: s for s in stacks}
 
             # intersection of bays magnum has and heat has
@@ -102,6 +111,36 @@ class MagnumPeriodicTasks(periodic_task.PeriodicTasks):
                 "Ignore error [%s] when syncing up bay status."
             ), e, exc_info=True)
 
+    def _get_bay_stacks(self, bays, sid_to_bay_mapping, bay_stack_ids):
+        stacks = []
+
+        _bays = bays
+        _sid_to_bay_mapping = sid_to_bay_mapping
+        _bay_stack_ids = bay_stack_ids
+
+        for bay in _bays:
+            try:
+                # Create client with bay's trustee user context
+                bosc = clients.OpenStackClients(
+                    context.make_bay_context(bay))
+                stack = bosc.heat().stacks.get(bay.stack_id)
+                stacks.append(stack)
+            # No need to do anything in this case
+            except heat_exc.HTTPNotFound:
+                pass
+            except Exception as e:
+                # Any other exception means we do not perform any
+                # action on this bay in the current sync run, so remove
+                # it from all records.
+                LOG.warning("Exception while attempting to retrieve "
+                            "Heat stack %s for bay %s. Traceback "
+                            "follows.")
+                LOG.warning(e)
+                _sid_to_bay_mapping.pop(bay.stack_id)
+                _bay_stack_ids.remove(bay.stack_id)
+                _bays.remove(bay)
+        return [stacks, _bays, _bay_stack_ids, _sid_to_bay_mapping]
+
     def _sync_existing_bay(self, bay, stack):
         if bay.status != stack.stack_status:
             old_status = bay.status

magnum/tests/unit/db/utils.py

@@ -73,7 +73,7 @@ def create_test_baymodel(**kw):
 
 
 def get_test_bay(**kw):
-    return {
+    attrs = {
         'id': kw.get('id', 42),
         'uuid': kw.get('uuid', '5d12f6fd-a196-4bf0-ae4c-1f639a523a52'),
         'name': kw.get('name', 'bay1'),
@@ -97,6 +97,14 @@ def get_test_bay(**kw):
         'updated_at': kw.get('updated_at'),
     }
 
+    # Only add Keystone trusts related attributes on demand since they may
+    # break other tests.
+    for attr in ['trustee_username', 'trustee_password', 'trust_id']:
+        if attr in kw:
+            attrs[attr] = kw[attr]
+
+    return attrs
+
 
 def create_test_bay(**kw):
     """Create test bay entry in DB and return Bay DB object.

magnum/tests/unit/service/test_periodic.py

@@ -12,6 +12,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from heatclient import exc as heat_exc
 import mock
 
 from magnum.common import context
@@ -37,14 +38,25 @@ class PeriodicTestCase(base.TestCase):
 
         ctx = context.make_admin_context()
 
-        bay1 = utils.get_test_bay(id=1, stack_id='11',
+        # Can be identical for all bays.
-                                  status=bay_status.CREATE_IN_PROGRESS)
+        trust_attrs = {
-        bay2 = utils.get_test_bay(id=2, stack_id='22',
+            'trustee_username': '5d12f6fd-a196-4bf0-ae4c-1f639a523a52',
-                                  status=bay_status.DELETE_IN_PROGRESS)
+            'trustee_password': 'ain7einaebooVaig6d',
-        bay3 = utils.get_test_bay(id=3, stack_id='33',
+            'trust_id': '39d920ca-67c6-4047-b57a-01e9e16bb96f',
-                                  status=bay_status.UPDATE_IN_PROGRESS)
+            }
-        bay4 = utils.get_test_bay(id=4, stack_id='44',
+
-                                  status=bay_status.CREATE_COMPLETE)
+        trust_attrs.update({'id': 1, 'stack_id': '11',
+                           'status': bay_status.CREATE_IN_PROGRESS})
+        bay1 = utils.get_test_bay(**trust_attrs)
+        trust_attrs.update({'id': 2, 'stack_id': '22',
+                           'status': bay_status.DELETE_IN_PROGRESS})
+        bay2 = utils.get_test_bay(**trust_attrs)
+        trust_attrs.update({'id': 3, 'stack_id': '33',
+                           'status': bay_status.UPDATE_IN_PROGRESS})
+        bay3 = utils.get_test_bay(**trust_attrs)
+        trust_attrs.update({'id': 4, 'stack_id': '44',
+                           'status': bay_status.CREATE_COMPLETE})
+        bay4 = utils.get_test_bay(**trust_attrs)
 
         self.bay1 = objects.Bay(ctx, **bay1)
         self.bay2 = objects.Bay(ctx, **bay2)
@@ -63,6 +75,14 @@ class PeriodicTestCase(base.TestCase):
         stack3 = fake_stack(id='33', stack_status=bay_status.UPDATE_COMPLETE,
                             stack_status_reason='fake_reason_33')
         mock_heat_client.stacks.list.return_value = [stack1, stack3]
+        get_stacks = {'11': stack1, '33': stack3}
+
+        def stack_get_sideefect(arg):
+            if arg == '22':
+                raise heat_exc.HTTPNotFound
+            return get_stacks[arg]
+
+        mock_heat_client.stacks.get.side_effect = stack_get_sideefect
         mock_osc = mock_oscc.return_value
         mock_osc.heat.return_value = mock_heat_client
         mock_bay_list.return_value = [self.bay1, self.bay2, self.bay3]
@@ -79,6 +99,33 @@ class PeriodicTestCase(base.TestCase):
         self.assertEqual(bay_status.UPDATE_COMPLETE, self.bay3.status)
         self.assertEqual('fake_reason_33', self.bay3.status_reason)
 
+    @mock.patch.object(objects.Bay, 'list')
+    @mock.patch('magnum.common.clients.OpenStackClients')
+    def test_sync_auth_fail(self, mock_oscc, mock_bay_list):
+        """Tests handling for unexpected exceptions in _get_bay_stacks()
+
+        It does this by raising an a HTTPUnauthorized exception in Heat client.
+        The affected stack thus missing from the stack list should not lead to
+        bay state changing in this case. Likewise, subsequent bays should still
+        change state, despite the affected bay being skipped.
+        """
+        stack1 = fake_stack(id='11',
+                            stack_status=bay_status.CREATE_COMPLETE)
+
+        mock_heat_client = mock.MagicMock()
+
+        def stack_get_sideefect(arg):
+            raise heat_exc.HTTPUnauthorized
+
+        mock_heat_client.stacks.get.side_effect = stack_get_sideefect
+        mock_heat_client.stacks.list.return_value = [stack1]
+        mock_osc = mock_oscc.return_value
+        mock_osc.heat.return_value = mock_heat_client
+        mock_bay_list.return_value = [self.bay1]
+        periodic.MagnumPeriodicTasks(CONF).sync_bay_status(None)
+
+        self.assertEqual(bay_status.CREATE_IN_PROGRESS, self.bay1.status)
+
     @mock.patch.object(objects.Bay, 'list')
     @mock.patch('magnum.common.clients.OpenStackClients')
     def test_sync_bay_status_not_changes(self, mock_oscc, mock_bay_list):
@@ -89,6 +136,14 @@ class PeriodicTestCase(base.TestCase):
                             stack_status=bay_status.DELETE_IN_PROGRESS)
         stack3 = fake_stack(id='33',
                             stack_status=bay_status.UPDATE_IN_PROGRESS)
+        get_stacks = {'11': stack1, '22': stack2, '33': stack3}
+
+        def stack_get_sideefect(arg):
+            if arg == '22':
+                raise heat_exc.HTTPNotFound
+            return get_stacks[arg]
+
+        mock_heat_client.stacks.get.side_effect = stack_get_sideefect
         mock_heat_client.stacks.list.return_value = [stack1, stack2, stack3]
         mock_osc = mock_oscc.return_value
         mock_osc.heat.return_value = mock_heat_client