From 99d39545a68c4b9a47de87eb1f6954330a22596a Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 26 Jan 2023 15:34:35 -0800 Subject: [PATCH] Add an !unsafe change_message variable In I9628e2770dda120b269612e28bb6217036942b8e we switched zuul.change from a plain string tagged with !unsafe to base64 encoded and no !unsafe tag. The idea was to make the inventory file parseable by external tools while avoiding accidental interpolation of the commit message by Ansible. That doesn't work in all cases -- it's not hard to construct a scenario where after base64 decoding the message any further processing by Ansible causes it to undergo interpolation. Moreover, since then we have made many changes to how we deal with variables; notably, the inventory.yaml is no longer actually used by Zuul's Anisble -- it is now there only for human and downstream processing. We call it the "debug inventory". The actual inventory is much more complex and in some cases has lots of !unsafe tags in it. Given all that, it now seems like the most straightforward way to deal with this is to tag the message variable as !unsafe when passing it to Zuul's Ansible, but render it as plain text in the inventory.yaml. To address backwards compatability, this is done in a new variable called zuul.change_message. Since that's a more descriptive variable anyway, we will just keep that one in the future and drop the current base64- encoded zuul.message variable Change-Id: Iea86de15e722bc271c1bf0540db2c9efb032500c --- doc/source/job-content.rst | 19 +++++++++++-------- .../change_message-18207e18b5dfffd3.yaml | 12 ++++++++++++ .../playbooks/jinja2-message.yaml | 4 ++++ tests/unit/test_inventory.py | 12 +++++++++--- zuul/executor/common.py | 1 + zuul/executor/server.py | 17 +++++++++++++---- 6 files changed, 50 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/change_message-18207e18b5dfffd3.yaml diff --git a/doc/source/job-content.rst b/doc/source/job-content.rst index 75044cf1c4..d6bb07683b 100644 --- a/doc/source/job-content.rst +++ b/doc/source/job-content.rst @@ -612,7 +612,6 @@ of item. The patchset identifier for the change. If a change is revised, this will have a different value. - .. var:: resources :type: dict @@ -706,14 +705,18 @@ are available: The commit or pull request message of the change base64 encoded. Use the `b64decode` filter in ansible when working with it. - .. code-block:: yaml + .. warning:: This variable is deprecated and will be removed in + a future version. Use :var:`zuul.change_message` + instead. - - hosts: all - tasks: - - name: Dump commit message - copy: - content: "{{ zuul.message | b64decode }}" - dest: "{{ zuul.executor.log_root }}/commit-message.txt" + .. var:: change_message + + The commit or pull request message of the change. When Zuul + runs Ansible, this variable is tagged with the ``!unsafe`` YAML + tag so that Ansible will not interpolate values into it. Note, + however, that the `inventory.yaml` file placed in the build's + workspace for debugging and inspection purposes does not inclued + the ``!unsafe`` tag. Branch Items diff --git a/releasenotes/notes/change_message-18207e18b5dfffd3.yaml b/releasenotes/notes/change_message-18207e18b5dfffd3.yaml new file mode 100644 index 0000000000..1fd0056846 --- /dev/null +++ b/releasenotes/notes/change_message-18207e18b5dfffd3.yaml @@ -0,0 +1,12 @@ +--- +features: + - | + The change message (commit message, or pull request message + depending on the driver) is now available in plain text form + annotated with the Ansible `!unsafe` tag as + :var:`zuul.change_message`. +deprecations: + - | + The base64 encoded version of the change message available as + :var:`zuul.message` is deprecated and will be removed in a future + version of Zuul. Use :var:`zuul.change_message` instead. diff --git a/tests/fixtures/config/inventory/git/common-config/playbooks/jinja2-message.yaml b/tests/fixtures/config/inventory/git/common-config/playbooks/jinja2-message.yaml index 834c3cbe84..cea1ffe8c6 100644 --- a/tests/fixtures/config/inventory/git/common-config/playbooks/jinja2-message.yaml +++ b/tests/fixtures/config/inventory/git/common-config/playbooks/jinja2-message.yaml @@ -4,3 +4,7 @@ copy: content: "{{ zuul.message | b64decode }}" dest: "{{ zuul.executor.log_root }}/commit-message.txt" + - name: Dump commit message + copy: + content: "{{ zuul.change_message }}" + dest: "{{ zuul.executor.log_root }}/change-message.txt" diff --git a/tests/unit/test_inventory.py b/tests/unit/test_inventory.py index 5cafe2ddf5..40f8586249 100644 --- a/tests/unit/test_inventory.py +++ b/tests/unit/test_inventory.py @@ -417,23 +417,29 @@ class TestAnsibleInventory(AnsibleZuulTestCase): zv_path = os.path.join(build.jobdir.root, 'ansible', 'zuul_vars.yaml') with open(zv_path, 'r') as f: - zv = yaml.safe_load(f) + zv = yaml.ansible_unsafe_load(f) # TODO(corvus): zuul vars aren't really stored here anymore; # rework these tests to examine them separately. inventory['all']['vars'] = {'zuul': zv['zuul']} + # The deprecated base64 version decoded_message = base64.b64decode( inventory['all']['vars']['zuul']['message']).decode('utf-8') self.assertEqual(decoded_message, expected_message) - obtained_message = self._get_file(self.history[0], 'work/logs/commit-message.txt') + self.assertEqual(obtained_message, expected_message) + # The new !unsafe version + decoded_message = inventory['all']['vars']['zuul']['change_message'] + self.assertEqual(decoded_message, expected_message) + obtained_message = self._get_file(self.history[0], + 'work/logs/change-message.txt') self.assertEqual(obtained_message, expected_message) def test_jinja2_message_brackets(self): - self._jinja2_message("This message has {{ jinja2 }} in it ") + self._jinja2_message("This message has {{ ansible_host }} in it ") def test_jinja2_message_raw(self): self._jinja2_message("This message has {% raw %} in {% endraw %} it ") diff --git a/zuul/executor/common.py b/zuul/executor/common.py index ff4522d226..b8393903e9 100644 --- a/zuul/executor/common.py +++ b/zuul/executor/common.py @@ -65,6 +65,7 @@ def construct_build_params(uuid, connections, job, item, pipeline, zuul_params['patchset'] = str(item.change.patchset) if hasattr(item.change, 'message'): zuul_params['message'] = strings.b64encode(item.change.message) + zuul_params['change_message'] = item.change.message if (hasattr(item.change, 'oldrev') and item.change.oldrev and item.change.oldrev != '0' * 40): zuul_params['oldrev'] = item.change.oldrev diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 72e5cf7793..a49bbbbbf9 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -14,6 +14,7 @@ # under the License. import collections +import copy import datetime import json import logging @@ -1049,7 +1050,7 @@ class AnsibleJob(object): # The same, but frozen self.frozen_hostvars = {} # The zuul.* vars - self.zuul_vars = {} + self.debug_zuul_vars = {} self.waiting_for_semaphores = False def run(self): @@ -2497,10 +2498,18 @@ class AnsibleJob(object): if ri.role_path is not None], )) + # The zuul vars in the debug inventory.yaml file should not + # have any !unsafe tags, so save those before we update the + # execution version of those. + self.debug_zuul_vars = copy.deepcopy(zuul_vars) + if 'change_message' in zuul_vars: + zuul_vars['change_message'] = yaml.mark_strings_unsafe( + zuul_vars['change_message']) + with open(self.jobdir.zuul_vars, 'w') as zuul_vars_yaml: zuul_vars_yaml.write( - yaml.safe_dump({'zuul': zuul_vars}, default_flow_style=False)) - self.zuul_vars = zuul_vars + yaml.ansible_unsafe_dump({'zuul': zuul_vars}, + default_flow_style=False)) # Squash all and extra vars into localhost (it's not # explicitly listed). @@ -2554,7 +2563,7 @@ class AnsibleJob(object): inventory = make_inventory_dict( self.host_list, self.nodeset, self.original_hostvars) - inventory['all']['vars']['zuul'] = self.zuul_vars + inventory['all']['vars']['zuul'] = self.debug_zuul_vars with open(self.jobdir.inventory, 'w') as inventory_yaml: inventory_yaml.write( yaml.ansible_unsafe_dump(