From 9f6465fe8c6732da7bb4f401b230a17f021f47a8 Mon Sep 17 00:00:00 2001 From: Ben Nemec Date: Fri, 10 Feb 2017 19:27:13 +0000 Subject: [PATCH] Change _member_role_exists to work with current upgrade flow Because we now shut down all OpenStack services before upgrading the undercloud, we can't run API queries against them prior to running puppet. This means we now need to handle the _member_ role in a post config step. Note that this was only not failing before because the check in _stackrc_exists was always returning false, so the old code was never actually run. That also means it was not doing what it was supposed to, of course. Change-Id: Iaf81adc48321dbc985c50aeb5bfc0cc94810cb1d Closes-Bug: 1663719 --- .../puppet-stack-config.pp | 9 -- .../puppet-stack-config.yaml.template | 4 - instack_undercloud/tests/test_undercloud.py | 87 ++++++++++++------- instack_undercloud/undercloud.py | 75 ++++++++-------- ...maintain-member-role-ecc556d81ce583a1.yaml | 9 ++ 5 files changed, 98 insertions(+), 86 deletions(-) create mode 100644 releasenotes/notes/maintain-member-role-ecc556d81ce583a1.yaml diff --git a/elements/puppet-stack-config/puppet-stack-config.pp b/elements/puppet-stack-config/puppet-stack-config.pp index c781867ef..1ccd35c04 100644 --- a/elements/puppet-stack-config/puppet-stack-config.pp +++ b/elements/puppet-stack-config/puppet-stack-config.pp @@ -307,15 +307,6 @@ keystone_config { 'ec2/driver': value => 'keystone.contrib.ec2.backends.sql.Ec2'; } -if str2bool(hiera('member_role_exists', false)) { - # Old deployments where assigning _member_ role to admin user. - # The _member_ role is needed because it's delegated via heat trusts in - # existing deployments, hence existing role assignments can't just be - # deleted. This Puppet Collector will allow to update deployments with - # admin role managed by Puppet. - Keystone_user_role<| title == 'admin@admin' |> { roles +> ['_member_'] } -} - # TODO: notifications, scrubber, etc. class { '::glance::api': enable_proxy_headers_parsing => $enable_proxy_headers_parsing, diff --git a/elements/puppet-stack-config/puppet-stack-config.yaml.template b/elements/puppet-stack-config/puppet-stack-config.yaml.template index 61a98cefc..0a6d9b8aa 100644 --- a/elements/puppet-stack-config/puppet-stack-config.yaml.template +++ b/elements/puppet-stack-config/puppet-stack-config.yaml.template @@ -31,10 +31,6 @@ tripleo::profile::base::haproxy::certificates_specs: # CA defaults certmonger_ca: {{CERTIFICATE_GENERATION_CA}} -# Workaround for puppet deleting _member_ role assignment on old deployments -member_role_exists: {{MEMBER_ROLE_EXISTS}} - - # Common Hiera data gets applied to all nodes ssh::server::storeconfigs_enabled: false diff --git a/instack_undercloud/tests/test_undercloud.py b/instack_undercloud/tests/test_undercloud.py index 3db58ae5d..8d22df72e 100644 --- a/instack_undercloud/tests/test_undercloud.py +++ b/instack_undercloud/tests/test_undercloud.py @@ -20,6 +20,7 @@ import subprocess import tempfile import fixtures +from keystoneauth1 import exceptions as ks_exceptions import mock from mistralclient.api import base as mistralclient_base from novaclient import exceptions @@ -322,11 +323,6 @@ class TestGenerateEnvironment(BaseTestCase): self.useFixture(self.mock_distro) self.mock_distro.mock.return_value = [ 'Red Hat Enterprise Linux Server 7.1'] - stackrc_exists_patcher = mock.patch( - 'instack_undercloud.undercloud._stackrc_exists', - return_value=False) - stackrc_exists_patcher.start() - self.addCleanup(stackrc_exists_patcher.stop) @mock.patch('socket.gethostname') def test_hostname_set(self, mock_gethostname): @@ -620,6 +616,7 @@ class TestConfigureSshKeys(base.BaseTestCase): class TestPostConfig(base.BaseTestCase): + @mock.patch('instack_undercloud.undercloud._member_role_exists') @mock.patch('novaclient.client.Client', autospec=True) @mock.patch('mistralclient.api.client.client', autospec=True) @mock.patch('instack_undercloud.undercloud._delete_default_flavors') @@ -631,7 +628,7 @@ class TestPostConfig(base.BaseTestCase): def test_post_config(self, mock_post_config_mistral, mock_ensure_flavor, mock_configure_ssh_keys, mock_get_auth_values, mock_copy_stackrc, mock_delete, mock_mistral_client, - mock_nova_client): + mock_nova_client, mock_member_role_exists): instack_env = { 'UNDERCLOUD_ENDPOINT_MISTRAL_PUBLIC': 'http://192.168.24.1:8989/v2', @@ -746,13 +743,6 @@ class TestPostConfig(base.BaseTestCase): ] mock_run.assert_has_calls(calls) - @mock.patch('instack_undercloud.undercloud._stackrc_exists') - def test_member_role_exists_nostackrc(self, mock_stackrc_exists): - mock_stackrc_exists.return_value = False - instack_env = {} - undercloud._member_role_exists(instack_env) - self.assertEqual({'MEMBER_ROLE_EXISTS': 'False'}, instack_env) - def _mock_ksclient_roles(self, mock_auth_values, mock_ksdiscover, roles): mock_auth_values.return_value = ('user', 'password', 'tenant', 'http://test:123') @@ -769,29 +759,62 @@ class TestPostConfig(base.BaseTestCase): mock_client.roles = mock_roles mock_discover.create_client.return_value = mock_client - @mock.patch('keystoneclient.discover.Discover') - @mock.patch('instack_undercloud.undercloud._get_auth_values') - @mock.patch('instack_undercloud.undercloud._stackrc_exists') - def test_member_role_exists(self, mock_stackrc_exists, mock_auth_values, - mock_ksdiscover): - mock_stackrc_exists.return_value = True - self._mock_ksclient_roles(mock_auth_values, mock_ksdiscover, - ['admin']) - instack_env = {} - undercloud._member_role_exists(instack_env) - self.assertEqual({'MEMBER_ROLE_EXISTS': 'False'}, instack_env) + mock_tenant_list = [mock.Mock(), mock.Mock()] + mock_tenant_list[0].name = 'admin' + mock_tenant_list[0].id = 'admin-id' + mock_tenant_list[1].name = 'service' + mock_tenant_list[1].id = 'service-id' + mock_client.tenants.list.return_value = mock_tenant_list + + mock_user_list = [mock.Mock(), mock.Mock()] + mock_user_list[0].name = 'admin' + mock_user_list[1].name = 'nova' + mock_client.users.list.return_value = mock_user_list + return mock_client @mock.patch('keystoneclient.discover.Discover') @mock.patch('instack_undercloud.undercloud._get_auth_values') - @mock.patch('instack_undercloud.undercloud._stackrc_exists') - def test_member_role_exists_true(self, mock_stackrc_exists, + @mock.patch('os.path.isfile') + def test_member_role_exists(self, mock_isfile, mock_auth_values, + mock_ksdiscover): + mock_isfile.return_value = True + mock_client = self._mock_ksclient_roles(mock_auth_values, + mock_ksdiscover, + ['admin']) + undercloud._member_role_exists() + self.assertFalse(mock_client.tenants.list.called) + + @mock.patch('keystoneclient.discover.Discover') + @mock.patch('instack_undercloud.undercloud._get_auth_values') + @mock.patch('os.path.isfile') + def test_member_role_exists_true(self, mock_isfile, mock_auth_values, mock_ksdiscover): - mock_stackrc_exists.return_value = True - self._mock_ksclient_roles(mock_auth_values, mock_ksdiscover, - ['admin', '_member_']) - instack_env = {} - undercloud._member_role_exists(instack_env) - self.assertEqual({'MEMBER_ROLE_EXISTS': 'True'}, instack_env) + mock_isfile.return_value = True + mock_client = self._mock_ksclient_roles(mock_auth_values, + mock_ksdiscover, + ['admin', '_member_']) + undercloud._member_role_exists() + mock_user = mock_client.users.list.return_value[0] + mock_role = mock_client.roles.list.return_value[1] + mock_client.roles.add_user_role.assert_called_once_with( + mock_user, mock_role, 'admin-id') + + @mock.patch('keystoneclient.discover.Discover') + @mock.patch('instack_undercloud.undercloud._get_auth_values') + @mock.patch('os.path.isfile') + def test_has_member_role(self, mock_isfile, mock_auth_values, + mock_ksdiscover): + mock_isfile.return_value = True + mock_client = self._mock_ksclient_roles(mock_auth_values, + mock_ksdiscover, + ['admin', '_member_']) + fake_exception = ks_exceptions.http.Conflict('test') + mock_client.roles.add_user_role.side_effect = fake_exception + undercloud._member_role_exists() + mock_user = mock_client.users.list.return_value[0] + mock_role = mock_client.roles.list.return_value[1] + mock_client.roles.add_user_role.assert_called_once_with( + mock_user, mock_role, 'admin-id') def _create_flavor_mocks(self): mock_nova = mock.Mock() diff --git a/instack_undercloud/undercloud.py b/instack_undercloud/undercloud.py index 3a0dcb1d9..c726d91b8 100644 --- a/instack_undercloud/undercloud.py +++ b/instack_undercloud/undercloud.py @@ -30,7 +30,7 @@ import tempfile import time import uuid -from keystoneclient import exceptions as ks_exceptions +from keystoneauth1 import exceptions as ks_exceptions from keystoneclient import auth from keystoneclient import session from keystoneclient import discover @@ -901,39 +901,40 @@ def _write_password_file(instack_env): password_file.write('%s=%s\n' % (opt.name, value)) -def _member_role_exists(instack_env): +def _member_role_exists(): # This is a workaround for puppet removing the deprecated _member_ - # role on upgrade - if it exists we must not remove role assignments + # role on upgrade - if it exists we must restore role assignments # or trusts stored in the undercloud heat will break - if not _stackrc_exists(): - instack_env['MEMBER_ROLE_EXISTS'] = 'False' - return user, password, tenant, auth_url = _get_auth_values() - role_exists = False + # Note this is made somewhat verbose due to trying to handle + # any format auth_url (versionless, v2,0/v3 suffix) + auth_plugin_class = auth.get_plugin_class('password') + auth_kwargs = { + 'auth_url': auth_url, + 'username': user, + 'password': password, + 'project_name': tenant} + if 'v2.0' not in auth_url: + auth_kwargs.update({ + 'project_domain_name': 'default', + 'user_domain_name': 'default'}) + auth_plugin = auth_plugin_class(**auth_kwargs) + sess = session.Session(auth=auth_plugin) + disc = discover.Discover(session=sess) + c = disc.create_client() try: - # Note this is made somewhat verbose due to trying to handle - # any format auth_url (versionless, v2,0/v3 suffix) - auth_plugin_class = auth.get_plugin_class('password') - auth_kwargs = { - 'auth_url': auth_url, - 'username': user, - 'password': password, - 'project_name': tenant} - if 'v2.0' not in auth_url: - auth_kwargs.update({ - 'project_domain_name': 'default', - 'user_domain_name': 'default'}) - auth_plugin = auth_plugin_class(**auth_kwargs) - sess = session.Session(auth=auth_plugin) - disc = discover.Discover(session=sess) - c = disc.create_client() - role_names = [r.name for r in c.roles.list()] - role_exists = '_member_' in role_names - except ks_exceptions.ConnectionError: - # This will happen on initial deployment, assume False - # as no new deployments should have _member_ - role_exists = False - instack_env['MEMBER_ROLE_EXISTS'] = six.text_type(role_exists) + member_role = [r for r in c.roles.list() if r.name == '_member_'][0] + except IndexError: + # Do nothing if there is no _member_ role + return + admin_tenant = [t for t in c.tenants.list() if t.name == 'admin'][0] + admin_user = [u for u in c.users.list() if u.name == 'admin'][0] + try: + c.roles.add_user_role(admin_user, member_role, admin_tenant.id) + LOG.info('Added _member_ role to admin user') + except ks_exceptions.http.Conflict: + # They already had the role + pass class InstackEnvironment(dict): @@ -950,7 +951,7 @@ class InstackEnvironment(dict): DYNAMIC_KEYS = {'INSPECTION_COLLECTORS', 'INSPECTION_KERNEL_ARGS', 'INSPECTION_NODE_NOT_FOUND_HOOK', 'TRIPLEO_INSTALL_USER', 'TRIPLEO_UNDERCLOUD_CONF_FILE', - 'TRIPLEO_UNDERCLOUD_PASSWORD_FILE', 'MEMBER_ROLE_EXISTS'} + 'TRIPLEO_UNDERCLOUD_PASSWORD_FILE'} """The variables we calculate in _generate_environment call.""" PUPPET_KEYS = DYNAMIC_KEYS | {opt.name.upper() for _, group in list_opts() @@ -1089,8 +1090,6 @@ def _generate_environment(instack_root): instack_env['UNDERCLOUD_SERVICE_CERTIFICATE'] = ( '/etc/pki/tls/certs/undercloud-%s.pem' % public_host) - _member_role_exists(instack_env) - return instack_env @@ -1257,14 +1256,6 @@ def _ensure_flavor(nova, name, profile=None): LOG.info(message, name, profile) -def _stackrc_exists(): - user_stackrc = os.path.expanduser('~/stackrc') - # We gotta check if the copying of stackrc has already been done. - if os.path.isfile('/root/stackrc') and os.path.isfile(user_stackrc): - return True - return False - - def _copy_stackrc(): args = ['sudo', 'cp', '/root/stackrc', os.path.expanduser('~')] try: @@ -1378,6 +1369,8 @@ def _post_config(instack_env): auth_url=auth_url) _post_config_mistral(instack_env, mistral) + _member_role_exists() + def _handle_upgrade_fact(upgrade=False): """Create an upgrade fact for use in puppet diff --git a/releasenotes/notes/maintain-member-role-ecc556d81ce583a1.yaml b/releasenotes/notes/maintain-member-role-ecc556d81ce583a1.yaml new file mode 100644 index 000000000..4c9aea2e8 --- /dev/null +++ b/releasenotes/notes/maintain-member-role-ecc556d81ce583a1.yaml @@ -0,0 +1,9 @@ +--- +upgrade: + - | + The _member_ role (if it exists) on the admin user will now be retained + automatically during undercloud upgrades. This functionality was + originally added to work around an issue with upgrading very old versions + of TripleO, but was broken by changes to the upgrade process. It will + no longer be necessary to manually add the _member_ role to the admin user + after upgrading an affected deployment.