From 9b3e9b2c45e3d694e46505eb5179e6cbb8cb6a96 Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Fri, 18 Jan 2019 22:21:03 +0530 Subject: [PATCH] Check for physical_resource_id before getting deployments Heat does non-strict validation of nested stack resources. Most constraints are not enforced for nested stack resources It's possible to set {Role}DeploymentActions as something like [''] and it would not fail in validation. This would result in a deployment resource with no physical_resource_id. The existing check[1] for config-download would not be sufficient for some cases as it can be another nested stack (we alias both OS::Tripleo::SoftwareDeployment and OS::Heat::StructuredDeployment). [1] deployment.attributes['value'].get('deployment') == \ 'TripleOSoftwareDeployment' This adds a new check not to try and download heat deployments with no physical_resource_id. Change-Id: I234e629cc5377cda8d5d01bfe4416ad967cdc067 Closes-Bug: #1812604 (cherry picked from commit 54c448f68354d48df49ca83a71932938e9c84810) --- tripleo_common/tests/utils/test_config.py | 43 ++++++++++++++++++----- tripleo_common/utils/config.py | 18 +++++++--- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/tripleo_common/tests/utils/test_config.py b/tripleo_common/tests/utils/test_config.py index 845228636..37f3bc3ea 100644 --- a/tripleo_common/tests/utils/test_config.py +++ b/tripleo_common/tests/utils/test_config.py @@ -231,7 +231,12 @@ class TestConfig(base.TestCase): return deployment_data, configs - def _get_config_dict(self, deployment): + def _get_deployment_id(self, deployment): + return deployment.attributes['value']['deployment'] + + def _get_config_dict(self, deployment_id): + deployment = list(filter( + lambda d: d.id == deployment_id, self.deployments))[0] config = self.configs[deployment.attributes['value']['config']].copy() config['inputs'] = [] config['inputs'].append(dict( @@ -247,15 +252,16 @@ class TestConfig(base.TestCase): return yaml.safe_load(open(file_path).read()) @patch.object(ooo_config.Config, 'initialize_git_repo') + @patch('tripleo_common.utils.config.Config.get_deployment_resource_id') @patch('tripleo_common.utils.config.Config.get_config_dict') @patch('tripleo_common.utils.config.Config.get_deployment_data') def test_config_download(self, mock_deployment_data, mock_config_dict, + mock_deployment_resource_id, mock_git_init): heat = mock.MagicMock() self.config = ooo_config.Config(heat) stack = mock.MagicMock() heat.stacks.get.return_value = stack - stack.outputs = [ {'output_key': 'RoleNetHostnameMap', 'output_value': { @@ -279,8 +285,9 @@ class TestConfig(base.TestCase): deployment_data, configs = \ self._get_config_data('config_data.yaml') self.configs = configs - + self.deployments = deployment_data mock_deployment_data.return_value = deployment_data + mock_deployment_resource_id.side_effect = self._get_deployment_id mock_config_dict.side_effect = self._get_config_dict self.tmp_dir = self.useFixture(fixtures.TempDir()).path @@ -346,15 +353,17 @@ class TestConfig(base.TestCase): d))) @patch.object(ooo_config.Config, 'initialize_git_repo') + @patch('tripleo_common.utils.config.Config.get_deployment_resource_id') @patch('tripleo_common.utils.config.Config.get_config_dict') @patch('tripleo_common.utils.config.Config.get_deployment_data') def test_config_download_os_apply_config( - self, mock_deployment_data, mock_config_dict, mock_git_init): + self, mock_deployment_data, mock_config_dict, + mock_deployment_resource_id, mock_git_init): heat = mock.MagicMock() self.config = ooo_config.Config(heat) stack = mock.MagicMock() heat.stacks.get.return_value = stack - + heat.resources.get.return_value = mock.MagicMock() stack.outputs = [ {'output_key': 'RoleNetHostnameMap', 'output_value': { @@ -397,9 +406,10 @@ class TestConfig(base.TestCase): deployment_data.append(deployment_mock) self.configs = configs - + self.deployments = deployment_data mock_deployment_data.return_value = deployment_data mock_config_dict.side_effect = self._get_config_dict + mock_deployment_resource_id.side_effect = self._get_deployment_id self.tmp_dir = self.useFixture(fixtures.TempDir()).path with warnings.catch_warnings(record=True) as w: @@ -410,21 +420,26 @@ class TestConfig(base.TestCase): assert "group:os-apply-config is deprecated" in str(w[-1].message) @patch.object(ooo_config.Config, 'initialize_git_repo') + @patch('tripleo_common.utils.config.Config.get_deployment_resource_id') @patch('tripleo_common.utils.config.Config.get_deployment_data') - def test_config_download_no_deployment_name(self, mock_deployment_data, - mock_git_init): + def test_config_download_no_deployment_name( + self, mock_deployment_data, mock_deployment_resource_id, + mock_git_init): heat = mock.MagicMock() self.config = ooo_config.Config(heat) stack = mock.MagicMock() heat.stacks.get.return_value = stack + heat.resources.get.return_value = mock.MagicMock() deployment_data, _ = self._get_config_data('config_data.yaml') # Delete the name of the first deployment and his parent. del deployment_data[0].attributes['value']['name'] deployment_data[0].parent_resource = None + self.deployments = deployment_data mock_deployment_data.return_value = deployment_data + mock_deployment_resource_id.side_effect = self._get_deployment_id self.tmp_dir = self.useFixture(fixtures.TempDir()).path self.assertRaises(ValueError, @@ -432,15 +447,19 @@ class TestConfig(base.TestCase): mock_git_init.assert_called_once_with(self.tmp_dir) @patch.object(ooo_config.Config, 'initialize_git_repo') + @patch('tripleo_common.utils.config.Config.get_deployment_resource_id') @patch('tripleo_common.utils.config.Config.get_config_dict') @patch('tripleo_common.utils.config.Config.get_deployment_data') def test_config_download_no_deployment_uuid(self, mock_deployment_data, mock_config_dict, + mock_deployment_resource_id, mock_git_init): heat = mock.MagicMock() self.config = ooo_config.Config(heat) stack = mock.MagicMock() heat.stacks.get.return_value = stack + heat.resources.get.return_value = mock.MagicMock() + stack.outputs = [ {'output_key': 'RoleNetHostnameMap', 'output_value': { @@ -477,14 +496,20 @@ class TestConfig(base.TestCase): deployment_data[0].attributes['value']['deployment'] = \ 'TripleOSoftwareDeployment' + # Set the physical_resource_id as '' for the second deployment + deployment_data[1].attributes['value']['deployment'] = '' + self.configs = configs + self.deployments = deployment_data mock_deployment_data.return_value = deployment_data mock_config_dict.side_effect = self._get_config_dict + mock_deployment_resource_id.side_effect = self._get_deployment_id self.tmp_dir = self.useFixture(fixtures.TempDir()).path with warnings.catch_warnings(record=True) as w: self.config.download_config(stack, self.tmp_dir) assert "Skipping deployment" in str(w[-1].message) + assert "Skipping deployment" in str(w[-2].message) @patch.object(ooo_config.Config, 'initialize_git_repo') @patch.object(ooo_config.git, 'Repo') @@ -587,11 +612,11 @@ class TestConfig(base.TestCase): deployment_data, configs = \ self._get_config_data('config_data.yaml') self.configs = configs + self.deployments = deployment_data stack_data = self.config.fetch_config('overcloud') mock_deployment_data.return_value = deployment_data mock_config_dict.side_effect = self._get_config_dict - config_dir = self.useFixture(fixtures.TempDir()).path self.config.write_config(stack_data, 'overcloud', config_dir) diff --git a/tripleo_common/utils/config.py b/tripleo_common/utils/config.py index 511a9ddfe..a55c95201 100644 --- a/tripleo_common/utils/config.py +++ b/tripleo_common/utils/config.py @@ -72,7 +72,7 @@ class Config(object): if server_id in v: return k - def get_config_dict(self, deployment): + def get_deployment_resource_id(self, deployment): if '/' in deployment.attributes['value']['deployment']: deployment_stack_id = \ deployment.attributes['value']['deployment'].split('/')[-1] @@ -82,6 +82,9 @@ class Config(object): else: deployment_resource_id = \ deployment.attributes['value']['deployment'] + return deployment_resource_id + + def get_config_dict(self, deployment_resource_id): deployment_rsrc = self.client.software_deployments.get( deployment_resource_id) config = self.client.software_configs.get( @@ -261,8 +264,15 @@ class Config(object): # create. If that's the case, just skip this deployment, otherwise # it will result in a Not found error if we try and query the # deployment API for this deployment. - if deployment.attributes['value'].get('deployment') == \ - 'TripleOSoftwareDeployment': + dep_value_resource_name = deployment.attributes[ + 'value'].get('deployment') == 'TripleOSoftwareDeployment' + + # if not check the physical_resource_id + if not dep_value_resource_name: + deployment_resource_id = self.get_deployment_resource_id( + deployment) + + if dep_value_resource_name or not deployment_resource_id: warnings.warn('Skipping deployment %s because it has no ' 'valid uuid (physical_resource_id) ' 'associated.' % @@ -270,7 +280,7 @@ class Config(object): continue server_id = deployment.attributes['value']['server'] - config_dict = self.get_config_dict(deployment) + config_dict = self.get_config_dict(deployment_resource_id) # deployment_name should be set via the name property on the # Deployment resources in the templates, however, if it's None