[BUG] ldap.managed entries keep getting reapplied

[jerrykan]

Description
When using ldap.managed to manage entries in OpenLDAP it keeps re-applying the correct values and fails to detect that they are already correct.

Setup
Debian 10/buster minion with OpenLDAP installed and the following test.sls state file:

ldap-mdb-conf:
  ldap.managed:
    - connect_spec:
        url: ldapi:///
        bind:
          method: sasl
          mechanism: EXTERNAL

    - entries:
      # Data DB
      - olcDatabase={1}mdb,cn=config:
        - replace:
            olcSuffix: dc=example,dc=com
            olcRootDN: cn=admin,dc=example,dc=com

Steps to Reproduce the behavior

# salt-call state.apply test.apply --output-diff
local:
----------
          ID: ldap-mdb-conf
    Function: ldap.managed
      Result: True
     Comment: Successfully updated LDAP entries
     Started: 15:35:20.594905
    Duration: 4.603 ms
     Changes:   
              ----------
              olcDatabase={1}mdb,cn=config:
                  ----------
                  new:
                      ----------
                      olcRootDN:
                          - cn=admin,dc=example,dc=com
                      olcSuffix:
                          - dc=example,dc=com
                  old:
                      ----------
                      olcRootDN:
                          - cn=admin,dc=example,dc=com
                      olcSuffix:
                          - dc=example,dc=com

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   4.603 ms

Expected behavior
No changes are applied.

Versions Report
Using current master d750d86

Salt Version:
           Salt: 3000
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.7.3 (default, Dec 20 2019, 18:57:59)
   python-gnupg: Not Installed
         PyYAML: 3.13
          PyZMQ: 17.1.2
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1
 
System Versions:
           dist: debian 10.3 
         locale: UTF-8
        machine: x86_64
        release: 4.19.0-8-amd64
         system: Linux
        version: debian 10.3 

Additional context
It seems to be caused by mixing strings and bytes in the salt/states/ldap.py.

Looking at the output of old and new from old, new = _process_entries(l, entries) we get the following:

old:

OrderedDict([('olcDatabase={1}mdb,cn=config',
              {'objectClass': OrderedSet([b'olcDatabaseConfig', b'olcMdbConfig']),
               'olcDatabase': OrderedSet([b'{1}mdb']),
               'olcLastMod': OrderedSet([b'TRUE']),
               'olcRootDN': OrderedSet([b'cn=admin,dc=example,dc=com']),
               'olcSuffix': OrderedSet([b'dc=example,dc=com'])})])

new:

OrderedDict([('olcDatabase={1}mdb,cn=config',
              {'objectClass': OrderedSet([b'olcDatabaseConfig', b'olcMdbConfig']),
               'olcDatabase': OrderedSet([b'{1}mdb']),
               'olcLastMod': OrderedSet([b'TRUE']),
               'olcRootDN': OrderedSet(['cn=admin,dc=example,dc=com']),
               'olcSuffix': OrderedSet(['dc=example,dc=com'])})])

we can see that the olcRootDN and olcSuffix entries returned for the LDAP server in old are bytes, but the same entries are strings in new.

[MarcoSteinacher]

I assume this problem was introduced with a change in salt/modules/ldap3.py related to #48666, which was not reflected in salt/states/ldap.py. On my system, I applied the following patch to work around the issue for the replace directive:

--- ldap.py.orig        2020-07-13 13:48:55.692548871 +0200
+++ ldap.py     2020-07-13 13:48:44.248401198 +0200
@@ -19,6 +19,7 @@
 from salt.ext import six
 from salt.utils.odict import OrderedDict
 from salt.utils.oset import OrderedSet
+from salt.utils.stringutils import to_bytes

 log = logging.getLogger(__name__)

@@ -489,7 +490,7 @@
             elif directive == 'replace':
                 entry.pop(attr, None)
                 if len(vals):
-                    entry[attr] = vals
+                    entry[attr] = [to_bytes(val) for val in vals]
             else:
                 raise ValueError('unknown directive: ' + directive)

DISCLAIMER: This works for me (I'm only using the default und replace directives). I'm not familiar enough with the code, so I can't tell if this is a reasonable patch or if there might be side effects.

[sjtbham]

We needed a bit more in the patch:

--- ldap.py.orig    2020-11-10 09:24:49.998783985 +0000
+++ ldap.py 2020-11-10 09:25:54.964404506 +0000
@@ -20,6 +20,7 @@ import logging
 from salt.ext import six
 from salt.utils.odict import OrderedDict
 from salt.utils.oset import OrderedSet
+from salt.utils.stringutils import to_bytes

 log = logging.getLogger(__name__)

@@ -491,7 +492,7 @@ def _update_entry(entry, status, directi
             elif directive == "add":
                 vals.update(entry.get(attr, ()))
                 if vals:
-                    entry[attr] = vals
+                    entry[attr] = [to_bytes(val) for val in vals]
             elif directive == "delete":
                 existing_vals = entry.pop(attr, OrderedSet())
                 if vals:
@@ -501,7 +502,7 @@ def _update_entry(entry, status, directi
             elif directive == "replace":
                 entry.pop(attr, None)
                 if vals:
-                    entry[attr] = vals
+                    entry[attr] = [to_bytes(val) for val in vals]
             else:
                 raise ValueError("unknown directive: " + directive)

I wonder if something might also need doing in:

            elif directive == "delete":
                existing_vals = entry.pop(attr, OrderedSet())
                if vals:
                    existing_vals -= vals
                    if existing_vals:
                        entry[attr] = existing_vals

But we're not using delete, so maybe this is OK?

I'm not sure this is a complete fix though as when the state is applied, we get an error message:

     Comment: failed to modify entry cn=config(exception in ldap backend: NO_SUCH_ATTRIBUTE({'desc': 'No such attribute'},))

Though the replace has actually been done, and rerunning a second time shows the state as clean.

[oeuftete]

ZD-6163.

This is a more serious problem when using LDAP servers that will reject no-op changes, such as (I think) Oracle Unified Directory.

Reproduced in 3002.2.

[sagetherage]

I am going to attempt to get this looked at in the Aluminium release, but we cannot commit to the work right now. I will see if I can get it assigned, unless anyone here wants to open a PR?

[twangboy]

@jerrykan Could you verify that the changes here (#59373) address this issue?

[jerrykan]

@twangboy I ran a few tests on a v3000.2 setup patched using the changes from the pull request and it seems to resolve the problem.