From 59f72258d8c1b689ca44cd5e5baf6a9a46d4139c Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Tue, 9 Jan 2018 23:56:51 +0000 Subject: [PATCH] fix(chartbuilder): Fix ChartBuilder failing on get_files(). This PS fixes a recent commit to chartbuilder module [0] causing HelmChartBuildException and DependencyException errors to be thrown. This is because of the recent implementation of get_files() which uses string primitives rather than instances of `google.protobuf.Any` [1]. The solution was to wrap each file name in the above instance. Unit tests were added to validate much of the ChartBuilder class. [0] https://review.gerrithub.io/#/c/393958/ [1] https://github.com/google/protobuf/blob/master/src/google/protobuf/any.proto Change-Id: Iddb276b917b43754511234737b749182caf4ff14 --- armada/handlers/chartbuilder.py | 41 +++- .../tests/unit/handlers/test_chartbuilder.py | 208 +++++++++++++++--- 2 files changed, 211 insertions(+), 38 deletions(-) diff --git a/armada/handlers/chartbuilder.py b/armada/handlers/chartbuilder.py index 53d382b6..0d3a1f41 100644 --- a/armada/handlers/chartbuilder.py +++ b/armada/handlers/chartbuilder.py @@ -15,6 +15,7 @@ import os import yaml +from google.protobuf.any_pb2 import Any from hapi.chart.chart_pb2 import Chart from hapi.chart.config_pb2 import Config from hapi.chart.metadata_pb2 import Metadata @@ -33,7 +34,7 @@ CONF = cfg.CONF class ChartBuilder(object): ''' - This class handles taking chart intentions as a paramter and + This class handles taking chart intentions as a parameter and turning those into proper protoc helm charts that can be pushed to tiller. @@ -128,17 +129,43 @@ class ChartBuilder(object): def get_files(self): ''' - Return (non-template) files in this chart + Return (non-template) files in this chart. + + Non-template files include all files **not** under templates and + charts subfolders, except: Chart.yaml, values.yaml, values.toml and + charts/.prov + + The class :class:`google.protobuf.any_pb2.Any` is wrapped around + each file as that is what Helm uses. + + For more information, see: + https://github.com/kubernetes/helm/blob/fa06dd176dbbc247b40950e38c09f978efecaecc/pkg/chartutil/load.go + + :returns: List of non-template files. + :rtype: List[:class:`google.protobuf.any_pb2.Any`] ''' + files_to_ignore = ['Chart.yaml', 'values.yaml', 'values.toml'] non_template_files = [] + + def _append_file_to_result(root, file): + abspath = os.path.abspath(os.path.join(root, file)) + with open(abspath, 'r') as f: + file_contents = f.read().encode('utf-8') + non_template_files.append( + Any(type_url=abspath, + value=file_contents)) + for root, dirs, files in os.walk(self.source_directory): relfolder = os.path.split(root)[-1] - if not relfolder == 'templates': + if relfolder not in ['charts', 'templates']: for file in files: - if (file not in ['Chart.yaml', 'values.yaml'] and + if (file not in files_to_ignore and file not in non_template_files): - non_template_files.append(file) + _append_file_to_result(root, file) + elif relfolder == 'charts' and '.prov' in files: + _append_file_to_result(root, '.prov') + return non_template_files def get_values(self): @@ -190,11 +217,9 @@ class ChartBuilder(object): ''' Return a helm chart object ''' - if self._helm_chart: return self._helm_chart - # dependencies - # [process_chart(x, is_dependency=True) for x in chart.dependencies] + dependencies = [] for dep in self.chart.dependencies: LOG.info("Building dependency chart %s for release %s", diff --git a/armada/tests/unit/handlers/test_chartbuilder.py b/armada/tests/unit/handlers/test_chartbuilder.py index a4537c97..492bf91d 100644 --- a/armada/tests/unit/handlers/test_chartbuilder.py +++ b/armada/tests/unit/handlers/test_chartbuilder.py @@ -12,12 +12,16 @@ # See the License for the specific language governing permissions and # limitations under the License. +import inspect import os import shutil +import yaml import fixtures +from hapi.chart.chart_pb2 import Chart from hapi.chart.metadata_pb2 import Metadata import mock +from supermutes.dot import dotify import testtools from armada.handlers.chartbuilder import ChartBuilder @@ -26,7 +30,7 @@ from armada.handlers.chartbuilder import ChartBuilder class ChartBuilderTestCase(testtools.TestCase): chart_stream = """ chart: - name: mariadb + chart_name: mariadb release_name: mariadb namespace: openstack install: @@ -75,18 +79,28 @@ class ChartBuilderTestCase(testtools.TestCase): memory: 128Mi """ - def test_chart_source_clone(self): + def _write_temporary_file_contents(self, directory, filename, contents): + path = os.path.join(directory, filename) + fd = os.open(path, os.O_CREAT | os.O_WRONLY) + try: + os.write(fd, contents.encode('utf-8')) + finally: + os.close(fd) + + def _make_temporary_subdirectory(self, parent_path, sub_path): + subdir = os.path.join(parent_path, sub_path) + if not os.path.exists(subdir): + os.makedirs(subdir) + self.addCleanup(shutil.rmtree, subdir) + return subdir + + def test_source_clone(self): # Create a temporary directory with Chart.yaml that contains data # from ``self.chart_yaml``. chart_dir = self.useFixture(fixtures.TempDir()) self.addCleanup(shutil.rmtree, chart_dir.path) - - path = os.path.join(chart_dir.path, 'Chart.yaml') - fd = os.open(path, os.O_CREAT | os.O_WRONLY) - try: - os.write(fd, self.chart_yaml.encode('utf-8')) - finally: - os.close(fd) + self._write_temporary_file_contents(chart_dir.path, 'Chart.yaml', + self.chart_yaml) mock_chart = mock.Mock(source_dir=[chart_dir.path, '']) chartbuilder = ChartBuilder(mock_chart) @@ -95,7 +109,7 @@ class ChartBuilderTestCase(testtools.TestCase): resp = chartbuilder.get_metadata() self.assertIsInstance(resp, Metadata) - def test_chart_get_non_template_files(self): + def test_get_files(self): """Validates that ``get_files()`` ignores 'Chart.yaml', 'values.yaml' and 'templates' subfolder and all the files contained therein. """ @@ -105,33 +119,167 @@ class ChartBuilderTestCase(testtools.TestCase): # should be ignored by `get_files()`. chart_dir = self.useFixture(fixtures.TempDir()) self.addCleanup(shutil.rmtree, chart_dir.path) - for filename in ['foo', 'bar', 'Chart.yaml', 'values.yaml']: - path = os.path.join(chart_dir.path, filename) - fd = os.open(path, os.O_CREAT | os.O_WRONLY) - try: - os.write(fd, "".encode('utf-8')) - finally: - os.close(fd) + self._write_temporary_file_contents(chart_dir.path, filename, "") # Create a template directory -- 'templates' -- nested inside the chart # directory which should also be ignored. - template_dir = os.path.join(chart_dir.path, 'templates') - if not os.path.exists(template_dir): - os.makedirs(template_dir) - self.addCleanup(shutil.rmtree, template_dir) - + templates_subdir = self._make_temporary_subdirectory( + chart_dir.path, 'templates') for filename in ['template%d' % x for x in range(3)]: - path = os.path.join(template_dir, filename) - fd = os.open(path, os.O_CREAT | os.O_WRONLY) - try: - os.write(fd, "".encode('utf-8')) - finally: - os.close(fd) + self._write_temporary_file_contents(templates_subdir, filename, "") mock_chart = mock.Mock(source_dir=[chart_dir.path, '']) chartbuilder = ChartBuilder(mock_chart) + expected_files = ('[type_url: "%s"\n, type_url: "%s"\n]' % ( + os.path.join(chart_dir.path, 'bar'), + os.path.join(chart_dir.path, 'foo'))) + # Validate that only 'foo' and 'bar' are returned. - files = chartbuilder.get_files() - self.assertEqual(['bar', 'foo'], sorted(files)) + actual_files = sorted(chartbuilder.get_files(), + key=lambda x: x.type_url) + self.assertEqual(expected_files, repr(actual_files).strip()) + + def test_get_basic_helm_chart(self): + # Before ChartBuilder is executed the `source_dir` points to a + # directory that was either clone or unpacked from a tarball... pretend + # that that logic has already been performed. + chart_dir = self.useFixture(fixtures.TempDir()) + self.addCleanup(shutil.rmtree, chart_dir.path) + self._write_temporary_file_contents(chart_dir.path, 'Chart.yaml', + self.chart_yaml) + ch = yaml.safe_load(self.chart_stream)['chart'] + ch['source_dir'] = (chart_dir.path, '') + + test_chart = dotify(ch) + chartbuilder = ChartBuilder(test_chart) + helm_chart = chartbuilder.get_helm_chart() + + expected = inspect.cleandoc( + """ + metadata { + name: "hello-world-chart" + version: "0.1.0" + description: "A Helm chart for Kubernetes" + } + values { + } + """ + ).strip() + + self.assertIsInstance(helm_chart, Chart) + self.assertTrue(hasattr(helm_chart, 'metadata')) + self.assertTrue(hasattr(helm_chart, 'values')) + self.assertEqual(expected, repr(helm_chart).strip()) + + def test_get_helm_chart_with_values(self): + chart_dir = self.useFixture(fixtures.TempDir()) + self.addCleanup(shutil.rmtree, chart_dir.path) + + self._write_temporary_file_contents(chart_dir.path, 'Chart.yaml', + self.chart_yaml) + self._write_temporary_file_contents(chart_dir.path, 'values.yaml', + self.chart_value) + + ch = yaml.safe_load(self.chart_stream)['chart'] + ch['source_dir'] = (chart_dir.path, '') + + test_chart = dotify(ch) + chartbuilder = ChartBuilder(test_chart) + helm_chart = chartbuilder.get_helm_chart() + + self.assertIsInstance(helm_chart, Chart) + self.assertTrue(hasattr(helm_chart, 'metadata')) + self.assertTrue(hasattr(helm_chart, 'values')) + self.assertTrue(hasattr(helm_chart.values, 'raw')) + self.assertEqual(self.chart_value, helm_chart.values.raw) + + def test_get_helm_chart_with_files(self): + # Create a chart directory with some test files. + chart_dir = self.useFixture(fixtures.TempDir()) + self.addCleanup(shutil.rmtree, chart_dir.path) + # Chart.yaml is mandatory for `ChartBuilder.get_metadata`. + self._write_temporary_file_contents(chart_dir.path, 'Chart.yaml', + self.chart_yaml) + self._write_temporary_file_contents(chart_dir.path, 'foo', "foobar") + self._write_temporary_file_contents(chart_dir.path, 'bar', "bazqux") + + # Also create a nested directory and verify that files from it are also + # added. + nested_dir = self._make_temporary_subdirectory( + chart_dir.path, 'nested') + self._write_temporary_file_contents(nested_dir, 'nested0', "random") + + ch = yaml.safe_load(self.chart_stream)['chart'] + ch['source_dir'] = (chart_dir.path, '') + + test_chart = dotify(ch) + chartbuilder = ChartBuilder(test_chart) + helm_chart = chartbuilder.get_helm_chart() + + expected_files = ('[type_url: "%s"\nvalue: "bazqux"\n, ' + 'type_url: "%s"\nvalue: "foobar"\n, ' + 'type_url: "%s"\nvalue: "random"\n]' % ( + os.path.join(chart_dir.path, 'bar'), + os.path.join(chart_dir.path, 'foo'), + os.path.join(nested_dir, 'nested0'))) + + self.assertIsInstance(helm_chart, Chart) + self.assertTrue(hasattr(helm_chart, 'metadata')) + self.assertTrue(hasattr(helm_chart, 'values')) + self.assertTrue(hasattr(helm_chart, 'files')) + actual_files = sorted(helm_chart.files, key=lambda x: x.value) + self.assertEqual(expected_files, repr(actual_files).strip()) + + def test_get_helm_chart_includes_only_relevant_files(self): + chart_dir = self.useFixture(fixtures.TempDir()) + self.addCleanup(shutil.rmtree, chart_dir.path) + + templates_subdir = self._make_temporary_subdirectory( + chart_dir.path, 'templates') + charts_subdir = self._make_temporary_subdirectory( + chart_dir.path, 'charts') + + self._write_temporary_file_contents(chart_dir.path, 'Chart.yaml', + self.chart_yaml) + self._write_temporary_file_contents(chart_dir.path, 'foo', "foobar") + self._write_temporary_file_contents(chart_dir.path, 'bar', "bazqux") + + # Files to ignore within top-level directory. + files_to_ignore = ['Chart.yaml', 'values.yaml', 'values.toml'] + for file in files_to_ignore: + self._write_temporary_file_contents(chart_dir.path, file, "") + # Files to ignore within templates/ subdirectory. + for filename in ['template%d' % x for x in range(3)]: + self._write_temporary_file_contents(templates_subdir, filename, "") + # Files to ignore within charts/ subdirectory. + for filename in ['chart%d' % x for x in range(3)]: + self._write_temporary_file_contents(charts_subdir, filename, "") + # Files to **include** within charts/ subdirectory. + self._write_temporary_file_contents(charts_subdir, '.prov', "xyzzy") + + ch = yaml.safe_load(self.chart_stream)['chart'] + ch['source_dir'] = (chart_dir.path, '') + + test_chart = dotify(ch) + chartbuilder = ChartBuilder(test_chart) + helm_chart = chartbuilder.get_helm_chart() + + expected_files = ('[type_url: "%s"\nvalue: "bazqux"\n, ' + 'type_url: "%s"\nvalue: "foobar"\n, ' + 'type_url: "%s"\nvalue: "xyzzy"\n]' % ( + os.path.join(chart_dir.path, 'bar'), + os.path.join(chart_dir.path, 'foo'), + os.path.join(charts_subdir, '.prov'))) + + # Validate that only relevant files are included, that the ignored + # files are present. + self.assertIsInstance(helm_chart, Chart) + self.assertTrue(hasattr(helm_chart, 'metadata')) + self.assertTrue(hasattr(helm_chart, 'values')) + self.assertTrue(hasattr(helm_chart, 'files')) + actual_files = sorted(helm_chart.files, key=lambda x: x.value) + self.assertEqual(expected_files, repr(actual_files).strip()) + + # TODO(fmontei): Add a test for test_get_helm_chart_with_dependencies.