Merge branch 'tristan/fix-artifact-config-crash' into 'master'

Fix artifact config crash

Closes #625

See merge request BuildStream/buildstream!804

5 files changed, 46 insertions, 1 deletions

diff --git a/buildstream/_artifactcache/artifactcache.py b/buildstream/_artifactcache/artifactcache.py
index b5c27a165..f4157c7ef 100644
--- a/buildstream/_artifactcache/artifactcache.py
+++ b/buildstream/_artifactcache/artifactcache.py
@@ -51,7 +51,7 @@ class ArtifactCacheSpec(namedtuple('ArtifactCacheSpec', 'url push server_cert cl
         url = _yaml.node_get(spec_node, str, 'url')
         push = _yaml.node_get(spec_node, bool, 'push', default_value=False)
         if not url:
-            provenance = _yaml.node_get_provenance(spec_node)
+            provenance = _yaml.node_get_provenance(spec_node, 'url')
             raise LoadError(LoadErrorReason.INVALID_DATA,
                             "{}: empty artifact cache URL".format(provenance))
 
@@ -67,6 +67,16 @@ class ArtifactCacheSpec(namedtuple('ArtifactCacheSpec', 'url push server_cert cl
         if client_cert and basedir:
             client_cert = os.path.join(basedir, client_cert)
 
+        if client_key and not client_cert:
+            provenance = _yaml.node_get_provenance(spec_node, 'client-key')
+            raise LoadError(LoadErrorReason.INVALID_DATA,
+                            "{}: 'client-key' was specified without 'client-cert'".format(provenance))
+
+        if client_cert and not client_key:
+            provenance = _yaml.node_get_provenance(spec_node, 'client-cert')
+            raise LoadError(LoadErrorReason.INVALID_DATA,
+                            "{}: 'client-cert' was specified without 'client-key'".format(provenance))
+
         return ArtifactCacheSpec(url, push, server_cert, client_key, client_cert)
 
 
diff --git a/tests/artifactcache/config.py b/tests/artifactcache/config.py
index f59474708..8ab0ecee1 100644
--- a/tests/artifactcache/config.py
+++ b/tests/artifactcache/config.py
@@ -9,8 +9,12 @@ from buildstream._context import Context
 from buildstream._project import Project
 from buildstream.utils import _deduplicate
 from buildstream import _yaml
+from buildstream._exceptions import ErrorDomain, LoadErrorReason
 
+from tests.testutils.runcli import cli
 
+
+DATA_DIR = os.path.dirname(os.path.realpath(__file__))
 cache1 = ArtifactCacheSpec(url='https://example.com/cache1', push=True)
 cache2 = ArtifactCacheSpec(url='https://example.com/cache2', push=False)
 cache3 = ArtifactCacheSpec(url='https://example.com/cache3', push=False)
@@ -106,3 +110,33 @@ def test_artifact_cache_precedence(tmpdir, override_caches, project_caches, user
     # Verify that it was correctly read.
     expected_cache_specs = list(_deduplicate(itertools.chain(override_caches, project_caches, user_caches)))
     assert parsed_cache_specs == expected_cache_specs
+
+
+# Assert that if either the client key or client cert is specified
+# without specifying it's counterpart, we get a comprehensive LoadError
+# instead of an unhandled exception.
+@pytest.mark.datafiles(DATA_DIR)
+@pytest.mark.parametrize('config_key, config_value', [
+    ('client-cert', 'client.crt'),
+    ('client-key', 'client.key')
+])
+def test_missing_certs(cli, datafiles, config_key, config_value):
+    project = os.path.join(datafiles.dirname, datafiles.basename, 'missing-certs')
+
+    project_conf = {
+        'name': 'test',
+
+        'artifacts': {
+            'url': 'https://cache.example.com:12345',
+            'push': 'true',
+            config_key: config_value
+        }
+    }
+    project_conf_file = os.path.join(project, 'project.conf')
+    _yaml.dump(project_conf, project_conf_file)
+
+    # Use `pull` here to ensure we try to initialize the remotes, triggering the error
+    #
+    # This does not happen for a simple `bst show`.
+    result = cli.run(project=project, args=['pull', 'element.bst'])
+    result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA)
diff --git a/tests/artifactcache/missing-certs/certificates/client.crt b/tests/artifactcache/missing-certs/certificates/client.crt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/artifactcache/missing-certs/certificates/client.crt
diff --git a/tests/artifactcache/missing-certs/certificates/client.key b/tests/artifactcache/missing-certs/certificates/client.key
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/artifactcache/missing-certs/certificates/client.key
diff --git a/tests/artifactcache/missing-certs/element.bst b/tests/artifactcache/missing-certs/element.bst
new file mode 100644
index 000000000..3c29b4ea1
--- /dev/null
+++ b/tests/artifactcache/missing-certs/element.bst
@@ -0,0 +1 @@
+kind: autotools