On 04/29/2013 08:13 PM, Rob Crittenden wrote:
Tomas Babej wrote:
On 04/25/2013 12:42 PM, Martin Kosek wrote:
On 04/25/2013 12:29 PM, Jan Cholasta wrote:
On 25.4.2013 08:51, Martin Kosek wrote:
On 04/24/2013 08:02 PM, Rob Crittenden wrote:
Jan Cholasta wrote:
On 24.4.2013 14:54, Martin Kosek wrote:
On 04/24/2013 02:51 PM, Rob Crittenden wrote:
Jan Cholasta wrote:
Hi,

On 23.4.2013 12:28, Tomas Babej wrote:
Hi,

We should respect already configured options present in
/etc/openldap/ldap.conf when generating our own configuration.
With this patch, we only rewrite URI, BASE and TLS_CACERT
options.

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


the changeConf call will fail when the file does not exist, we
might
want to handle that gracefully.

Honza


We also need to handle the case where these items are already
defined. I'm
honestly not sure what the behavior should be: overwrite, warn and
overwrite,
fail.

rob


I am also thinking that we may want to be more cautious before
updating this
file. AFAIK, we do not need the updated file for our function, its
only updated
for user convenience so that he can run ldapsearches more easily.

I see several options here that could help this goal:
1) Update ldap.conf if BASE and URI and TLS_CACERT only if these
options are
not set. If the options are already set, we could just print a note
that we
skipped it. When I see my vanilla /etc/openldap/ldap.conf, it has
these options
commented out, so it should be possible to implement this check.

2) Do ldap.conf changes only if a new special option is passe (e.g.
--configure-ldap-cong)

3) Do not update ldap.conf when a new special option is not
passed (e.g.
--no-ldap-conf

Martin


If we don't need the file for our function, we can just not
configure it
at all IMO. We can document how to configure it for users who want
it.

It was an RFE that we create this file. It is handy to have
pre-configured, I
like having it actually.

We just need to try to have a gentler touch than my first crack at
it, which
overwrote it completely. I think #1 is probably enough for now. I'm
not sure I
want to add two new options this late in the game, and the client
already has a
lot of knobs.

rob


Yeah, I also agree that 1) is enough. It will not add any more
options and will
let us be more gentle and respectful to already existent custom user
settings
in ldap.conf. So Tomas, this seems like the way to go :-)

Martin


I don't see the point of updating only some of these values. What about

Not some of them - either all of them (BASE, URI, TLS_CACERT) when
none of them is already configured or none at all.


4) Update BASE and URI and TLS_CACERT, comment any old settings out.

?

This would still break an existing user configuration, we would just
tell user what we broke :-)

Martin


Honza


The following version of the patch configures (BASE, URI, TLS_CACERT)
attributes if they are not set.

However, to preserve user-friendliness, our suggested option is added as
an comment. See commit message for details.

Tomas

Ok, this works as advertised, I just have a general question.

This could enable a mix of IPA and non-IPA settings. In my case I left BASE configured and only URI and TLS_CACERT got set.

This could cause some unexpected results I think, depending on what got changed.

Do we rather want to punt on all of them if any of them can't be updated? This would require a bit more code, and wouldn't be as generic. I just wonder if this would cause subtle failures.

rob

After IRC conversation with Rob, we decided to keep the behaviour, while having it explicitly mentioned in the ldap.conf file.

For illustration, the ldap.conf file could look like this:

[/etc/openldap/ldap.conf]
# File modified by ipa-client-install

# We do not want to break your existing configuration, hence:
#   URI, BASE and TLS_CACERT have been added if they were not set.
#   In case any of them were set, a comment with trailing note
#   "# modified by IPA" note has been inserted.
# To use IPA server with openLDAP tools, please comment out your
# existing configuration for these options and uncomment the
# corresponding lines generated by IPA.


#
# LDAP Defaults
#

# See ldap.conf(5) for details
# This file should be world readable but not world writable.

#BASE dc=ipa,dc=example,dc=com # modified by IPA
BASE    dc=example,dc=com
#URI ldaps://ipa.example.com # modified by IPA
URI     ldap://ldap.example.com

#SIZELIMIT      12
#TIMELIMIT      15
#DEREF          never

TLS_CACERTDIR /etc/openldap/cacerts
TLS_CACERT /etc/ipa/ca.crt

Tomas
From 9a41e0d875517be599d6393090b856cd98d8602c Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Mon, 22 Apr 2013 12:55:38 +0200
Subject: [PATCH] Preserve already configured options in openldap conf

We should respect already configured options present in
/etc/openldap/ldap.conf when generating our own configuration.

With this patch, we only rewrite URI, BASE and TLS_CACERT options
only if they are not configured. In the case they are, our suggested
configuration is inserted as a comment.

Also adds tab as a delimeter character in /etc/openldap/ldap.conf

https://fedorahosted.org/freeipa/ticket/3582
---
 ipa-client/ipa-install/ipa-client-install | 62 ++++++++++++++++++++++++++-----
 ipa-client/ipaclient/ipachangeconf.py     | 14 ++++++-
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 29adc93f3bcb3ccc81c31237af314af0ba61b8c9..d5b16dad806ab9b58921264c560c7dcab868235b 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -815,19 +815,61 @@ def configure_nslcd_conf(fstore, cli_basedn, cli_realm, cli_domain, cli_server,
 
 def configure_openldap_conf(fstore, cli_basedn, cli_server):
     ldapconf = ipaclient.ipachangeconf.IPAChangeConf("IPA Installer")
-    ldapconf.setOptionAssignment(" ")
+    ldapconf.setOptionAssignment((" ", "\t"))
 
-    opts = [{'name':'comment', 'type':'comment', 'value':'File modified by ipa-client-install'},
+    opts = [{'name':'comment', 'type':'comment',
+                'value':' File modified by ipa-client-install'},
             {'name':'empty', 'type':'empty'},
-            {'name':'URI', 'type':'option', 'value':'ldaps://'+  cli_server[0]},
-            {'name':'BASE', 'type':'option', 'value':cli_basedn},
-            {'name':'TLS_CACERT', 'type':'option', 'value':CACERT},
-            {'name':'empty', 'type':'empty'}]
+            {'name':'comment', 'type':'comment',
+                'value':' We do not want to break your existing configuration, '
+                        'hence:'},
+            # this needs to be kept updated if we change more options
+            {'name':'comment', 'type':'comment',
+                'value':'   URI, BASE and TLS_CACERT have been added if they '
+                        'were not set.'},
+            {'name':'comment', 'type':'comment',
+                'value':'   In case any of them were set, a comment with '
+                         'trailing note'},
+            {'name':'comment', 'type':'comment',
+                'value':'   "# modified by IPA" note has been inserted.'},
+            {'name':'comment', 'type':'comment',
+                'value':' To use IPA server with openLDAP tools, please comment '
+                        'out your'},
+            {'name':'comment', 'type':'comment',
+                'value':' existing configuration for these options and '
+                        'uncomment the'},
+            {'name':'comment', 'type':'comment',
+                'value':' corresponding lines generated by IPA.'},
+            {'name':'empty', 'type':'empty'},
+            {'name':'empty', 'type':'empty'},
+            {'action':'addifnotset', 'name':'URI', 'type':'option',
+                'value':'ldaps://'+  cli_server[0]},
+            {'action':'addifnotset', 'name':'BASE', 'type':'option',
+                'value':str(cli_basedn)},
+            {'action':'addifnotset', 'name':'TLS_CACERT', 'type':'option',
+                'value':CACERT},]
 
     target_fname = '/etc/openldap/ldap.conf'
     fstore.backup_file(target_fname)
-    ldapconf.newConf(target_fname, opts)
+
+    error_msg = "Configuring {path} failed with: {err}"
+
+    try:
+        ldapconf.changeConf(target_fname, opts)
+    except SyntaxError, e:
+        root_logger.info("Could not parse {path}".format(path=target_fname))
+        root_logger.debug(error_msg.format(path=target_fname, err=str(e)))
+        return False
+    except IOError,e :
+        root_logger.info("{path} does not exist.".format(path=target_fname))
+        root_logger.debug(error_msg.format(path=target_fname, err=str(e)))
+        return False
+    except Exception, e: #  we do not want to fail in an optional step
+        root_logger.debug(error_msg.format(path=target_fname, err=str(e)))
+        return False
+
     os.chmod(target_fname, 0644)
+    return True
 
 def hardcode_ldap_server(cli_server):
     """
@@ -2373,8 +2415,10 @@ def install(options, env, fstore, statestore):
                         "%s configured using configuration file(s) %s",
                         conf, filenames)
 
-        configure_openldap_conf(fstore, cli_basedn, cli_server)
-        root_logger.info("Configured /etc/openldap/ldap.conf")
+        if configure_openldap_conf(fstore, cli_basedn, cli_server):
+            root_logger.info("Configured /etc/openldap/ldap.conf")
+        else:
+            root_logger.info("Failed to configure /etc/openldap/ldap.conf")
 
         #Check that nss is working properly
         if not options.on_master:
diff --git a/ipa-client/ipaclient/ipachangeconf.py b/ipa-client/ipaclient/ipachangeconf.py
index bdc5579fccd82193e837b5e86cbc694c2619317c..e802e177efcb0ff997f47f4f391c6787849f0c3f 100644
--- a/ipa-client/ipaclient/ipachangeconf.py
+++ b/ipa-client/ipaclient/ipachangeconf.py
@@ -338,7 +338,16 @@ class IPAChangeConf:
                 if no['action'] == 'set':
                     opts.append(no)
                     continue
-                raise SyntaxError('Unknown action: [%s]' % o['action'])
+                if no['action'] == 'addifnotset':
+                    opts.append({'name': 'comment', 'type': 'comment',
+                                'value': self._dump_line(no['name'],
+                                                         self.dassign,
+                                                         no['value'],
+                                                         u' # modified by IPA'
+                                                         )})
+                    opts.append(o)
+                    continue
+                raise SyntaxError('Unknown action: [%s]' % no['action'])
 
             raise SyntaxError('Unknown type: [%s]' % o['type'])
 
@@ -365,7 +374,7 @@ class IPAChangeConf:
             if no['type'] == "option":
                 (num, o) = self.findOpts(opts, no['type'], no['name'], True)
                 if not o:
-                    if no['action'] == 'set':
+                    if no['action'] == 'set' or no['action'] == 'addifnotset':
                         opts.append(no)
                     continue
                 cline = num + 1
@@ -385,6 +394,7 @@ class IPAChangeConf:
         #  the options as indicated by the contents of newopts
         #Second we fill in the new opts tree with options as indicated
         #  in the newopts tree (this is becaus eentire (sub)sections may
+        #  in the newopts tree (this is becaus entire (sub)sections may
         #  exist in the newopts that do not exist in oldopts)
 
         opts = self.mergeOld(oldopts, newopts)
-- 
1.8.1.4

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to