Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDAP state module improvements #62791

Closed
wants to merge 33 commits into from

Commits on Oct 25, 2022

  1. yaml: Fix documentation about datetime conversion

    I believe the behavior changed with commit
    002aa88 which was first released in
    Salt v2018.3.0.
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    8740213 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    40668bc View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    47e7ec6 View commit details
    Browse the repository at this point in the history
  4. yaml: Add integration test for YAML map iteration order

    This demonstrates that saltstack#12161
    has already been fixed (thanks to Python 3.6 changing `dict` to
    iterate in insertion order).
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    4a431eb View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    0b481a6 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    329e269 View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    747e465 View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    9f060cc View commit details
    Browse the repository at this point in the history
  9. 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()`.
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    fbf37d0 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    602c2c8 View commit details
    Browse the repository at this point in the history
  11. 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
    
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    7a90349 View commit details
    Browse the repository at this point in the history
  12. 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
    <salt.utils.yamlloader.SaltYamlSafeLoader object at 0xc31a10>
    >>> y.load("!!omap [{foo: bar}, {baz: bif}]")  # exact same as before
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      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 "<unicode string>", 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 "<stdin>", line 1, in <module>
      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 "<unicode string>", 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.)
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    0356097 View commit details
    Browse the repository at this point in the history
  13. 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)))
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    507ea71 View commit details
    Browse the repository at this point in the history
  14. 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 "<stdin>", line 1, in <module>
      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 "<unicode string>", 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: saltstack#12161
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    6f5df3c View commit details
    Browse the repository at this point in the history
  15. 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: saltstack#12161
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    fe33d75 View commit details
    Browse the repository at this point in the history
  16. 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 "<stdin>", line 1, in <module>
      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 "<unicode string>", line 1, column 1
      ```
    
    Behavior after:
    
    ```
    $ python
    >>> import salt.utils.yaml as y
    >>> y.load("!!python/tuple [foo, bar]")
    ('foo', 'bar')
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    72a6ebc View commit details
    Browse the repository at this point in the history
  17. 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
    
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    6a4cf23 View commit details
    Browse the repository at this point in the history
  18. 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
    
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    5b7c40b View commit details
    Browse the repository at this point in the history
  19. 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
    
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    3cbe12c View commit details
    Browse the repository at this point in the history
  20. 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
    
    ```
    rhansen committed Oct 25, 2022
    Configuration menu
    Copy the full SHA
    db44b30 View commit details
    Browse the repository at this point in the history

Commits on Oct 26, 2022

  1. 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.
    rhansen committed Oct 26, 2022
    Configuration menu
    Copy the full SHA
    a586969 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    3b5434d View commit details
    Browse the repository at this point in the history
  3. 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.
    rhansen committed Oct 26, 2022
    Configuration menu
    Copy the full SHA
    d9b4556 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    6ac5c34 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    ff8024e View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    d6d3f45 View commit details
    Browse the repository at this point in the history
  7. 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.
    rhansen committed Oct 26, 2022
    Configuration menu
    Copy the full SHA
    452bc7b View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    c6f64d3 View commit details
    Browse the repository at this point in the history
  9. ldap: Define a new class for holding attribute values

    Future commits will modify this class to fix some bugs.
    rhansen committed Oct 26, 2022
    Configuration menu
    Copy the full SHA
    e33d167 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    2c45dec View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    2f1ee6b View commit details
    Browse the repository at this point in the history
  12. 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.
    rhansen committed Oct 26, 2022
    Configuration menu
    Copy the full SHA
    3de11c0 View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    89b405e View commit details
    Browse the repository at this point in the history