YAML documentation, test, and readability improvements

changelog/63158.fixed.md

@@ -0,0 +1,2 @@
+Updated YAML idiosyncrasies documentation, improved YAML tests, and improved
+readability of YAML code

doc/topics/troubleshooting/yaml_idiosyncrasies.rst

@@ -382,50 +382,54 @@ Here's an example:
 Automatic ``datetime`` conversion
 =================================
 
-If there is a value in a YAML file formatted ``2014-01-20 14:23:23`` or
-similar, YAML will automatically convert this to a Python ``datetime`` object.
-These objects are not msgpack serializable, and so may break core salt
-functionality.  If values such as these are needed in a salt YAML file
-(specifically a configuration file), they should be formatted with surrounding
-strings to force YAML to serialize them as strings:
+.. versionchanged:: 2018.3.0
+
+    A YAML scalar node containing a timestamp now always produces a string.
+    Previously, Salt would attempt to create a Python ``datetime.datetime``
+    object, even if the node contained an invalid date (for example,
+    ``4017-16-20``).
+
+Salt overrides PyYAML's default behavior and always loads YAML nodes that look
+like timestamps (including nodes explicitly tagged with ``!!timestamp``) as
+strings:
 
 .. code-block:: pycon
 
-    >>> import yaml
-    >>> yaml.safe_load("2014-01-20 14:23:23")
-    datetime.datetime(2014, 1, 20, 14, 23, 23)
-    >>> yaml.safe_load('"2014-01-20 14:23:23"')
+    >>> import salt.utils.yaml
+    >>> salt.utils.yaml.safe_load("2014-01-20 14:23:23")
+    '2014-01-20 14:23:23'
+    >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23")
     '2014-01-20 14:23:23'
 
-Additionally, numbers formatted like ``XXXX-XX-XX`` will also be converted (or
-YAML will attempt to convert them, and error out if it doesn't think the date
-is a real one).  Thus, for example, if a minion were to have an ID of
-``4017-16-20`` the minion would not start because YAML would complain that the
-date was out of range.  The workaround is the same, surround the offending
-string with quotes:
+There is currently no way to force Salt to produce a Python
+``datetime.datetime`` object from a timestamp in a YAML file.
 
-.. code-block:: pycon
+Ordered Dictionaries
+====================
 
-    >>> import yaml
-    >>> yaml.safe_load("4017-16-20")
-    Traceback (most recent call last):
-      File "<stdin>", line 1, in <module>
-      File "/usr/local/lib/python2.7/site-packages/yaml/__init__.py", line 93, in safe_load
-        return load(stream, SafeLoader)
-      File "/usr/local/lib/python2.7/site-packages/yaml/__init__.py", line 71, in load
-        return loader.get_single_data()
-      File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 39, in get_single_data
-        return self.construct_document(node)
-      File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 43, in construct_document
-        data = self.construct_object(node)
-      File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 88, in construct_object
-        data = constructor(self, node)
-      File "/usr/local/lib/python2.7/site-packages/yaml/constructor.py", line 312, in construct_yaml_timestamp
-        return datetime.date(year, month, day)
-    ValueError: month must be in 1..12
-    >>> yaml.safe_load('"4017-16-20"')
-    '4017-16-20'
+The YAML specification defines an `ordered mapping type
+<https://yaml.org/type/omap>`_ which is equivalent to a plain mapping except
+iteration order is preserved.  (YAML makes no guarantees about iteration order
+for entries loaded from a plain mapping.)
+
+Ordered mappings are represented as an ``!!omap`` tagged sequence of
+single-entry mappings:
+
+.. code-block:: yaml
+
+    !!omap
+    - key1: value1
+    - key2: value2
+
+Starting with Python 3.6, plain ``dict`` objects iterate in insertion order so
+there is no longer a strong need for the ``!!omap`` type.  However, some users
+may prefer the ``!!omap`` type over the plain ``!!map`` type because (1) it
+makes it obvious that the order of entries is significant, and (2) it provides a
+stronger guarantee of iteration order (plain mapping iteration order can be
+thought of as a Salt implementation detail that may change in the future).
 
+Unfortunately, ``!!omap`` nodes should be avoided due to bugs in the way Salt
+processes such nodes.
 
 Keys Limited to 1024 Characters
 ===============================

salt/utils/yamldumper.py

@@ -35,17 +35,6 @@
 ]
 
 
-class IndentMixin(Dumper):
-    """
-    Mixin that improves YAML dumped list readability
-    by indenting them by two spaces,
-    instead of being flush with the key they are under.
-    """
-
-    def increase_indent(self, flow=False, indentless=False):
-        return super().increase_indent(flow, False)
-
-
 class OrderedDumper(Dumper):
     """
     A YAML dumper that represents python OrderedDict as simple YAML map.
@@ -58,11 +47,11 @@ class SafeOrderedDumper(SafeDumper):
     """
 
 
-class IndentedSafeOrderedDumper(IndentMixin, SafeOrderedDumper):
-    """
-    A YAML safe dumper that represents python OrderedDict as simple YAML map,
-    and also indents lists by two spaces.
-    """
+class IndentedSafeOrderedDumper(SafeOrderedDumper):
+    """Like ``SafeOrderedDumper``, except it indents lists for readability."""
+
+    def increase_indent(self, flow=False, indentless=False):
+        return super().increase_indent(flow, False)
 
 
 def represent_ordereddict(dumper, data):
@@ -89,56 +78,45 @@ def represent_listproxy(dumper, data):
     return dumper.represent_list(list(data))
 
 
-OrderedDumper.add_representer(OrderedDict, represent_ordereddict)
-OrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict)
-SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict)
-SafeOrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict)
-SafeOrderedDumper.add_representer(None, represent_undefined)
+# OrderedDumper does not inherit from SafeOrderedDumper, so any applicable
+# representers added to SafeOrderedDumper must also be explicitly added to
+# OrderedDumper.
+for D in (SafeOrderedDumper, OrderedDumper):
+    # This default registration matches types that don't match any other
+    # registration, overriding PyYAML's default behavior of raising an
+    # exception.  This representer instead produces null nodes.
+    #
+    # TODO: Why does this registration exist?  Isn't it better to raise an
+    # exception for unsupported types?
+    D.add_representer(None, represent_undefined)
+    D.add_representer(OrderedDict, represent_ordereddict)
+    D.add_representer(HashableOrderedDict, represent_ordereddict)
+    D.add_representer(
+        collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict
+    )
+    D.add_representer(
+        salt.utils.context.NamespacedDictWrapper,
+        yaml.representer.SafeRepresenter.represent_dict,
+    )
+    D.add_representer(OptsDict, represent_optsdict)
+    D.add_representer(DictProxy, represent_dictproxy)
+    D.add_representer(ListProxy, represent_listproxy)
+    # Pillar containers are wrapped in MaskedDict / MaskedList for repr redaction;
+    # they are still plain dict / list at the data level, so dump them as such
+    # instead of falling through to represent_undefined (which would emit NULL).
+    D.add_representer(MaskedDict, yaml.representer.SafeRepresenter.represent_dict)
+    D.add_representer(MaskedList, yaml.representer.SafeRepresenter.represent_list)
+del D
+
 IndentedSafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict)
 IndentedSafeOrderedDumper.add_representer(HashableOrderedDict, represent_ordereddict)
-
-OrderedDumper.add_representer(
-    collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict
-)
-SafeOrderedDumper.add_representer(
-    collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict
-)
-OrderedDumper.add_representer(
-    salt.utils.context.NamespacedDictWrapper,
-    yaml.representer.SafeRepresenter.represent_dict,
-)
-SafeOrderedDumper.add_representer(
-    salt.utils.context.NamespacedDictWrapper,
-    yaml.representer.SafeRepresenter.represent_dict,
-)
-
-OrderedDumper.add_representer(OptsDict, represent_optsdict)
-SafeOrderedDumper.add_representer(OptsDict, represent_optsdict)
-OrderedDumper.add_representer(DictProxy, represent_dictproxy)
-SafeOrderedDumper.add_representer(DictProxy, represent_dictproxy)
-OrderedDumper.add_representer(ListProxy, represent_listproxy)
-SafeOrderedDumper.add_representer(ListProxy, represent_listproxy)
-# Pillar containers are wrapped in MaskedDict / MaskedList for repr redaction;
-# they are still plain dict / list at the data level, so dump them as such
-# instead of falling through to represent_undefined (which would emit NULL).
-OrderedDumper.add_representer(
-    MaskedDict, yaml.representer.SafeRepresenter.represent_dict
-)
-SafeOrderedDumper.add_representer(
-    MaskedDict, yaml.representer.SafeRepresenter.represent_dict
-)
 IndentedSafeOrderedDumper.add_representer(
     MaskedDict, yaml.representer.SafeRepresenter.represent_dict
 )
-OrderedDumper.add_representer(
-    MaskedList, yaml.representer.SafeRepresenter.represent_list
-)
-SafeOrderedDumper.add_representer(
-    MaskedList, yaml.representer.SafeRepresenter.represent_list
-)
 IndentedSafeOrderedDumper.add_representer(
     MaskedList, yaml.representer.SafeRepresenter.represent_list
 )
+
 # Also register with base YAML dumpers for salt.utils.yaml.dump()
 yaml.Dumper.add_representer(OptsDict, represent_optsdict)
 yaml.SafeDumper.add_representer(OptsDict, represent_optsdict)
@@ -155,13 +133,6 @@ def represent_listproxy(dumper, data):
     MaskedList, yaml.representer.SafeRepresenter.represent_list
 )
 
-OrderedDumper.add_representer(
-    "tag:yaml.org,2002:timestamp", OrderedDumper.represent_scalar
-)
-SafeOrderedDumper.add_representer(
-    "tag:yaml.org,2002:timestamp", SafeOrderedDumper.represent_scalar
-)
-
 
 def get_dumper(dumper_name):
     return {
@@ -178,8 +149,7 @@ def dump(data, stream=None, **kwargs):
     Helper that wraps yaml.dump and ensures that we encode unicode strings
     unless explicitly told not to.
     """
-    if "allow_unicode" not in kwargs:
-        kwargs["allow_unicode"] = True
+    kwargs.setdefault("allow_unicode", True)
     kwargs.setdefault("default_flow_style", None)
     return yaml.dump(data, stream, **kwargs)
 
@@ -190,7 +160,4 @@ def safe_dump(data, stream=None, **kwargs):
     represented properly. Ensure that unicode strings are encoded unless
     explicitly told not to.
     """
-    if "allow_unicode" not in kwargs:
-        kwargs["allow_unicode"] = True
-    kwargs.setdefault("default_flow_style", None)
-    return yaml.dump(data, stream, Dumper=SafeOrderedDumper, **kwargs)
+    return dump(data, stream, Dumper=SafeOrderedDumper, **kwargs)

tests/pytests/integration/pillar/test_pillar_map_order.py

@@ -0,0 +1,95 @@
+import random
+import textwrap
+
+import pytest
+
+pytestmark = [
+    pytest.mark.slow_test,
+]
+
+
+@pytest.fixture(scope="module")
+def minion_run(salt_minion, salt_cli):
+    """Convenience fixture that runs the ``salt`` CLI targeting the minion."""
+
+    def _run(*args, minion_tgt=salt_minion.id, **kwargs):
+        ret = salt_cli.run(*args, minion_tgt=minion_tgt, **kwargs)
+        assert ret.returncode == 0
+        return ret.data
+
+    yield _run
+
+
+def test_pillar_map_order(salt_master, minion_run):
+    """Test iteration order of YAML map entries in a Pillar ``.sls`` file.
+
+    This test generates a Pillar ``.sls`` file containing an ordinary YAML map
+    and tests whether the resulting Python object preserves iteration order.
+    Random keys are used to ensure that iteration order does not coincidentally
+    match.  The generated Pillar YAML file looks like this:
+
+    .. code-block:: yaml
+
+        data:
+          k3334244338: 0
+          k3444116829: 1
+          k2072366017: 2
+          # ... omitted for brevity ...
+          k1638299831: 19
+
+    A jinja template iterates over the entries in the resulting object to ensure
+    that iteration order is preserved.  The expected output looks like:
+
+    .. code-block:: text
+
+        k3334244338 0
+        k3444116829 1
+        k2072366017 2
+        ... omitted for brevity ...
+        k1638299831 19
+
+    Note: Python 3.6 switched to a new ``dict`` implementation that iterates in
+    insertion order.  This behavior was made an official part of the ``dict``
+    API in Python 3.7:
+
+    * https://docs.python.org/3.6/whatsnew/3.6.html#new-dict-implementation
+    * https://mail.python.org/pipermail/python-dev/2017-December/151283.html
+    * https://docs.python.org/3.7/whatsnew/3.7.html
+
+    Thus, this test may fail on Python 3.5 and older.  However, Salt currently
+    requires a newer version of Python, so this should not be a problem.
+
+    This is a regression test for:
+    https://github.com/saltstack/salt/issues/12161
+    """
+    # Filter the random keys through a set to avoid duplicates.
+    keys = list({f"k{random.getrandbits(32)}" for _ in range(20)})
+    # Avoid unintended correlation with set()'s iteration order.
+    random.shuffle(keys)
+    items = [(k, i) for i, k in enumerate(keys)]
+    top_yaml = "base: {'*': [data]}\n"
+    top_sls = salt_master.pillar_tree.base.temp_file("top.sls", top_yaml)
+    data_yaml = "data:\n" + "".join(f"  {k}: {v}\n" for k, v in items)
+    data_sls = salt_master.pillar_tree.base.temp_file("data.sls", data_yaml)
+    tmpl_jinja = textwrap.dedent(
+        """\
+            {%- for k, v in pillar['data'].items() %}
+            {{ k }} {{ v }}
+            {%- endfor %}
+        """
+    )
+    want = "\n" + "".join(f"{k} {v}\n" for k, v in items)
+    try:
+        with top_sls, data_sls:
+            assert minion_run("saltutil.refresh_pillar", wait=True) is True
+            got = minion_run(
+                "file.apply_template_on_contents",
+                tmpl_jinja,
+                template="jinja",
+                context={},
+                defaults={},
+                saltenv="base",
+            )
+            assert got == want
+    finally:
+        assert minion_run("saltutil.refresh_pillar", wait=True) is True

tests/pytests/unit/modules/test_schedule.py

@@ -9,6 +9,7 @@
 import pytest
 
 import salt.modules.schedule as schedule
+import salt.utils.yaml
 from salt.utils.event import SaltEvent
 from tests.support.mock import MagicMock, call, mock_open, patch
 
@@ -338,7 +339,22 @@ def test_add():
                 ) == {"comment": comm1, "changes": changes1, "result": True}
 
                 _call = call(
-                    b"schedule:\n  job3: {function: test.ping, seconds: 3600, maxrunning: 1, name: job3, enabled: true,\n    jid_include: true}\n"
+                    salt.utils.yaml.safe_dump(
+                        {
+                            "schedule": {
+                                "job3": OrderedDict(
+                                    [
+                                        ("function", "test.ping"),
+                                        ("seconds", 3600),
+                                        ("maxrunning", 1),
+                                        ("name", "job3"),
+                                        ("enabled", True),
+                                        ("jid_include", True),
+                                    ],
+                                ),
+                            },
+                        }
+                    ).encode()
                 )
                 write_calls = fopen_mock.filehandles[schedule_config_file][
                     1

tests/pytests/unit/modules/test_seed.py

@@ -11,6 +11,7 @@
 
 import salt.modules.seed as seed
 import salt.utils.files
+import salt.utils.yaml
 from tests.support.mock import MagicMock, patch
 
 
@@ -28,7 +29,7 @@ def test_mkconfig_odict():
         data = seed.mkconfig(ddd, approve_key=False)
         with salt.utils.files.fopen(data["config"]) as fic:
             fdata = fic.read()
-            assert fdata == "b: b\na: b\nmaster: foo\n"
+            assert fdata == salt.utils.yaml.safe_dump(ddd, default_flow_style=False)
 
 
 def test_prep_bootstrap():