Handle zuul_return format errors

This change does two things:

First, it corrects an error in the zuul_return schema validation
which would, in a job where someone returned an artifact with a
missing required attribute such as "url", cause an exception in
the SQL reporter (which would lead to incomplete build results in
the db, and possibly other pipeline-related errors).

This change is made by correcting the voluptuous schema so that
the "Required" flag is on the key, not the value.

Secondly, it performs schema validation within the zuul_return
module itself.  This will cause such errors to become user-visible
at the time they occur (during the job) rather than post-facto.
To accomplish this, the schema definition is moved into the
shared zuul/ansible module, and a dependency on voluptuous within
the in-job ansible virtualenvs is added.

The first fix is still needed even with the second because users
can return data to Zuul without using the zuul_return plugin.

Change-Id: I6991863b2b5986b8067124b92c141c3c59240d18

releasenotes/notes/zuul-return-validate-5158d5400c8a7dc9.yaml

@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    The zuul_return Ansible plugin will now validate the schema of the
+    data supplied to it; particularly the data for warnings and
+    artifacts.  Previously the behavior on incorrectly structured data
+    was undefined and ranged from being silently ignorred to causing
+    exceptions in pipeline processing.  Data format errors will now be
+    detected while the job is running and will cause Ansible errors
+    and (if not ignored) job failures.

tests/fixtures/config/artifact-return/git/common-config/playbooks/bad-metadata.yaml

@@ -0,0 +1,9 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            artifacts:
+              - name: image
+                url: something
+                metadata: [1]

tests/fixtures/config/artifact-return/git/common-config/playbooks/no-name.yaml

@@ -0,0 +1,9 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            artifacts:
+              - url: something
+                metadata:
+                  type: test_data

tests/fixtures/config/artifact-return/git/common-config/playbooks/no-url.yaml

@@ -0,0 +1,9 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            artifacts:
+              - name: image
+                metadata:
+                  type: test_data

tests/fixtures/config/artifact-return/git/common-config/playbooks/ok.yaml

@@ -0,0 +1,10 @@
+- hosts: localhost
+  tasks:
+    - zuul_return:
+        data:
+          zuul:
+            artifacts:
+              - name: image
+                url: something
+                metadata:
+                  type: test_data

tests/fixtures/config/artifact-return/git/common-config/zuul.yaml

@@ -0,0 +1,46 @@
+- pipeline:
+    name: check
+    manager: independent
+    post-review: true
+    trigger:
+      gerrit:
+        - event: patchset-created
+    success:
+      gerrit:
+        Verified: 1
+    failure:
+      gerrit:
+        Verified: -1
+
+- job:
+    name: base
+    parent: null
+
+- job:
+    name: ok
+    run: playbooks/ok.yaml
+    files: ['ok']
+
+- job:
+    name: no-url
+    run: playbooks/no-url.yaml
+    files: ['no-url']
+
+- job:
+    name: no-name
+    run: playbooks/no-name.yaml
+    files: ['no-name']
+
+- job:
+    name: bad-metadata
+    run: playbooks/bad-metadata.yaml
+    files: ['bad-metadata']
+
+- project:
+    name: org/project
+    check:
+      jobs:
+        - ok
+        - no-url
+        - no-name
+        - bad-metadata

tests/fixtures/config/artifact-return/git/org_project/README

@@ -0,0 +1 @@
+test

tests/fixtures/config/artifact-return/main.yaml

@@ -0,0 +1,8 @@
+- tenant:
+    name: tenant-one
+    source:
+      gerrit:
+        config-projects:
+          - common-config
+        untrusted-projects:
+          - org/project

tests/unit/test_v3.py

@@ -6184,6 +6184,146 @@ class TestShadow(ZuulTestCase):
         ], ordered=False)
 
 
+class TestArtifactReturnSynthetic(ZuulTestCase):
+    # Artifact data return tests that don't run Ansible
+    tenant_config_file = 'config/single-tenant/main.yaml'
+
+    def _get_artifacts(self):
+        connection = self.scheds.first.sched.sql.connection
+        builds = connection.getBuilds()
+        return [dict(name=a.name,
+                     url=a.url,
+                     metadata=a.meta)
+                for a in builds[0].artifacts]
+
+    def _test_artifact_return(self, artifacts):
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
+        self.executor_server.returnData(
+            'check-job', A,
+            {'zuul':
+             {'artifacts': artifacts}
+             }
+        )
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name='check-job', result='SUCCESS', changes='1,1'),
+        ], ordered=False)
+
+    @simple_layout('layouts/simple.yaml')
+    def test_artifact_return_ok(self):
+        # Test the normal case.
+        self._test_artifact_return(
+            [
+                {'name': 'image',
+                 'url': 'something',
+                 'metadata': {
+                     'type': 'test_data'
+                 }},
+            ])
+        self.assertEqual([{'name': 'image',
+                           'url': 'something',
+                           'metadata': '{"type": "test_data"}'}],
+                         self._get_artifacts())
+
+    @simple_layout('layouts/simple.yaml')
+    def test_artifact_return_no_url(self):
+        # Test that if we return malformed data, the scheduler doesn't
+        # break.
+        self._test_artifact_return(
+            [
+                {'name': 'image',
+                 'metadata': {
+                     'type': 'test_data'
+                 }},
+            ])
+        self.assertEqual([], self._get_artifacts())
+
+    @simple_layout('layouts/simple.yaml')
+    def test_artifact_return_no_name(self):
+        # Test that if we return malformed data, the scheduler doesn't
+        # break.
+        self._test_artifact_return(
+            [
+                {'url': 'something',
+                 'metadata': {
+                     'type': 'test_data'
+                 }},
+            ])
+        self.assertEqual([], self._get_artifacts())
+
+    @simple_layout('layouts/simple.yaml')
+    def test_artifact_return_bad_metadata(self):
+        # Test that if we return malformed data, the scheduler doesn't
+        # break.
+        self._test_artifact_return(
+            [
+                {'name': 'image',
+                 'url': 'something',
+                 'metadata': [1],
+                 },
+            ])
+        self.assertEqual([], self._get_artifacts())
+
+
+class TestArtifactReturn(AnsibleZuulTestCase):
+    tenant_config_file = 'config/artifact-return/main.yaml'
+
+    def _get_artifacts(self):
+        connection = self.scheds.first.sched.sql.connection
+        builds = connection.getBuilds()
+        return [dict(name=a.name,
+                     url=a.url,
+                     metadata=a.meta)
+                for a in builds[0].artifacts]
+
+    def _get_file(self, build, path):
+        p = os.path.join(build.jobdir.root, path)
+        with open(p) as f:
+            return f.read()
+
+    def _test_artifact_return(self, job, error):
+        self.executor_server.keep_jobdir = True
+        expected_result = error and 'FAILURE' or 'SUCCESS'
+        A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
+                                           files={job: ''})
+        self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
+        self.waitUntilSettled()
+        self.assertHistory([
+            dict(name=job, result=expected_result, changes='1,1'),
+        ], ordered=False)
+        j = json.loads(self._get_file(self.history[0],
+                                      'work/logs/job-output.json'))
+        result = j[0]['plays'][0]['tasks'][1]['hosts']['localhost']
+        self.assertEqual('zuul_return', result['action'])
+        if error:
+            self.assertEqual([], self._get_artifacts())
+            self.assertIn(error, result['msg'])
+        else:
+            self.assertEqual([{'name': 'image',
+                               'url': 'something',
+                               'metadata': '{"type": "test_data"}'}],
+                             self._get_artifacts())
+            self.assertNotIn('msg', result)
+
+    def test_artifact_return_ok(self):
+        # Test the normal case.
+        self._test_artifact_return('ok', None)
+
+    def test_artifact_return_no_url(self):
+        # Test that bad data results in a user-visible error.
+        self._test_artifact_return('no-url', 'required key not provided')
+
+    def test_artifact_return_no_name(self):
+        # Test that bad data results in a user-visible error.
+        self._test_artifact_return('no-name', 'required key not provided')
+
+    def test_artifact_return_bad_metadata(self):
+        # Test that bad data results in a user-visible error.
+        self._test_artifact_return('bad-metadata',
+                                   'expected dict for dictionary value')
+
+
 class TestDataReturn(AnsibleZuulTestCase):
     tenant_config_file = 'config/data-return/main.yaml'
 

zuul/ansible/8/schema.py

@@ -0,0 +1 @@
+../schema.py

zuul/ansible/9/schema.py

@@ -0,0 +1 @@
+../schema.py

zuul/ansible/base/action/zuul_return.py

@@ -19,6 +19,15 @@ import tempfile
 from copy import deepcopy
 
 from ansible.plugins.action import ActionBase
+from zuul.ansible.schema import (
+    artifact_schema,
+    warning_schema,
+)
+
+
+def validate_schema(data):
+    artifact_schema(data)
+    warning_schema(data)
 
 
 def merge_dict(dict_a, dict_b):
@@ -119,6 +128,10 @@ def set_value(path, new_data, new_file, new_secret_data, new_secret_file):
     if new_secret_data:
         merge_data(new_secret_data, secret_data)
 
+    # Validate the schema:
+    validate_schema(data)
+    validate_schema(secret_data)
+
     # Replace our results file ('path') with the updated data.
     (f, tmp_path) = tempfile.mkstemp(dir=workdir)
     try:

zuul/ansible/schema.py

@@ -0,0 +1,44 @@
+# Copyright 2018-2019 Red Hat, Inc.
+# Copyright 2024 Acme Gating, LLC
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import voluptuous as vs
+
+
+artifact = {
+    vs.Required('name'): str,
+    vs.Required('url'): str,
+    'metadata': dict,
+}
+
+artifact_data = {
+    'zuul': {
+        'log_url': str,
+        'artifacts': [artifact],
+        vs.Extra: object,
+    },
+    vs.Extra: object,
+}
+
+warning_data = {
+    'zuul': {
+        'log_url': str,
+        'warnings': [str],
+        vs.Extra: object,
+    },
+    vs.Extra: object,
+}
+
+artifact_schema = vs.Schema(artifact_data)
+warning_schema = vs.Schema(warning_data)

zuul/lib/ansible-config.conf

@@ -5,7 +5,7 @@ default_version = 8
 # 2024-04-24: urllib3 = 2.1.0 causes problems with some Windows nodes, fixed in
 #             2.2.0 but ibm-cos-sdk is currently holding it back.
 #             https://github.com/urllib3/urllib3/pull/3326
-requirements = openstacksdk<0.99 openshift jmespath google-cloud-storage pywinrm boto3 azure-storage-blob ibm-cos-sdk netaddr passlib google-re2 urllib3!=2.1.0
+requirements = openstacksdk<0.99 openshift jmespath google-cloud-storage pywinrm boto3 azure-storage-blob ibm-cos-sdk netaddr passlib google-re2 urllib3!=2.1.0 voluptuous
 
 [8]
 requirements = ansible>=8.0,<9.0

zuul/lib/result_data.py

@@ -12,35 +12,12 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import voluptuous as v
 import urllib.parse
 
-artifact = {
-    'name': v.Required(str),
-    'url': v.Required(str),
-    'metadata': dict,
-}
-
-artifact_data = {
-    'zuul': {
-        'log_url': str,
-        'artifacts': [artifact],
-        v.Extra: object,
-    },
-    v.Extra: object,
-}
-
-warning_data = {
-    'zuul': {
-        'log_url': str,
-        'warnings': [str],
-        v.Extra: object,
-    },
-    v.Extra: object,
-}
-
-artifact_schema = v.Schema(artifact_data)
-warning_schema = v.Schema(warning_data)
+from zuul.ansible.schema import (
+    artifact_schema,
+    warning_schema,
+)
 
 
 def validate_schema(data, schema):