mirror of
https://github.com/kevinveenbirkenbach/computer-playbook.git
synced 2026-11-06 10:59:59 +00:00
fix(cache): refuse a render that captured a template failure
Ansible's marker behaviour replaces an undefined name with the literal
'<< error N - NAME is undefined >>' instead of raising, and it is
inherited ambiently from whatever task is rendering, so code calling
templar.template(fail_on_undefined=True) gets that string back rather
than an exception.
_render_with_templar then returns it, get_merged_applications memoises
it unconditionally under a key that fingerprints only applications,
users and two domains, and hands the same object back without a
deepcopy. The first caller in the process therefore freezes the value
for every later one: a lookup from another role renders a role's addon
config without that role's vars, and the role itself is served the
poisoned result afterwards even though its own read happens in the right
scope.
It reached a running Nextcloud as the stored Collabora WOPI URL. A
configuration key set to an error message is worse than a missing one,
because the application stores it and reports itself configured.
The check sits at the end of the one function every merge render passes
through, so applications, users and domains are covered, and it runs
before any caller can memoise. The path is carried down while
descending, so the message names the offending key rather than the
payload. The pattern requires the ordinal and the dash so prose about
errors does not trip it.
2678090934 made the same choice for the lookup layer; this is the cache
layer.
This raises where the code used to continue. That is the point - a value
nobody can act on should stop a deploy rather than configure an
application - but the blast radius is only measured by the next deploy.
This commit is contained in:
1 parent
42c7df9ed9
commit
49f2dd516b
2 files changed
+69
No files matched your search
+34
@@ -182,6 +182,40 @@ class TestResolveOverrideMapping(unittest.TestCase):
|
||||
self.assertEqual(result, {"web-app-x": {"foo": 1}})
|
||||
|
||||
|
||||
class TestErrorMarkersAreRefused(unittest.TestCase):
|
||||
"""A captured template failure must not become a cached configuration value.
|
||||
|
||||
Ansible's marker behaviour replaces an undefined name with a literal
|
||||
``<< error N - ... >>`` string instead of raising. Nextcloud stored one as
|
||||
the Collabora WOPI URL and reported itself configured.
|
||||
"""
|
||||
|
||||
MARKER = "<< error 10 - 'NEXTCLOUD_COLLABORA_URL' is undefined >>"
|
||||
|
||||
def test_a_clean_payload_passes(self):
|
||||
base.assert_no_error_markers({"a": {"b": ["fine", 42, None]}})
|
||||
|
||||
def test_a_marker_in_a_nested_list_is_refused(self):
|
||||
payload = {"applications": {"app": {"config": [{"value": self.MARKER}]}}}
|
||||
with self.assertRaises(ValueError) as caught:
|
||||
base.assert_no_error_markers(payload)
|
||||
self.assertIn("applications.app.config[0].value", str(caught.exception))
|
||||
|
||||
def test_the_message_carries_the_offending_value(self):
|
||||
with self.assertRaises(ValueError) as caught:
|
||||
base.assert_no_error_markers({"k": self.MARKER})
|
||||
self.assertIn("NEXTCLOUD_COLLABORA_URL", str(caught.exception))
|
||||
|
||||
def test_prose_mentioning_an_error_is_not_a_marker(self):
|
||||
"""The pattern must not fire on a description that discusses errors."""
|
||||
base.assert_no_error_markers({"note": "returns << error >> on failure"})
|
||||
base.assert_no_error_markers({"note": "error 10 - see the log"})
|
||||
|
||||
def test_a_marker_anywhere_in_a_longer_string_is_caught(self):
|
||||
with self.assertRaises(ValueError):
|
||||
base.assert_no_error_markers({"url": f"https://x/{self.MARKER}/y"})
|
||||
|
||||
|
||||
class TestFingerprintIsContentOnly(unittest.TestCase):
|
||||
def test_fingerprint_survives_reset_because_it_holds_no_state(self):
|
||||
before = base._fingerprint_mapping({"a": 1})
|
||||
|
||||
Vendored
+35
@@ -16,6 +16,7 @@ only when actually invoked.
|
||||
from __future__ import annotations
|
||||
|
||||
import copy
|
||||
import re
|
||||
import threading
|
||||
from collections.abc import Mapping
|
||||
from pathlib import Path
|
||||
@@ -186,6 +187,39 @@ def _resolve_override_mapping(
|
||||
return {}
|
||||
|
||||
|
||||
ERROR_MARKER_RE = re.compile(r"<<\s*error\s+\d+\s*-")
|
||||
|
||||
|
||||
def assert_no_error_markers(data: Any, path: str = "") -> None:
|
||||
"""Refuse a render that captured a template failure instead of raising.
|
||||
|
||||
Args:
|
||||
data: the rendered structure.
|
||||
path: dotted position reached so far, for the message.
|
||||
|
||||
Ansible's marker behaviour, inherited from whatever task is rendering,
|
||||
replaces an undefined name with the literal ``<< error N - ... >>`` rather
|
||||
than raising. Left alone that string is memoised and handed to every later
|
||||
caller, and it reaches applications as a configuration value: a key set to
|
||||
an error message, which the application stores and reports as configured.
|
||||
Failing here keeps a value nobody can act on out of the cache.
|
||||
"""
|
||||
if isinstance(data, Mapping):
|
||||
for key, item in data.items():
|
||||
assert_no_error_markers(item, f"{path}.{key}" if path else str(key))
|
||||
return
|
||||
if isinstance(data, (list, tuple)):
|
||||
for index, item in enumerate(data):
|
||||
assert_no_error_markers(item, f"{path}[{index}]")
|
||||
return
|
||||
if isinstance(data, str) and ERROR_MARKER_RE.search(data):
|
||||
raise ValueError(
|
||||
f"{path or '<root>'} rendered to {data!r}. A template failed and "
|
||||
f"the failure was captured as text; caching it would hand that "
|
||||
f"string to every later reader and to the application itself."
|
||||
)
|
||||
|
||||
|
||||
def _render_with_templar(
|
||||
value: Any,
|
||||
*,
|
||||
@@ -274,6 +308,7 @@ def _render_with_templar(
|
||||
if hasattr(templar, "available_variables"):
|
||||
templar.available_variables = prev_templar_avail
|
||||
|
||||
assert_no_error_markers(data)
|
||||
return _decrypt_ansible_encrypted_strings(data)
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user