From f895b2bd0922f29a9d6b08617cb60258fa101c68 Mon Sep 17 00:00:00 2001 From: Johannes Grassler Date: Tue, 7 Jun 2016 14:26:29 +0200 Subject: [PATCH] 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 Closes-Bug: #1589955 Change-Id: I67b176c137c463e37e037970cc4e468d51db30c9 --- doc/source/install-guide-from-source.rst | 12 +--- doc/source/userguide.rst | 28 +++++++- .../common/configure_4_update_heat_policy.rst | 9 --- .../source/install-debian-manual.rst | 2 - install-guide/source/install-obs.rst | 2 - install-guide/source/install-rdo.rst | 2 - install-guide/source/install-ubuntu.rst | 2 - magnum/common/context.py | 31 +++++++- magnum/common/keystone.py | 14 +++- magnum/common/rpc_service.py | 7 ++ magnum/service/periodic.py | 43 ++++++++++- magnum/tests/unit/db/utils.py | 10 ++- magnum/tests/unit/service/test_periodic.py | 71 ++++++++++++++++--- 13 files changed, 191 insertions(+), 42 deletions(-) delete mode 100644 install-guide/source/common/configure_4_update_heat_policy.rst diff --git a/doc/source/install-guide-from-source.rst b/doc/source/install-guide-from-source.rst index cb0fb601e0..065403d303 100644 --- a/doc/source/install-guide-from-source.rst +++ b/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, - usually ``/etc/heat/policy.json``: - - .. code-block:: ini - - ... - stacks:global_index: "role:admin", - - Now restart heat. - -10. Set magnum for log rotation: +9. Set magnum for log rotation: .. code-block:: console diff --git a/doc/source/userguide.rst b/doc/source/userguide.rst index 5149ce4626..445368453e 100644 --- a/doc/source/userguide.rst +++ b/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 ======= diff --git a/install-guide/source/common/configure_4_update_heat_policy.rst b/install-guide/source/common/configure_4_update_heat_policy.rst deleted file mode 100644 index 8362283bb3..0000000000 --- a/install-guide/source/common/configure_4_update_heat_policy.rst +++ /dev/null @@ -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. diff --git a/install-guide/source/install-debian-manual.rst b/install-guide/source/install-debian-manual.rst index e9ee28e24b..557c7a6299 100644 --- a/install-guide/source/install-debian-manual.rst +++ b/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 --------------------- diff --git a/install-guide/source/install-obs.rst b/install-guide/source/install-obs.rst index 1593fb44fe..0f69c2ab63 100644 --- a/install-guide/source/install-obs.rst +++ b/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 --------------------- diff --git a/install-guide/source/install-rdo.rst b/install-guide/source/install-rdo.rst index 09ff60887f..83123873e7 100644 --- a/install-guide/source/install-rdo.rst +++ b/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 --------------------- diff --git a/install-guide/source/install-ubuntu.rst b/install-guide/source/install-ubuntu.rst index 829acf9a1b..2e9613b9b8 100644 --- a/install-guide/source/install-ubuntu.rst +++ b/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 --------------------- diff --git a/magnum/common/context.py b/magnum/common/context.py index 9e8ebf8300..c1f52053a1 100644 --- a/magnum/common/context.py +++ b/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' diff --git a/magnum/common/keystone.py b/magnum/common/keystone.py index 4195f30a33..c31188256a 100644 --- a/magnum/common/keystone.py +++ b/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.')) diff --git a/magnum/common/rpc_service.py b/magnum/common/rpc_service.py index 152f51b78b..b363eafbc1 100644 --- a/magnum/common/rpc_service.py +++ b/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.'), diff --git a/magnum/service/periodic.py b/magnum/service/periodic.py index 3ca9f4e018..b7a80c7866 100644 --- a/magnum/service/periodic.py +++ b/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, - filters={'id': bay_stack_ids}) + if CONF.periodic_global_stack_list: + 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 diff --git a/magnum/tests/unit/db/utils.py b/magnum/tests/unit/db/utils.py index 68c4b90933..9817ed4fc1 100644 --- a/magnum/tests/unit/db/utils.py +++ b/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. diff --git a/magnum/tests/unit/service/test_periodic.py b/magnum/tests/unit/service/test_periodic.py index 91a498f210..b32c901086 100644 --- a/magnum/tests/unit/service/test_periodic.py +++ b/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', - status=bay_status.CREATE_IN_PROGRESS) - bay2 = utils.get_test_bay(id=2, stack_id='22', - status=bay_status.DELETE_IN_PROGRESS) - bay3 = utils.get_test_bay(id=3, stack_id='33', - status=bay_status.UPDATE_IN_PROGRESS) - bay4 = utils.get_test_bay(id=4, stack_id='44', - status=bay_status.CREATE_COMPLETE) + # Can be identical for all bays. + trust_attrs = { + 'trustee_username': '5d12f6fd-a196-4bf0-ae4c-1f639a523a52', + 'trustee_password': 'ain7einaebooVaig6d', + 'trust_id': '39d920ca-67c6-4047-b57a-01e9e16bb96f', + } + + 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