On 14.10.2015 16:10, Martin Basti wrote:


On 14.10.2015 15:51, Martin Babinsky wrote:
On 10/13/2015 06:38 PM, Martin Basti wrote:


On 12.10.2015 12:30, Martin Babinsky wrote:
On 10/08/2015 05:58 PM, Martin Basti wrote:
The attached patches fix following tickets:
     https://fedorahosted.org/freeipa/ticket/4949
     https://fedorahosted.org/freeipa/ticket/4048
     https://fedorahosted.org/freeipa/ticket/1930

With these patches, an administrator can specify LDIF file that contains
modifications to be applied to dse.ldif right after creation of DS
instance.


Hi,

Functionally the paches work as expected. However I have following
nitpicks:

in patch 318:

1.) there is a typo in ModifyLDIF class docstring:

+    Operations keep the order in whihc were specified per DN.

in patch 320:

1.) you should use 'os.path.join' to construct FS paths:


-        dse_filename = '%s/%s' % (
+        dse_filename = os.path.join(
             paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid,
-            'dse.ldif',
+            'dse.ldif'
         )

2.) IIUC the 'config_ldif_file' knob in BaseServer holds the path to
LDIF containing the mod operations to dse.ldif. However, the knob name
sounds like the option accepts the path of dse.ldif itself. I propose
to rename the knob to something more in-line with the supposed
function, like 'dse_mods_file'.


Updated patches + CI test attached

Patches work as expected and CI tests are OK.

However it seems that warning level messages are not logged to installer output but only to log file (*waves hand* magic).

Maybe it would be a good idea to raise the message level to "error", so that it is immediately obvious to the user that his DSE mods contain an error and cannot be parsed.

Also you have a typo in the commit message of patch 320 (s/chages/changes/).

Updated patches attached.



Rebased patches for master branch.
From 158ea6f79f4930362e749703e15e19ee0f48f6c4 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Mon, 5 Oct 2015 14:37:05 +0200
Subject: [PATCH 1/3] 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
---
 ipaserver/install/installutils.py    |  98 ++++++++++++++++++++++++++++++
 ipaserver/install/upgradeinstance.py | 112 ++++-------------------------------
 2 files changed, 109 insertions(+), 101 deletions(-)

diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 52a110bb96b074f6a92e8e1f2434c3235b27f65d..8e9e43d15e14eed1cfe1030d83e6e16ddfe25530 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 684a3dd99e2215c86b92dcb7ba9d00ee9e17b8fb..602e6ec4930cd9d2b9e686a5ec2ed3de10cb082f 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)
-- 
2.4.3

From f85f64094f22739f7abbbf432fb3e8073224276d Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 7 Oct 2015 17:15:34 +0200
Subject: [PATCH 2/3] Add method to read changes from LDIF

modifications_from_ldif will read LDIF file and changes in LDIF will
be cached until parse() is called. After calling parse() method changes
will be applied into destination LDIF.

Only changetype modify is supported, the default operation is add.

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

Also fixes:
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930
---
 ipaserver/install/installutils.py | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 8e9e43d15e14eed1cfe1030d83e6e16ddfe25530..1d3551f8bb9cfcac1f6fa24043aea4b5d0a07719 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -1280,6 +1280,46 @@ class ModifyLDIF(ldif.LDIFParser):
         self.remove_value(dn, attr)
         self.add_value(dn, attr, values)
 
+    def modifications_from_ldif(self, ldif_file):
+        """
+        Parse ldif file. Default operation is add, only changetypes "add"
+        and "modify" are supported.
+        :param ldif_file: an opened file for read
+        :raises: ValueError
+        """
+        parser = ldif.LDIFRecordList(ldif_file)
+        parser.parse()
+
+        last_dn = None
+        for dn, entry in parser.all_records:
+            if dn is None:
+                # ldif parser return None, if records belong to previous DN
+                dn = last_dn
+            else:
+                last_dn = dn
+
+            if "replace" in entry:
+                for attr in entry["replace"]:
+                    try:
+                        self.replace_value(dn, attr, entry[attr])
+                    except KeyError:
+                        raise ValueError("replace: {dn}, {attr}: values are "
+                                         "missing".format(dn=dn, attr=attr))
+            elif "delete" in entry:
+                for attr in entry["delete"]:
+                    self.remove_value(dn, attr, entry.get(attr, None))
+            elif "add" in entry:
+                for attr in entry["add"]:
+                    try:
+                        self.replace_value(dn, attr, entry[attr])
+                    except KeyError:
+                        raise ValueError("add: {dn}, {attr}: values are "
+                                         "missing".format(dn=dn, attr=attr))
+            else:
+                root_logger.error("Ignoring entry: %s : only modifications "
+                                  "are allowed (missing \"changetype: "
+                                  "modify\")", dn)
+
     def handle(self, dn, entry):
         if dn in self.modifications:
             self.dn_updated.add(dn)
-- 
2.4.3

From cbf5e160ac3ab905dbdfc7581d9e00970ac854b4 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Thu, 8 Oct 2015 10:38:44 +0200
Subject: [PATCH 3/3] Add option to specify LDIF file that contains DS
 configuration changes

This allows to user modify configuration changes of the directory server
instance during installation of DS

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

Also fixes:
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930
---
 install/tools/man/ipa-replica-install.1    |  3 +++
 install/tools/man/ipa-server-install.1     |  4 ++-
 ipaserver/install/dsinstance.py            | 43 ++++++++++++++++++++++++------
 ipaserver/install/server/common.py         | 14 ++++++++++
 ipaserver/install/server/install.py        |  6 +++--
 ipaserver/install/server/replicainstall.py |  9 ++++---
 6 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/install/tools/man/ipa-replica-install.1 b/install/tools/man/ipa-replica-install.1
index ff4d7d99991c09a875bff6a49070fbba3d13fb63..12a5dd7b85af30023614567e9d70073082bda9c1 100644
--- a/install/tools/man/ipa-replica-install.1
+++ b/install/tools/man/ipa-replica-install.1
@@ -74,6 +74,9 @@ Enable debug logging when more verbose output is needed
 .TP
 \fB\-U\fR, \fB\-\-unattended\fR
 An unattended installation that will never prompt for user input
+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance
 
 .SS "CERTIFICATE SYSTEM OPTIONS"
 .TP
diff --git a/install/tools/man/ipa-server-install.1 b/install/tools/man/ipa-server-install.1
index 2e0ff803c1b185d699f6f15dfb487e455404932e..ba43c80a6589aedea01111c9889d6710adf7d35e 100644
--- a/install/tools/man/ipa-server-install.1
+++ b/install/tools/man/ipa-server-install.1
@@ -78,7 +78,9 @@ Enable debug logging when more verbose output is needed
 .TP
 \fB\-U\fR, \fB\-\-unattended\fR
 An unattended installation that will never prompt for user input
-
+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance
 
 .SS "CERTIFICATE SYSTEM OPTIONS"
 .TP
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 1e0c8393964ffc79a099d6cd1b62d0c4365db9ae..f572c8773a5f2d5d324eb44dd13a8afa0b6f070a 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -206,7 +206,7 @@ info: IPA V2.0
 
 class DsInstance(service.Service):
     def __init__(self, realm_name=None, domain_name=None, dm_password=None,
-                 fstore=None, domainlevel=None):
+                 fstore=None, domainlevel=None, config_ldif=None):
         service.Service.__init__(self, "dirsrv",
             service_desc="directory server",
             dm_password=dm_password,
@@ -229,6 +229,7 @@ class DsInstance(service.Service):
         self.subject_base = None
         self.open_ports = []
         self.run_init_memberof = True
+        self.config_ldif = config_ldif  # updates for dse.ldif
         self.domainlevel = domainlevel
         if realm_name:
             self.suffix = ipautil.realm_to_suffix(self.realm)
@@ -248,6 +249,9 @@ class DsInstance(service.Service):
 
         self.step("creating directory server user", create_ds_user)
         self.step("creating directory server instance", self.__create_instance)
+        if self.config_ldif:
+            self.step("updating configuration in dse.ldif", self.__update_dse_ldif)
+        self.step("restarting directory server", self.__restart_instance)
         self.step("adding default schema", self.__add_default_schemas)
         self.step("enabling memberof plugin", self.__add_memberof_module)
         self.step("enabling winsync plugin", self.__add_winsync_module)
@@ -544,16 +548,39 @@ class DsInstance(service.Service):
         # check for open port 389 from now on
         self.open_ports.append(389)
 
-        root_logger.debug("restarting ds instance")
-        try:
-            self.__restart_instance()
-            root_logger.debug("done restarting ds instance")
-        except ipautil.CalledProcessError as e:
-            print("failed to restart ds instance", e)
-            root_logger.debug("failed to restart ds instance %s" % e)
         inf_fd.close()
         os.remove(paths.DIRSRV_BOOT_LDIF)
 
+    def __update_dse_ldif(self):
+        """
+        This method updates dse.ldif right after instance creation. This is
+        supposed to allow admin modify configuration of the DS which has to be
+        done before IPA is fully installed (for example: settings for
+        replication on replicas)
+        DS must be turned off.
+        """
+        self.stop()
+
+        dse_filename = os.path.join(
+            paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE % self.serverid,
+            'dse.ldif'
+        )
+
+        with tempfile.NamedTemporaryFile(delete=False) as new_dse_ldif:
+            temp_filename = new_dse_ldif.name
+            with open(dse_filename, "r") as input_file:
+                parser = installutils.ModifyLDIF(input_file, new_dse_ldif)
+                # parse modification from config ldif
+                with open(self.config_ldif, "r") as config_ldif:
+                    parser.modifications_from_ldif(config_ldif)
+                parser.parse()
+            new_dse_ldif.flush()
+        shutil.copy2(temp_filename, dse_filename)
+        try:
+            os.remove(temp_filename)
+        except OSError as e:
+            root_logger.debug("Failed to clean temporary file: %s" % e)
+
     def __add_default_schemas(self):
         pent = pwd.getpwnam(DS_USER)
         for schema_fname in IPA_SCHEMA_FILES:
diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
index 3eb7279d200ffd6ab33d8d914c8d4f13e567a171..a09f394d613429818d9a09c14d1cfa6b7fe67186 100644
--- a/ipaserver/install/server/common.py
+++ b/ipaserver/install/server/common.py
@@ -343,6 +343,20 @@ class BaseServer(common.Installable, common.Interactive, core.Composite):
         description="Do not automatically create DNS SSHFP records",
     )
 
+    dirsrv_config_mods = Knob(
+        str, None,
+        description="The path to LDIF file that will be used to modify "
+                    "configuration of dse.ldif during installation of the "
+                    "directory server instance",
+        cli_metavar='FILE',
+    )
+
+    @dirsrv_config_mods.validator
+    def dirsrv_config_mods(self, value):
+        if not os.path.exists(value):
+            raise ValueError("File %s does not exist." % value)
+
+
     def __init__(self, **kwargs):
         super(BaseServer, self).__init__(**kwargs)
 
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 3164d0b9472de9819061e7ee656884a6342ab9c5..566af69a29d62023b5bc2d015c8cc3d490983f51 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -734,7 +734,8 @@ def install(installer):
 
         if options.dirsrv_cert_files:
             ds = dsinstance.DsInstance(fstore=fstore,
-                                       domainlevel=options.domainlevel)
+                                       domainlevel=options.domainlevel,
+                                       config_ldif=options.dirsrv_config_mods)
             installer._ds = ds
             ds.create_instance(realm_name, host_name, domain_name,
                                dm_password, dirsrv_pkcs12_info,
@@ -743,7 +744,8 @@ def install(installer):
                                hbac_allow=not options.no_hbac_allow)
         else:
             ds = dsinstance.DsInstance(fstore=fstore,
-                                       domainlevel=options.domainlevel)
+                                       domainlevel=options.domainlevel,
+                                       config_ldif=options.dirsrv_config_mods)
             installer._ds = ds
             ds.create_instance(realm_name, host_name, domain_name,
                                dm_password,
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 06e936818054af81b22cd4c6db7586624020acd8..071b12bc883bef7d7dce810dfefa2034427d91a7 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -87,7 +87,7 @@ def install_http_certs(config, fstore):
     # FIXME: need Signing-Cert too ?
 
 
-def install_replica_ds(config, promote=False):
+def install_replica_ds(config, options, promote=False):
     dsinstance.check_ports()
 
     # if we have a pkcs12 file, create the cert db from
@@ -95,7 +95,8 @@ def install_replica_ds(config, promote=False):
     # cert
     pkcs12_info = make_pkcs12_info(config.dir, "dscert.p12", "dirsrv_pin.txt")
 
-    ds = dsinstance.DsInstance()
+    ds = dsinstance.DsInstance(
+        config_ldif=options.dirsrv_config_mods)
     ds.create_replica(
         realm_name=config.realm_name,
         master_fqdn=config.master_host_name,
@@ -668,7 +669,7 @@ def install(installer):
             ntp.create_instance()
 
         # Configure dirsrv
-        ds = install_replica_ds(config)
+        ds = install_replica_ds(config, options)
 
         # Always try to install DNS records
         install_dns_records(config, options, remote_api)
@@ -1015,7 +1016,7 @@ def promote(installer):
         ntp.create_instance()
 
     # Configure dirsrv
-    ds = install_replica_ds(config, promote=True)
+    ds = install_replica_ds(config, options, promote=True)
 
     # Always try to install DNS records
     install_dns_records(config, options, api)
-- 
2.4.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to