Make offline LDIF modify more robust

* move code to installutils
* add replace_value method
* use lists instead of single values for add_value, remove_value methods

https://fedorahosted.org/freeipa/ticket/4949

Also fixes:
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930

Reviewed-By: Martin Babinsky <mbabinsk@redhat.com>

2 files changed, 109 insertions, 101 deletions

diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 52a110bb9..8e9e43d15 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -23,6 +23,7 @@ from __future__ import print_function
 import socket
 import getpass
 import gssapi
+import ldif
 import os
 import re
 import fileinput
@@ -1215,3 +1216,100 @@ def check_creds(options, realm_name):
             raise ScriptError("Invalid credentials: %s" % e)
 
         os.environ['KRB5CCNAME'] = ccache_name
+
+
+class ModifyLDIF(ldif.LDIFParser):
+    """
+    Allows to modify LDIF file.
+
+    Operations keep the order in which were specified per DN.
+    Warning: only modifications of existing DNs are supported
+    """
+    def __init__(self, input_file, output_file):
+        """
+        :param input_file: an LDIF
+        :param output_file: an LDIF file
+        """
+        ldif.LDIFParser.__init__(self, input_file)
+        self.writer = ldif.LDIFWriter(output_file)
+        self.dn_updated = set()
+
+        self.modifications = {}  # keep modify operations in original order
+
+    def add_value(self, dn, attr, values):
+        """
+        Add value to LDIF.
+        :param dn: DN of entry (must exists)
+        :param attr: attribute name
+        :param value: value to be added
+        """
+        assert isinstance(values, list)
+        self.modifications.setdefault(dn, []).append(
+            dict(
+                op="add",
+                attr=attr,
+                values=values,
+            )
+        )
+
+    def remove_value(self, dn, attr, values=None):
+        """
+        Remove value from LDIF.
+        :param dn: DN of entry
+        :param attr: attribute name
+        :param value: value to be removed, if value is None, attribute will
+        be removed
+        """
+        assert values is None or isinstance(values, list)
+        self.modifications.setdefault(dn, []).append(
+            dict(
+                op="del",
+                attr=attr,
+                values=values,
+            )
+        )
+
+    def replace_value(self, dn, attr, values):
+        """
+        Replace values in LDIF with new value.
+        :param dn: DN of entry
+        :param attr: attribute name
+        :param value: new value for atribute
+        """
+        assert isinstance(values, list)
+        self.remove_value(dn, attr)
+        self.add_value(dn, attr, values)
+
+    def handle(self, dn, entry):
+        if dn in self.modifications:
+            self.dn_updated.add(dn)
+        for mod in self.modifications.get(dn, []):
+            attr_name = mod["attr"]
+            values = mod["values"]
+
+            if mod["op"] == "del":
+                # delete
+                attribute = entry.setdefault(attr_name, [])
+                if values is None:
+                    attribute = []
+                else:
+                    attribute = [v for v in attribute if v not in values]
+                if not attribute:  # empty
+                    del entry[attr_name]
+            elif mod["op"] == "add":
+                # add
+                attribute = entry.setdefault(attr_name, [])
+                attribute.extend([v for v in values if v not in attribute])
+            else:
+                assert False, "Unknown operation: %r" % mod["op"]
+
+        self.writer.unparse(dn, entry)
+
+    def parse(self):
+        ldif.LDIFParser.parse(self)
+
+        # check if there are any remaining modifications
+        remaining_changes = set(self.modifications.keys()) - self.dn_updated
+        for dn in remaining_changes:
+            root_logger.error(
+                "DN: %s does not exists or haven't been updated", dn)
diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py
index 684a3dd99..602e6ec49 100644
--- a/ipaserver/install/upgradeinstance.py
+++ b/ipaserver/install/upgradeinstance.py
@@ -66,85 +66,6 @@ class GetEntryFromLDIF(ldif.LDIFParser):
         self.results[dn] = entry
 
 
-class ModifyLDIF(ldif.LDIFParser):
-    """
-    Allows to modify LDIF file.
-
-    Remove operations are executed before add operations
-    """
-    def __init__(self, input_file, writer):
-        """
-        :param input_file: an LDIF
-        :param writer: ldif.LDIFWriter instance where modified LDIF will
-        be written
-        """
-        ldif.LDIFParser.__init__(self, input_file)
-        self.writer = writer
-
-        self.add_dict = {}
-        self.remove_dict = {}
-
-    def add_value(self, dn, attr, value):
-        """
-        Add value to LDIF.
-        :param dn: DN of entry (must exists)
-        :param attr: attribute name
-        :param value: value to be added
-        """
-        attr = attr.lower()
-        entry = self.add_dict.setdefault(dn, {})
-        attribute = entry.setdefault(attr, [])
-        if value not in attribute:
-            attribute.append(value)
-
-    def remove_value(self, dn, attr, value=None):
-        """
-        Remove value from LDIF.
-        :param dn: DN of entry
-        :param attr: attribute name
-        :param value: value to be removed, if value is None, attribute will
-        be removed
-        """
-        attr = attr.lower()
-        entry = self.remove_dict.setdefault(dn, {})
-
-        if entry is None:
-            return
-        attribute = entry.setdefault(attr, [])
-        if value is None:
-            # remove all values
-            entry[attr] = None
-            return
-        elif attribute is None:
-            # already marked to remove all values
-            return
-        if value not in attribute:
-            attribute.append(value)
-
-    def handle(self, dn, entry):
-        if dn in self.remove_dict:
-            for name, value in self.remove_dict[dn].items():
-                if value is None:
-                    attribute = []
-                else:
-                    attribute = entry.setdefault(name, [])
-                    attribute = [v for v in attribute if v not in value]
-                entry[name] = attribute
-
-                if not attribute:  # empty
-                    del entry[name]
-
-        if dn in self.add_dict:
-            for name, value in self.add_dict[dn].items():
-                attribute = entry.setdefault(name, [])
-                attribute.extend([v for v in value if v not in attribute])
-
-        if not entry:  # empty
-            return
-
-        self.writer.unparse(dn, entry)
-
-
 class IPAUpgrade(service.Service):
     """
     Update the LDAP data in an instance by turning off all network
@@ -235,13 +156,11 @@ class IPAUpgrade(service.Service):
     def __enable_ds_global_write_lock(self):
         ldif_outfile = "%s.modified.out" % self.filename
         with open(ldif_outfile, "wb") as out_file:
-            ldif_writer = ldif.LDIFWriter(out_file)
             with open(self.filename, "rb") as in_file:
-                parser = ModifyLDIF(in_file, ldif_writer)
+                parser = installutils.ModifyLDIF(in_file, out_file)
 
-                parser.remove_value("cn=config", "nsslapd-global-backend-lock")
-                parser.add_value("cn=config", "nsslapd-global-backend-lock",
-                                 "on")
+                parser.replace_value(
+                    "cn=config", "nsslapd-global-backend-lock", ["on"])
                 parser.parse()
 
         shutil.copy2(ldif_outfile, self.filename)
@@ -253,22 +172,20 @@ class IPAUpgrade(service.Service):
 
         ldif_outfile = "%s.modified.out" % self.filename
         with open(ldif_outfile, "wb") as out_file:
-            ldif_writer = ldif.LDIFWriter(out_file)
             with open(self.filename, "rb") as in_file:
-                parser = ModifyLDIF(in_file, ldif_writer)
+                parser = installutils.ModifyLDIF(in_file, out_file)
 
                 if port is not None:
-                    parser.remove_value("cn=config", "nsslapd-port")
-                    parser.add_value("cn=config", "nsslapd-port", port)
+                    parser.replace_value("cn=config", "nsslapd-port", [port])
                 if security is not None:
-                    parser.remove_value("cn=config", "nsslapd-security")
-                    parser.add_value("cn=config", "nsslapd-security", security)
+                    parser.replace_value("cn=config", "nsslapd-security",
+                                         [security])
 
                 # disable global lock by default
                 parser.remove_value("cn=config", "nsslapd-global-backend-lock")
                 if global_lock is not None:
                     parser.add_value("cn=config", "nsslapd-global-backend-lock",
-                                     global_lock)
+                                     [global_lock])
 
                 parser.parse()
 
@@ -277,18 +194,11 @@ class IPAUpgrade(service.Service):
     def __disable_listeners(self):
         ldif_outfile = "%s.modified.out" % self.filename
         with open(ldif_outfile, "wb") as out_file:
-            ldif_writer = ldif.LDIFWriter(out_file)
             with open(self.filename, "rb") as in_file:
-                parser = ModifyLDIF(in_file, ldif_writer)
-
-                parser.remove_value("cn=config", "nsslapd-port")
-                parser.add_value("cn=config", "nsslapd-port", "0")
-
-                parser.remove_value("cn=config", "nsslapd-security")
-                parser.add_value("cn=config", "nsslapd-security", "off")
-
+                parser = installutils.ModifyLDIF(in_file, out_file)
+                parser.replace_value("cn=config", "nsslapd-port", ["0"])
+                parser.replace_value("cn=config", "nsslapd-security", ["off"])
                 parser.remove_value("cn=config", "nsslapd-ldapientrysearchbase")
-
                 parser.parse()
 
         shutil.copy2(ldif_outfile, self.filename)