Fix chartbuilder get_files raising UnicodeDecodeError

This adds better exception handling ang logging to
_append_file_to_result helper in get_files. When reading
arbitrary file data and attempting to encode to utf-8
this can cause UnicodeDecodeError to be raised.

However, Armada will not skip over such files; it will
raise an exception with appropriate details instead.

Closes #195
Closes #196

Change-Id: Id7c32c17e351d1ffe042e3755c116c36b6380223

armada/exceptions/chartbuilder_exceptions.py

@@ -18,7 +18,7 @@ from armada.exceptions import base_exception
 class ChartBuilderException(base_exception.ArmadaBaseException):
     '''Base class for the Chartbuilder handler exception and error handling.'''
 
-    message = 'An unknown Armada handler error occured.'
+    message = 'An unknown Armada handler error occurred.'
 
 
 class DependencyException(ChartBuilderException):
@@ -45,14 +45,29 @@ class HelmChartBuildException(ChartBuilderException):
     *Coming Soon*
     '''
 
-    def __init__(self, chart_name):
+    def __init__(self, chart_name, details):
         self._chart_name = chart_name
-        self._message = 'Failed to build Helm chart for ' + \
-                        self._chart_name + '.'
+        self._message = ('Failed to build Helm chart for {chart_name}. '
+                         'Details: {details}'.format(
+                             **{'chart_name': chart_name,
+                                'details': details}))
 
         super(HelmChartBuildException, self).__init__(self._message)
 
 
+class FilesLoadException(ChartBuilderException):
+    '''
+    Exception that occurs while trying to read a file in the chart directory.
+
+    **Troubleshoot:**
+
+    * Ensure that the file can be encoded to utf-8 or else it cannot be parsed.
+    '''
+
+    message = ('A %(clazz)s exception occurred while trying to read '
+               'file: %(file)s. Details:\n%(details)s')
+
+
 class IgnoredFilesLoadException(ChartBuilderException):
     '''
     Exception that occurs when there is an error loading files contained in

armada/handlers/chartbuilder.py

@@ -152,8 +152,33 @@ class ChartBuilder(object):
             abspath = os.path.abspath(os.path.join(root, file))
             relpath = os.path.join(rel_folder_path, file)
 
-            with open(abspath, 'r') as f:
-                file_contents = f.read().encode('utf-8')
+            encodings = ('utf-8', 'latin1')
+            unicode_errors = []
+
+            for encoding in encodings:
+                try:
+                    with open(abspath, 'r') as f:
+                        file_contents = f.read().encode(encoding)
+                except OSError as e:
+                    LOG.debug('Failed to open and read file %s in the helm '
+                              'chart directory.', abspath)
+                    raise chartbuilder_exceptions.FilesLoadException(
+                        file=abspath, details=e)
+                except UnicodeError as e:
+                    LOG.debug('Attempting to read %s using encoding %s.',
+                              abspath, encoding)
+                    msg = "(encoding=%s) %s" % (encoding, str(e))
+                    unicode_errors.append(msg)
+                else:
+                    break
+
+            if len(unicode_errors) == 2:
+                LOG.debug('Failed to read file %s in the helm chart directory.'
+                          ' Ensure that it is encoded using utf-8.', abspath)
+                raise chartbuilder_exceptions.FilesLoadException(
+                    file=abspath, clazz=unicode_errors[0].__class__.__name__,
+                    details='\n'.join(e for e in unicode_errors))
+
             non_template_files.append(
                 Any(type_url=relpath,
                     value=file_contents))
@@ -242,9 +267,10 @@ class ChartBuilder(object):
                 dependencies=dependencies,
                 values=self.get_values(),
                 files=self.get_files())
-        except Exception:
+        except Exception as e:
             chart_name = self.chart.chart_name
-            raise chartbuilder_exceptions.HelmChartBuildException(chart_name)
+            raise chartbuilder_exceptions.HelmChartBuildException(
+                chart_name, details=e)
 
         self._helm_chart = helm_chart
         return helm_chart

armada/tests/unit/handlers/test_chartbuilder.py

@@ -28,7 +28,7 @@ from armada.handlers.chartbuilder import ChartBuilder
 from armada.exceptions import chartbuilder_exceptions
 
 
-class ChartBuilderTestCase(testtools.TestCase):
+class BaseChartBuilderTestCase(testtools.TestCase):
     chart_yaml = """
         apiVersion: v1
         description: A sample Helm chart for Kubernetes
@@ -121,6 +121,9 @@ class ChartBuilderTestCase(testtools.TestCase):
             self.addCleanup(shutil.rmtree, subdir)
         return subdir
 
+
+class ChartBuilderTestCase(BaseChartBuilderTestCase):
+
     def test_source_clone(self):
         # Create a temporary directory with Chart.yaml that contains data
         # from ``self.chart_yaml``.
@@ -177,6 +180,17 @@ class ChartBuilderTestCase(testtools.TestCase):
                               key=lambda x: x.type_url)
         self.assertEqual(expected_files, repr(actual_files).strip())
 
+    def test_get_files_with_unicode_characters(self):
+        chart_dir = self.useFixture(fixtures.TempDir())
+        self.addCleanup(shutil.rmtree, chart_dir.path)
+        for filename in ['foo', 'bar', 'Chart.yaml', 'values.yaml']:
+            self._write_temporary_file_contents(
+                chart_dir.path, filename, "DIRC^@^@^@^B^@^@^@×Z®<86>F.1")
+
+        mock_chart = mock.Mock(source_dir=[chart_dir.path, ''])
+        chartbuilder = ChartBuilder(mock_chart)
+        chartbuilder.get_files()
+
     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
@@ -427,3 +441,53 @@ class ChartBuilderTestCase(testtools.TestCase):
             dependency-chart.*Another sample Helm chart for Kubernetes.*
         """).replace('\n', '').strip()
         self.assertRegex(repr(chartbuilder.dump()), re)
+
+
+class ChartBuilderNegativeTestCase(BaseChartBuilderTestCase):
+
+    def setUp(self):
+        super(ChartBuilderNegativeTestCase, self).setUp()
+        # Create an exception for testing since instantiating one manually
+        # is tedious.
+        try:
+            str(b'\xff', 'utf8')
+        except UnicodeDecodeError as e:
+            self.exc_to_raise = e
+        else:
+            self.fail('Failed to create an exception needed for testing.')
+
+    def test_get_files_always_fails_to_read_binary_file_raises_exc(self):
+        chart_dir = self.useFixture(fixtures.TempDir())
+        self.addCleanup(shutil.rmtree, chart_dir.path)
+        for filename in ['foo', 'bar', 'Chart.yaml', 'values.yaml']:
+            self._write_temporary_file_contents(
+                chart_dir.path, filename, "DIRC^@^@^@^B^@^@^@×Z®<86>F.1")
+
+        mock_chart = mock.Mock(source_dir=[chart_dir.path, ''])
+        chartbuilder = ChartBuilder(mock_chart)
+
+        # Confirm it failed for both encodings.
+        error_re = (r'.*A str exception occurred while trying to read file:'
+                    '.*Details:\n.*\(encoding=utf-8\).*\n\(encoding=latin1\)')
+        with mock.patch("builtins.open", mock.mock_open(read_data="")) \
+                as mock_file:
+            mock_file.return_value.read.side_effect = self.exc_to_raise
+            self.assertRaisesRegexp(chartbuilder_exceptions.FilesLoadException,
+                                    error_re, chartbuilder.get_files)
+
+    def test_get_files_fails_once_to_read_binary_file_passes(self):
+        chart_dir = self.useFixture(fixtures.TempDir())
+        self.addCleanup(shutil.rmtree, chart_dir.path)
+        files = ['foo', 'bar']
+        for filename in files:
+            self._write_temporary_file_contents(
+                chart_dir.path, filename, "DIRC^@^@^@^B^@^@^@×Z®<86>F.1")
+
+        mock_chart = mock.Mock(source_dir=[chart_dir.path, ''])
+        chartbuilder = ChartBuilder(mock_chart)
+
+        side_effects = [self.exc_to_raise, "", ""]
+        with mock.patch("builtins.open", mock.mock_open(read_data="")) \
+                as mock_file:
+            mock_file.return_value.read.side_effect = side_effects
+            chartbuilder.get_files()

docs/source/commands/apply.rst

@@ -9,7 +9,7 @@ Commands
 
     Usage: armada apply [OPTIONS] FILENAME
 
-      This command install and updates charts defined in armada manifest
+      This command installs and updates charts defined in armada manifest
 
       The apply argument must be relative path to Armada Manifest. Executing
       apply command once will install all charts defined in manifest. Re-

docs/source/operations/exceptions/chartbuilder-exceptions.inc

@@ -25,6 +25,11 @@
          :members:
          :show-inheritance:
          :undoc-members:
+  * - FilesLoadException
+    - .. autoexception:: armada.exceptions.chartbuilder_exceptions.FilesLoadException
+         :members:
+         :show-inheritance:
+         :undoc-members:
   * - HelmChartBuildException
     - .. autoexception:: armada.exceptions.chartbuilder_exceptions.HelmChartBuildException
          :members:

tox.ini

@@ -15,6 +15,7 @@ install_command = pip install {opts} {packages}
 whitelist_externals =
     find
     flake8
+    rm
 commands =
     find . -type f -name "*.pyc" -delete
     python -V