From 874021326ca747717791dbffcf07a11eb6793b03 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 19:09:59 -0400 Subject: [PATCH 01/33] yaml: Fix documentation about `datetime` conversion I believe the behavior changed with commit 002aa88a97ed8a1c51f4ab1c965d22d064ea30a8 which was first released in Salt v2018.3.0. --- .../troubleshooting/yaml_idiosyncrasies.rst | 53 ++++++------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 1ee1f5326f21..c9be42e08b60 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -382,50 +382,27 @@ 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 -.. 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"') - '2014-01-20 14:23:23' + 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``). -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: +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("4017-16-20") - Traceback (most recent call last): - File "", line 1, in - 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' + >>> 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' +There is currently no way to force Salt to produce a Python +``datetime.datetime`` object from a timestamp in a YAML file. Keys Limited to 1024 Characters =============================== From 40668bc4534a94c06fc25f9297bad861781c71d6 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 22 Oct 2022 20:43:20 -0400 Subject: [PATCH 02/33] yaml: Document that `!!omap` should be avoided due to bugs --- .../troubleshooting/yaml_idiosyncrasies.rst | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index c9be42e08b60..8f5caf287b0a 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -404,6 +404,33 @@ strings: There is currently no way to force Salt to produce a Python ``datetime.datetime`` object from a timestamp in a YAML file. +Ordered Dictionaries +==================== + +The YAML specification defines an `ordered mapping type +`_ 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 =============================== From 47e7ec631ac7163d13fefb45ee8d6ea37113aa46 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 13 Oct 2022 23:52:47 -0400 Subject: [PATCH 03/33] yaml: Convert `salt.utils.yaml` tests to pytest --- tests/pytests/unit/utils/test_yaml.py | 155 ++++++++++++++++++++++++++ tests/unit/utils/test_yamldumper.py | 37 ------ tests/unit/utils/test_yamlloader.py | 140 ----------------------- 3 files changed, 155 insertions(+), 177 deletions(-) create mode 100644 tests/pytests/unit/utils/test_yaml.py delete mode 100644 tests/unit/utils/test_yamldumper.py delete mode 100644 tests/unit/utils/test_yamlloader.py diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py new file mode 100644 index 000000000000..8d9ad5b21da7 --- /dev/null +++ b/tests/pytests/unit/utils/test_yaml.py @@ -0,0 +1,155 @@ +import textwrap + +import pytest +import yaml +from yaml.constructor import ConstructorError + +import salt.utils.files +import salt.utils.yaml as salt_yaml +from tests.support.mock import mock_open, patch + + +def test_dump(): + data = {"foo": "bar"} + assert salt_yaml.dump(data) == "{foo: bar}\n" + assert salt_yaml.dump(data, default_flow_style=False) == "foo: bar\n" + + +def test_safe_dump(): + data = {"foo": "bar"} + assert salt_yaml.safe_dump(data) == "{foo: bar}\n" + assert salt_yaml.safe_dump(data, default_flow_style=False) == "foo: bar\n" + + +def render_yaml(data): + """ + Takes a YAML string, puts it into a mock file, passes that to the YAML + SaltYamlSafeLoader and then returns the rendered/parsed YAML data + """ + with patch("salt.utils.files.fopen", mock_open(read_data=data)) as mocked_file: + with salt.utils.files.fopen(mocked_file) as mocked_stream: + return salt_yaml.SaltYamlSafeLoader(mocked_stream).get_data() + + +def test_load_basics(): + """ + Test parsing an ordinary path + """ + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: + - alpha + - beta + """ + ) + ) + == {"p1": ["alpha", "beta"]} + ) + + +def test_load_merge(): + """ + Test YAML anchors + """ + # Simple merge test + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v2: beta + """ + ) + ) + == {"p1": {"v1": "alpha"}, "p2": {"v1": "alpha", "v2": "beta"}} + ) + + # Test that keys/nodes are overwritten + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v1: new_alpha + """ + ) + ) + == {"p1": {"v1": "alpha"}, "p2": {"v1": "new_alpha"}} + ) + + # Test merging of lists + assert ( + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: &v1 + - t1 + - t2 + p2: + v2: *v1 + """ + ) + ) + == {"p2": {"v2": ["t1", "t2"]}, "p1": {"v1": ["t1", "t2"]}} + ) + + +def test_load_duplicates(): + """ + Test that duplicates still throw an error + """ + with pytest.raises(ConstructorError): + render_yaml( + textwrap.dedent( + """\ + p1: alpha + p1: beta + """ + ) + ) + + with pytest.raises(ConstructorError): + render_yaml( + textwrap.dedent( + """\ + p1: &p1 + v1: alpha + p2: + <<: *p1 + v2: beta + v2: betabeta + """ + ) + ) + + +def test_load_with_plain_scalars(): + """ + Test that plain (i.e. unqoted) string and non-string scalars are + properly handled + """ + assert ( + render_yaml( + textwrap.dedent( + """\ + foo: + b: {foo: bar, one: 1, list: [1, two, 3]} + """ + ) + ) + == {"foo": {"b": {"foo": "bar", "one": 1, "list": [1, "two", 3]}}} + ) + + +def test_not_yaml_monkey_patching(): + if hasattr(yaml, "CSafeLoader"): + assert yaml.SafeLoader != yaml.CSafeLoader diff --git a/tests/unit/utils/test_yamldumper.py b/tests/unit/utils/test_yamldumper.py deleted file mode 100644 index 9a1a6ab103ba..000000000000 --- a/tests/unit/utils/test_yamldumper.py +++ /dev/null @@ -1,37 +0,0 @@ -""" - Unit tests for salt.utils.yamldumper -""" - -import salt.utils.yamldumper -from tests.support.unit import TestCase - - -class YamlDumperTestCase(TestCase): - """ - TestCase for salt.utils.yamldumper module - """ - - def test_yaml_dump(self): - """ - Test yaml.dump a dict - """ - data = {"foo": "bar"} - exp_yaml = "{foo: bar}\n" - - assert salt.utils.yamldumper.dump(data) == exp_yaml - - assert salt.utils.yamldumper.dump( - data, default_flow_style=False - ) == exp_yaml.replace("{", "").replace("}", "") - - def test_yaml_safe_dump(self): - """ - Test yaml.safe_dump a dict - """ - data = {"foo": "bar"} - assert salt.utils.yamldumper.safe_dump(data) == "{foo: bar}\n" - - assert ( - salt.utils.yamldumper.safe_dump(data, default_flow_style=False) - == "foo: bar\n" - ) diff --git a/tests/unit/utils/test_yamlloader.py b/tests/unit/utils/test_yamlloader.py deleted file mode 100644 index 04370a97083c..000000000000 --- a/tests/unit/utils/test_yamlloader.py +++ /dev/null @@ -1,140 +0,0 @@ -""" - Unit tests for salt.utils.yamlloader.SaltYamlSafeLoader -""" - -import textwrap - -from yaml.constructor import ConstructorError - -import salt.utils.files -from salt.utils.yamlloader import SaltYamlSafeLoader, yaml -from tests.support.mock import mock_open, patch -from tests.support.unit import TestCase - - -class YamlLoaderTestCase(TestCase): - """ - TestCase for salt.utils.yamlloader module - """ - - @staticmethod - def render_yaml(data): - """ - Takes a YAML string, puts it into a mock file, passes that to the YAML - SaltYamlSafeLoader and then returns the rendered/parsed YAML data - """ - with patch("salt.utils.files.fopen", mock_open(read_data=data)) as mocked_file: - with salt.utils.files.fopen(mocked_file) as mocked_stream: - return SaltYamlSafeLoader(mocked_stream).get_data() - - def test_yaml_basics(self): - """ - Test parsing an ordinary path - """ - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: - - alpha - - beta""" - ) - ), - {"p1": ["alpha", "beta"]}, - ) - - def test_yaml_merge(self): - """ - Test YAML anchors - """ - # Simple merge test - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v2: beta""" - ) - ), - {"p1": {"v1": "alpha"}, "p2": {"v1": "alpha", "v2": "beta"}}, - ) - - # Test that keys/nodes are overwritten - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v1: new_alpha""" - ) - ), - {"p1": {"v1": "alpha"}, "p2": {"v1": "new_alpha"}}, - ) - - # Test merging of lists - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: &v1 - - t1 - - t2 - p2: - v2: *v1""" - ) - ), - {"p2": {"v2": ["t1", "t2"]}, "p1": {"v1": ["t1", "t2"]}}, - ) - - def test_yaml_duplicates(self): - """ - Test that duplicates still throw an error - """ - with self.assertRaises(ConstructorError): - self.render_yaml( - textwrap.dedent( - """\ - p1: alpha - p1: beta""" - ) - ) - - with self.assertRaises(ConstructorError): - self.render_yaml( - textwrap.dedent( - """\ - p1: &p1 - v1: alpha - p2: - <<: *p1 - v2: beta - v2: betabeta""" - ) - ) - - def test_yaml_with_plain_scalars(self): - """ - Test that plain (i.e. unqoted) string and non-string scalars are - properly handled - """ - self.assertEqual( - self.render_yaml( - textwrap.dedent( - """\ - foo: - b: {foo: bar, one: 1, list: [1, two, 3]}""" - ) - ), - {"foo": {"b": {"foo": "bar", "one": 1, "list": [1, "two", 3]}}}, - ) - - def test_not_yaml_monkey_patching(self): - if hasattr(yaml, "CSafeLoader"): - assert yaml.SafeLoader != yaml.CSafeLoader From 4a431ebd16a4a60d663e00f95c8749a7e7fe7bda Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 16 Oct 2022 15:56:00 -0400 Subject: [PATCH 04/33] yaml: Add integration test for YAML map iteration order This demonstrates that https://github.com/saltstack/salt/issues/12161 has already been fixed (thanks to Python 3.6 changing `dict` to iterate in insertion order). --- .../pillar/test_pillar_map_order.py | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 tests/pytests/integration/pillar/test_pillar_map_order.py diff --git a/tests/pytests/integration/pillar/test_pillar_map_order.py b/tests/pytests/integration/pillar/test_pillar_map_order.py new file mode 100644 index 000000000000..8a8160de875a --- /dev/null +++ b/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, **kwargs): + ret = salt_cli.run(*args, **{"minion_tgt": salt_minion.id, **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 From 0b481a62605e8cc1509133c0523072ee9690c3e6 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:25:30 -0400 Subject: [PATCH 05/33] yaml: Add TODO comments next to puzzling code --- salt/utils/yamldumper.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index e5e937cac7d6..5fe9d408831b 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -71,6 +71,10 @@ def represent_undefined(dumper, data): OrderedDumper.add_representer(OrderedDict, represent_ordereddict) SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) + +# TODO: Why does this representer exist? It doesn't seem to do anything +# different compared to PyYAML's yaml.SafeDumper. +# TODO: Why isn't this representer also registered with OrderedDumper? SafeOrderedDumper.add_representer(None, represent_undefined) OrderedDumper.add_representer( @@ -88,6 +92,7 @@ def represent_undefined(dumper, data): yaml.representer.SafeRepresenter.represent_dict, ) +# TODO: These seem wrong: the first argument should be a type, not a tag. OrderedDumper.add_representer( "tag:yaml.org,2002:timestamp", OrderedDumper.represent_scalar ) From 329e269f01f166177c5311f921e770d63c3250c1 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 00:52:36 -0400 Subject: [PATCH 06/33] yaml: Use a `for` loop to factor out duplicate code --- changelog/62932.fixed | 2 ++ salt/utils/yamldumper.py | 39 +++++++++++++++------------------------ 2 files changed, 17 insertions(+), 24 deletions(-) create mode 100644 changelog/62932.fixed diff --git a/changelog/62932.fixed b/changelog/62932.fixed new file mode 100644 index 000000000000..3578c796086a --- /dev/null +++ b/changelog/62932.fixed @@ -0,0 +1,2 @@ +Improvements to YAML processing (`salt.utils.yaml`): + * Code health clean-ups. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 5fe9d408831b..f19896ae52b1 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -69,36 +69,27 @@ def represent_undefined(dumper, data): return dumper.represent_scalar("tag:yaml.org,2002:null", "NULL") -OrderedDumper.add_representer(OrderedDict, represent_ordereddict) -SafeOrderedDumper.add_representer(OrderedDict, represent_ordereddict) +# OrderedDumper does not inherit from SafeOrderedDumper, so any applicable +# representers added to SafeOrderedDumper must also be explicitly added to +# OrderedDumper. # TODO: Why does this representer exist? It doesn't seem to do anything # different compared to PyYAML's yaml.SafeDumper. # TODO: Why isn't this representer also registered with OrderedDumper? SafeOrderedDumper.add_representer(None, represent_undefined) -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, -) - -# TODO: These seem wrong: the first argument should be a type, not a tag. -OrderedDumper.add_representer( - "tag:yaml.org,2002:timestamp", OrderedDumper.represent_scalar -) -SafeOrderedDumper.add_representer( - "tag:yaml.org,2002:timestamp", SafeOrderedDumper.represent_scalar -) +for D in (SafeOrderedDumper, OrderedDumper): + D.add_representer(OrderedDict, represent_ordereddict) + D.add_representer( + collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict + ) + D.add_representer( + salt.utils.context.NamespacedDictWrapper, + yaml.representer.SafeRepresenter.represent_dict, + ) + # TODO: This seems wrong: the first argument should be a type, not a tag. + D.add_representer("tag:yaml.org,2002:timestamp", Dumper.represent_scalar) +del D def get_dumper(dumper_name): From 747e4657f553a2cecaba4fd06559f2e5c90178ca Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 19 Oct 2022 22:37:47 -0400 Subject: [PATCH 07/33] yaml: Factor out duplicate code in `salt.utils.yaml.safe_dump()` --- salt/utils/yamldumper.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index f19896ae52b1..aee6dd154f2d 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -119,7 +119,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) From 9f060cc33660659f6ae311e8734f9fbc70252e9d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:14:58 -0400 Subject: [PATCH 08/33] yaml: Improve readability of `salt.utils.yaml.dump()` --- salt/utils/yamldumper.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index aee6dd154f2d..91763b3ed5e0 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -107,9 +107,11 @@ 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("default_flow_style", None) + kwargs = { + "allow_unicode": True, + "default_flow_style": None, + **kwargs, + } return yaml.dump(data, stream, **kwargs) From fbf37d0a959c39751146c3c1650501c2afcbaa6c Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 19 Oct 2022 22:33:57 -0400 Subject: [PATCH 09/33] yaml: Default to `OrderedDumper` in `salt.utils.yaml.dump()` I believe this was the original intention. Even if it was not, the symmetry with `salt.utils.yaml.save_dump()` makes `dump()` less surprising now, and it makes it easier to introduce representer changes that affect both `dump()` and `safe_dump()`. --- changelog/62932.fixed | 2 ++ salt/utils/yamldumper.py | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 3578c796086a..b78438403417 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,2 +1,4 @@ Improvements to YAML processing (`salt.utils.yaml`): + * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` + instead of `yaml.Dumper`. * Code health clean-ups. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 91763b3ed5e0..771218d08bf5 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -104,12 +104,18 @@ def dump(data, stream=None, **kwargs): """ .. versionadded:: 2018.3.0 + .. versionchanged:: 3006.0 + + The default ``Dumper`` class is now ``OrderedDumper`` instead of + ``yaml.Dumper``. + Helper that wraps yaml.dump and ensures that we encode unicode strings unless explicitly told not to. """ kwargs = { "allow_unicode": True, "default_flow_style": None, + "Dumper": OrderedDumper, **kwargs, } return yaml.dump(data, stream, **kwargs) From 602c2c88259e69effb679c14d3f90417554f90aa Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 00:56:04 -0400 Subject: [PATCH 10/33] yaml: Delete unnecessary `IndentMixin` class to improve readability --- salt/utils/yamldumper.py | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 771218d08bf5..a970d2f589c6 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -31,17 +31,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. @@ -54,11 +43,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): From 7a903491e50a808d7e6a00f981061371925a5143 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 00:51:55 -0400 Subject: [PATCH 11/33] yaml: Fix IndentedSafeOrderedDumper indentation Before, the indentation would only be increased if the compiled module `yaml.CSafeDumper` did not exist. Now it is indented even if it does. Behavior before (assuming `yaml.CSafeDumper` exists): ``` $ python >>> import salt.utils.yaml as y >>> print(y.dump({"foo": ["bar"]}, Dumper=y.IndentedSafeOrderedDumper, default_flow_style=False)) foo: - bar ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> print(y.dump({"foo": ["bar"]}, Dumper=y.IndentedSafeOrderedDumper, default_flow_style=False)) foo: - bar ``` --- changelog/62932.fixed | 1 + salt/utils/yamldumper.py | 23 ++++++++++++----------- tests/pytests/unit/utils/test_yaml.py | 11 +++++++++++ 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index b78438403417..c55009a980f3 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,4 +1,5 @@ Improvements to YAML processing (`salt.utils.yaml`): * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. + * Fixed indentation in `salt.utils.yaml.IndentedSafeOrderedDumper` output. * Code health clean-ups. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index a970d2f589c6..32b0fa226660 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -43,7 +43,10 @@ class SafeOrderedDumper(SafeDumper): """ -class IndentedSafeOrderedDumper(SafeOrderedDumper): +# This must inherit from yaml.SafeDumper, not yaml.CSafeDumper, because the +# increase_indent hack doesn't work with yaml.CSafeDumper. +# https://github.com/yaml/pyyaml/issues/234#issuecomment-786026671 +class IndentedSafeOrderedDumper(yaml.SafeDumper): """Like ``SafeOrderedDumper``, except it indents lists for readability.""" def increase_indent(self, flow=False, indentless=False): @@ -58,16 +61,14 @@ def represent_undefined(dumper, data): return dumper.represent_scalar("tag:yaml.org,2002:null", "NULL") -# OrderedDumper does not inherit from SafeOrderedDumper, so any applicable -# representers added to SafeOrderedDumper must also be explicitly added to -# OrderedDumper. - -# TODO: Why does this representer exist? It doesn't seem to do anything -# different compared to PyYAML's yaml.SafeDumper. -# TODO: Why isn't this representer also registered with OrderedDumper? -SafeOrderedDumper.add_representer(None, represent_undefined) - -for D in (SafeOrderedDumper, OrderedDumper): +# The above Dumper classes do not inherit from each other, so any applicable +# representers must be added to each. +for D in (SafeOrderedDumper, IndentedSafeOrderedDumper): + # TODO: Why does this representer exist? It doesn't seem to do anything + # different compared to PyYAML's yaml.SafeDumper. + # TODO: Why isn't this representer also registered with OrderedDumper? + D.add_representer(None, represent_undefined) +for D in (SafeOrderedDumper, IndentedSafeOrderedDumper, OrderedDumper): D.add_representer(OrderedDict, represent_ordereddict) D.add_representer( collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 8d9ad5b21da7..1143bf7ccd3d 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -21,6 +21,17 @@ def test_safe_dump(): assert salt_yaml.safe_dump(data, default_flow_style=False) == "foo: bar\n" +def test_dump_indented(): + data = {"foo": ["bar"]} + got = salt_yaml.dump( + data, + Dumper=salt_yaml.IndentedSafeOrderedDumper, + default_flow_style=False, + ) + want = "foo:\n - bar\n" + assert got == want + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From 035609710917db7b74e9a3c792ce584fea9c4bcc Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 15 Oct 2022 01:01:38 -0400 Subject: [PATCH 12/33] yaml: Fix custom YAML to object constructor registration The `SaltYamlSafeLoader.add_constructor()` method is a class method, not an instance method. Therefore, any call to that method in an instance method (such as `__init__`) affects all future instances, not just the current instance (`self`). That's not a problem if the registration calls simply re-register the same constructor functions over and over, but that was not the case here: different functions were registered depending on the value of the `dictclass` parameter to `__init__`. The net effect was that an `!!omap` node was correctly processed as a sequence of single-entry maps (and a list of (key, value) tuples returned) until the first time a SaltYamlSafeLoader was constructed with a non-`dict` class. After that, every `!!omap` node was always incorrectly processed as a map node regardless of `dictclass`. Now `!!omap` processing is performed as originally intended: * If the `dictclass` parameter is `dict` (the default), the behavior when loading a `!!map` or `!!omap` node is unchanged from the base class's behavior. * If the `dictclass` parameter is not `dict`: * `!!map` nodes are loaded like they are in the base class except the custom class is used instead of `dict`. * `!!omap` nodes are loaded the same as `!!map` nodes. (This is a bug because an `!!omap` node is a sequence node of single-valued map nodes, not an ordinary map node. A future commit will fix this.) Behavior before: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("", dictclass=collections.OrderedDict) # created for side-effect only >>> y.load("!!omap [{foo: bar}, {baz: bif}]") # exact same as before Traceback (most recent call last): File "", line 1, in File "salt/utils/yamlloader.py", line 159, in load return yaml.load(stream, Loader=Loader) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 81, in load return loader.get_single_data() File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 45, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 56, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() Traceback (most recent call last): File "", line 1, in File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 42, in construct_yaml_omap return (yield from self.construct_yaml_map(node)) File "salt/utils/yamlloader.py", line 36, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 52, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] ``` This commit also adds a unit test for the `dictclass` parameter, though it's important to note that the new test is not a regression test for the bug fixed by this commit. (I don't think it would be worthwhile to write a regression test because the test code would be complicated and unreadable.) --- changelog/62932.fixed | 3 +++ salt/utils/yamlloader.py | 31 +++++++++++++++++---------- tests/pytests/unit/utils/test_yaml.py | 12 +++++++++++ 3 files changed, 35 insertions(+), 11 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index c55009a980f3..be8477c35ea9 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,4 +1,7 @@ Improvements to YAML processing (`salt.utils.yaml`): + * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` + constructor no longer causes all future `!!omap` nodes to throw an exception + when loading. * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Fixed indentation in `salt.utils.yaml.IndentedSafeOrderedDumper` output. diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 25b4b3bb9360..160968af42df 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -26,25 +26,21 @@ class SaltYamlSafeLoader(BaseLoader): def __init__(self, stream, dictclass=dict): super().__init__(stream) - if dictclass is not dict: - # then assume ordered dict and use it for both !map and !omap - self.add_constructor("tag:yaml.org,2002:map", type(self).construct_yaml_map) - self.add_constructor( - "tag:yaml.org,2002:omap", type(self).construct_yaml_map - ) - self.add_constructor("tag:yaml.org,2002:str", type(self).construct_yaml_str) - self.add_constructor( - "tag:yaml.org,2002:python/unicode", type(self).construct_unicode - ) - self.add_constructor("tag:yaml.org,2002:timestamp", type(self).construct_scalar) self.dictclass = dictclass def construct_yaml_map(self, node): + if self.dictclass is dict: + return (yield from super().construct_yaml_map(node)) data = self.dictclass() yield data value = self.construct_mapping(node) data.update(value) + def construct_yaml_omap(self, node): + if self.dictclass is dict: + return (yield from super().construct_yaml_omap(node)) + return (yield from self.construct_yaml_map(node)) + def construct_unicode(self, node): return node.value @@ -155,6 +151,19 @@ def flatten_mapping(self, node): node.value = mergeable_items + node.value +# BaseLoader.add_constructor() is a class method, not an instance method, so +# custom constructors should be registered at class creation time, not instance +# creation time. +for tag, constructor in [ + ("tag:yaml.org,2002:map", SaltYamlSafeLoader.construct_yaml_map), + ("tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap), + ("tag:yaml.org,2002:str", SaltYamlSafeLoader.construct_yaml_str), + ("tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode), + ("tag:yaml.org,2002:timestamp", SaltYamlSafeLoader.construct_scalar), +]: + SaltYamlSafeLoader.add_constructor(tag, constructor) + + def load(stream, Loader=SaltYamlSafeLoader): return yaml.load(stream, Loader=Loader) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 1143bf7ccd3d..1b79b37e88e2 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,3 +1,4 @@ +import collections import textwrap import pytest @@ -161,6 +162,17 @@ def test_load_with_plain_scalars(): ) +@pytest.mark.parametrize("dictclass", [dict, collections.OrderedDict]) +def test_load_dictclass(dictclass): + l = salt_yaml.SaltYamlSafeLoader("k1: v1\nk2: v2\n", dictclass=dictclass) + try: + d = l.get_single_data() + finally: + l.dispose() + assert isinstance(d, dictclass) + assert d == dictclass([("k1", "v1"), ("k2", "v2")]) + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 507ea7158cdaf995430fa85c8747dca44497bcf0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:40:43 -0400 Subject: [PATCH 13/33] yaml: Load `!!timestamp` nodes as `datetime.datetime` objects YAML nodes tagged with `!!timestamp` nodes now become Python `datetime` objects rather than strings. To preserve compatibility with existing YAML files, timestamp-like strings without `!!timestamp` are still loaded as strings. Behavior before: ``` $ python >>> import datetime >>> import salt.utils.yaml as y >>> y.load("'2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' >>> y.load("!!timestamp '2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' ``` Behavior after: ``` $ python >>> import datetime >>> import salt.utils.yaml as y >>> y.load("'2022-10-21T18:16:03.1-04:00'") '2022-10-21T18:16:03.1-04:00' >>> y.load("!!timestamp '2022-10-21T18:16:03.1-04:00'") datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=72000))) ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 21 ++++++++++++++----- salt/utils/yamlloader.py | 13 +++++++++++- tests/pytests/unit/utils/test_yaml.py | 19 +++++++++++++++++ 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index be8477c35ea9..ffe3c074a695 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -2,6 +2,8 @@ Improvements to YAML processing (`salt.utils.yaml`): * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` constructor no longer causes all future `!!omap` nodes to throw an exception when loading. + * Loading an explicitly tagged `!!timestamp` node now produces a + `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Fixed indentation in `salt.utils.yaml.IndentedSafeOrderedDumper` output. diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 8f5caf287b0a..37d489dbfde9 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -389,20 +389,31 @@ Automatic ``datetime`` conversion 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: +.. versionchanged:: 3006.0 + + Loading a YAML ``!!timestamp`` node now produces a ``datetime.datetime`` + object. Previously, nodes tagged with ``!!timestamp`` produced strings. + +Salt overrides PyYAML's default behavior and loads YAML nodes that look like +timestamps as strings: .. code-block:: pycon >>> import salt.utils.yaml >>> salt.utils.yaml.safe_load("2014-01-20 14:23:23") '2014-01-20 14:23:23' + +To force Salt to produce a ``datetime.datetime`` object instead of a string, +explicitly tag the node with ``!!timestamp``: + +.. code-block:: pycon + + >>> import salt.utils.yaml >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23") '2014-01-20 14:23:23' -There is currently no way to force Salt to produce a Python -``datetime.datetime`` object from a timestamp in a YAML file. +Beware that Salt is currently unable to serialize ``datetime.datetime`` objects, +so ``!!timestamp`` nodes cannot be used in pillar SLS files. Ordered Dictionaries ==================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 160968af42df..d93270385fa7 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -24,6 +24,14 @@ class SaltYamlSafeLoader(BaseLoader): to make things like sls file more intuitive. """ + @classmethod + def remove_implicit_resolver(cls, tag): + """Remove a previously registered implicit resolver for a tag.""" + cls.yaml_implicit_resolvers = { + first_char: [r for r in resolver_list if r[0] != tag] + for first_char, resolver_list in cls.yaml_implicit_resolvers.items() + } + def __init__(self, stream, dictclass=dict): super().__init__(stream) self.dictclass = dictclass @@ -159,10 +167,13 @@ def flatten_mapping(self, node): ("tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap), ("tag:yaml.org,2002:str", SaltYamlSafeLoader.construct_yaml_str), ("tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode), - ("tag:yaml.org,2002:timestamp", SaltYamlSafeLoader.construct_scalar), ]: SaltYamlSafeLoader.add_constructor(tag, constructor) +# Require users to explicitly provide the `!!timestamp` tag if a datetime object +# is desired. +SaltYamlSafeLoader.remove_implicit_resolver("tag:yaml.org,2002:timestamp") + def load(stream, Loader=SaltYamlSafeLoader): return yaml.load(stream, Loader=Loader) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 1b79b37e88e2..524cadccae64 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,4 +1,5 @@ import collections +import datetime import textwrap import pytest @@ -173,6 +174,24 @@ def test_load_dictclass(dictclass): assert d == dictclass([("k1", "v1"), ("k2", "v2")]) +@pytest.mark.parametrize( + "input_yaml,want", + [ + ( + "!!timestamp 2022-10-21T18:16:03.1-04:00", + datetime.datetime( + *(2022, 10, 21, 18, 16, 3, 100000), + tzinfo=datetime.timezone(datetime.timedelta(hours=-4)), + ), + ), + ("2022-10-21T18:16:03.1-04:00", "2022-10-21T18:16:03.1-04:00"), + ], +) +def test_load_timestamp(input_yaml, want): + got = salt_yaml.load(input_yaml) + assert got == want + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 6f5df3c1832576caf3df7642bb0868e3bcbbc7a5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sat, 15 Oct 2022 04:18:49 -0400 Subject: [PATCH 14/33] yaml: Load `!!omap` nodes as sequences of mappings Before, `!!omap` nodes were incorrectly assumed to be mapping nodes when `dictclass` was not `dict`. Now they are correctly processed as sequences of single-entry mappings. The resulting Python object has type `dictclass` (usually `collections.OrderedDict` or a subclass). This commit does not change the behavior when `dictclass` is `dict`; loading an `!!omap` node still returns a list of (key, value) tuples (PyYAML's default behavior). I consider that to be a bug in PyYAML, so a future commit may change the behavior in the `dict` case to match the non-`dict` behavior. (This commit uses `dictclass` for the non-`dict` case to match what appears to be the original intention.) Behavior before: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() Traceback (most recent call last): File "", line 1, in File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 60, in construct_document for dummy in generator: File "salt/utils/yamlloader.py", line 42, in construct_yaml_omap return (yield from self.construct_yaml_map(node)) File "salt/utils/yamlloader.py", line 36, in construct_yaml_map value = self.construct_mapping(node) File "salt/utils/yamlloader.py", line 52, in construct_mapping raise ConstructorError( yaml.constructor.ConstructorError: expected a mapping node, but found sequence in "", line 1, column 1 ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Relevant bug: https://github.com/saltstack/salt/issues/12161 --- changelog/62932.fixed | 3 + salt/utils/yamlloader.py | 19 ++++- .../pillar/test_pillar_map_order.py | 31 +++++++-- tests/pytests/unit/utils/test_yaml.py | 69 +++++++++++++++++++ 4 files changed, 114 insertions(+), 8 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index ffe3c074a695..fc60349a638d 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,4 +1,7 @@ Improvements to YAML processing (`salt.utils.yaml`): + * Loading an `!!omap` node with a `salt.utils.yaml.SaltYamlSafeLoader` that + was constructed with a non-`dict` class now returns a + `collections.OrderedMap` object instead of raising an exception. * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` constructor no longer causes all future `!!omap` nodes to throw an exception when loading. diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index d93270385fa7..071e92fc4444 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -47,7 +47,24 @@ def construct_yaml_map(self, node): def construct_yaml_omap(self, node): if self.dictclass is dict: return (yield from super().construct_yaml_omap(node)) - return (yield from self.construct_yaml_map(node)) + # BaseLoader.construct_yaml_omap() returns a list of (key, value) + # tuples, which doesn't match the semantics of the `!!omap` YAML type. + # Convert the list of tuples to an OrderedDict. + d = self.dictclass() + yield d + (entries,) = super().construct_yaml_omap(node) + if hasattr(entries, "keys"): + entries = ((k, entries[k]) for k in entries.keys()) + for k, v in entries: + if k in d: + raise ConstructorError( + f"while constructing an ordered map", + node.start_mark, + f"duplicate key encountered: {k!r}", + # TODO: Can we get the location of the duplicate key? + node.start_mark, + ) + d[k] = v def construct_unicode(self, node): return node.value diff --git a/tests/pytests/integration/pillar/test_pillar_map_order.py b/tests/pytests/integration/pillar/test_pillar_map_order.py index 8a8160de875a..fcccdc4b2694 100644 --- a/tests/pytests/integration/pillar/test_pillar_map_order.py +++ b/tests/pytests/integration/pillar/test_pillar_map_order.py @@ -20,13 +20,15 @@ def _run(*args, **kwargs): yield _run -def test_pillar_map_order(salt_master, minion_run): +@pytest.mark.parametrize("omap", [False, True]) +def test_pillar_map_order(salt_master, minion_run, omap): """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: + This test generates a Pillar ``.sls`` file containing either an ordinary + YAML map or a YAML `!!omap` and tests whether the resulting Python object + preserves iteration order. Random keys are used to ensure that iteration + order does not coincidentally match. Depending on the `omap` parameter, the + generated Pillar YAML file looks like this: .. code-block:: yaml @@ -37,6 +39,17 @@ def test_pillar_map_order(salt_master, minion_run): # ... omitted for brevity ... k1638299831: 19 + or like this: + + .. code-block:: yaml + + data: !!omap + - 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: @@ -59,7 +72,7 @@ def test_pillar_map_order(salt_master, minion_run): 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: + The non-``!!omap`` case is a regression test for: https://github.com/saltstack/salt/issues/12161 """ # Filter the random keys through a set to avoid duplicates. @@ -69,7 +82,11 @@ def test_pillar_map_order(salt_master, minion_run): 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_yaml = "data:" + if omap: + data_yaml += " !!omap\n" + "".join(f" - {k}: {v}\n" for k, v in items) + else: + data_yaml += "\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( """\ diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 524cadccae64..ca07652b5516 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,5 +1,6 @@ import collections import datetime +import random import textwrap import pytest @@ -11,6 +12,11 @@ from tests.support.mock import mock_open, patch +class _OrderedDictLoader(salt_yaml.SaltYamlSafeLoader): + def __init__(self, stream): + super().__init__(stream, dictclass=collections.OrderedDict) + + def test_dump(): data = {"foo": "bar"} assert salt_yaml.dump(data) == "{foo: bar}\n" @@ -174,6 +180,69 @@ def test_load_dictclass(dictclass): assert d == dictclass([("k1", "v1"), ("k2", "v2")]) +def test_load_omap(): + """Test OrderedDict values via the YAML ``!!omap`` tag. + + This test uses random keys to ensure that iteration order does not + coincidentally match. The generated YAML looks like this: + + .. code-block:: yaml + + !!omap + - k3334244338: 0 + - k3444116829: 1 + - k2072366017: 2 + # ... omitted for brevity ... + - k1638299831: 19 + """ + # 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) + want_items = [(k, i) for i, k in enumerate(keys)] + yaml = "!!omap\n" + "".join(f"- {k}: {v}\n" for k, v in want_items) + got = salt_yaml.load(yaml, Loader=_OrderedDictLoader) + assert isinstance(got, collections.OrderedDict) + assert got == collections.OrderedDict(want_items) + assert list(got.items()) == want_items + + +def test_load_omap_empty(): + got = salt_yaml.load("!!omap []\n", Loader=_OrderedDictLoader) + assert isinstance(got, collections.OrderedDict) + assert got == collections.OrderedDict() + assert list(got.items()) == [] + + +@pytest.mark.parametrize( + "input", + [ + "!!omap {}\n", # Not a sequence node. + "!!omap\n- this is not a mapping node\n", + "!!omap\n- k1: 0\n k2: multiple entries in mapping node\n", + "!!omap [{}]\n", # Mapping node has no entries. + "!!omap\n- duplicate key: 0\n- duplicate key: 1\n", + ], +) +def test_load_omap_invalid(input): + with pytest.raises(ConstructorError): + salt_yaml.load(input, Loader=_OrderedDictLoader) + + +def test_load_untagged_omaplike_is_seq(): + # The YAML spec allows the loader to interpret something that looks like an + # !!omap but doesn't actually have an !!omap tag as an !!omap. (If the user + # intends to express a sequence of single-entry maps and not an ordered map, + # the user must explicitly tag the sequence node with !seq.) Out of concern + # for backwards compatibility, and to avoid ambiguity with an empty + # sequence, implicit !!omap behavior is currently not supported. That may + # change in the future, but for now make sure that sequences are not + # interpreted as ordered maps. + got = salt_yaml.load("- a: 0\n- b: 1\n") + assert not isinstance(got, collections.OrderedDict) + assert got == [{"a": 0}, {"b": 1}] + + @pytest.mark.parametrize( "input_yaml,want", [ From fe33d75a60b066b57efe52b8c8f0b88cfd1e1642 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 16 Oct 2022 03:10:06 -0400 Subject: [PATCH 15/33] yaml: Load `!!omap` nodes as `collections.OrderedDict` objects Now the behavior is consistent regardless of the value of the Loader's `dictclass` constructor parameter. Starting with Python 3.6, Python `dict` objects always iterate in insertion order, so iteration order was already guaranteed when using an ordinary YAML map. However, `!!omap` provides a stronger guarantee to users (plain map iteration order can be thought of as a Salt implementation detail that might change in the future), and it allows them to make their `.sls` files self-documenting. For example, instead of: ```yaml my_pillar_data: key1: val1 key2: val2 ``` users can now do: ```yaml my_pillar_data: !!omap - key1: val1 - key2: val2 ``` to make it clear to readers that the entries are intended to be processed in order. Behavior before: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") [('foo', 'bar'), ('baz', 'bif')] >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> import collections >>> y.load("!!omap [{foo: bar}, {baz: bif}]") OrderedDict([('foo', 'bar'), ('baz', 'bif')]) >>> y.SaltYamlSafeLoader("!!omap [{foo: bar}, {baz: bif}]", dictclass=collections.OrderedDict).get_single_data() OrderedDict([('foo', 'bar'), ('baz', 'bif')]) ``` Relevant bug: https://github.com/saltstack/salt/issues/12161 --- changelog/62932.fixed | 9 +++----- .../troubleshooting/yaml_idiosyncrasies.rst | 21 +++++++++++++++++-- salt/utils/yamlloader.py | 6 +++--- tests/pytests/unit/utils/test_yaml.py | 11 +++------- 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index fc60349a638d..f91eecab2de4 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -1,10 +1,7 @@ Improvements to YAML processing (`salt.utils.yaml`): - * Loading an `!!omap` node with a `salt.utils.yaml.SaltYamlSafeLoader` that - was constructed with a non-`dict` class now returns a - `collections.OrderedMap` object instead of raising an exception. - * Passing a non-`dict` class to the `salt.utils.yaml.SaltYamlSafeLoader` - constructor no longer causes all future `!!omap` nodes to throw an exception - when loading. + * Loading an `!!omap` node now always returns a `collections.OrderedMap` + object. Before it would sometimes return a list of (key, value) tuples + and sometimes raise an exception. * Loading an explicitly tagged `!!timestamp` node now produces a `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 37d489dbfde9..a898d9aeb3b6 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -418,6 +418,13 @@ so ``!!timestamp`` nodes cannot be used in pillar SLS files. Ordered Dictionaries ==================== +.. versionchanged:: 3006.0 + + Loading a YAML ``!!omap`` node now reliably produces a + ``collections.OrderedDict`` object. Previously, an ``!!omap`` node would + sometimes produce a ``list`` of (key, value) ``tuple`` objects and sometimes + raise an exception. + The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except iteration order is preserved. (YAML makes no guarantees about iteration order @@ -439,8 +446,18 @@ 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. +Salt produces a ``collections.OrderedDict`` object when it loads an ``!!omap`` +node. (Salt's behavior differs from PyYAML's default behavior, which is to +produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a +subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. + +Unfortunately, ``collections.OrderedDict`` objects should be avoided when +creating YAML programmatically (such as with the ``yaml`` Jinja filter) due to +bugs in the way ``collections.OrderedDict`` objects are converted to YAML. + +Beware that Salt currently serializes ``collections.OrderedDict`` objects the +same way it serializes plain ``dict`` objects, so they become plain ``dict`` +objects when deserialized by the recipient. Keys Limited to 1024 Characters =============================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 071e92fc4444..7e28be813704 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -3,6 +3,8 @@ """ +import collections + import yaml # pylint: disable=blacklisted-import from yaml.constructor import ConstructorError from yaml.nodes import MappingNode, SequenceNode @@ -45,12 +47,10 @@ def construct_yaml_map(self, node): data.update(value) def construct_yaml_omap(self, node): - if self.dictclass is dict: - return (yield from super().construct_yaml_omap(node)) # BaseLoader.construct_yaml_omap() returns a list of (key, value) # tuples, which doesn't match the semantics of the `!!omap` YAML type. # Convert the list of tuples to an OrderedDict. - d = self.dictclass() + d = collections.OrderedDict() yield d (entries,) = super().construct_yaml_omap(node) if hasattr(entries, "keys"): diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index ca07652b5516..472c9fa0e77a 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -12,11 +12,6 @@ from tests.support.mock import mock_open, patch -class _OrderedDictLoader(salt_yaml.SaltYamlSafeLoader): - def __init__(self, stream): - super().__init__(stream, dictclass=collections.OrderedDict) - - def test_dump(): data = {"foo": "bar"} assert salt_yaml.dump(data) == "{foo: bar}\n" @@ -201,14 +196,14 @@ def test_load_omap(): random.shuffle(keys) want_items = [(k, i) for i, k in enumerate(keys)] yaml = "!!omap\n" + "".join(f"- {k}: {v}\n" for k, v in want_items) - got = salt_yaml.load(yaml, Loader=_OrderedDictLoader) + got = salt_yaml.load(yaml) assert isinstance(got, collections.OrderedDict) assert got == collections.OrderedDict(want_items) assert list(got.items()) == want_items def test_load_omap_empty(): - got = salt_yaml.load("!!omap []\n", Loader=_OrderedDictLoader) + got = salt_yaml.load("!!omap []\n") assert isinstance(got, collections.OrderedDict) assert got == collections.OrderedDict() assert list(got.items()) == [] @@ -226,7 +221,7 @@ def test_load_omap_empty(): ) def test_load_omap_invalid(input): with pytest.raises(ConstructorError): - salt_yaml.load(input, Loader=_OrderedDictLoader) + salt_yaml.load(input) def test_load_untagged_omaplike_is_seq(): From 72a6ebc4b77584d54c1e5b8a841706d677a380fe Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 17:44:37 -0400 Subject: [PATCH 16/33] yaml: Load `!!python/tuple` nodes as `tuple` objects My main motivation for adding this is to facilitate testing, though it also gives module authors greater flexibility (in particular: a `tuple` can be a `dict` key, but a `list` cannot because it is not hashable), and I don't see a strong reason why this shouldn't be added. Behavior before: ``` $ python >>> import salt.utils.yaml as y >>> y.load("!!python/tuple [foo, bar]") Traceback (most recent call last): File "", line 1, in File "salt/utils/yamlloader.py", line 159, in load return yaml.load(stream, Loader=Loader) File "venv/lib/python3.8/site-packages/yaml/__init__.py", line 81, in load return loader.get_single_data() File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 51, in get_single_data return self.construct_document(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 55, in construct_document data = self.construct_object(node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 100, in construct_object data = constructor(self, node) File "venv/lib/python3.8/site-packages/yaml/constructor.py", line 427, in construct_undefined raise ConstructorError(None, None, yaml.constructor.ConstructorError: could not determine a constructor for the tag 'tag:yaml.org,2002:python/tuple' in "", line 1, column 1 ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> y.load("!!python/tuple [foo, bar]") ('foo', 'bar') ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 25 +++++++++++++++++++ salt/utils/yamlloader.py | 4 +++ tests/pytests/unit/utils/test_yaml.py | 7 ++++++ 4 files changed, 38 insertions(+) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index f91eecab2de4..d04b2a413e3a 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -2,6 +2,8 @@ Improvements to YAML processing (`salt.utils.yaml`): * Loading an `!!omap` node now always returns a `collections.OrderedMap` object. Before it would sometimes return a list of (key, value) tuples and sometimes raise an exception. + * Loading a sequence node tagged with `!!python/tuple` is now supported, and + produces a Python `tuple` object. * Loading an explicitly tagged `!!timestamp` node now produces a `datetime.datetime` object instead of a string. * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index a898d9aeb3b6..613026e48f79 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -459,6 +459,31 @@ Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` objects when deserialized by the recipient. +Tuples +====== + +.. versionchanged:: 3006.0 + + Loading a YAML ``!!python/tuple`` node is now supported. + +The YAML ``!!python/tuple`` type can be used to produce a Python ``tuple`` +object when loaded: + +.. code-block:: yaml + + !!python/tuple + - first item + - second item + +When dumped to YAML with ``salt.utils.yaml.dump()``, a ``tuple`` object produces +a ``!!python/tuple`` node. When dumped to YAML with +``salt.utils.yaml.safe_dump()``, a ``tuple`` object produces a plain sequence +node (which will be loaded as a ``list`` object). + +Beware that Salt currently serializes ``tuple`` objects the same way it +serializes ``list`` objects, so they become ``list`` objects when deserialized +by the recipient. + Keys Limited to 1024 Characters =============================== diff --git a/salt/utils/yamlloader.py b/salt/utils/yamlloader.py index 7e28be813704..c76551656b7a 100644 --- a/salt/utils/yamlloader.py +++ b/salt/utils/yamlloader.py @@ -107,6 +107,9 @@ def construct_mapping(self, node, deep=False): mapping[key] = value return mapping + def construct_python_tuple(self, node): + return tuple(self.construct_sequence(node)) + def construct_scalar(self, node): """ Verify integers and pass them in correctly is they are declared @@ -183,6 +186,7 @@ def flatten_mapping(self, node): ("tag:yaml.org,2002:map", SaltYamlSafeLoader.construct_yaml_map), ("tag:yaml.org,2002:omap", SaltYamlSafeLoader.construct_yaml_omap), ("tag:yaml.org,2002:str", SaltYamlSafeLoader.construct_yaml_str), + ("tag:yaml.org,2002:python/tuple", SaltYamlSafeLoader.construct_python_tuple), ("tag:yaml.org,2002:python/unicode", SaltYamlSafeLoader.construct_unicode), ]: SaltYamlSafeLoader.add_constructor(tag, constructor) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 472c9fa0e77a..08abbb899382 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -256,6 +256,13 @@ def test_load_timestamp(input_yaml, want): assert got == want +def test_load_tuple(): + input = "!!python/tuple\n- foo\n- bar\n" + got = salt_yaml.load(input) + want = ("foo", "bar") + assert got == want + + def test_not_yaml_monkey_patching(): if hasattr(yaml, "CSafeLoader"): assert yaml.SafeLoader != yaml.CSafeLoader From 6a4cf23261c8324d322d5666a52a4845232183ce Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:40:43 -0400 Subject: [PATCH 17/33] yaml: Dump `datetime.datetime` objects with `!!timestamp` tag Delete the ineffectual representer, and unregister the implicit resolver for timestamp strings so that `datetime.datetime` objects are always written with an accompaying `!!timestamp` tag. This is a behavior change: Any users that depend on a `datetime` becoming a plain YAML string (which would be read in as a `str` object) must convert the object to string themselves. This change preserves the semantics of the object, and it preserves round-trip identity. Behavior before: ``` $ python >>> import datetime >>> import salt.utils.yaml as y >>> print(y.dump(datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(hours=-4))))) 2022-10-21 18:16:03.100000-04:00 ``` Behavior after: ``` $ python >>> import datetime >>> import salt.utils.yaml as y >>> print(y.dump(datetime.datetime(2022, 10, 21, 18, 16, 3, 100000, tzinfo=datetime.timezone(datetime.timedelta(hours=-4))))) !!timestamp 2022-10-21 18:16:03.100000-04:00 ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 9 +++++++++ salt/utils/yamldumper.py | 20 ++++++++++++++----- tests/pytests/unit/utils/test_yaml.py | 19 ++++++++++++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index d04b2a413e3a..7eab96a5b722 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -6,6 +6,8 @@ Improvements to YAML processing (`salt.utils.yaml`): produces a Python `tuple` object. * Loading an explicitly tagged `!!timestamp` node now produces a `datetime.datetime` object instead of a string. + * Dumping a `datetime.datetime` object now explicitly tags the node with + `!!timestamp`. * `salt.utils.yaml.dump()` now defaults to `salt.utils.yaml.OrderedDumper` instead of `yaml.Dumper`. * Fixed indentation in `salt.utils.yaml.IndentedSafeOrderedDumper` output. diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 613026e48f79..04c74081168d 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -394,6 +394,11 @@ Automatic ``datetime`` conversion Loading a YAML ``!!timestamp`` node now produces a ``datetime.datetime`` object. Previously, nodes tagged with ``!!timestamp`` produced strings. +.. versionchanged:: 3006.0 + + Dumping a ``datetime.datetime`` object to YAML now explicitly tags the node + with ``!!timestamp``. Previously the ``!!timestamp`` tag was omitted. + Salt overrides PyYAML's default behavior and loads YAML nodes that look like timestamps as strings: @@ -412,6 +417,10 @@ explicitly tag the node with ``!!timestamp``: >>> salt.utils.yaml.safe_load("!!timestamp 2014-01-20 14:23:23") '2014-01-20 14:23:23' +When dumping a ``datetime.datetime`` object to YAML, Salt tags the node with +``!!timestamp`` so that it will be loaded back as a ``datetime.datetime`` +object. + Beware that Salt is currently unable to serialize ``datetime.datetime`` objects, so ``!!timestamp`` nodes cannot be used in pillar SLS files. diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 32b0fa226660..471ba131b58a 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -31,13 +31,23 @@ ] -class OrderedDumper(Dumper): +class _RemoveImplicitResolverMixin: + @classmethod + def remove_implicit_resolver(cls, tag): + """Remove a previously registered implicit resolver for a tag.""" + cls.yaml_implicit_resolvers = { + first_char: [r for r in resolver_list if r[0] != tag] + for first_char, resolver_list in cls.yaml_implicit_resolvers.items() + } + + +class OrderedDumper(Dumper, _RemoveImplicitResolverMixin): """ A YAML dumper that represents python OrderedDict as simple YAML map. """ -class SafeOrderedDumper(SafeDumper): +class SafeOrderedDumper(SafeDumper, _RemoveImplicitResolverMixin): """ A YAML safe dumper that represents python OrderedDict as simple YAML map. """ @@ -46,7 +56,7 @@ class SafeOrderedDumper(SafeDumper): # This must inherit from yaml.SafeDumper, not yaml.CSafeDumper, because the # increase_indent hack doesn't work with yaml.CSafeDumper. # https://github.com/yaml/pyyaml/issues/234#issuecomment-786026671 -class IndentedSafeOrderedDumper(yaml.SafeDumper): +class IndentedSafeOrderedDumper(yaml.SafeDumper, _RemoveImplicitResolverMixin): """Like ``SafeOrderedDumper``, except it indents lists for readability.""" def increase_indent(self, flow=False, indentless=False): @@ -77,8 +87,8 @@ def represent_undefined(dumper, data): salt.utils.context.NamespacedDictWrapper, yaml.representer.SafeRepresenter.represent_dict, ) - # TODO: This seems wrong: the first argument should be a type, not a tag. - D.add_representer("tag:yaml.org,2002:timestamp", Dumper.represent_scalar) + # Explicitly include the `!!timestamp` tag when dumping datetime objects. + D.remove_implicit_resolver("tag:yaml.org,2002:timestamp") del D diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 08abbb899382..720ff71a39fb 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -1,6 +1,7 @@ import collections import datetime import random +import re import textwrap import pytest @@ -35,6 +36,24 @@ def test_dump_indented(): assert got == want +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +def test_dump_timestamp(dumpercls): + dt = datetime.datetime( + *(2022, 10, 21, 18, 16, 3, 100000), + tzinfo=datetime.timezone(datetime.timedelta(hours=-4)), + ) + got = salt_yaml.dump(dt, Dumper=dumpercls) + want_re = r"""!!timestamp (['"]?)2022-10-21[T ]18:16:03.10*-04:00\1\n""" + assert re.fullmatch(want_re, got) + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From 5b7c40b359702c7e4d79484dd6da759101d95907 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 01:43:09 -0400 Subject: [PATCH 18/33] yaml: Dump all `OrderedDict` types the same way Behavior before: ``` $ python >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!python/object/apply:collections.OrderedDict - - - foo - bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) NULL ``` Behavior after: ``` $ python >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 11 ++++++--- salt/utils/yamldumper.py | 10 ++++++-- tests/pytests/unit/utils/test_yaml.py | 24 +++++++++++++++++++ 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 7eab96a5b722..593d7aaa6988 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -2,6 +2,8 @@ Improvements to YAML processing (`salt.utils.yaml`): * Loading an `!!omap` node now always returns a `collections.OrderedMap` object. Before it would sometimes return a list of (key, value) tuples and sometimes raise an exception. + * Dumping a `collections.OrderedMap` now consistently produces a plain mapping + node. * Loading a sequence node tagged with `!!python/tuple` is now supported, and produces a Python `tuple` object. * Loading an explicitly tagged `!!timestamp` node now produces a diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 04c74081168d..8603e9898d44 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -434,6 +434,12 @@ Ordered Dictionaries sometimes produce a ``list`` of (key, value) ``tuple`` objects and sometimes raise an exception. +.. versionchanged:: 3006.0 + + Dumping any ``collections.OrderedDict`` object to YAML now reliably produces + a plain mapping node. Previously, only the subtype + ``salt.utils.odict.OrderedDict`` was supported. + The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except iteration order is preserved. (YAML makes no guarantees about iteration order @@ -460,9 +466,8 @@ node. (Salt's behavior differs from PyYAML's default behavior, which is to produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. -Unfortunately, ``collections.OrderedDict`` objects should be avoided when -creating YAML programmatically (such as with the ``yaml`` Jinja filter) due to -bugs in the way ``collections.OrderedDict`` objects are converted to YAML. +When dumping a ``collections.OrderedDict`` object to YAML, Salt generates a +plain mapping, not an ``!!omap`` node. Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 471ba131b58a..96b660bd6c17 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -12,7 +12,6 @@ import yaml # pylint: disable=blacklisted-import import salt.utils.context -from salt.utils.odict import OrderedDict try: from yaml import CDumper as Dumper @@ -79,7 +78,14 @@ def represent_undefined(dumper, data): # TODO: Why isn't this representer also registered with OrderedDumper? D.add_representer(None, represent_undefined) for D in (SafeOrderedDumper, IndentedSafeOrderedDumper, OrderedDumper): - D.add_representer(OrderedDict, represent_ordereddict) + # This multi representer covers collections.OrderedDict and all of its + # subclasses, including salt.utils.odict.OrderedDict. + D.add_multi_representer(collections.OrderedDict, represent_ordereddict) + # This non-multi representer may seem redundant given the multi representer + # registered above, but it is needed to override the non-multi representer + # that exists in the ancestor Representer class. (Non-multi representers + # take priority over multi representers.) + D.add_representer(collections.OrderedDict, represent_ordereddict) D.add_representer( collections.defaultdict, yaml.representer.SafeRepresenter.represent_dict ) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index 720ff71a39fb..cf6fff45b02d 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -10,6 +10,7 @@ import salt.utils.files import salt.utils.yaml as salt_yaml +from salt.utils.odict import OrderedDict from tests.support.mock import mock_open, patch @@ -36,6 +37,29 @@ def test_dump_indented(): assert got == want +@pytest.mark.parametrize("dictcls", [OrderedDict, collections.OrderedDict]) +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +def test_dump_omap(dictcls, dumpercls): + # The random keys are filtered through a set to avoid duplicates. + keys = list({f"random key {random.getrandbits(32)}" for _ in range(20)}) + # Avoid unintended correlation with set()'s iteration order. + random.shuffle(keys) + d = dictcls((k, i) for i, k in enumerate(keys)) + # Note that there is no extra indentation added for the + # IndentedSafeOrderedDumper case because the omap node is the top-level node + # so there is no indentation for the sequence elements. + want = "".join(f"{k}: {i}\n" for i, k in enumerate(keys)) + got = salt_yaml.dump(d, Dumper=dumpercls, default_flow_style=False) + assert got == want + + @pytest.mark.parametrize( "dumpercls", [ From 3cbe12cbb824f21cc41063d6da95c6660682d4ea Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 17 Oct 2022 01:01:43 -0400 Subject: [PATCH 19/33] yaml: Dump `OrderedDict` objects as `!!omap` nodes Before, an `OrderedDict` object was represented as a plain YAML mapping. Now it is represented as an `!!omap` node, which is a sequence of single-valued mappings. This is a behavior change: Any users that depend on an `OrderedDict` becoming a plain YAML mapping (which would be read in as a `dict` object) must first convert the `OrderedDict` to `dict`. This change preserves the semantics of the object, and it preserves round-trip identity. Behavior before: ``` $ python >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) foo: bar ``` Behavior after: ``` $ python >>> from salt.utils.odict import OrderedDict >>> import salt.utils.yaml as y >>> import collections >>> print(y.dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.safe_dump(OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar >>> print(y.safe_dump(collections.OrderedDict([("foo", "bar")]), default_flow_style=False)) !!omap - foo: bar ``` --- changelog/62932.fixed | 4 +-- .../troubleshooting/yaml_idiosyncrasies.rst | 9 ++++--- salt/utils/yamldumper.py | 26 ++++++++++++++----- .../utils/jinja/test_custom_extensions.py | 3 +++ tests/pytests/unit/utils/test_yaml.py | 2 +- 5 files changed, 30 insertions(+), 14 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index 593d7aaa6988..d7e87deb6631 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -2,8 +2,8 @@ Improvements to YAML processing (`salt.utils.yaml`): * Loading an `!!omap` node now always returns a `collections.OrderedMap` object. Before it would sometimes return a list of (key, value) tuples and sometimes raise an exception. - * Dumping a `collections.OrderedMap` now consistently produces a plain mapping - node. + * Dumping a `collections.OrderedMap` now consistently produces an `!!omap` + node (a sequence of single-entry mappings). * Loading a sequence node tagged with `!!python/tuple` is now supported, and produces a Python `tuple` object. * Loading an explicitly tagged `!!timestamp` node now produces a diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index 8603e9898d44..d6722f80d6e0 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -437,8 +437,9 @@ Ordered Dictionaries .. versionchanged:: 3006.0 Dumping any ``collections.OrderedDict`` object to YAML now reliably produces - a plain mapping node. Previously, only the subtype - ``salt.utils.odict.OrderedDict`` was supported. + an ``!!omap`` node. Previously, only the subtype + ``salt.utils.odict.OrderedDict`` was supported, and it produced a plain + mapping node. The YAML specification defines an `ordered mapping type `_ which is equivalent to a plain mapping except @@ -466,8 +467,8 @@ node. (Salt's behavior differs from PyYAML's default behavior, which is to produce a ``list`` of (key, value) ``tuple`` objects.) These objects are a subtype of ``dict``, so ``!!omap`` is a drop-in replacement for a plain mapping. -When dumping a ``collections.OrderedDict`` object to YAML, Salt generates a -plain mapping, not an ``!!omap`` node. +When dumping a ``collections.OrderedDict`` object to YAML, Salt produces an +``!!omap`` node. Beware that Salt currently serializes ``collections.OrderedDict`` objects the same way it serializes plain ``dict`` objects, so they become plain ``dict`` diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 96b660bd6c17..45ea8293f74c 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -40,15 +40,26 @@ def remove_implicit_resolver(cls, tag): } -class OrderedDumper(Dumper, _RemoveImplicitResolverMixin): - """ - A YAML dumper that represents python OrderedDict as simple YAML map. - """ +class SafeOrderedDumper(SafeDumper, _RemoveImplicitResolverMixin): + """A safe YAML dumper that uses the YAML ``!!omap`` type for ``OrderedDict`` + ``OrderedDict``s are represented as a a sequence of single-entry mappings + and tagged with ``!!omap``: -class SafeOrderedDumper(SafeDumper, _RemoveImplicitResolverMixin): + .. code-block:: yaml + + !!omap + - first key: first value + - second key: second value + + See https://yaml.org/type/omap.html for details. """ - A YAML safe dumper that represents python OrderedDict as simple YAML map. + + +class OrderedDumper(Dumper, _RemoveImplicitResolverMixin): + """A YAML dumper that uses the YAML ``!!omap`` type for ``OrderedDict`` + + See ``SafeOrderedDumper`` for details. """ @@ -63,7 +74,8 @@ def increase_indent(self, flow=False, indentless=False): def represent_ordereddict(dumper, data): - return dumper.represent_dict(list(data.items())) + seq = [{k: v} for k, v in data.items()] + return dumper.represent_sequence("tag:yaml.org,2002:omap", seq) def represent_undefined(dumper, data): diff --git a/tests/pytests/unit/utils/jinja/test_custom_extensions.py b/tests/pytests/unit/utils/jinja/test_custom_extensions.py index 1fa3c9c678a3..1f0ad41ed717 100644 --- a/tests/pytests/unit/utils/jinja/test_custom_extensions.py +++ b/tests/pytests/unit/utils/jinja/test_custom_extensions.py @@ -3,6 +3,7 @@ """ import ast +import collections import itertools import os import pprint @@ -135,6 +136,8 @@ def test_serialize_yaml(): env = Environment(extensions=[SerializerExtension]) rendered = env.from_string("{{ dataset|yaml }}").render(dataset=dataset) assert dataset == salt.utils.yaml.safe_load(rendered) + assert isinstance(dataset["spam"], collections.OrderedDict) + assert isinstance(dataset["spam"]["foo"], collections.OrderedDict) def test_serialize_yaml_str(): diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index cf6fff45b02d..e8a22ec14aea 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -55,7 +55,7 @@ def test_dump_omap(dictcls, dumpercls): # Note that there is no extra indentation added for the # IndentedSafeOrderedDumper case because the omap node is the top-level node # so there is no indentation for the sequence elements. - want = "".join(f"{k}: {i}\n" for i, k in enumerate(keys)) + want = "!!omap\n" + "".join(f"- {k}: {i}\n" for i, k in enumerate(keys)) got = salt_yaml.dump(d, Dumper=dumpercls, default_flow_style=False) assert got == want From db44b3082aad27df22abf675a2b7a456ca4ac5e5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 20 Oct 2022 02:54:43 -0400 Subject: [PATCH 20/33] yaml: Dump `tuple` objects as `!!python/tuple` nodes Before, `!!python/tuple` was only used for `OrderedDumper`. Now it is also used for `SafeOrderedDumper` and `IndentedSafeOrderedDumper`. This is a behavior change: Any users that depended on a `tuple` becoming a plain YAML sequence (which would be read in as a `list` object) must first convert the `tuple` to `list`. This change preserves the semantics of the object, and it preserves round-trip identity. Preserving round-trip identity is particularly important if the tuple is used as a key in a `dict` because `list` objects are not hashable. Behavior before: ``` $ python >>> import salt.utils.yaml as y >>> print(y.dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar >>> print(y.safe_dump(("foo", "bar"), default_flow_style=False)) - foo - bar ``` Behavior after: ``` $ python >>> import salt.utils.yaml as y >>> print(y.dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar >>> print(y.safe_dump(("foo", "bar"), default_flow_style=False)) !!python/tuple - foo - bar ``` --- changelog/62932.fixed | 2 ++ .../troubleshooting/yaml_idiosyncrasies.rst | 12 ++++++---- salt/utils/yamldumper.py | 4 ++++ .../utils/jinja/test_custom_extensions.py | 1 + tests/pytests/unit/utils/test_yaml.py | 22 +++++++++++++++++++ 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/changelog/62932.fixed b/changelog/62932.fixed index d7e87deb6631..7740357c4c61 100644 --- a/changelog/62932.fixed +++ b/changelog/62932.fixed @@ -6,6 +6,8 @@ Improvements to YAML processing (`salt.utils.yaml`): node (a sequence of single-entry mappings). * Loading a sequence node tagged with `!!python/tuple` is now supported, and produces a Python `tuple` object. + * Dumping a `tuple` object now consistently produces a sequence node + explicitly tagged with `!!python/tuple`. * Loading an explicitly tagged `!!timestamp` node now produces a `datetime.datetime` object instead of a string. * Dumping a `datetime.datetime` object now explicitly tags the node with diff --git a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst index d6722f80d6e0..b51bf6c0eda0 100644 --- a/doc/topics/troubleshooting/yaml_idiosyncrasies.rst +++ b/doc/topics/troubleshooting/yaml_idiosyncrasies.rst @@ -481,6 +481,12 @@ Tuples Loading a YAML ``!!python/tuple`` node is now supported. +.. versionchanged:: 3006.0 + + Dumping a ``tuple`` object to YAML now always produces a sequence node + tagged with ``!!python/tuple``. Previously, ``salt.utils.yaml.safe_dump()`` + did not tag the node. + The YAML ``!!python/tuple`` type can be used to produce a Python ``tuple`` object when loaded: @@ -490,10 +496,8 @@ object when loaded: - first item - second item -When dumped to YAML with ``salt.utils.yaml.dump()``, a ``tuple`` object produces -a ``!!python/tuple`` node. When dumped to YAML with -``salt.utils.yaml.safe_dump()``, a ``tuple`` object produces a plain sequence -node (which will be loaded as a ``list`` object). +When dumped to YAML, a ``tuple`` object produces a sequence node tagged with +``!!python/tuple``. Beware that Salt currently serializes ``tuple`` objects the same way it serializes ``list`` objects, so they become ``list`` objects when deserialized diff --git a/salt/utils/yamldumper.py b/salt/utils/yamldumper.py index 45ea8293f74c..9f3b18805acb 100644 --- a/salt/utils/yamldumper.py +++ b/salt/utils/yamldumper.py @@ -105,6 +105,10 @@ def represent_undefined(dumper, data): salt.utils.context.NamespacedDictWrapper, yaml.representer.SafeRepresenter.represent_dict, ) + # SafeDumper represents tuples as lists, but Dumper's behavior (sequence + # tagged with `!!python/tuple`) is safe, so use it for all dumpers. + D.add_multi_representer(tuple, Dumper.yaml_representers[tuple]) + D.add_representer(tuple, Dumper.yaml_representers[tuple]) # Explicitly include the `!!timestamp` tag when dumping datetime objects. D.remove_implicit_resolver("tag:yaml.org,2002:timestamp") del D diff --git a/tests/pytests/unit/utils/jinja/test_custom_extensions.py b/tests/pytests/unit/utils/jinja/test_custom_extensions.py index 1f0ad41ed717..0d6c84f6744f 100644 --- a/tests/pytests/unit/utils/jinja/test_custom_extensions.py +++ b/tests/pytests/unit/utils/jinja/test_custom_extensions.py @@ -132,6 +132,7 @@ def test_serialize_yaml(): "baz": [1, 2, 3], "qux": 2.0, "spam": OrderedDict([("foo", OrderedDict([("bar", "baz"), ("qux", 42)]))]), + "tuple": ("foo", "bar"), } env = Environment(extensions=[SerializerExtension]) rendered = env.from_string("{{ dataset|yaml }}").render(dataset=dataset) diff --git a/tests/pytests/unit/utils/test_yaml.py b/tests/pytests/unit/utils/test_yaml.py index e8a22ec14aea..8558c93e1240 100644 --- a/tests/pytests/unit/utils/test_yaml.py +++ b/tests/pytests/unit/utils/test_yaml.py @@ -78,6 +78,28 @@ def test_dump_timestamp(dumpercls): assert re.fullmatch(want_re, got) +@pytest.mark.parametrize( + "mktuple", + [ + lambda *args: tuple(args), + collections.namedtuple("TestTuple", "a b"), + ], +) +@pytest.mark.parametrize( + "dumpercls", + [ + salt_yaml.OrderedDumper, + salt_yaml.SafeOrderedDumper, + salt_yaml.IndentedSafeOrderedDumper, + ], +) +def test_dump_tuple(mktuple, dumpercls): + data = mktuple("foo", "bar") + want = "!!python/tuple [foo, bar]\n" + got = salt_yaml.dump(data, Dumper=dumpercls) + assert got == want + + def render_yaml(data): """ Takes a YAML string, puts it into a mock file, passes that to the YAML From a586969f9c0a2b06fa0059037980aa22713dc7ed Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 25 Sep 2022 00:00:57 -0400 Subject: [PATCH 21/33] ldap: Convert unit tests to integration tests The existing unit tests I wrote long ago were overly complicated and not that great, so this commit converts them into integration tests against a real OpenLDAP server. Docker is required to run the integration tests; the tests are skipped if Docker isn't available. --- tests/pytests/integration/states/test_ldap.py | 735 ++++++++++++++++++ tests/pytests/unit/states/test_ldap.py | 418 ---------- tests/support/pytest/ldap.py | 392 ++++++++++ 3 files changed, 1127 insertions(+), 418 deletions(-) create mode 100644 tests/pytests/integration/states/test_ldap.py delete mode 100644 tests/pytests/unit/states/test_ldap.py create mode 100644 tests/support/pytest/ldap.py diff --git a/tests/pytests/integration/states/test_ldap.py b/tests/pytests/integration/states/test_ldap.py new file mode 100644 index 000000000000..45fe4711550e --- /dev/null +++ b/tests/pytests/integration/states/test_ldap.py @@ -0,0 +1,735 @@ +import pytest + +pytest_plugins = [ + "tests.support.pytest.ldap", +] +pytestmark = [ + pytest.mark.destructive_test, + pytest.mark.skip_if_binaries_missing("docker"), + pytest.mark.slow_test, +] + + +# These tests assume OpenLDAP returns attribute values in insertion order, which +# is not guaranteed but is the current behavior. We could define a custom +# X-ORDERED attribute to enforce this, but that adds a lot of complexity and +# there are several awkward corner cases that make it impractical. + + +def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtree): + u1dn = f"cn=u1,{subtree}" + entries = [ + { + u1dn: [ + { + "add": { + "objectClass": ["person"], + # Note that this is not a list. It should behave as if + # we passed the list ["u1"]. + "cn": "u1", + # Note that this is not a list, nor is it a string. It + # should behave as if we passed the list ["1234"] + # (numbers are stringified and then put in a list). + "sn": 1234, + # List of values of various types. + # + # Note that ldap.managed accepts arbitrary iterables, + # not just lists. Unfortunately, Salt's YAML loader + # does not currently (as of 2022-10-11) support any + # ordered non-list types (such as tuple or OrderedDict) + # so we don't test them here. (A dict can be used, but + # iteration order isn't guaranteed so the tests would be + # flaky.) + # + # Instead of a salt CLI fixture we could use a + # LocalClient fixture (see salt_master.salt_client()), + # which is able to pass tuples to the minion. Using + # such a fixture with state.single would make these + # tests more like unit tests and less like integration + # tests. + # + # Alternatively the YAML loader can be extended to + # support tuples and/or OrderedDict. + "description": [ + "Non-ASCII characters should be supported: 🙂", + 4567, + b"abcd", + ], + "userPassword": [ + # Intentionally invalid UTF-8. The syntax for + # userPassword is Octet String, not Directory String + # (like description), so this is acceptable. + # + # TODO: bytes objects must be in a list (this test + # can't do `"userPassword": b"..."`) due to a bug in + # the way values are turned into sets. + b"\x00\x01\x02\x03\x80", + ], + # Empty list should be a no-op. + "telephoneNumber": [], + # None should be equivalent to an empty list. + "seeAlso": None, + }, + }, + ], + }, + ] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u1dn: { + "old": None, + "new": { + "objectClass": ["person"], + "cn": ["u1"], + "sn": ["1234"], + "description": [ + "4567", + "Non-ASCII characters should be supported: 🙂", + "abcd", + ], + "userPassword": [b"\x00\x01\x02\x03\x80"], + }, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u1dn) == { + u1dn: { + "objectClass": ["person"], + "cn": ["u1"], + "sn": ["1234"], + "description": [ + "Non-ASCII characters should be supported: 🙂", + "4567", + "abcd", + ], + "userPassword": [b"\x00\x01\x02\x03\x80"], + }, + } + + +def test_managed_add_new_attribute(openldap_minion_run, openldap_minion_apply, u0dn): + entries = [{u0dn: [{"add": {"userPassword": ["p"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {u0dn: {"old": {}, "new": {"userPassword": ["p"]}}}, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + "userPassword": ["p"], + }, + } + + +def test_managed_add_no_values_to_existing_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_add_no_values_to_new_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"telephoneNumber": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_add_new_value_to_existing_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": ["and another"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["and another", "another desc", "desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["and another", "desc", "another desc"], + }, + } + + +def test_managed_add_same_values_to_existing_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": ["desc", "another desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_add_overlapping_values( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": ["desc", "and another"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["and another", "another desc", "desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "and another", "another desc"], + }, + } + + +def test_managed_add_overlapping_values_different_order( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": ["and another", "desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["and another", "another desc", "desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["and another", "desc", "another desc"], + }, + } + + +def test_managed_add_repeated_values(openldap_minion_run, openldap_minion_apply, u0dn): + entries = [{u0dn: [{"add": {"description": ["val", "val"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["another desc", "desc", "val"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["val", "desc", "another desc"], + }, + } + + +def test_managed_replace_new_entry(openldap_minion_run, openldap_minion_apply, subtree): + u1dn = f"cn=u1,{subtree}" + entries = [ + { + u1dn: [ + { + "replace": { + "objectClass": ["person"], + "cn": "u1", + "sn": "surname", + }, + }, + ], + }, + ] + want = { + "objectClass": ["person"], + "cn": ["u1"], + "sn": ["surname"], + } + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {u1dn: {"old": None, "new": want}}, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u1dn) == {u1dn: want} + + +def test_managed_replace_new_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"userPassword": ["p"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {}, + "new": {"userPassword": ["p"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + "userPassword": ["p"], + }, + } + + +def test_managed_replace_no_value_for_one_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"description": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + }, + } + + +def test_managed_replace_no_values_for_all_attributes( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [ + { + u0dn: [ + { + "replace": { + "objectClass": [], + "cn": [], + "sn": [], + "description": [], + }, + }, + ], + }, + ] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["another desc", "desc"], + }, + "new": None, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == {} + + +def test_managed_replace_no_values_for_new_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"userPassword": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_replace_new_values(openldap_minion_run, openldap_minion_apply, u0dn): + entries = [{u0dn: [{"replace": {"description": ["new desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["new desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["new desc"], + }, + } + + +def test_managed_replace_same_values(openldap_minion_run, openldap_minion_apply, u0dn): + entries = [{u0dn: [{"replace": {"description": ["desc", "another desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_replace_overlapping_values( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"description": ["desc", "new desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["desc", "new desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "new desc"], + }, + } + + +def test_managed_replace_overlapping_values_different_order( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"description": ["new desc", "desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["desc", "new desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["new desc", "desc"], + }, + } + + +def test_managed_delete_value_from_nonexistent_entry( + openldap_minion_run, openldap_minion_apply, subtree +): + u1dn = f"cn=u1,{subtree}" + entries = [{u1dn: [{"delete": {"description": ["foo"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u1dn) == {} + + +def test_managed_delete_value_from_nonexistent_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"userPassword": ["foo"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_delete_nonexistent_value( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"description": ["foo"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_delete_all_values_from_nonexistent_entry( + openldap_minion_run, openldap_minion_apply, subtree +): + u1dn = f"cn=u1,{subtree}" + entries = [{u1dn: [{"delete": {"description": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u1dn) == {} + + +def test_managed_delete_all_values_from_nonexistent_attribute( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"userPassword": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_managed_delete_remaining_attribute_values( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"description": ["desc", "another desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + }, + } + + +def test_managed_delete_all_attribute_values( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"description": []}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + }, + } + + +def test_managed_delete_all_values_all_attributes( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [ + { + u0dn: [ + { + "delete": { + "objectClass": [], + "cn": [], + "sn": [], + "description": [], + }, + }, + ], + }, + ] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["another desc", "desc"], + }, + "new": None, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == {} + + +def test_managed_delete_remaining_values_all_attributes( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [ + { + u0dn: [ + { + "delete": { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + }, + ], + }, + ] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["another desc", "desc"], + }, + "new": None, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == {} + + +def test_managed_delete_not_all_values( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"delete": {"description": ["another desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": { + u0dn: { + "old": {"description": ["another desc", "desc"]}, + "new": {"description": ["desc"]}, + }, + }, + "comment": "Successfully updated LDAP entries", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc"], + }, + } + + +def test_managed_delete_empty_dict(openldap_minion_run, openldap_minion_apply, u0dn): + entries = [{u0dn: [{"delete": {}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } diff --git a/tests/pytests/unit/states/test_ldap.py b/tests/pytests/unit/states/test_ldap.py deleted file mode 100644 index bf57549fd9c0..000000000000 --- a/tests/pytests/unit/states/test_ldap.py +++ /dev/null @@ -1,418 +0,0 @@ -"""Test cases for the ``ldap`` state module - -This code is gross. I started out trying to remove some of the -duplicate code in the test cases, and before I knew it the test code -was an ugly second implementation. - -I'm leaving it for now, but this should really be gutted and replaced -with something sensible. -""" -import copy -import logging - -import attr -import pytest - -import salt.states.ldap -from salt.utils.oset import OrderedSet -from salt.utils.stringutils import to_bytes - -log = logging.getLogger(__name__) - - -# emulates the LDAP database. each key is the DN of an entry and it -# maps to a dict which maps attribute names to sets of values. -@attr.s -class LdapDB: - db = attr.ib(init=False, default=attr.Factory(dict)) - - def dummy_connect(self, connect_spec): - return _dummy_ctx() - - def dummy_search(self, connect_spec, base, scope): - if base not in self.db: - return {} - return { - base: { - attr: list(self.db[base][attr]) - for attr in self.db[base] - if len(self.db[base][attr]) - } - } - - def dummy_add(self, connect_spec, dn, attributes): - assert dn not in self.db - assert attributes - self.db[dn] = {} - for attr, vals in attributes.items(): - assert vals - self.db[dn][attr] = OrderedSet(vals) - return True - - def dummy_delete(self, connect_spec, dn): - assert dn in self.db - del self.db[dn] - return True - - def dummy_change(self, connect_spec, dn, before, after): - assert before != after - assert before - assert after - assert dn in self.db - e = self.db[dn] - assert e == before - all_attrs = OrderedSet() - all_attrs.update(before) - all_attrs.update(after) - directives = [] - for attr in all_attrs: - if attr not in before: - assert attr in after - assert after[attr] - directives.append(("add", attr, after[attr])) - elif attr not in after: - assert attr in before - assert before[attr] - directives.append(("delete", attr, ())) - else: - assert before[attr] - assert after[attr] - to_del = before[attr] - after[attr] - if to_del: - directives.append(("delete", attr, to_del)) - to_add = after[attr] - before[attr] - if to_add: - directives.append(("add", attr, to_add)) - return self.dummy_modify(connect_spec, dn, directives) - - def dummy_modify(self, connect_spec, dn, directives): - assert dn in self.db - e = self.db[dn] - for op, attr, vals in directives: - if op == "add": - assert vals - existing_vals = e.setdefault(attr, OrderedSet()) - for val in vals: - assert val not in existing_vals - existing_vals.add(val) - elif op == "delete": - assert attr in e - existing_vals = e[attr] - assert existing_vals - if not vals: - del e[attr] - continue - for val in vals: - assert val in existing_vals - existing_vals.remove(val) - if not existing_vals: - del e[attr] - elif op == "replace": - e.pop(attr, None) - e[attr] = OrderedSet(vals) - else: - raise ValueError() - return True - - def dump_db(self, d=None): - if d is None: - d = self.db - return {dn: {attr: list(d[dn][attr]) for attr in d[dn]} for dn in d} - - -@pytest.fixture -def db(): - return LdapDB() - - -@pytest.fixture -def complex_db(db): - db.db = { - "dnfoo": { - "attrfoo1": OrderedSet( - ( - b"valfoo1.1", - b"valfoo1.2", - ) - ), - "attrfoo2": OrderedSet((b"valfoo2.1",)), - }, - "dnbar": { - "attrbar1": OrderedSet( - ( - b"valbar1.1", - b"valbar1.2", - ) - ), - "attrbar2": OrderedSet((b"valbar2.1",)), - }, - } - return db - - -@pytest.fixture -def no_change_complex_db(db): - db.db = { - "dnfoo": { - "attrfoo1": OrderedSet( - ( - b"valfoo1.1", - b"valfoo1.2", - ) - ), - "attrfoo2": OrderedSet((b"valfoo2.1",)), - }, - "dnbar": { - "attrbar1": OrderedSet( - ( - b"valbar1.1", - b"valbar1.2", - ) - ), - "attrbar2": OrderedSet((b"valbar2.1",)), - }, - } - return db - - -class _dummy_ctx: - def __init__(self): - pass - - def __enter__(self): - return self - - def __exit__(self, *exc): - pass - - -@pytest.fixture -def configure_loader_modules(db): - salt_dunder = { - "ldap3.connect": db.dummy_connect, - "ldap3.search": db.dummy_search, - "ldap3.add": db.dummy_add, - "ldap3.delete": db.dummy_delete, - "ldap3.change": db.dummy_change, - "ldap3.modify": db.dummy_modify, - } - return {salt.states.ldap: {"__opts__": {"test": False}, "__salt__": salt_dunder}} - - -def _test_helper(init_db, expected_ret, replace, delete_others=False): - old = init_db.dump_db() - new = init_db.dump_db() - expected_db = copy.deepcopy(init_db.db) - for dn, attrs in replace.items(): - for attr, vals in attrs.items(): - vals = [to_bytes(val) for val in vals] - if vals: - new.setdefault(dn, {})[attr] = list(OrderedSet(vals)) - expected_db.setdefault(dn, {})[attr] = OrderedSet(vals) - elif dn in expected_db: - new[dn].pop(attr, None) - expected_db[dn].pop(attr, None) - if not expected_db.get(dn, {}): - new.pop(dn, None) - expected_db.pop(dn, None) - if delete_others: - dn_to_delete = OrderedSet() - for dn, attrs in expected_db.items(): - if dn in replace: - to_delete = OrderedSet() - for attr, vals in attrs.items(): - if attr not in replace[dn]: - to_delete.add(attr) - for attr in to_delete: - del attrs[attr] - del new[dn][attr] - if not attrs: - dn_to_delete.add(dn) - for dn in dn_to_delete: - del new[dn] - del expected_db[dn] - name = "ldapi:///" - expected_ret["name"] = name - expected_ret.setdefault("result", True) - expected_ret.setdefault("comment", "Successfully updated LDAP entries") - expected_ret.setdefault( - "changes", - { - dn: { - "old": { - attr: vals - for attr, vals in old[dn].items() - if vals != new.get(dn, {}).get(attr, ()) - } - if dn in old - else None, - "new": { - attr: vals - for attr, vals in new[dn].items() - if vals != old.get(dn, {}).get(attr, ()) - } - if dn in new - else None, - } - for dn in replace - if old.get(dn, {}) != new.get(dn, {}) - }, - ) - entries = [ - {dn: [{"replace": attrs}, {"delete_others": delete_others}]} - for dn, attrs in replace.items() - ] - actual = salt.states.ldap.managed(name, entries) - assert expected_ret == actual - assert expected_db == init_db.db - - -def _test_helper_success(db, replace, delete_others=False): - _test_helper(db, {}, replace, delete_others) - - -def _test_helper_nochange(db, replace, delete_others=False): - expected = { - "changes": {}, - "comment": "LDAP entries already set", - } - _test_helper(db, expected, replace, delete_others) - - -def _test_helper_add(db, expected_ret, add_items, delete_others=False): - old = db.dump_db() - new = db.dump_db() - expected_db = copy.deepcopy(db.db) - for dn, attrs in add_items.items(): - for attr, vals in attrs.items(): - vals = [to_bytes(val) for val in vals] - - vals.extend(old.get(dn, {}).get(attr, OrderedSet())) - vals.sort() - - if vals: - new.setdefault(dn, {})[attr] = list(OrderedSet(vals)) - expected_db.setdefault(dn, {})[attr] = OrderedSet(vals) - elif dn in expected_db: - new[dn].pop(attr, None) - expected_db[dn].pop(attr, None) - if not expected_db.get(dn, {}): - new.pop(dn, None) - expected_db.pop(dn, None) - if delete_others: - dn_to_delete = OrderedSet() - for dn, attrs in expected_db.items(): - if dn in add_items: - to_delete = OrderedSet() - for attr, vals in attrs.items(): - if attr not in add_items[dn]: - to_delete.add(attr) - for attr in to_delete: - del attrs[attr] - del new[dn][attr] - if not attrs: - dn_to_delete.add(dn) - for dn in dn_to_delete: - del new[dn] - del expected_db[dn] - name = "ldapi:///" - expected_ret["name"] = name - expected_ret.setdefault("result", True) - expected_ret.setdefault("comment", "Successfully updated LDAP entries") - expected_ret.setdefault( - "changes", - { - dn: { - "old": { - attr: vals - for attr, vals in old[dn].items() - if vals != new.get(dn, {}).get(attr, ()) - } - if dn in old - else None, - "new": { - attr: vals - for attr, vals in new[dn].items() - if vals != old.get(dn, {}).get(attr, ()) - } - if dn in new - else None, - } - for dn in add_items - if old.get(dn, {}) != new.get(dn, {}) - }, - ) - entries = [ - {dn: [{"add": attrs}, {"delete_others": delete_others}]} - for dn, attrs in add_items.items() - ] - actual = salt.states.ldap.managed(name, entries) - assert expected_ret == actual - assert expected_db == db.db - - -def _test_helper_success_add(db, add_items, delete_others=False): - _test_helper_add(db, {}, add_items, delete_others) - - -def test_managed_empty(db): - name = "ldapi:///" - expected = { - "name": name, - "changes": {}, - "result": True, - "comment": "LDAP entries already set", - } - actual = salt.states.ldap.managed(name, {}) - assert expected == actual - - -def test_managed_add_entry(db): - _test_helper_success_add(db, {"dummydn": {"foo": ["bar", "baz"]}}) - - -def test_managed_add_attr(complex_db): - _test_helper_success_add(complex_db, {"dnfoo": {"attrfoo1": ["valfoo1.3"]}}) - _test_helper_success_add(complex_db, {"dnfoo": {"attrfoo4": ["valfoo4.1"]}}) - - -def test_managed_replace_attr(complex_db): - _test_helper_success(complex_db, {"dnfoo": {"attrfoo3": ["valfoo3.1"]}}) - - -def test_managed_simplereplace(complex_db): - _test_helper_success(complex_db, {"dnfoo": {"attrfoo1": ["valfoo1.3"]}}) - - -def test_managed_deleteattr(complex_db): - _test_helper_success(complex_db, {"dnfoo": {"attrfoo1": []}}) - - -def test_managed_deletenonexistattr(no_change_complex_db): - _test_helper_nochange(no_change_complex_db, {"dnfoo": {"dummyattr": []}}) - - -def test_managed_deleteentry(complex_db): - _test_helper_success(complex_db, {"dnfoo": {}}, True) - - -def test_managed_deletenonexistentry(no_change_complex_db): - _test_helper_nochange(no_change_complex_db, {"dummydn": {}}, True) - - -def test_managed_deletenonexistattrinnonexistentry(no_change_complex_db): - _test_helper_nochange(no_change_complex_db, {"dummydn": {"dummyattr": []}}) - - -def test_managed_add_attr_delete_others(complex_db): - _test_helper_success(complex_db, {"dnfoo": {"dummyattr": ["dummyval"]}}, True) - - -def test_managed_no_net_change(no_change_complex_db): - _test_helper_nochange( - no_change_complex_db, {"dnfoo": {"attrfoo1": ["valfoo1.1", "valfoo1.2"]}} - ) - - -def test_managed_repeated_values(db): - _test_helper_success(db, {"dummydn": {"dummyattr": ["dummyval", "dummyval"]}}) diff --git a/tests/support/pytest/ldap.py b/tests/support/pytest/ldap.py new file mode 100644 index 000000000000..eff5f236d523 --- /dev/null +++ b/tests/support/pytest/ldap.py @@ -0,0 +1,392 @@ +import logging +import os +import re +import tempfile +import textwrap +import time +from typing import Optional + +import attr +import importlib_metadata +import pytest +import saltfactories.cli.salt +from pytestshellutils.utils import ports +from saltfactories.daemons.container import SaltMinion +from saltfactories.utils import random_string + +import salt.utils.yaml +from tests.support.runtests import RUNTIME_VARS + +_outarg_regex = re.compile(r"^--out(?:put)?(?:=(.*))?$") +log = logging.getLogger(__name__) + + +@attr.s(kw_only=True) +class Salt(saltfactories.cli.salt.Salt): + """``salt`` command-line factory that defaults to YAML instead of JSON. + + Unlike JSON, YAML supports bytes objects via the ``!!binary`` tag. + + TODO: Modify saltfactories.bases.SaltCli to support --out=yaml and delete + this class. + """ + + out: str = "yaml" + minion_tgt: Optional[str] = None + + def cmdline(self, *args, minion_tgt=None, **kwargs): + self.minion_tgt = minion_tgt + has_out = False + for i, arg in enumerate(args): + m = _outarg_regex.fullmatch(arg) + if not m: + continue + has_out = True + fmt = m.group(1) + arg_next = args[i + 1] if len(args) > i else None + self.out = fmt if fmt is not None else arg_next + if not self.out: + raise ValueError("missing outputter name") + break + if not has_out: + args = ["--out", self.out] + list(args) + return super().cmdline(*args, minion_tgt=minion_tgt, **kwargs) + + def process_output(self, stdout, stderr, cmdline=None): + try: + obj = self._decode(stdout) + except NotImplementedError: + return super().process_output(stdout, stderr, cmdline) + t = self.minion_tgt + if t is not None and t != "*" and not isinstance(obj, str) and t in obj: + obj = obj[t] + return (stdout, stderr, obj) + + def _decode(self, out): + if self.out != "yaml": + raise NotImplementedError(f"unsupported outputter: {self.out!r}") + # yaml.load() (when using CLoader at least) does not accept subclasses + # of str or bytes, but out is probably a + # pytestshellutils.utils.processes.MatchString which is a subclass of + # str. Convert subclasses of str/bytes to actual str/bytes objects. + if isinstance(out, str) and type(out) != str: + out = str(out) + elif isinstance(out, bytes) and type(out) != bytes: + out = bytes(out) + return salt.utils.yaml.load(out) + + +@pytest.fixture(scope="module") +def salt_yaml_cli(salt_master): + assert salt_master.is_running() + return salt_master.salt_cli(factory_class=Salt) + + +@attr.s(kw_only=True, slots=True) +class SlapdMinion(SaltMinion): + """Minion in a Docker container with OpenLDAP installed and running.""" + + base: str = "dc=example,dc=org" + password: str = "adminpassword" + port: int = attr.ib(default=attr.Factory(ports.get_unused_localhost_port)) + user: str = "admin" + + @property + def userdn(self) -> str: + return f"cn={self.user},{self.base}" + + @property + def uri(self) -> str: + """LDAP URI for use inside and outside the container. + + This URI works both inside and outside because the network mode is set + to "host". + """ + return f"ldap://localhost:{self.port}" + + @property + def connect_spec(self): + return { + "url": self.uri, + "bind": { + "method": "simple", + "dn": self.userdn, + "password": self.password, + }, + } + + @classmethod + def default_config(cls, *args, **kwargs): + cfg = super().default_config(*args, **kwargs) + + # Requirements on the minion daemon user: + # + # * The `RUNTIME_VARS.CODE_DIR` directory must be readable by the + # minion daemon user, otherwise Python will fail to import required + # modules during minion start-up. + # + # * As of 2022-10-05, the minion code requires the minion daemon user + # to have an entry in the password database (`/etc/passwd`). + # + # The current user (`os.getuid()`) can access `RUNTIME_VARS.CODE_DIR`, + # but is unlikely to have an entry in the Docker container's password + # database. We could pick a user that is known to exist in the image, + # but that user might not have access to `RUNTIME_VARS.CODE_DIR`. The + # easiest way to ensure that both requirements are satisfied is to run + # the minion as root. If the password database entry requirement is + # removed, the minion can be run as the current user by replacing + # `"root"` here with `os.getuid()` and changing `cmdline()` to pass + # `["--user", f"{os.getuid()}:{os.getgid()}"]`. + cfg.setdefault("user", "root") + + return cfg + + def __attrs_post_init__(self): + super().__attrs_post_init__() + self.container_run_kwargs.setdefault("environment", {}).update( + { + "LDAP_ADMIN_PASSWORD": self.password, + "LDAP_ADMIN_USERNAME": self.user, + "LDAP_PORT_NUMBER": self.port, + "LDAP_ROOT": self.base, + }, + ) + # Note that the target directories have the same names as the source + # directories. This avoids the need to translate path names. + self.container_run_kwargs.setdefault("volumes", {}).update( + { + # Bind mount the checked-out source code into the Docker + # container so that a minion daemon started inside the container + # will run the code to be tested. + RUNTIME_VARS.CODE_DIR: { + "bind": RUNTIME_VARS.CODE_DIR, + "mode": "z", + }, + RUNTIME_VARS.TMP: {"bind": RUNTIME_VARS.TMP, "mode": "z"}, + }, + ) + # Use host network mode so that we don't have to define a network and + # configure the minion with the master's IP address (we can simply use + # localhost). Note that port mapping (the "ports" option) is not + # applicable in host mode. + self.container_run_kwargs["network_mode"] = "host" + self.check_ports[self.port] = self.port + self.container_start_check(self.__start_check) + + def __run_unpriv(self, args): + ret = self.run(*args, user=f"{os.getuid()}:{os.getgid()}") + cmd_str = " ".join(args) + msg = f"command '{cmd_str}' returned {ret.returncode}" + if ret.stdout: + msg += f"\n >>>>> STDOUT >>>>>\n{ret.stdout}" + if not msg.endswith("\n"): + msg += "\n" + msg += " <<<<< STDOUT <<<<<" + if ret.stderr: + msg += f"\n >>>>> STDERR >>>>>\n{ret.stderr}" + if not msg.endswith("\n"): + msg += "\n" + msg += " <<<<< STDERR <<<<<" + if ret.returncode != 0: + raise Exception(msg) + log.debug(msg) + return ret + + def __slapd_running(self) -> bool: + try: + self.__run_unpriv( + [ + "ldapsearch", + "-H", + self.uri, + "-x", + "-D", + self.userdn, + "-w", + self.password, + "-b", + self.base, + ], + ) + log.debug("slapd is running") + return True + except Exception as ex: # pylint: disable=broad-except + log.debug(ex) + log.debug("slapd is not running yet") + return False + + def __start_check(self, timeout_at) -> bool: + while time.time() <= timeout_at: + if self.__slapd_running(): + return True + time.sleep(1) + return False + + def cmdline(self, *args): + cmd = list(super().cmdline(*args)) + # See the comment in `default_config()` for why the minion runs as root. + cmd[2:2] = ["--user", "0:0"] + return cmd + + def ldapadd(self, ldif): + if isinstance(ldif, str): + ldif = ldif.encode() + tmpf = tempfile.NamedTemporaryFile(dir=RUNTIME_VARS.TMP, suffix=".ldif") + with tmpf as f: + f.write(ldif) + f.flush() + self.__run_unpriv(["cat", f.name]) + self.__run_unpriv( + [ + "ldapadd", + "-H", + self.uri, + "-x", + "-D", + self.userdn, + "-w", + self.password, + "-f", + # RUNTIME_VARS.TMP is bind-mounted to RUNTIME_VARS.TMP so + # the path name is the same inside and outside the + # container. + f.name, + ], + ) + + def ldapdelete(self, *dns, recursive=False): + cmd = [ + "ldapdelete", + "-H", + self.uri, + "-x", + "-D", + self.userdn, + "-w", + self.password, + ] + if recursive: + cmd.append("-r") + cmd.extend(dns) + self.__run_unpriv(cmd) + + +@pytest.fixture(scope="module") +def openldap_minion(salt_master, salt_yaml_cli, salt_key_cli): + name = random_string("openldap-minion-") + c = salt_master.salt_minion_daemon( + name, + image="ghcr.io/saltstack/salt-ci-containers/openldap-minion:latest", + factory_class=SlapdMinion, + pull_before_start=True, + skip_on_pull_failure=True, + skip_if_docker_client_not_connectable=True, + ) + # For some reason passing this as an argument to the salt_minion_daemon() + # call above fails with "TypeError: type object got multiple values for + # keyword argument 'python_executable'". + c.python_executable = "python3" + + # Workaround for: + # https://github.com/saltstack/pytest-salt-factories/issues/139 + def _after_start(): + v = importlib_metadata.version("pytest-salt-factories") + for cmd in [ + ("install_packages", "python3-pip"), + ("pip", "install", f"pytest-salt-factories=={v}"), + ]: + ret = c.run(*cmd, user="0:0") + assert ret.returncode == 0, ret + c.after_start(_after_start) + + log.debug(f"starting OpenLDAP minion container {name}...") + with c.started(): + assert c.is_running() + ret = salt_yaml_cli.run("test.ping", minion_tgt=c.id) + assert ret.returncode == 0, ret + assert ret.data is True + yield c + log.debug(f"stopping OpenLDAP minion container {name}...") + assert not c.is_running() + salt_key_cli.run("-y", "-d", c.id) + log.debug(f"OpenLDAP minion container {name} stopped") + + +@pytest.fixture(scope="module") +def openldap_minion_run(openldap_minion, salt_yaml_cli): + def _run(fn, *args, **kwargs): + if fn.startswith("ldap3."): + kwargs.setdefault("connect_spec", openldap_minion.connect_spec) + ret = salt_yaml_cli.run( + fn, + *args, + minion_tgt=openldap_minion.id, + **kwargs, + ) + assert ret.returncode == 0, ret + return ret.data + + yield _run + + +@pytest.fixture(scope="module") +def openldap_minion_apply(salt_master, openldap_minion, openldap_minion_run): + def _apply(fn, **kwargs): + has_name = "name" in kwargs + name = kwargs.pop("name", "x") + if fn.startswith("ldap."): + kwargs.setdefault("connect_spec", openldap_minion.connect_spec) + sls_data = { + name: { + fn: [{k: v} for k, v in kwargs.items()], + }, + } + sls_yaml = salt.utils.yaml.dump(sls_data) + with salt_master.state_tree.base.temp_file("test_state.sls", sls_yaml): + ret = openldap_minion_run("state.apply", "test_state") + # Normalize the return value for easier checking. + assert len(ret) == 1 + (ret,) = ret.values() + assert ret["name"] == name + entries = ["changes", "comment", "result"] + if has_name: + entries.append("name") + ret = {k: ret[k] for k in entries if k in ret} + return ret + + yield _apply + + +@pytest.fixture +def subtree(openldap_minion, request): + dc = request.function.__name__ + dn = f"dc={dc},{openldap_minion.base}" + log.debug(f"Creating and populating temporary subtree {dn}...") + openldap_minion.ldapadd( + textwrap.dedent( + f"""\ + dn: {dn} + objectClass: dcObject + objectClass: organization + dc: {dc} + o: {dc} + + dn: cn=u0,{dn} + objectClass: person + cn: u0 + sn: Lastname + description: desc + description: another desc + """, + ), + ) + log.debug("Created temporary subtree") + yield dn + log.debug("Cleaning up temporary subtree...") + openldap_minion.ldapdelete(dn, recursive=True) + log.debug("Temporary subtree cleaned up") + + +@pytest.fixture +def u0dn(subtree): + yield f"cn=u0,{subtree}" From 3b5434dc96628f36730aeccfbbb85a04e1a2ebf2 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 30 Sep 2022 22:58:15 -0400 Subject: [PATCH 22/33] ldap: Add integration tests for `ldap3.search` --- .../pytests/integration/modules/test_ldap3.py | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 tests/pytests/integration/modules/test_ldap3.py diff --git a/tests/pytests/integration/modules/test_ldap3.py b/tests/pytests/integration/modules/test_ldap3.py new file mode 100644 index 000000000000..e45d6617be9a --- /dev/null +++ b/tests/pytests/integration/modules/test_ldap3.py @@ -0,0 +1,39 @@ +import pytest + +pytest_plugins = [ + "tests.support.pytest.ldap", +] +pytestmark = [ + pytest.mark.destructive_test, + pytest.mark.skip_if_binaries_missing("docker"), + pytest.mark.slow_test, +] + + +def test_search(openldap_minion_run, subtree, u0dn): + assert openldap_minion_run("ldap3.search", base=subtree) == { + subtree: { + "objectClass": ["dcObject", "organization"], + "dc": ["test_search"], + "o": ["test_search"], + }, + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + +def test_search_filter(openldap_minion_run, subtree, u0dn): + assert openldap_minion_run( + "ldap3.search", base=subtree, filterstr="(sn=Lastname)" + ) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } From d9b455636eb6ea8812208b752e0479b2dae8488b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 3 Aug 2022 22:38:12 -0400 Subject: [PATCH 23/33] ldap: Minor documentation improvements * Clarify that the `_format_unicode_password()` return value is double quoted, not the argument. * Refine the CLI examples: * Wrap lines with line continuations for readability. * Add missing backslashes before line continuations. * Add missing quotes. * Add missing square brackets for list values. * Rewrap some comments at column 80. * Miscellaneous wording and formatting refinements. --- changelog/62791.fixed | 2 + salt/modules/ldap3.py | 138 +++++++++++++++++++++++------------------- salt/states/ldap.py | 115 +++++++++++++++++------------------ 3 files changed, 134 insertions(+), 121 deletions(-) create mode 100644 changelog/62791.fixed diff --git a/changelog/62791.fixed b/changelog/62791.fixed new file mode 100644 index 000000000000..0d29296e47fe --- /dev/null +++ b/changelog/62791.fixed @@ -0,0 +1,2 @@ +Improvements to `salt.state.ldap` and `salt.modules.ldap3`: + * Documentation improvements. diff --git a/salt/modules/ldap3.py b/salt/modules/ldap3.py index a065b306b6d4..87d7068cbcd0 100644 --- a/salt/modules/ldap3.py +++ b/salt/modules/ldap3.py @@ -81,14 +81,15 @@ def _bind(l, bind=None): def _format_unicode_password(pwd): """Formats a string per Microsoft AD password specifications. - The string must be enclosed in double quotes and UTF-16 encoded. + See: https://msdn.microsoft.com/en-us/library/cc223248.aspx :param pwd: - The desired password as a string + The desired password as a string. :returns: - A unicode string + A ``bytes`` object that is the result of enclosing ``pwd`` in double + quotes then encoding with UTF-16. """ return '"{}"'.format(pwd).encode("utf-16-le") @@ -250,13 +251,15 @@ def connect(connect_spec=None): .. code-block:: bash - salt '*' ldap3.connect "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'dn': 'cn=admin,dc=example,dc=com', - 'password': 'secret'} - }" + salt minion.example.com ldap3.connect \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'dn': 'cn=admin,dc=example,dc=com', + 'password': 'secret', + }, + }" """ if isinstance(connect_spec, _connect_ctx): return connect_spec @@ -357,14 +360,16 @@ def search( .. code-block:: bash - salt '*' ldap3.search "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'dn': 'cn=admin,dc=example,dc=com', - 'password': 'secret', - }, - }" "base='dc=example,dc=com'" + salt minion.example.com ldap3.search \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'dn': 'cn=admin,dc=example,dc=com', + 'password': 'secret', + }, + }" \ + "base='dc=example,dc=com'" """ l = connect(connect_spec) scope = getattr(ldap, "SCOPE_" + scope.upper()) @@ -398,13 +403,16 @@ def add(connect_spec, dn, attributes): .. code-block:: bash - salt '*' ldap3.add "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'password': 'secret', - }, - }" "dn='dc=example,dc=com'" "attributes={'example': 'values'}" + salt minion.example.com ldap3.add \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'password': 'secret', + }, + }" \ + "dn='dc=example,dc=com'" \ + "attributes={'example': ['values']}" """ l = connect(connect_spec) # convert the "iterable of values" to lists in case that's what @@ -445,12 +453,15 @@ def delete(connect_spec, dn): .. code-block:: bash - salt '*' ldap3.delete "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'password': 'secret'} - }" dn='cn=admin,dc=example,dc=com' + salt minion.example.com ldap3.delete \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'password': 'secret', + }, + }" \ + "dn='cn=admin,dc=example,dc=com'" """ l = connect(connect_spec) log.info("deleting entry: dn: %s", repr(dn)) @@ -500,13 +511,16 @@ def modify(connect_spec, dn, directives): .. code-block:: bash - salt '*' ldap3.modify "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'password': 'secret'} - }" dn='cn=admin,dc=example,dc=com' - directives="('add', 'example', ['example_val'])" + salt minion.example.com ldap3.modify \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'password': 'secret', + }, + }" \ + "dn='cn=admin,dc=example,dc=com'" \ + "directives=[('add', 'example', ['example_val'])]" """ l = connect(connect_spec) # convert the "iterable of values" to lists in case that's what @@ -536,18 +550,17 @@ def modify(connect_spec, dn, directives): def change(connect_spec, dn, before, after): """Modify an entry in an LDAP database. - This does the same thing as :py:func:`modify`, but with a simpler - interface. Instead of taking a list of directives, it takes a - before and after view of an entry, determines the differences - between the two, computes the directives, and executes them. + This does the same thing as :py:func:`modify`, but with a simpler interface. + Instead of taking a list of directives, it takes a before and after view of + an entry, determines the differences between the two, computes directives + based on the differences, and executes the directives. - Any attribute value present in ``before`` but missing in ``after`` - is deleted. Any attribute value present in ``after`` but missing - in ``before`` is added. Any attribute value in the database that - is not mentioned in either ``before`` or ``after`` is not altered. - Any attribute value that is present in both ``before`` and - ``after`` is ignored, regardless of whether that attribute value - exists in the database. + Any attribute value present in ``before`` but missing in ``after`` is + deleted. Any attribute value present in ``after`` but missing in ``before`` + is added. Any attribute value in the database that is not mentioned in + either ``before`` or ``after`` is not altered. Any attribute value that is + present in both ``before`` and ``after`` is ignored, regardless of whether + that attribute value exists in the database. :param connect_spec: See the documentation for the ``connect_spec`` parameter for @@ -557,12 +570,12 @@ def change(connect_spec, dn, before, after): Distinguished name of the entry. :param before: - The expected state of the entry before modification. This is - a dict mapping each attribute name to an iterable of values. + The expected state of the entry before modification. This is a mapping + that maps each attribute name to an iterable of values. :param after: - The desired state of the entry after modification. This is a - dict mapping each attribute name to an iterable of values. + The desired state of the entry after modification. This is a mapping + that maps each attribute name to an iterable of values. :returns: ``True`` if successful, raises an exception otherwise. @@ -571,14 +584,17 @@ def change(connect_spec, dn, before, after): .. code-block:: bash - salt '*' ldap3.change "{ - 'url': 'ldaps://ldap.example.com/', - 'bind': { - 'method': 'simple', - 'password': 'secret'} - }" dn='cn=admin,dc=example,dc=com' - before="{'example_value': 'before_val'}" - after="{'example_value': 'after_val'}" + salt minion.example.com ldap3.change \ + "{ + 'url': 'ldaps://ldap.example.com/', + 'bind': { + 'method': 'simple', + 'password': 'secret', + }, + }" \ + "dn='cn=admin,dc=example,dc=com'" \ + "before={'example_value': ['before_val']}" \ + "after={'example_value': ['after_val']}" """ l = connect(connect_spec) # convert the "iterable of values" to lists in case that's what diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 227182b828b1..215442cec623 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -20,7 +20,7 @@ def managed(name, entries, connect_spec=None): - """Ensure the existence (or not) of LDAP entries and their attributes + """Ensure the existence (or not) of LDAP entries and their attributes. Example: @@ -162,10 +162,9 @@ def managed(name, entries, connect_spec=None): of the attribute's values are deleted. * ``'replace'`` - Attributes to replace. This is a dict mapping an - attribute name to an iterable of values. Any existing - values for the attribute are deleted, then the given - values are added. The iterable may be empty. + Attribute values that will replace any existing values. This is a + dict (which may be empty) mapping an attribute name to an iterable + (which may be empty) of values. In the above directives, the iterables of attribute values may instead be ``None``, in which case an empty list is used, or a @@ -367,46 +366,46 @@ def managed(name, entries, connect_spec=None): def _process_entries(l, entries): - """Helper for managed() to process entries and return before/after views + """Helper for managed() to process entries and obtain before/after views. - Collect the current database state and update it according to the - data in :py:func:`managed`'s ``entries`` parameter. Return the - current database state and what it will look like after - modification. + Collects the current database state and updates it according to the data in + :py:func:`managed`'s ``entries`` parameter. Returns the current database + state and what it will look like after modification. :param l: - the LDAP connection object + The LDAP connection object. :param entries: - the same object passed to the ``entries`` parameter of - :py:func:`manage` - - :return: - an ``(old, new)`` tuple that describes the current state of - the entries and what they will look like after modification. - Each item in the tuple is an OrderedDict that maps an entry DN - to another dict that maps an attribute name to a set of its - values (it's a set because according to the LDAP spec, - attribute value ordering is unspecified and there can't be - duplicates). The structure looks like this: - - {dn1: {attr1: set([val1])}, - dn2: {attr1: set([val2]), attr2: set([val3, val4])}} - - All of an entry's attributes and values will be included, even - if they will not be modified. If an entry mentioned in the - entries variable doesn't yet exist in the database, the DN in - ``old`` will be mapped to an empty dict. If an entry in the - database will be deleted, the DN in ``new`` will be mapped to - an empty dict. All value sets are non-empty: An attribute - that will be added to an entry is not included in ``old``, and - an attribute that will be deleted frm an entry is not included - in ``new``. - - These are OrderedDicts to ensure that the user-supplied - entries are processed in the user-specified order (in case - there are dependencies, such as ACL rules specified in an - early entry that make it possible to modify a later entry). + The same object passed to the ``entries`` parameter of + :py:func:`manage`. + + :returns: + An ``(old, new)`` tuple that describes the current state of the entries + and what they will look like after modification. Each item in the tuple + is an ``OrderedDict`` that maps an entry DN to a ``dict`` that maps an + attribute name to an ``OrderedSet`` of its values. (``OrderedSet`` is + used because the LDAP spec says there can't be duplicates, and it must + be ordered to support the `X-ORDERED + `_ + extension used by OpenLDAP.) The structure looks like this:: + + OrderedDict([(dn1, {attr1: OrderedSet([val1])}), + (dn2, {attr1: OrderedSet([val2]), + attr2: OrderedSet([val3, val4])})]) + + All of an entry's attributes and values will be included, even if they + will not be modified. If an entry mentioned in ``entries`` does not yet + exist in the database, the DN in ``old`` will be mapped to an empty + ``dict``. If an entry in the database will be deleted, the DN in ``new`` + will be mapped to an empty ``dict``. All value sets are non-empty: An + attribute that will be added to an entry is not included in ``old``, and + an attribute that will be deleted from an entry is not included in + ``new``. + + These are ``OrderedDicts`` to ensure that the user-supplied entries are + processed in the user-specified order (in case there are dependencies, + such as ACL rules specified in an early entry that make it possible to + modify a later entry). """ old = OrderedDict() @@ -495,26 +494,22 @@ def _update_entry(entry, status, directives): def _toset(thing): - """helper to convert various things to a set - - This enables flexibility in what users provide as the list of LDAP - entry attribute values. Note that the LDAP spec prohibits - duplicate values in an attribute. - - RFC 2251 states that: - "The order of attribute values within the vals set is undefined and - implementation-dependent, and MUST NOT be relied upon." - However, OpenLDAP have an X-ORDERED that is used in the config schema. - Using sets would mean we can't pass ordered values and therefore can't - manage parts of the OpenLDAP configuration, hence the use of OrderedSet. - - Sets are also good for automatically removing duplicates. - - None becomes an empty set. Iterables except for strings have - their elements added to a new set. Non-None scalars (strings, - numbers, non-iterable objects, etc.) are added as the only member - of a new set. - + """Helper to convert various things to an ``OrderedSet``. + + This enables flexibility in what users provide as the list of LDAP entry + attribute values. Note that the LDAP spec prohibits duplicate values in an + attribute, so a set type is used. + + `RFC 4511 section 4.1.7 + `_ says, "The + set of attribute values is unordered." Despite this, the returned set is + ordered so that it can support the `X-ORDERED + `_ + extension. (OpenLDAP has some X-ORDERED attributes in its config schema.) + + ``None`` becomes an empty set. Iterables except for strings have their + elements added to a new set. Non-``None` scalars (strings, numbers, + non-iterable objects, etc.) are added as the only member of a new set. """ if thing is None: return OrderedSet() From 6ac5c344e0d506c636d20cead7d21960e95195f0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Oct 2022 18:16:26 -0400 Subject: [PATCH 24/33] ldap: Fix support for bare `bytes` values, add test coverage --- changelog/62791.fixed | 1 + salt/states/ldap.py | 59 ++++++++++------ tests/pytests/integration/states/test_ldap.py | 18 ++--- tests/pytests/unit/states/test_ldap.py | 67 +++++++++++++++++++ 4 files changed, 114 insertions(+), 31 deletions(-) create mode 100644 tests/pytests/unit/states/test_ldap.py diff --git a/changelog/62791.fixed b/changelog/62791.fixed index 0d29296e47fe..88cb0884bd34 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -1,2 +1,3 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: + * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 215442cec623..3ef903ef4f65 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -14,7 +14,6 @@ from salt.utils.odict import OrderedDict from salt.utils.oset import OrderedSet -from salt.utils.stringutils import to_bytes log = logging.getLogger(__name__) @@ -493,6 +492,31 @@ def _update_entry(entry, status, directives): raise ValueError("unknown directive: " + directive) +def _normalize(thing): + """Convert thing to bytes. + + Details: + * bytes-like values are copied and returned. + * str values are encoded to UTF-8 bytes. + * int values (including Booleans) are converted to str then encoded. + * Everything else raises an exception. + """ + try: + # If thing is already a bytes-like object, return a copy of the bytes. + # The intermediate call to memoryview prevents this function from + # interpreting an iterable of 8-bit integer values as a string of bytes. + thing = bytes(memoryview(thing)) + except: # pylint: disable=bare-except + pass + if isinstance(thing, int): + thing = str(thing) + if isinstance(thing, str): + thing = thing.encode() + if not isinstance(thing, bytes): + raise TypeError(f"expected an int, str, or bytes-like value, got {type(thing)}") + return thing + + def _toset(thing): """Helper to convert various things to an ``OrderedSet``. @@ -507,24 +531,19 @@ def _toset(thing): `_ extension. (OpenLDAP has some X-ORDERED attributes in its config schema.) - ``None`` becomes an empty set. Iterables except for strings have their - elements added to a new set. Non-``None` scalars (strings, numbers, - non-iterable objects, etc.) are added as the only member of a new set. + Details: + * ``None`` becomes a new empty set. + * ``bytes``-like, ``str``, and ``int`` values are normalized with + ``_normalize()`` and returned as the only member of a new set. + * Other values are assumed to be iterables of values to normalize and + return in the new set. (If it is not an iterable, or if an entry can't + be normalized, an exception is raised). """ if thing is None: - return OrderedSet() - if isinstance(thing, str): - return OrderedSet((to_bytes(thing),)) - if isinstance(thing, int): - return OrderedSet((to_bytes(str(thing)),)) - # convert numbers to strings and then bytes - # so that equality checks work - # (LDAP stores numbers as strings) - try: - return OrderedSet( - to_bytes(str(x)) if isinstance(x, int) else to_bytes(x) for x in thing - ) - except TypeError: - return OrderedSet( - str(thing), - ) + coll = () + else: + try: + coll = (_normalize(thing),) + except TypeError: + coll = (_normalize(x) for x in thing) + return OrderedSet(coll) diff --git a/tests/pytests/integration/states/test_ldap.py b/tests/pytests/integration/states/test_ldap.py index 45fe4711550e..47db39fc139c 100644 --- a/tests/pytests/integration/states/test_ldap.py +++ b/tests/pytests/integration/states/test_ldap.py @@ -46,7 +46,9 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr # which is able to pass tuples to the minion. Using # such a fixture with state.single would make these # tests more like unit tests and less like integration - # tests. + # tests. There are existing unit test cases for + # non-list iterables of values, so it's no big deal that + # there isn't integration test coverage here. # # Alternatively the YAML loader can be extended to # support tuples and/or OrderedDict. @@ -55,16 +57,10 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr 4567, b"abcd", ], - "userPassword": [ - # Intentionally invalid UTF-8. The syntax for - # userPassword is Octet String, not Directory String - # (like description), so this is acceptable. - # - # TODO: bytes objects must be in a list (this test - # can't do `"userPassword": b"..."`) due to a bug in - # the way values are turned into sets. - b"\x00\x01\x02\x03\x80", - ], + # Intentionally invalid UTF-8. The syntax for + # userPassword is Octet String, not Directory String + # (like description), so this is acceptable. + "userPassword": b"\x00\x01\x02\x03\x80", # Empty list should be a no-op. "telephoneNumber": [], # None should be equivalent to an empty list. diff --git a/tests/pytests/unit/states/test_ldap.py b/tests/pytests/unit/states/test_ldap.py new file mode 100644 index 000000000000..19d9064d451e --- /dev/null +++ b/tests/pytests/unit/states/test_ldap.py @@ -0,0 +1,67 @@ +import pytest + +import salt.states.ldap as ldap +from salt.utils.oset import OrderedSet + + +def _toset_testcases(): + def _gen(x): + yield x + + # Single values: + for input, want_list in [ + ("", [b""]), + (b"", [b""]), + (False, [b"False"]), + (True, [b"True"]), + (0, [b"0"]), + (-1, [b"-1"]), + (0xF, [b"15"]), + ("🚀", ["🚀".encode()]), + ("🚀".encode(), ["🚀".encode()]), + (bytearray("🚀".encode()), ["🚀".encode()]), + (memoryview("🚀".encode()), ["🚀".encode()]), + (b"\x80", [b"\x80"]), # Intentionally invalid UTF-8. + ]: + # Single values can be provided directly or in an iterable. + for xform in [ + lambda x: x, + lambda x: [x], + lambda x: (x,), + _gen, + ]: + yield (xform(input), want_list) + + yield from [ + # Sequences: + (None, []), + ([], []), + ((), []), + (set(), []), + (["a", "b"], [b"a", b"b"]), + ([b"a", b"b"], [b"a", b"b"]), + (["a", b"b", 0], [b"a", b"b", b"0"]), # Mix of types. + # A sequence of integers in [0, 256) can be converted to a bytes object, + # but they shouldn't be -- they should be treated as a sequence of + # integers. (Otherwise, it would be impossible to store integers unless + # one of the values was outside [0, 256).) + ([128], [b"128"]), + ((128,), [b"128"]), + (list("🚀".encode()), [b"240", b"159", b"154", b"128"]), + (tuple("🚀".encode()), [b"240", b"159", b"154", b"128"]), + # Invalid values: + (1.1, TypeError), + ([[]], TypeError), + ] + + +@pytest.mark.parametrize("input,want_list", _toset_testcases()) +def test__toset(input, want_list): + if isinstance(want_list, type): + with pytest.raises(want_list): + got = ldap._toset(input) + else: + want = OrderedSet(want_list) + got = ldap._toset(input) + assert got == want + assert list(got) == want_list From ff8024ecf45b1b8942fe9e05333b2575388248ae Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Oct 2022 18:29:58 -0400 Subject: [PATCH 25/33] ldap: Exercise non-list iterable in integration test --- tests/pytests/integration/states/test_ldap.py | 50 +++++++++---------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/tests/pytests/integration/states/test_ldap.py b/tests/pytests/integration/states/test_ldap.py index 47db39fc139c..4c4a09b8d437 100644 --- a/tests/pytests/integration/states/test_ldap.py +++ b/tests/pytests/integration/states/test_ldap.py @@ -32,35 +32,27 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr # (numbers are stringified and then put in a list). "sn": 1234, # List of values of various types. - # - # Note that ldap.managed accepts arbitrary iterables, - # not just lists. Unfortunately, Salt's YAML loader - # does not currently (as of 2022-10-11) support any - # ordered non-list types (such as tuple or OrderedDict) - # so we don't test them here. (A dict can be used, but - # iteration order isn't guaranteed so the tests would be - # flaky.) - # - # Instead of a salt CLI fixture we could use a - # LocalClient fixture (see salt_master.salt_client()), - # which is able to pass tuples to the minion. Using - # such a fixture with state.single would make these - # tests more like unit tests and less like integration - # tests. There are existing unit test cases for - # non-list iterables of values, so it's no big deal that - # there isn't integration test coverage here. - # - # Alternatively the YAML loader can be extended to - # support tuples and/or OrderedDict. "description": [ "Non-ASCII characters should be supported: 🙂", 4567, b"abcd", ], - # Intentionally invalid UTF-8. The syntax for - # userPassword is Octet String, not Directory String - # (like description), so this is acceptable. - "userPassword": b"\x00\x01\x02\x03\x80", + # Non-list iterable to exercise support for arbitrary + # iterables of values, not just lists. Note that tuples + # are currently (as of 2022-10-23) serialized as lists, + # but this value is not serialized before it is + # processed by the minion. (The state SLS file + # generated from this object by the + # openldap_minion_apply fixture is loaded on the minion + # and processed directly, not loaded on the master and + # sent to the minion in serialized form). + "userPassword": ( + "password", + # Intentionally invalid UTF-8. The syntax for + # userPassword is Octet String, not Directory String + # (like description), so this is acceptable. + b"\x00\x01\x02\x03\x80", + ), # Empty list should be a no-op. "telephoneNumber": [], # None should be equivalent to an empty list. @@ -83,7 +75,10 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr "Non-ASCII characters should be supported: 🙂", "abcd", ], - "userPassword": [b"\x00\x01\x02\x03\x80"], + "userPassword": [ + b"\x00\x01\x02\x03\x80", + "password", + ], }, }, }, @@ -100,7 +95,10 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr "4567", "abcd", ], - "userPassword": [b"\x00\x01\x02\x03\x80"], + "userPassword": [ + "password", + b"\x00\x01\x02\x03\x80", + ], }, } From d6d3f454b295e7730ef0367b0a3fbe063d2c0dd5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 5 Aug 2022 17:43:48 -0400 Subject: [PATCH 26/33] ldap: Use standard `OrderedDict` class --- changelog/62791.fixed | 1 + salt/states/ldap.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/changelog/62791.fixed b/changelog/62791.fixed index 88cb0884bd34..f42456e92986 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -1,3 +1,4 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. + * Miscellaneous code cleanups. diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 3ef903ef4f65..e80efc8b545c 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -11,8 +11,8 @@ import copy import inspect import logging +from collections import OrderedDict -from salt.utils.odict import OrderedDict from salt.utils.oset import OrderedSet log = logging.getLogger(__name__) From 452bc7b61022ce304627ed80d3448d1e4df6a422 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 3 Aug 2022 22:56:16 -0400 Subject: [PATCH 27/33] ldap: Replace `OrderedDict` with `OrderedSet` where appropriate Now that an `OrderedSet` class exists, use it instead of `OrderedDict` in the places where `OrderedDict` was used to emulate an ordered set. --- salt/states/ldap.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/salt/states/ldap.py b/salt/states/ldap.py index e80efc8b545c..5bf5934b2502 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -254,10 +254,7 @@ def managed(name, entries, connect_spec=None): old, new = _process_entries(l, entries) - # collect all of the affected entries (only the key is - # important in this dict; would have used an OrderedSet if - # there was one) - dn_set = OrderedDict() + dn_set = OrderedSet() dn_set.update(old) dn_set.update(new) @@ -280,7 +277,7 @@ def managed(name, entries, connect_spec=None): for dn in dn_to_delete: for x in old, new: x.pop(dn, None) - del dn_set[dn] + dn_set.remove(dn) ret = { "name": name, @@ -307,7 +304,7 @@ def managed(name, entries, connect_spec=None): ret["result"] = True ret["comment"] = "Successfully updated LDAP entries" errs = [] - success_dn_set = OrderedDict() + success_dn_set = OrderedSet() for dn in dn_set: o = old.get(dn, {}) n = new.get(dn, {}) @@ -331,7 +328,7 @@ def managed(name, entries, connect_spec=None): # is raised changed_old[dn] = o changed_new[dn] = n - success_dn_set[dn] = True + success_dn_set.add(dn) except ldap3.LDAPError as err: log.exception("failed to %s entry %s (%s)", op, dn, err) errs.append((op, dn, err)) From c6f64d3a5a5d36e589305310252e6f4925797f1b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 3 Aug 2022 23:02:37 -0400 Subject: [PATCH 28/33] ldap: Move `LDAPError` class to a utility module --- salt/modules/ldap3.py | 16 +--------------- salt/states/ldap.py | 13 +++---------- salt/utils/ldap.py | 16 ++++++++++++++++ tests/pytests/unit/utils/test_ldap.py | 9 +++++++++ 4 files changed, 29 insertions(+), 25 deletions(-) create mode 100644 salt/utils/ldap.py create mode 100644 tests/pytests/unit/utils/test_ldap.py diff --git a/salt/modules/ldap3.py b/salt/modules/ldap3.py index 87d7068cbcd0..1ddd09d32449 100644 --- a/salt/modules/ldap3.py +++ b/salt/modules/ldap3.py @@ -14,6 +14,7 @@ import logging import salt.utils.data +from salt.utils.ldap import LDAPError available_backends = set() try: @@ -35,21 +36,6 @@ def __virtual__(): return bool(len(available_backends)) -class LDAPError(Exception): - """Base class of all LDAP exceptions raised by backends. - - This is only used for errors encountered while interacting with - the LDAP server; usage errors (e.g., invalid backend name) will - have a different type. - - :ivar cause: backend exception object, if applicable - """ - - def __init__(self, message, cause=None): - super().__init__(message) - self.cause = cause - - def _convert_exception(e): """Convert an ldap backend exception to an LDAPError and raise it.""" raise LDAPError("exception in ldap backend: {!r}".format(e), e) from e diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 5bf5934b2502..84be25cc98cc 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -9,10 +9,10 @@ """ import copy -import inspect import logging from collections import OrderedDict +from salt.utils.ldap import LDAPError from salt.utils.oset import OrderedSet log = logging.getLogger(__name__) @@ -244,14 +244,7 @@ def managed(name, entries, connect_spec=None): # already a connection object pass - connect = __salt__["ldap3.connect"] - - # hack to get at the ldap3 module to access the ldap3.LDAPError - # exception class. https://github.com/saltstack/salt/issues/27578 - ldap3 = inspect.getmodule(connect) - - with connect(connect_spec) as l: - + with __salt__["ldap3.connect"](connect_spec) as l: old, new = _process_entries(l, entries) dn_set = OrderedSet() @@ -329,7 +322,7 @@ def managed(name, entries, connect_spec=None): changed_old[dn] = o changed_new[dn] = n success_dn_set.add(dn) - except ldap3.LDAPError as err: + except LDAPError as err: log.exception("failed to %s entry %s (%s)", op, dn, err) errs.append((op, dn, err)) continue diff --git a/salt/utils/ldap.py b/salt/utils/ldap.py new file mode 100644 index 000000000000..c2938fe3f9d4 --- /dev/null +++ b/salt/utils/ldap.py @@ -0,0 +1,16 @@ +"""Common classes shared between LDAP execution and state modules.""" + + +class LDAPError(Exception): + """Base class of all LDAP exceptions raised by backends. + + This is only used for errors encountered while interacting with + the LDAP server; usage errors (e.g., invalid backend name) will + have a different type. + + :ivar cause: backend exception object, if applicable + """ + + def __init__(self, message, cause=None): + super().__init__(message) + self.cause = cause diff --git a/tests/pytests/unit/utils/test_ldap.py b/tests/pytests/unit/utils/test_ldap.py new file mode 100644 index 000000000000..f78229105f88 --- /dev/null +++ b/tests/pytests/unit/utils/test_ldap.py @@ -0,0 +1,9 @@ +from salt.utils.ldap import LDAPError + + +def test_ldap_error(): + cause = RuntimeError("cause") + err = LDAPError("foo", cause) + assert isinstance(err, Exception) + assert err.cause is cause + assert "foo" in str(err) From e33d167af7666f434e8f43af4532faee83add0a2 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 12 Oct 2022 18:43:49 -0400 Subject: [PATCH 29/33] ldap: Define a new class for holding attribute values Future commits will modify this class to fix some bugs. --- salt/states/ldap.py | 52 +++++++++++--------------- salt/utils/ldap.py | 17 +++++++++ tests/pytests/unit/states/test_ldap.py | 4 +- tests/pytests/unit/utils/test_ldap.py | 33 +++++++++++++++- 4 files changed, 72 insertions(+), 34 deletions(-) diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 84be25cc98cc..105c49266e27 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -8,11 +8,10 @@ their attributes. """ -import copy import logging from collections import OrderedDict -from salt.utils.ldap import LDAPError +from salt.utils.ldap import AttributeValueSet, LDAPError from salt.utils.oset import OrderedSet log = logging.getLogger(__name__) @@ -348,7 +347,10 @@ def managed(name, entries, connect_spec=None): changes[xn] = { attr: sorted(vals) for attr, vals in x.items() - if o.get(attr, ()) != n.get(attr, ()) + if ( + o.get(attr, AttributeValueSet()) + != n.get(attr, AttributeValueSet()) + ) } return ret @@ -372,15 +374,12 @@ def _process_entries(l, entries): An ``(old, new)`` tuple that describes the current state of the entries and what they will look like after modification. Each item in the tuple is an ``OrderedDict`` that maps an entry DN to a ``dict`` that maps an - attribute name to an ``OrderedSet`` of its values. (``OrderedSet`` is - used because the LDAP spec says there can't be duplicates, and it must - be ordered to support the `X-ORDERED - `_ - extension used by OpenLDAP.) The structure looks like this:: + attribute name to an :py:class:`~salt.utils.ldap.AttributeValueSet` of + its values. The structure looks like this:: - OrderedDict([(dn1, {attr1: OrderedSet([val1])}), - (dn2, {attr1: OrderedSet([val2]), - attr2: OrderedSet([val3, val4])})]) + OrderedDict([(dn1, {attr1: AttributeValueSet([val1])}), + (dn2, {attr1: AttributeValueSet([val2]), + attr2: AttributeValueSet([val3, val4])})]) All of an entry's attributes and values will be included, even if they will not be modified. If an entry mentioned in ``entries`` does not yet @@ -411,19 +410,18 @@ def _process_entries(l, entries): if len(results) == 1: attrs = results[dn] olde = { - attr: OrderedSet(attrs[attr]) - for attr in attrs - if len(attrs[attr]) + attr: AttributeValueSet(vals) + for attr, vals in attrs.items() + if len(vals) } else: # nothing, so it must be a brand new entry assert len(results) == 0 olde = {} old[dn] = olde - # copy the old entry to create the new (don't do a simple - # assignment or else modifications to newe will affect - # olde) - newe = copy.deepcopy(olde) + # Deep copy the old entry to create the new (don't do a simple + # assignment or else modifications to `newe` will affect `olde`). + newe = {attr: AttributeValueSet(vals) for attr, vals in olde.items()} new[dn] = newe # process the directives @@ -465,11 +463,11 @@ def _update_entry(entry, status, directives): if vals and (attr not in entry or not entry[attr]): entry[attr] = vals elif directive == "add": - vals.update(entry.get(attr, OrderedSet())) + vals.update(entry.get(attr, AttributeValueSet())) if vals: entry[attr] = vals elif directive == "delete": - existing_vals = entry.pop(attr, OrderedSet()) + existing_vals = entry.pop(attr, AttributeValueSet()) if vals: existing_vals -= vals if existing_vals: @@ -508,18 +506,10 @@ def _normalize(thing): def _toset(thing): - """Helper to convert various things to an ``OrderedSet``. + """Helper to convert various things to an ``AttributeValueSet``. This enables flexibility in what users provide as the list of LDAP entry - attribute values. Note that the LDAP spec prohibits duplicate values in an - attribute, so a set type is used. - - `RFC 4511 section 4.1.7 - `_ says, "The - set of attribute values is unordered." Despite this, the returned set is - ordered so that it can support the `X-ORDERED - `_ - extension. (OpenLDAP has some X-ORDERED attributes in its config schema.) + attribute values. Details: * ``None`` becomes a new empty set. @@ -536,4 +526,4 @@ def _toset(thing): coll = (_normalize(thing),) except TypeError: coll = (_normalize(x) for x in thing) - return OrderedSet(coll) + return AttributeValueSet(coll) diff --git a/salt/utils/ldap.py b/salt/utils/ldap.py index c2938fe3f9d4..0150e79b5f8f 100644 --- a/salt/utils/ldap.py +++ b/salt/utils/ldap.py @@ -1,6 +1,23 @@ """Common classes shared between LDAP execution and state modules.""" +from salt.utils.oset import OrderedSet + + +class AttributeValueSet(OrderedSet): + """Holds an attribute's values as an ordered set. + + `RFC 4511 section 4.1.7 + `_ says, "The + set of attribute values is unordered." Despite this, this set is ordered so + that it can support the `X-ORDERED + `_ + extension. (OpenLDAP has some X-ORDERED attributes in its ``cn=config`` + DIT.) + """ + pass + + class LDAPError(Exception): """Base class of all LDAP exceptions raised by backends. diff --git a/tests/pytests/unit/states/test_ldap.py b/tests/pytests/unit/states/test_ldap.py index 19d9064d451e..4a0c1ef3e212 100644 --- a/tests/pytests/unit/states/test_ldap.py +++ b/tests/pytests/unit/states/test_ldap.py @@ -1,7 +1,7 @@ import pytest import salt.states.ldap as ldap -from salt.utils.oset import OrderedSet +from salt.utils.ldap import AttributeValueSet def _toset_testcases(): @@ -61,7 +61,7 @@ def test__toset(input, want_list): with pytest.raises(want_list): got = ldap._toset(input) else: - want = OrderedSet(want_list) + want = AttributeValueSet(want_list) got = ldap._toset(input) assert got == want assert list(got) == want_list diff --git a/tests/pytests/unit/utils/test_ldap.py b/tests/pytests/unit/utils/test_ldap.py index f78229105f88..05e49bd37c5c 100644 --- a/tests/pytests/unit/utils/test_ldap.py +++ b/tests/pytests/unit/utils/test_ldap.py @@ -1,4 +1,35 @@ -from salt.utils.ldap import LDAPError +import random + +from salt.utils.ldap import AttributeValueSet, LDAPError + + +def test_attribute_value_set_empty(): + assert len(AttributeValueSet()) == 0 + + +def test_attribute_value_set_no_duplicates(): + assert list(AttributeValueSet(["a", "a"])) == ["a"] + + +def test_attribute_value_set_ordered(): + # Filter the values through a set to avoid duplicates. + v = list({str(random.getrandbits(32)).encode() for x in range(100)}) + assert len(v) > 90 + # Avoid unintended correlation with set()'s iteration order. + random.shuffle(v) + assert list(AttributeValueSet(v)) == v + + +def test_attribute_value_set_eq(): + s = AttributeValueSet(["a", "b"]) + assert s is not None + assert s != [] + assert s != AttributeValueSet() + assert s != AttributeValueSet(["x", "y"]) + assert s == s + assert s == ["a", "b"] + assert s == {"a", "b"} + assert s == AttributeValueSet(["a", "b"]) def test_ldap_error(): From 2c45decd748201300f7f0cd359f5ae7279915be5 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 6 Oct 2022 01:40:25 -0400 Subject: [PATCH 30/33] ldap: Preserve the order of multi-valued attributes --- changelog/62791.fixed | 2 + salt/states/ldap.py | 30 ++-- tests/pytests/integration/states/test_ldap.py | 135 ++++++++++-------- 3 files changed, 101 insertions(+), 66 deletions(-) diff --git a/changelog/62791.fixed b/changelog/62791.fixed index f42456e92986..572ef190a6de 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -1,4 +1,6 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: + * The order of an attribute's values is now preserved when reading from and + writing to the LDAP server. * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. * Miscellaneous code cleanups. diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 105c49266e27..8310c09e847b 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -172,6 +172,13 @@ def managed(name, entries, connect_spec=None): Note that if all attribute values are removed from an entry, the entire entry is deleted. + `RFC 4511 section 4.1.7 + `_ says, + "The set of attribute values is unordered." Despite this, if an + attribute has more than one new value, the new values are sent to the + server in the given order in case order is meaningful to the server (as + an extension to the standard). + :param connect_spec: See the description of the ``connect_spec`` parameter of the :py:func:`ldap3.connect ` function @@ -334,7 +341,7 @@ def managed(name, entries, connect_spec=None): # set ret['changes']. filter out any unchanged attributes, and # convert the value sets to lists before returning them to the - # user (sorted for easier comparisons) + # user. for dn in success_dn_set: o = changed_old.get(dn, {}) n = changed_new.get(dn, {}) @@ -345,7 +352,7 @@ def managed(name, entries, connect_spec=None): changes[xn] = None continue changes[xn] = { - attr: sorted(vals) + attr: list(vals) for attr, vals in x.items() if ( o.get(attr, AttributeValueSet()) @@ -463,9 +470,12 @@ def _update_entry(entry, status, directives): if vals and (attr not in entry or not entry[attr]): entry[attr] = vals elif directive == "add": - vals.update(entry.get(attr, AttributeValueSet())) - if vals: - entry[attr] = vals + existing_vals = entry.get(attr, AttributeValueSet()) + # Preserve the order of pre-existing values by updating + # existing_vals with vals rather than the other way around. + existing_vals |= vals + if existing_vals: + entry[attr] = existing_vals elif directive == "delete": existing_vals = entry.pop(attr, AttributeValueSet()) if vals: @@ -473,9 +483,13 @@ def _update_entry(entry, status, directives): if existing_vals: entry[attr] = existing_vals elif directive == "replace": - entry.pop(attr, None) - if vals: - entry[attr] = vals + existing_vals = entry.pop(attr, AttributeValueSet()) + # Preserve the order of pre-existing values by first keeping + # the common values then inserting the new values. + existing_vals &= vals + existing_vals |= vals + if existing_vals: + entry[attr] = existing_vals else: raise ValueError("unknown directive: " + directive) diff --git a/tests/pytests/integration/states/test_ldap.py b/tests/pytests/integration/states/test_ldap.py index 4c4a09b8d437..a9818d4dcfad 100644 --- a/tests/pytests/integration/states/test_ldap.py +++ b/tests/pytests/integration/states/test_ldap.py @@ -62,45 +62,26 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr ], }, ] + want = { + "objectClass": ["person"], + "cn": ["u1"], + "sn": ["1234"], + "description": [ + "Non-ASCII characters should be supported: 🙂", + "4567", + "abcd", + ], + "userPassword": [ + "password", + b"\x00\x01\x02\x03\x80", + ], + } assert openldap_minion_apply("ldap.managed", entries=entries) == { - "changes": { - u1dn: { - "old": None, - "new": { - "objectClass": ["person"], - "cn": ["u1"], - "sn": ["1234"], - "description": [ - "4567", - "Non-ASCII characters should be supported: 🙂", - "abcd", - ], - "userPassword": [ - b"\x00\x01\x02\x03\x80", - "password", - ], - }, - }, - }, + "changes": {u1dn: {"old": None, "new": want}}, "comment": "Successfully updated LDAP entries", "result": True, } - assert openldap_minion_run("ldap3.search", base=u1dn) == { - u1dn: { - "objectClass": ["person"], - "cn": ["u1"], - "sn": ["1234"], - "description": [ - "Non-ASCII characters should be supported: 🙂", - "4567", - "abcd", - ], - "userPassword": [ - "password", - b"\x00\x01\x02\x03\x80", - ], - }, - } + assert openldap_minion_run("ldap3.search", base=u1dn) == {u1dn: want} def test_managed_add_new_attribute(openldap_minion_run, openldap_minion_apply, u0dn): @@ -166,8 +147,8 @@ def test_managed_add_new_value_to_existing_attribute( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, - "new": {"description": ["and another", "another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, + "new": {"description": ["desc", "another desc", "and another"]}, }, }, "comment": "Successfully updated LDAP entries", @@ -178,7 +159,7 @@ def test_managed_add_new_value_to_existing_attribute( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["and another", "desc", "another desc"], + "description": ["desc", "another desc", "and another"], }, } @@ -202,6 +183,25 @@ def test_managed_add_same_values_to_existing_attribute( } +def test_managed_add_same_values_different_order( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"add": {"description": ["another desc", "desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + def test_managed_add_overlapping_values( openldap_minion_run, openldap_minion_apply, u0dn ): @@ -209,8 +209,8 @@ def test_managed_add_overlapping_values( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, - "new": {"description": ["and another", "another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, + "new": {"description": ["desc", "another desc", "and another"]}, }, }, "comment": "Successfully updated LDAP entries", @@ -221,7 +221,7 @@ def test_managed_add_overlapping_values( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["desc", "and another", "another desc"], + "description": ["desc", "another desc", "and another"], }, } @@ -233,8 +233,8 @@ def test_managed_add_overlapping_values_different_order( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, - "new": {"description": ["and another", "another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, + "new": {"description": ["desc", "another desc", "and another"]}, }, }, "comment": "Successfully updated LDAP entries", @@ -245,7 +245,7 @@ def test_managed_add_overlapping_values_different_order( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["and another", "desc", "another desc"], + "description": ["desc", "another desc", "and another"], }, } @@ -255,8 +255,8 @@ def test_managed_add_repeated_values(openldap_minion_run, openldap_minion_apply, assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, - "new": {"description": ["another desc", "desc", "val"]}, + "old": {"description": ["desc", "another desc"]}, + "new": {"description": ["desc", "another desc", "val"]}, }, }, "comment": "Successfully updated LDAP entries", @@ -267,7 +267,7 @@ def test_managed_add_repeated_values(openldap_minion_run, openldap_minion_apply, "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["val", "desc", "another desc"], + "description": ["desc", "another desc", "val"], }, } @@ -332,7 +332,7 @@ def test_managed_replace_no_value_for_one_attribute( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {}, }, }, @@ -372,7 +372,7 @@ def test_managed_replace_no_values_for_all_attributes( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["another desc", "desc"], + "description": ["desc", "another desc"], }, "new": None, }, @@ -407,7 +407,7 @@ def test_managed_replace_new_values(openldap_minion_run, openldap_minion_apply, assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {"description": ["new desc"]}, }, }, @@ -441,6 +441,25 @@ def test_managed_replace_same_values(openldap_minion_run, openldap_minion_apply, } +def test_managed_replace_same_values_different_order( + openldap_minion_run, openldap_minion_apply, u0dn +): + entries = [{u0dn: [{"replace": {"description": ["another desc", "desc"]}}]}] + assert openldap_minion_apply("ldap.managed", entries=entries) == { + "changes": {}, + "comment": "LDAP entries already set", + "result": True, + } + assert openldap_minion_run("ldap3.search", base=u0dn) == { + u0dn: { + "objectClass": ["person"], + "cn": ["u0"], + "sn": ["Lastname"], + "description": ["desc", "another desc"], + }, + } + + def test_managed_replace_overlapping_values( openldap_minion_run, openldap_minion_apply, u0dn ): @@ -448,7 +467,7 @@ def test_managed_replace_overlapping_values( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {"description": ["desc", "new desc"]}, }, }, @@ -472,7 +491,7 @@ def test_managed_replace_overlapping_values_different_order( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {"description": ["desc", "new desc"]}, }, }, @@ -484,7 +503,7 @@ def test_managed_replace_overlapping_values_different_order( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["new desc", "desc"], + "description": ["desc", "new desc"], }, } @@ -579,7 +598,7 @@ def test_managed_delete_remaining_attribute_values( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {}, }, }, @@ -602,7 +621,7 @@ def test_managed_delete_all_attribute_values( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {}, }, }, @@ -642,7 +661,7 @@ def test_managed_delete_all_values_all_attributes( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["another desc", "desc"], + "description": ["desc", "another desc"], }, "new": None, }, @@ -677,7 +696,7 @@ def test_managed_delete_remaining_values_all_attributes( "objectClass": ["person"], "cn": ["u0"], "sn": ["Lastname"], - "description": ["another desc", "desc"], + "description": ["desc", "another desc"], }, "new": None, }, @@ -695,7 +714,7 @@ def test_managed_delete_not_all_values( assert openldap_minion_apply("ldap.managed", entries=entries) == { "changes": { u0dn: { - "old": {"description": ["another desc", "desc"]}, + "old": {"description": ["desc", "another desc"]}, "new": {"description": ["desc"]}, }, }, From 2f1ee6b8276fabff6e9e62c53ce729293c331784 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 3 Aug 2022 16:42:27 -0400 Subject: [PATCH 31/33] ldap: Ignore order when checking equality of attribute value sets --- changelog/62791.fixed | 2 ++ salt/utils/ldap.py | 15 ++++++++++++++- tests/pytests/unit/utils/test_ldap.py | 7 +++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/changelog/62791.fixed b/changelog/62791.fixed index 572ef190a6de..63211a8abf9f 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -1,6 +1,8 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: * The order of an attribute's values is now preserved when reading from and writing to the LDAP server. + * The order of an attribute's values is now ignored when checking value + equality, as required by RFC 4511. * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. * Miscellaneous code cleanups. diff --git a/salt/utils/ldap.py b/salt/utils/ldap.py index 0150e79b5f8f..c86366e21afd 100644 --- a/salt/utils/ldap.py +++ b/salt/utils/ldap.py @@ -14,8 +14,21 @@ class AttributeValueSet(OrderedSet): `_ extension. (OpenLDAP has some X-ORDERED attributes in its ``cn=config`` DIT.) + + RFC 4511 goes on to say, "Implementations MUST NOT rely upon the ordering + being repeatable." To conform to this, the + :py:meth:`~AttributeValueSet.__eq__` method ignores order. Salt will report + no differences and take no action when a desired set of values already + matches what is in LDAP, even if the reported order differs from the desired + order. """ - pass + + def __eq__(self, other): + if other is None: + return False + if other is self: + return True + return set(self) == set(other) class LDAPError(Exception): diff --git a/tests/pytests/unit/utils/test_ldap.py b/tests/pytests/unit/utils/test_ldap.py index 05e49bd37c5c..439270ce5e1b 100644 --- a/tests/pytests/unit/utils/test_ldap.py +++ b/tests/pytests/unit/utils/test_ldap.py @@ -32,6 +32,13 @@ def test_attribute_value_set_eq(): assert s == AttributeValueSet(["a", "b"]) +def test_attribute_value_set_eq_unordered(): + s = AttributeValueSet(["a", "b"]) + assert s == ["b", "a"] + assert s == {"b", "a"} + assert s == AttributeValueSet(["b", "a"]) + + def test_ldap_error(): cause = RuntimeError("cause") err = LDAPError("foo", cause) From 3de11c0363b64c7afc8de634d4c75d3fbccac05e Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 4 Aug 2022 16:38:06 -0400 Subject: [PATCH 32/33] ldap: Fix `str` <-> `bytes` encoding/decoding Move all encoding and decoding logic to the `AttributeValueSet` class and consistently use it when reading from or writing to LDAP. This fixes some encoding/decoding corner cases, and improves the API's usability. Technically this is a backwards-incompatible change: `ldap3.search` and `ldap.managed` now return decoded strings when possible. However, it appears that the Salt master (or maybe the master/minion protocol?) automatically decodes returned `bytes` objects to `str` when possible, so this change only affects direct function calls. --- changelog/62791.fixed | 1 + salt/modules/ldap3.py | 83 +++++++------------ salt/states/ldap.py | 38 ++++----- salt/utils/ldap.py | 74 ++++++++++++++++- tests/pytests/integration/states/test_ldap.py | 2 + tests/pytests/unit/states/test_ldap.py | 42 +++++----- tests/pytests/unit/utils/test_ldap.py | 59 ++++++++++--- 7 files changed, 194 insertions(+), 105 deletions(-) diff --git a/changelog/62791.fixed b/changelog/62791.fixed index 63211a8abf9f..ea80180f1c74 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -3,6 +3,7 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: writing to the LDAP server. * The order of an attribute's values is now ignored when checking value equality, as required by RFC 4511. + * Fixed attribute value encoding/decoding corner cases. * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. * Miscellaneous code cleanups. diff --git a/salt/modules/ldap3.py b/salt/modules/ldap3.py index 1ddd09d32449..460fbc51f634 100644 --- a/salt/modules/ldap3.py +++ b/salt/modules/ldap3.py @@ -13,8 +13,7 @@ import logging -import salt.utils.data -from salt.utils.ldap import LDAPError +from salt.utils.ldap import AttributeValueSet, LDAPError available_backends = set() try: @@ -65,21 +64,6 @@ def _bind(l, bind=None): ) -def _format_unicode_password(pwd): - """Formats a string per Microsoft AD password specifications. - - See: https://msdn.microsoft.com/en-us/library/cc223248.aspx - - :param pwd: - The desired password as a string. - - :returns: - A ``bytes`` object that is the result of enclosing ``pwd`` in double - quotes then encoding with UTF-16. - """ - return '"{}"'.format(pwd).encode("utf-16-le") - - class _connect_ctx: def __init__(self, c): self.c = c @@ -307,6 +291,9 @@ def search( ): """Search an LDAP database. + .. versionchanged:: 3006.0 + The returned attribute values are now decoded to strings when possible. + :param connect_spec: See the documentation for the ``connect_spec`` parameter for :py:func:`connect`. @@ -365,7 +352,13 @@ def search( results = [] except ldap.LDAPError as e: _convert_exception(e) - return dict(results) + return { + dn: { + attr: list(AttributeValueSet(attr, encvals)) + for attr, encvals in encattrs.items() + } + for dn, encattrs in results + } def add(connect_spec, dn, attributes): @@ -401,20 +394,12 @@ def add(connect_spec, dn, attributes): "attributes={'example': ['values']}" """ l = connect(connect_spec) - # convert the "iterable of values" to lists in case that's what - # addModlist() expects (also to ensure that the caller's objects - # are not modified) - attributes = { - attr: salt.utils.data.encode(list(vals)) for attr, vals in attributes.items() - } log.info("adding entry: dn: %s attributes: %s", repr(dn), repr(attributes)) - - if "unicodePwd" in attributes: - attributes["unicodePwd"] = [ - _format_unicode_password(x) for x in attributes["unicodePwd"] - ] - - modlist = ldap.modlist.addModlist(attributes) + encattrs = { + attr: AttributeValueSet(attr, vals).encode() + for attr, vals in attributes.items() + } + modlist = ldap.modlist.addModlist(encattrs) try: l.c.add_s(dn, modlist) except ldap.LDAPError as e: @@ -509,23 +494,14 @@ def modify(connect_spec, dn, directives): "directives=[('add', 'example', ['example_val'])]" """ l = connect(connect_spec) - # convert the "iterable of values" to lists in case that's what - # modify_s() expects (also to ensure that the caller's objects are - # not modified) modlist = [ - (getattr(ldap, "MOD_" + op.upper()), attr, list(vals)) + ( + getattr(ldap, "MOD_" + op.upper()), + attr, + AttributeValueSet(attr, vals).encode(), + ) for op, attr, vals in directives ] - - for idx, mod in enumerate(modlist): - if mod[1] == "unicodePwd": - modlist[idx] = ( - mod[0], - mod[1], - [_format_unicode_password(x) for x in mod[2]], - ) - - modlist = salt.utils.data.decode(modlist, to_str=True, preserve_tuples=True) try: l.c.modify_s(dn, modlist) except ldap.LDAPError as e: @@ -583,15 +559,14 @@ def change(connect_spec, dn, before, after): "after={'example_value': ['after_val']}" """ l = connect(connect_spec) - # convert the "iterable of values" to lists in case that's what - # modifyModlist() expects (also to ensure that the caller's dicts - # are not modified) - before = {attr: salt.utils.data.encode(list(vals)) for attr, vals in before.items()} - after = {attr: salt.utils.data.encode(list(vals)) for attr, vals in after.items()} - - if "unicodePwd" in after: - after["unicodePwd"] = [_format_unicode_password(x) for x in after["unicodePwd"]] - + before = { + attr: AttributeValueSet(attr, vals).encode() + for attr, vals in before.items() + } + after = { + attr: AttributeValueSet(attr, vals).encode() + for attr, vals in after.items() + } modlist = ldap.modlist.modifyModlist(before, after) try: diff --git a/salt/states/ldap.py b/salt/states/ldap.py index 8310c09e847b..4a577d8c3597 100644 --- a/salt/states/ldap.py +++ b/salt/states/ldap.py @@ -355,8 +355,8 @@ def managed(name, entries, connect_spec=None): attr: list(vals) for attr, vals in x.items() if ( - o.get(attr, AttributeValueSet()) - != n.get(attr, AttributeValueSet()) + o.get(attr, AttributeValueSet(attr)) + != n.get(attr, AttributeValueSet(attr)) ) } @@ -384,9 +384,11 @@ def _process_entries(l, entries): attribute name to an :py:class:`~salt.utils.ldap.AttributeValueSet` of its values. The structure looks like this:: - OrderedDict([(dn1, {attr1: AttributeValueSet([val1])}), - (dn2, {attr1: AttributeValueSet([val2]), - attr2: AttributeValueSet([val3, val4])})]) + OrderedDict([ + (dn1, {attr1: AttributeValueSet(attr1, [val1])}), + (dn2, {attr1: AttributeValueSet(attr1, [val2]), + attr2: AttributeValueSet(attr2, [val3, val4])}), + ]) All of an entry's attributes and values will be included, even if they will not be modified. If an entry mentioned in ``entries`` does not yet @@ -417,7 +419,7 @@ def _process_entries(l, entries): if len(results) == 1: attrs = results[dn] olde = { - attr: AttributeValueSet(vals) + attr: AttributeValueSet(attr, vals) for attr, vals in attrs.items() if len(vals) } @@ -428,7 +430,7 @@ def _process_entries(l, entries): old[dn] = olde # Deep copy the old entry to create the new (don't do a simple # assignment or else modifications to `newe` will affect `olde`). - newe = {attr: AttributeValueSet(vals) for attr, vals in olde.items()} + newe = {attr: AttributeValueSet(attr, vals) for attr, vals in olde.items()} new[dn] = newe # process the directives @@ -465,25 +467,25 @@ def _update_entry(entry, status, directives): continue for attr, vals in state.items(): status["mentioned_attributes"].add(attr) - vals = _toset(vals) + vals = _toset(attr, vals) if directive == "default": if vals and (attr not in entry or not entry[attr]): entry[attr] = vals elif directive == "add": - existing_vals = entry.get(attr, AttributeValueSet()) + existing_vals = entry.get(attr, AttributeValueSet(attr)) # Preserve the order of pre-existing values by updating # existing_vals with vals rather than the other way around. existing_vals |= vals if existing_vals: entry[attr] = existing_vals elif directive == "delete": - existing_vals = entry.pop(attr, AttributeValueSet()) + existing_vals = entry.pop(attr, AttributeValueSet(attr)) if vals: existing_vals -= vals if existing_vals: entry[attr] = existing_vals elif directive == "replace": - existing_vals = entry.pop(attr, AttributeValueSet()) + existing_vals = entry.pop(attr, AttributeValueSet(attr)) # Preserve the order of pre-existing values by first keeping # the common values then inserting the new values. existing_vals &= vals @@ -495,12 +497,12 @@ def _update_entry(entry, status, directives): def _normalize(thing): - """Convert thing to bytes. + """Convert thing to a str or bytes. Details: * bytes-like values are copied and returned. - * str values are encoded to UTF-8 bytes. - * int values (including Booleans) are converted to str then encoded. + * str values are returned as-is. + * int values (including Booleans) are converted to str. * Everything else raises an exception. """ try: @@ -512,14 +514,12 @@ def _normalize(thing): pass if isinstance(thing, int): thing = str(thing) - if isinstance(thing, str): - thing = thing.encode() - if not isinstance(thing, bytes): + if not isinstance(thing, (str, bytes)): raise TypeError(f"expected an int, str, or bytes-like value, got {type(thing)}") return thing -def _toset(thing): +def _toset(attr, thing): """Helper to convert various things to an ``AttributeValueSet``. This enables flexibility in what users provide as the list of LDAP entry @@ -540,4 +540,4 @@ def _toset(thing): coll = (_normalize(thing),) except TypeError: coll = (_normalize(x) for x in thing) - return AttributeValueSet(coll) + return AttributeValueSet(attr, coll) diff --git a/salt/utils/ldap.py b/salt/utils/ldap.py index c86366e21afd..e1a990011624 100644 --- a/salt/utils/ldap.py +++ b/salt/utils/ldap.py @@ -21,14 +21,86 @@ class AttributeValueSet(OrderedSet): no differences and take no action when a desired set of values already matches what is in LDAP, even if the reported order differs from the desired order. + + ``str`` values are stored as-is. Other types are first converted to + ``bytes``, then decoded before being stored. If decoding fails, the + ``bytes`` object is stored instead. (This makes it possible for users to + manually pre-encode a value in case this class's encoding behavior is not + suitable for the attribute.) + + ``bytes`` objects are decoded from UTF-8, with one exception: If the + attribute name is ``'unicodePwd'``, the values are decoded according to `the + Microsoft AD password specification + `_. """ + def __init__(self, attr, vals=None): + self.attr = attr + super().__init__(vals) + + # Used by collections.abc.MutableSet to construct a new AttributeValueSet + # object for operations such as set difference. + def _from_iterable(self, it): + return type(self)(self.attr, it) + + def _decode_val(self, v): + if isinstance(v, str): + return v + v = bytes(v) + try: + if self.attr == "unicodePwd": + tmp = v.decode("utf-16-le") + if len(tmp) < 2 or tmp[0] != '"' or tmp[-1] != '"': + raise ValueError("not enclosed in double quotes") + return tmp[1:-1] + else: + return v.decode() + except: # pylint: disable=bare-except + pass + return v + + def _encode_val(self, v): + if isinstance(v, bytes): + return v + assert isinstance(v, str) + if self.attr == "unicodePwd": + return f'"{v}"'.encode("utf-16-le") + return v.encode() + + def encode(self): + """Encodes the values for writing to LDAP. + + When writing to LDAP, the Python ``ldap`` module `expects attribute + values to be ``bytes`` objects + `_. + + :returns: + A list of ``bytes`` objects containing the encoded values. See the + class description for details about how the values are encoded. + """ + return [self._encode_val(v) for v in self] + + def copy(self): + return self.__class__(self.attr, self) + + def __contains__(self, key): + return super().__contains__(self._decode_val(key)) + + def add(self, key): + return super().add(self._decode_val(key)) + + def index(self, key): + return super().index(self._decode_val(key)) + def __eq__(self, other): if other is None: return False if other is self: return True - return set(self) == set(other) + return set(self) == {self._decode_val(v) for v in other} + + def __repr__(self): + return f"{self.__class__.__name__}({self.attr!r}, {list(self)!r})" class LDAPError(Exception): diff --git a/tests/pytests/integration/states/test_ldap.py b/tests/pytests/integration/states/test_ldap.py index a9818d4dcfad..cea9575c591f 100644 --- a/tests/pytests/integration/states/test_ldap.py +++ b/tests/pytests/integration/states/test_ldap.py @@ -35,6 +35,8 @@ def test_managed_add_new_entry(openldap_minion_run, openldap_minion_apply, subtr "description": [ "Non-ASCII characters should be supported: 🙂", 4567, + # This will become a str when returned because it is + # valid UTF-8. b"abcd", ], # Non-list iterable to exercise support for arbitrary diff --git a/tests/pytests/unit/states/test_ldap.py b/tests/pytests/unit/states/test_ldap.py index 4a0c1ef3e212..ccd990a21ff3 100644 --- a/tests/pytests/unit/states/test_ldap.py +++ b/tests/pytests/unit/states/test_ldap.py @@ -10,17 +10,17 @@ def _gen(x): # Single values: for input, want_list in [ - ("", [b""]), - (b"", [b""]), - (False, [b"False"]), - (True, [b"True"]), - (0, [b"0"]), - (-1, [b"-1"]), - (0xF, [b"15"]), - ("🚀", ["🚀".encode()]), - ("🚀".encode(), ["🚀".encode()]), - (bytearray("🚀".encode()), ["🚀".encode()]), - (memoryview("🚀".encode()), ["🚀".encode()]), + ("", [""]), + (b"", [""]), + (False, ["False"]), + (True, ["True"]), + (0, ["0"]), + (-1, ["-1"]), + (0xF, ["15"]), + ("🚀", ["🚀"]), + ("🚀".encode(), ["🚀"]), + (bytearray("🚀".encode()), ["🚀"]), + (memoryview("🚀".encode()), ["🚀"]), (b"\x80", [b"\x80"]), # Intentionally invalid UTF-8. ]: # Single values can be provided directly or in an iterable. @@ -38,17 +38,17 @@ def _gen(x): ([], []), ((), []), (set(), []), - (["a", "b"], [b"a", b"b"]), - ([b"a", b"b"], [b"a", b"b"]), - (["a", b"b", 0], [b"a", b"b", b"0"]), # Mix of types. + (["a", "b"], ["a", "b"]), + ([b"a", b"b"], ["a", "b"]), + (["a", b"b", 0], ["a", "b", "0"]), # Mix of types. # A sequence of integers in [0, 256) can be converted to a bytes object, # but they shouldn't be -- they should be treated as a sequence of # integers. (Otherwise, it would be impossible to store integers unless # one of the values was outside [0, 256).) - ([128], [b"128"]), - ((128,), [b"128"]), - (list("🚀".encode()), [b"240", b"159", b"154", b"128"]), - (tuple("🚀".encode()), [b"240", b"159", b"154", b"128"]), + ([128], ["128"]), + ((128,), ["128"]), + (list("🚀".encode()), ["240", "159", "154", "128"]), + (tuple("🚀".encode()), ["240", "159", "154", "128"]), # Invalid values: (1.1, TypeError), ([[]], TypeError), @@ -59,9 +59,9 @@ def _gen(x): def test__toset(input, want_list): if isinstance(want_list, type): with pytest.raises(want_list): - got = ldap._toset(input) + got = ldap._toset("attr", input) else: - want = AttributeValueSet(want_list) - got = ldap._toset(input) + want = AttributeValueSet("attr", want_list) + got = ldap._toset("attr", input) assert got == want assert list(got) == want_list diff --git a/tests/pytests/unit/utils/test_ldap.py b/tests/pytests/unit/utils/test_ldap.py index 439270ce5e1b..849cc3554f35 100644 --- a/tests/pytests/unit/utils/test_ldap.py +++ b/tests/pytests/unit/utils/test_ldap.py @@ -1,42 +1,81 @@ import random +import pytest + from salt.utils.ldap import AttributeValueSet, LDAPError def test_attribute_value_set_empty(): - assert len(AttributeValueSet()) == 0 + assert len(AttributeValueSet("attr")) == 0 def test_attribute_value_set_no_duplicates(): - assert list(AttributeValueSet(["a", "a"])) == ["a"] + assert list(AttributeValueSet("attr", ["a", "a"])) == ["a"] def test_attribute_value_set_ordered(): # Filter the values through a set to avoid duplicates. - v = list({str(random.getrandbits(32)).encode() for x in range(100)}) + v = list({str(random.getrandbits(32)) for x in range(100)}) assert len(v) > 90 # Avoid unintended correlation with set()'s iteration order. random.shuffle(v) - assert list(AttributeValueSet(v)) == v + assert list(AttributeValueSet("attr", v)) == v def test_attribute_value_set_eq(): - s = AttributeValueSet(["a", "b"]) + s = AttributeValueSet("attr", ["a", "b"]) assert s is not None assert s != [] - assert s != AttributeValueSet() - assert s != AttributeValueSet(["x", "y"]) + assert s != AttributeValueSet("attr") + assert s != AttributeValueSet("attr", ["x", "y"]) assert s == s assert s == ["a", "b"] assert s == {"a", "b"} - assert s == AttributeValueSet(["a", "b"]) + assert s == AttributeValueSet("attr", ["a", "b"]) def test_attribute_value_set_eq_unordered(): - s = AttributeValueSet(["a", "b"]) + s = AttributeValueSet("attr", ["a", "b"]) assert s == ["b", "a"] assert s == {"b", "a"} - assert s == AttributeValueSet(["b", "a"]) + assert s == AttributeValueSet("attr", ["b", "a"]) + + +# attr: Attribute Name. +# input: Input value. +# v: Wanted stored value. +# vx: Wanted encoded value. +@pytest.mark.parametrize( + "attr,input,v,vx", + [ + # str inputs are stored as-is. + ("attr", "🚀", "🚀", b"\xf0\x9f\x9a\x80"), + ("unicodePwd", "🚀", "🚀", b'"\x00=\xd8\x80\xde"\x00'), + # bytes inputs that can be decoded are decoded. + ("attr", b"\xf0\x9f\x9a\x80", "🚀", b"\xf0\x9f\x9a\x80"), + ("unicodePwd", b'"\x00=\xd8\x80\xde"\x00', "🚀", b'"\x00=\xd8\x80\xde"\x00'), + # bytes inputs that can't be decoded are stored as-is. + ("attr", b"\x80", b"\x80", b"\x80"), + ("unicodePwd", b"x", b"x", b"x"), + ("unicodePwd", b'"x"', b'"x"', b'"x"'), # Not utf-16-le encoded. + ("unicodePwd", b"x\x00", b"x\x00", b"x\x00"), # Missing double quotes. + # Non-bytes, non-str inputs. + ("attr", [], "", b""), + ("attr", (), "", b""), + ("attr", [112], "p", b"p"), + ("attr", [128], b"\x80", b"\x80"), + ("attr", bytearray(b"p"), "p", b"p"), + ("attr", memoryview(b"p"), "p", b"p"), + ], +) +def test_attribute_value_set_encode_decode(attr, input, v, vx): + s = AttributeValueSet(attr, [input]) + assert v in s + assert vx in s + assert s == [v] + assert s == [vx] + assert list(s) == [v] + assert s.encode() == [vx] def test_ldap_error(): From 89b405e7354fb01919b0702e1260401c29866c90 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 5 Aug 2022 17:35:25 -0400 Subject: [PATCH 33/33] ldap: Redo `change()` to avoid flaw in `python-ldap` package --- changelog/62791.fixed | 3 + salt/modules/ldap3.py | 76 +++++++++++++++++------- tests/pytests/unit/modules/test_ldap3.py | 17 ++++++ 3 files changed, 74 insertions(+), 22 deletions(-) create mode 100644 tests/pytests/unit/modules/test_ldap3.py diff --git a/changelog/62791.fixed b/changelog/62791.fixed index ea80180f1c74..4b7bada96fd6 100644 --- a/changelog/62791.fixed +++ b/changelog/62791.fixed @@ -4,6 +4,9 @@ Improvements to `salt.state.ldap` and `salt.modules.ldap3`: * The order of an attribute's values is now ignored when checking value equality, as required by RFC 4511. * Fixed attribute value encoding/decoding corner cases. + * Worked around a flaw in the way `python-ldap` adds new attribute values (the + flaw only affects entries where attribute value changes can trigger server + behavior changes, such as some of OpenLDAP's `cn=config` entries). * Fixed support for `bytes` attribute values that are not in a list. * Documentation improvements. * Miscellaneous code cleanups. diff --git a/salt/modules/ldap3.py b/salt/modules/ldap3.py index 460fbc51f634..368fefcbe8ae 100644 --- a/salt/modules/ldap3.py +++ b/salt/modules/ldap3.py @@ -517,12 +517,24 @@ def change(connect_spec, dn, before, after): an entry, determines the differences between the two, computes directives based on the differences, and executes the directives. - Any attribute value present in ``before`` but missing in ``after`` is - deleted. Any attribute value present in ``after`` but missing in ``before`` - is added. Any attribute value in the database that is not mentioned in - either ``before`` or ``after`` is not altered. Any attribute value that is - present in both ``before`` and ``after`` is ignored, regardless of whether - that attribute value exists in the database. + The directives are computed as follows: + + * If an attribute name is present in ``before`` but missing or mapped to a + zero-length iterable of values in ``after``, the attribute is deleted + (regardless of whether the values in the database match the values in + ``before``). + + * Otherwise, if some values are present in ``before`` but missing from + ``after`` and some values are present in ``after`` but missing from + ``before``, all of the attribute's values are replaced with the values in + ``after`` (regardless of whether the values in the database match the + values in ``before``). + + * Otherwise, if some values are present in ``before`` but missing from + ``after``, those specific values are deleted. + + * Otherwise, if some values are present in ``after`` but missing from + ``before``, those specific values are added. :param connect_spec: See the documentation for the ``connect_spec`` parameter for @@ -558,19 +570,39 @@ def change(connect_spec, dn, before, after): "before={'example_value': ['before_val']}" \ "after={'example_value': ['after_val']}" """ - l = connect(connect_spec) - before = { - attr: AttributeValueSet(attr, vals).encode() - for attr, vals in before.items() - } - after = { - attr: AttributeValueSet(attr, vals).encode() - for attr, vals in after.items() - } - modlist = ldap.modlist.modifyModlist(before, after) - - try: - l.c.modify_s(dn, modlist) - except ldap.LDAPError as e: - _convert_exception(e) - return True + # This function could instead use `ldap.modlist.modifyModlist()` to build a + # modlist from `before` and `after`, but the behavior of that function is + # unfortunate: When adding attribute values, the modlist that function + # returns first deletes the attribute then adds it back with the + # original+new values. This is problematic for certain OpenLDAP `cn=config` + # entries where adding and removing values triggers behavioral changes + # (e.g., `olcModuleLoad` in `cn=module{0},cn=config`). + + # Don't encode the values here -- modify() will encode them for us. + before = {attr: AttributeValueSet(attr, vals) for attr, vals in before.items()} + after = {attr: AttributeValueSet(attr, vals) for attr, vals in after.items()} + directives = [] + for attr, before_vals in before.items(): + after_vals = after.get(attr, AttributeValueSet(attr)) + if not after_vals: + directives.append(("delete", attr, ())) + continue + only_in_before = before_vals - after_vals + only_in_after = after_vals - before_vals + if only_in_before: + if only_in_after: + directives.append(("replace", attr, after_vals)) + else: + directives.append(("delete", attr, only_in_before)) + else: + if only_in_after: + directives.append(("add", attr, only_in_after)) + else: + # Nothing to do for this attribute because they already match. + assert before_vals == after_vals + for attr, after_vals in after.items(): + if attr in before or not after_vals: + # Either already handled above or nothing to add. + continue + directives.append(("add", attr, after_vals)) + return modify(connect_spec, dn, directives) diff --git a/tests/pytests/unit/modules/test_ldap3.py b/tests/pytests/unit/modules/test_ldap3.py new file mode 100644 index 000000000000..6f94b5323efb --- /dev/null +++ b/tests/pytests/unit/modules/test_ldap3.py @@ -0,0 +1,17 @@ +import salt.modules.ldap3 as ldap3 +from tests.support.mock import patch + + +def test_change_add_value(): + with patch.object(ldap3, "modify", autospec=True): + ldap3.change( + "connect_spec", + "dn", + {"attr": ["val before"]}, + {"attr": ["val before", "val after"]}, + ) + ldap3.modify.assert_called_once_with( + "connect_spec", + "dn", + [("add", "attr", ["val after"])], + )