bug(chartbuilder): uncaught exceptions on bad manifests

Armada was able to throw exceptions all the way up to invocation. To address:
- remove 'supermutes dotify', which was throwing exceptions
- refactor chartbuilder after removing dotify
- rework some helm wait/timeout logic, exposed during bug squash
- rename some variables to make their function more clear

Note: This has potentially breaking changes to existing charts, in the sense
    that documents previously validated (improperly) may now give errors.

Change-Id: I9a6c99aa8ba3d666405e0ccf49a847fd01807b69
This commit is contained in:
Marshall Margenau (mm8789) 2018-03-27 11:08:55 -05:00 committed by Marshall Margenau
parent 53dda10638
commit efd42dfab2
9 changed files with 133 additions and 121 deletions

View File

@ -79,8 +79,8 @@ class Apply(api.BaseResource):
enable_chart_cleanup=req.get_param_as_bool(
'enable_chart_cleanup'),
dry_run=req.get_param_as_bool('dry_run'),
wait=req.get_param_as_bool('wait'),
timeout=req.get_param_as_int('timeout') or 3600,
tiller_should_wait=req.get_param_as_bool('wait'),
tiller_timeout=req.get_param_as_int('timeout') or 3600,
tiller_host=req.get_param('tiller_host'),
tiller_port=req.get_param_as_int(
'tiller_port') or CONF.tiller_port,

View File

@ -208,8 +208,8 @@ class ApplyManifest(CliAction):
enable_chart_cleanup=self.enable_chart_cleanup,
dry_run=self.dry_run,
set_ovr=self.set,
wait=self.wait,
timeout=self.timeout,
tiller_should_wait=self.wait,
tiller_timeout=self.timeout,
tiller_host=self.tiller_host,
tiller_port=self.tiller_port,
tiller_namespace=self.tiller_namespace,

View File

@ -17,7 +17,6 @@ import yaml
from oslo_config import cfg
from oslo_log import log as logging
from supermutes.dot import dotify
from armada.handlers.chartbuilder import ChartBuilder
from armada.handlers.manifest import Manifest
@ -50,8 +49,8 @@ class Armada(object):
enable_chart_cleanup=False,
dry_run=False,
set_ovr=None,
wait=False,
timeout=DEFAULT_TIMEOUT,
tiller_should_wait=False,
tiller_timeout=DEFAULT_TIMEOUT,
tiller_host=None,
tiller_port=None,
tiller_namespace=None,
@ -68,8 +67,10 @@ class Armada(object):
operations.
:param bool enable_chart_cleanup: Clean up unmanaged charts.
:param bool dry_run: Run charts without installing them.
:param bool wait: Wait until all charts are deployed.
:param int timeout: Specifies time to wait for charts to deploy.
:param bool tiller_should_wait: Specifies whether Tiller should wait
until all charts are deployed.
:param int tiller_timeout: Specifies time Tiller should wait for charts
to deploy until timing out.
:param str tiller_host: Tiller host IP. Default is None.
:param int tiller_port: Tiller host port. Default is
``CONF.tiller_port``.
@ -90,8 +91,8 @@ class Armada(object):
self.enable_chart_cleanup = enable_chart_cleanup
self.dry_run = dry_run
self.overrides = set_ovr
self.wait = wait
self.timeout = timeout
self.tiller_should_wait = tiller_should_wait
self.tiller_timeout = tiller_timeout
self.tiller = Tiller(
tiller_host=tiller_host, tiller_port=tiller_port,
tiller_namespace=tiller_namespace)
@ -264,66 +265,76 @@ class Armada(object):
for entry in self.manifest[const.KEYWORD_ARMADA][const.KEYWORD_GROUPS]:
chart_wait = self.wait
tiller_should_wait = self.tiller_should_wait
tiller_timeout = self.tiller_timeout
desc = entry.get('description', 'A Chart Group')
chart_group = entry.get(const.KEYWORD_CHARTS, [])
chart_groups = entry.get(const.KEYWORD_CHARTS, [])
test_charts = entry.get('test_charts', False)
if entry.get('sequenced', False) or test_charts:
chart_wait = True
tiller_should_wait = True
LOG.info('Deploying: %s', desc)
for gchart in chart_group:
chart = dotify(gchart['chart'])
values = gchart.get('chart').get('values', {})
wait_values = gchart.get('chart').get('wait', {})
test_chart = gchart.get('chart').get('test', False)
for chartgroup in chart_groups:
chart = chartgroup.get('chart', {})
values = chart.get('values', {})
test_chart = chart.get('test', False)
namespace = chart.get('namespace', None)
release = chart.get('release', None)
pre_actions = {}
post_actions = {}
if chart.release is None:
if release is None:
continue
if test_chart is True:
chart_wait = True
tiller_should_wait = True
# retrieve appropriate timeout value if 'wait' is specified
chart_timeout = self.timeout
if chart_wait:
if chart_timeout == DEFAULT_TIMEOUT:
chart_timeout = getattr(
chart, 'timeout', chart_timeout)
# retrieve appropriate timeout value
# TODO(MarshM): chart's `data.timeout` should be deprecated
# to favor `data.wait.timeout`
# TODO(MarshM) also: timeout logic seems to prefer chart values
# over api/cli, probably should swap?
# (caution: it always default to 3600,
# take care to differentiate user input)
if tiller_should_wait and tiller_timeout == DEFAULT_TIMEOUT:
tiller_timeout = chart.get('timeout', tiller_timeout)
wait_values = chart.get('wait', {})
wait_timeout = wait_values.get('timeout', tiller_timeout)
wait_values_labels = wait_values.get('labels', {})
chartbuilder = ChartBuilder(chart)
protoc_chart = chartbuilder.get_helm_chart()
# determine install or upgrade by examining known releases
LOG.debug("RELEASE: %s", chart.release)
LOG.debug("RELEASE: %s", release)
deployed_releases = [x[0] for x in known_releases]
prefix_chart = release_prefix(prefix, chart.release)
prefix_chart = release_prefix(prefix, release)
if prefix_chart in deployed_releases:
# indicate to the end user what path we are taking
LOG.info("Upgrading release %s", chart.release)
LOG.info("Upgrading release %s", release)
# extract the installed chart and installed values from the
# latest release so we can compare to the intended state
LOG.info("Checking Pre/Post Actions")
apply_chart, apply_values = self.find_release_chart(
known_releases, prefix_chart)
upgrade = chart.get('upgrade', {})
disable_hooks = upgrade.get('no_hooks', False)
LOG.info("Checking Pre/Post Actions")
upgrade = gchart.get('chart', {}).get('upgrade', False)
if upgrade:
if not self.disable_update_pre and upgrade.get(
'pre', False):
pre_actions = getattr(chart.upgrade, 'pre', {})
upgrade_pre = upgrade.get('pre', {})
upgrade_post = upgrade.get('post', {})
if not self.disable_update_post and upgrade.get(
'post', False):
post_actions = getattr(chart.upgrade, 'post', {})
if not self.disable_update_pre and upgrade_pre:
pre_actions = upgrade_pre
if not self.disable_update_post and upgrade_post:
post_actions = upgrade_post
# show delta for both the chart templates and the chart
# values
@ -339,58 +350,54 @@ class Armada(object):
continue
# do actual update
LOG.info('wait: %s', chart_wait)
LOG.info('Beginning Upgrade, wait: %s, %s',
tiller_should_wait, wait_timeout)
self.tiller.update_release(
protoc_chart,
prefix_chart,
chart.namespace,
namespace,
pre_actions=pre_actions,
post_actions=post_actions,
dry_run=self.dry_run,
disable_hooks=chart.upgrade.no_hooks,
disable_hooks=disable_hooks,
values=yaml.safe_dump(values),
wait=chart_wait,
timeout=chart_timeout)
if chart_wait:
# TODO(gardlt): after v0.7.1 deprecate timeout values
if not wait_values.get('timeout', None):
wait_values['timeout'] = chart_timeout
wait=tiller_should_wait,
timeout=wait_timeout)
if tiller_should_wait:
self.tiller.k8s.wait_until_ready(
release=prefix_chart,
labels=wait_values.get('labels', ''),
namespace=chart.namespace,
labels=wait_values_labels,
namespace=namespace,
k8s_wait_attempts=self.k8s_wait_attempts,
k8s_wait_attempt_sleep=self.k8s_wait_attempt_sleep,
timeout=wait_values.get('timeout', DEFAULT_TIMEOUT)
timeout=wait_timeout
)
msg['upgrade'].append(prefix_chart)
# process install
else:
LOG.info("Installing release %s", chart.release)
LOG.info("Installing release %s", release)
LOG.info('Beginning Install, wait: %s, %s',
tiller_should_wait, wait_timeout)
self.tiller.install_release(
protoc_chart,
prefix_chart,
chart.namespace,
namespace,
dry_run=self.dry_run,
values=yaml.safe_dump(values),
wait=chart_wait,
timeout=chart_timeout)
if chart_wait:
if not wait_values.get('timeout', None):
wait_values['timeout'] = chart_timeout
wait=tiller_should_wait,
timeout=wait_timeout)
if tiller_should_wait:
self.tiller.k8s.wait_until_ready(
release=prefix_chart,
labels=wait_values.get('labels', ''),
namespace=chart.namespace,
labels=wait_values_labels,
namespace=namespace,
k8s_wait_attempts=self.k8s_wait_attempts,
k8s_wait_attempt_sleep=self.k8s_wait_attempt_sleep,
timeout=wait_values.get('timeout', DEFAULT_TIMEOUT)
timeout=wait_timeout
)
msg['install'].append(prefix_chart)
@ -410,10 +417,11 @@ class Armada(object):
LOG.info("FAILED: %s", prefix_chart)
# TODO(MarshM) does this need release/labels/namespace?
# TODO(MarshM) consider the tiller_timeout according to above logic
self.tiller.k8s.wait_until_ready(
k8s_wait_attempts=self.k8s_wait_attempts,
k8s_wait_attempt_sleep=self.k8s_wait_attempt_sleep,
timeout=chart_timeout)
timeout=tiller_timeout)
LOG.info("Performing Post-Flight Operations")
self.post_flight_ops()
@ -442,15 +450,17 @@ class Armada(object):
Produce a unified diff of the installed chart vs our intention
TODO(alanmeadows): This needs to be rewritten to produce better
unified diff output and avoid the use of print
unified diff output
'''
source = str(installed_chart.SerializeToString()).split('\n')
chart_diff = list(
difflib.unified_diff(source, str(target_chart).split('\n')))
chart_release = chart.get('release', None)
if len(chart_diff) > 0:
LOG.info("Chart Unified Diff (%s)", chart.release)
LOG.info("Chart Unified Diff (%s)", chart_release)
diff_msg = []
for line in chart_diff:
diff_msg.append(line)
@ -464,7 +474,7 @@ class Armada(object):
yaml.safe_dump(target_values).split('\n')))
if len(values_diff) > 0:
LOG.info("Values Unified Diff (%s)", chart.release)
LOG.info("Values Unified Diff (%s)", chart_release)
diff_msg = []
for line in values_diff:
diff_msg.append(line)

View File

@ -20,7 +20,6 @@ from hapi.chart.chart_pb2 import Chart
from hapi.chart.config_pb2 import Config
from hapi.chart.metadata_pb2 import Metadata
from hapi.chart.template_pb2 import Template
from supermutes.dot import dotify
from oslo_config import cfg
from oslo_log import log as logging
@ -46,7 +45,7 @@ class ChartBuilder(object):
'''
Initialize the ChartBuilder class
Note that tthis will trigger a source pull as part of
Note that this will trigger a source pull as part of
initialization as its necessary in order to examine
the source service many of the calls on ChartBuilder
'''
@ -70,11 +69,12 @@ class ChartBuilder(object):
'''
Return the joined path of the source directory and subpath
'''
source_dir = self.chart.get('source_dir')
return (
os.path.join(self.chart.source_dir[0], self.chart.source_dir[1])
if (hasattr(self.chart, 'source_dir') and
isinstance(self.chart.source_dir, (list, tuple)) and
len(self.chart.source_dir) == 2)
os.path.join(*source_dir)
if (source_dir and
isinstance(source_dir, (list, tuple)) and
len(source_dir) == 2)
else ""
)
@ -116,16 +116,16 @@ class ChartBuilder(object):
try:
with open(os.path.join(self.source_directory, 'Chart.yaml')) as f:
chart_yaml = dotify(yaml.safe_load(f.read().encode('utf-8')))
chart_yaml = yaml.safe_load(f.read().encode('utf-8'))
except Exception:
raise chartbuilder_exceptions.MetadataLoadException()
# construct Metadata object
return Metadata(
description=chart_yaml.description,
name=chart_yaml.name,
version=chart_yaml.version)
description=chart_yaml.get('description'),
name=chart_yaml.get('name'),
version=chart_yaml.get('version'))
def get_files(self):
'''
@ -221,11 +221,12 @@ class ChartBuilder(object):
# process all files in templates/ as a template to attach to the chart
# building a Template object
chart_name = self.chart.get('chart_name')
templates = []
if not os.path.exists(
os.path.join(self.source_directory, 'templates')):
LOG.warn("Chart %s has no templates directory. "
"No templates will be deployed", self.chart.chart_name)
"No templates will be deployed", chart_name)
for root, _, files in os.walk(
os.path.join(self.source_directory, 'templates'),
topdown=True):
@ -251,13 +252,17 @@ class ChartBuilder(object):
return self._helm_chart
dependencies = []
for dep in self.chart.dependencies:
chart_dependencies = self.chart.get('dependencies', [])
chart_name = self.chart.get('chart_name', None)
chart_release = self.chart.get('release', None)
for dep in chart_dependencies:
dep_chart = dep.get('chart', {})
dep_chart_name = dep_chart.get('chart_name', None)
LOG.info("Building dependency chart %s for release %s.",
dep.chart.chart_name, self.chart.release)
dep_chart_name, chart_release)
try:
dependencies.append(ChartBuilder(dep.chart).get_helm_chart())
dependencies.append(ChartBuilder(dep_chart).get_helm_chart())
except Exception:
chart_name = self.chart.chart_name
raise chartbuilder_exceptions.DependencyException(chart_name)
try:
@ -268,7 +273,6 @@ class ChartBuilder(object):
values=self.get_values(),
files=self.get_files())
except Exception as e:
chart_name = self.chart.chart_name
raise chartbuilder_exceptions.HelmChartBuildException(
chart_name, details=e)

View File

@ -48,8 +48,8 @@ class ArmadaControllerTest(base.BaseControllerTest):
'disable_update_post': False,
'enable_chart_cleanup': False,
'dry_run': False,
'wait': False,
'timeout': 100,
'tiller_should_wait': False,
'tiller_timeout': 100,
'tiller_host': None,
'tiller_port': 44134,
'tiller_namespace': 'kube-system',

View File

@ -212,8 +212,8 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
chart_1['namespace'],
dry_run=armada_obj.dry_run,
values=yaml.safe_dump(chart_1['values']),
wait=armada_obj.wait,
timeout=armada_obj.timeout),
wait=armada_obj.tiller_should_wait,
timeout=armada_obj.tiller_timeout),
mock.call(
mock_chartbuilder().get_helm_chart(),
"{}-{}".format(armada_obj.manifest['armada']['release_prefix'],
@ -221,7 +221,7 @@ class ArmadaHandlerTestCase(base.ArmadaTestCase):
chart_2['namespace'],
dry_run=armada_obj.dry_run,
values=yaml.safe_dump(chart_2['values']),
wait=armada_obj.wait,
timeout=armada_obj.timeout)
wait=armada_obj.tiller_should_wait,
timeout=armada_obj.tiller_timeout)
]
mock_tiller.return_value.install_release.assert_has_calls(method_calls)

View File

@ -21,7 +21,6 @@ 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
@ -132,8 +131,8 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
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)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
# Validate response type is :class:`hapi.chart.metadata_pb2.Metadata`
resp = chartbuilder.get_metadata()
@ -143,8 +142,8 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
chart_dir = self.useFixture(fixtures.TempDir())
self.addCleanup(shutil.rmtree, chart_dir.path)
mock_chart = mock.Mock(source_dir=[chart_dir.path, ''])
chartbuilder = ChartBuilder(mock_chart)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
self.assertRaises(
chartbuilder_exceptions.MetadataLoadException,
@ -170,8 +169,8 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
for filename in ['template%d' % x for x in range(3)]:
self._write_temporary_file_contents(templates_subdir, filename, "")
mock_chart = mock.Mock(source_dir=[chart_dir.path, ''])
chartbuilder = ChartBuilder(mock_chart)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
expected_files = ('[type_url: "%s"\n, type_url: "%s"\n]' % ('./bar',
'./foo'))
@ -187,8 +186,8 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
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)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
chartbuilder.get_files()
def test_get_basic_helm_chart(self):
@ -202,7 +201,7 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
ch = yaml.safe_load(self.chart_stream)['chart']
ch['source_dir'] = (chart_dir.path, '')
test_chart = dotify(ch)
test_chart = ch
chartbuilder = ChartBuilder(test_chart)
helm_chart = chartbuilder.get_helm_chart()
@ -235,7 +234,7 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
ch = yaml.safe_load(self.chart_stream)['chart']
ch['source_dir'] = (chart_dir.path, '')
test_chart = dotify(ch)
test_chart = ch
chartbuilder = ChartBuilder(test_chart)
helm_chart = chartbuilder.get_helm_chart()
@ -264,7 +263,7 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
ch = yaml.safe_load(self.chart_stream)['chart']
ch['source_dir'] = (chart_dir.path, '')
test_chart = dotify(ch)
test_chart = ch
chartbuilder = ChartBuilder(test_chart)
helm_chart = chartbuilder.get_helm_chart()
@ -322,7 +321,7 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
ch = yaml.safe_load(self.chart_stream)['chart']
ch['source_dir'] = (chart_dir.path, '')
test_chart = dotify(ch)
test_chart = ch
chartbuilder = ChartBuilder(test_chart)
helm_chart = chartbuilder.get_helm_chart()
@ -357,9 +356,9 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
dep_ch = yaml.safe_load(self.dependency_chart_stream)
dep_ch['chart']['source_dir'] = (dep_chart_dir.path, '')
main_chart = dotify(ch)
dependency_chart = dotify(dep_ch)
main_chart.dependencies = [dependency_chart]
main_chart = ch
dependency_chart = dep_ch
main_chart['dependencies'] = [dependency_chart]
chartbuilder = ChartBuilder(main_chart)
helm_chart = chartbuilder.get_helm_chart()
@ -418,7 +417,7 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
ch = yaml.safe_load(self.chart_stream)['chart']
ch['source_dir'] = (chart_dir.path, '')
test_chart = dotify(ch)
test_chart = ch
chartbuilder = ChartBuilder(test_chart)
self.assertRegex(
repr(chartbuilder.dump()),
@ -432,8 +431,8 @@ class ChartBuilderTestCase(BaseChartBuilderTestCase):
dep_ch = yaml.safe_load(self.dependency_chart_stream)
dep_ch['chart']['source_dir'] = (dep_chart_dir.path, '')
dependency_chart = dotify(dep_ch)
test_chart.dependencies = [dependency_chart]
dependency_chart = dep_ch
test_chart['dependencies'] = [dependency_chart]
chartbuilder = ChartBuilder(test_chart)
re = inspect.cleandoc("""
@ -463,8 +462,8 @@ class ChartBuilderNegativeTestCase(BaseChartBuilderTestCase):
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)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
# Confirm it failed for both encodings.
error_re = (r'.*A str exception occurred while trying to read file:'
@ -483,8 +482,8 @@ class ChartBuilderNegativeTestCase(BaseChartBuilderTestCase):
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)
test_chart = {'source_dir': (chart_dir.path, '')}
chartbuilder = ChartBuilder(test_chart)
side_effects = [self.exc_to_raise, "", ""]
with mock.patch("builtins.open", mock.mock_open(read_data="")) \

View File

@ -1,16 +1,15 @@
gitpython
grpcio==1.10.0
grpcio-tools==1.10.0
jsonschema>=2.6.0
keystoneauth1==2.21.0
keystonemiddleware==4.9.1
kubernetes>=1.0.0
Paste>=2.0.3
PasteDeploy>=1.5.2
protobuf>=3.4.0
PyYAML==3.12
requests
supermutes==0.2.5
Paste>=2.0.3
PasteDeploy>=1.5.2
jsonschema>=2.6.0
# API
falcon
@ -24,10 +23,10 @@ oslo.cache>=1.5.0 # Apache-2.0
oslo.concurrency>=3.8.0 # Apache-2.0
oslo.config!=4.3.0,!=4.4.0,>=4.0.0 # Apache-2.0
oslo.context>=2.14.0 # Apache-2.0
oslo.messaging!=5.25.0,>=5.24.2 # Apache-2.0
oslo.db>=4.24.0 # Apache-2.0
oslo.i18n!=3.15.2,>=2.1.0 # Apache-2.0
oslo.log>=3.22.0 # Apache-2.0
oslo.messaging!=5.25.0,>=5.24.2 # Apache-2.0
oslo.middleware>=3.27.0 # Apache-2.0
oslo.policy>=1.23.0 # Apache-2.0
oslo.serialization!=2.19.1,>=1.10.0 # Apache-2.0

View File

@ -6,12 +6,12 @@ sphinx>=1.6.2
sphinx_rtd_theme>=0.2.4
# Testing
flake8>=3.3.0
testtools==2.3.0
codecov
mock
bandit
codecov
flake8>=3.3.0
mock
pytest==3.2.1
pytest-mock
pytest-cov
pytest-mock
responses>=0.8.1
testtools==2.3.0