On 02/25/2016 12:03 PM, Martin Babinsky wrote:
On 02/24/2016 04:30 PM, thierry bordaz wrote:
On 01/21/2016 05:04 PM, Martin Babinsky wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:
Hi Thierry,
I have couple of comments to your patch:
1.)
there is a number of PEP8 errors in the patch
(http://paste.fedoraproject.org/313246/33893701), please fix them.
See http://www.freeipa.org/page/Python_Coding_Style for our
conventions used in Python code.
2.)
+ DNA_BIND_METHOD = "dnaRemoteBindMethod"
+ DNA_CONN_PROTOCOL = "dnaRemoteConnProtocol"
+ DNA_PLUGIN_DN = 'cn=Distributed Numeric Assignment
Plugin,cn=plugins,cn=config'
+ dna_config_base = 'cn=Posix IDs,%s' % DNA_PLUGIN_DN
Uppercase names are usually reserved for module-level constants. OTOH,
local variables should be lowercase. Also you can instantiate
dna_config_base directly as DN, using 2-member tuples, i. e:
"""
dna_config_base = DN(('cn', 'posix IDs'), ('cn', 'Distributed Numeric
Assignment Plugin'), ('cn', 'plugins'), ('cn', 'config'))
"""
When passing DN object to the formatting functions/operators, it is
automatically converted to string so no need to hold string and DN
object separately. This is done in other places (see function/methods
in replication.py).
3.)
+ for i in range(len(entries)) :
+
+ mod = []
+ if entries[i].single_value.get(DNA_BIND_METHOD) != method:
+ mod.append((ldap.MOD_REPLACE, DNA_BIND_METHOD,
method))
+
+ if entries[i].single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+ mod.append((ldap.MOD_REPLACE, DNA_CONN_PROTOCOL,
protocol))
please use idiomatic Python when handling list of entries, e.g.:
"""
for entry in entries:
mod = []
if entry.single_value.get(DNA_BIND_METHOD) != method:
...
"""
4.) I think that this method should in DSInstance class since it deals
with directory server configuration. Service is a parent object of all
other service installers/configurators and should contain only methods
common to more children.
5.) Since the method is called from every installer, it could be
beneficial to call it in DSInstance.__common_post_setup() as a part of
Directory server installation. Is there any reason why this is not the
case?
6.)
+ while attempt != MAX_WAIT:
+ try:
+ entries = conn.get_entries(sharedcfgdn,
scope=ldap.SCOPE_ONELEVEL, filter='dnaHostname=%s' % self.fqdn)
+ break
+ except errors.NotFound:
+ root_logger.debug("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
+ attempt = attempt + 1
+ time.sleep(2)
+ continue
+
+ # safety checking
+ # there is no return, if there are several entries, as a
workaround of #5510
+ if len(entries) != 1:
I am quite afraid what would happen if the server does not return any
entries until 30 s timeout. The code will then continue to the
condition which can potentially test an uninitialized variable and
blow up with 'NameError'. This should be handled more robustly, e. g.
raise an exception when a timeout is reached and no entries were
returned.
7.)
+ if len(mod) > 0:
A Pythonic way to test for non-empty container is
"""
if mods:
# do stuff
"""
since an empty list/dict/set evaluates to False and non-empty
containers are True.
8.)
+ entry = conn.get_entry(entries[i].dn)
+ if entry.single_value.get(DNA_BIND_METHOD) != method:
+ root_logger.error("Fail to set SASL/GSSAPI bind
method to %s" % (entries[i].dn))
+ if entry.single_value.get(DNA_CONN_PROTOCOL) !=
protocol:
+ root_logger.error("Fail to set LDAP protocol to
%s" % (entries[i].dn))
rather than re-fetching the modified entry and testing what happened,
you can just catch an exception raised by unsuccessfull mod and log an
error like this:
"""
try:
conn.modify_s(entry.dn, mod)
except Exception as e:
root_logger.error("Failed to modify entry {}: {}".format(entry, e))
"""
as a matter of fact, if the modify_s operation would fail for some
reason, an ldap exception would be raised and you checks would not
even be executed.
9.)
The debug message on line 219 should read "Unable to find DNA shared
config entry for dnaHostname=%s so far. Retry in 2 sec.". The errors
at the end of the method should have "Failed" instead of "Fail".
Hi Martin,
Finally tested... here is the updated patch. Thanks for you patience
thanks
thierry
Hi Thierry,
the patch works as expected. I have some more nitpicks though:
1.) Please fix the following pep8 errors:
http://paste.fedoraproject.org/329086/56397841/
you can check whether you recent commit conforms to PEP8 by running
"git show -U0 | pep8 --diff"
2.)
+ self.step("update DNA shared config entry",
self.update_dna_shared_config)
I would rather change the message to "Updating DNA shared config
entry" since all other messages use continuous tense.
3.)
+ else:
+ raise RuntimeError("Could not get dnaHostname entries
in {} seconds".format(max_wait * 2))
Please use root_logger.error() and then return as is used elsewhere in
the method. We do not want a runaway exception crashing upgrade.
Hi Martin,
Updated/tested the patch with your help/recommendations. pep8 is clear
now :-)
thanks
thierry
From 2d8087b3ffca056c066ee867c06cd35a4cc964a6 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz <tbor...@redhat.com>
Date: Mon, 8 Feb 2016 16:14:58 +0100
Subject: [PATCH] configure DNA plugin shared config entries to allow
connection with GSSAPI
https://fedorahosted.org/freeipa/ticket/4026
When a replica needs to extend its DNA range, it selects the remote replica with the
larger available range. If there is no replica agreement to that remote replica,
the shared config entry needs to contain the connection method/protocol.
This fix requires 389-ds
* https://fedorahosted.org/389/ticket/47779
* https://fedorahosted.org/389/ticket/48362
That are both fixed in 1.3.4.6
---
freeipa.spec.in | 4 +-
ipaserver/install/dsinstance.py | 106 +++++++++++++++++++++++++++++
ipaserver/install/server/install.py | 4 ++
ipaserver/install/server/replicainstall.py | 8 +++
ipaserver/install/server/upgrade.py | 1 +
5 files changed, 121 insertions(+), 2 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 54a11bf..926577b 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -130,7 +130,7 @@ Requires: %{name}-client = %{version}-%{release}
Requires: %{name}-admintools = %{version}-%{release}
Requires: %{name}-common = %{version}-%{release}
Requires: python2-ipaserver = %{version}-%{release}
-Requires: 389-ds-base >= 1.3.4.4
+Requires: 389-ds-base >= 1.3.4.6
Requires: openldap-clients > 2.4.35-4
Requires: nss >= 3.14.3-12.0
Requires: nss-tools >= 3.14.3-12.0
@@ -162,7 +162,7 @@ Requires: zip
Requires: policycoreutils >= 2.1.12-5
Requires: tar
Requires(pre): certmonger >= 0.78
-Requires(pre): 389-ds-base >= 1.3.4.4
+Requires(pre): 389-ds-base >= 1.3.4.6
Requires: fontawesome-fonts
Requires: open-sans-fonts
Requires: openssl
diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 2905c31..0aae861 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -290,6 +290,8 @@ class DsInstance(service.Service):
self.step("tuning directory server", self.__tuning)
self.step("configuring directory to start on boot", self.__enable)
+ self.step("Updating DNA shared config entry",
+ self.update_dna_shared_config)
def init_info(self, realm_name, fqdn, domain_name, dm_password,
subject_base, idstart, idmax, pkcs12_info, ca_file=None):
@@ -1248,3 +1250,107 @@ class DsInstance(service.Service):
# check for open secure port 636 from now on
self.open_ports.append(636)
+
+ def update_dna_shared_config(self, method="SASL/GSSAPI", protocol="LDAP"):
+
+ dna_bind_method = "dnaRemoteBindMethod"
+ dna_conn_protocol = "dnaRemoteConnProtocol"
+ dna_plugin = DN(('cn', 'Distributed Numeric Assignment Plugin'),
+ ('cn', 'plugins'),
+ ('cn', 'config'))
+ dna_config_base = DN(('cn', 'posix IDs'), dna_plugin)
+
+ if not self.admin_conn:
+ self.ldap_connect()
+ conn = self.admin_conn
+
+ # Check the plugin is enabled else it is useless to update
+ # the shared entry
+ try:
+ entry = conn.get_entry(dna_plugin)
+ if entry.single_value.get('nsslapd-pluginenabled') == 'off':
+ return
+ except errors.NotFound:
+ root_logger.error("Could not find DNA plugin entry: %s" %
+ dna_config_base)
+ return
+
+ try:
+ entry = conn.get_entry(dna_config_base)
+ except errors.NotFound:
+ root_logger.error("Could not find DNA config entry: %s" %
+ dna_config_base)
+ return
+
+ sharedcfgdn = entry.single_value.get("dnaSharedCfgDN")
+ if sharedcfgdn is not None:
+ sharedcfgdn = DN(sharedcfgdn)
+ else:
+ root_logger.error(
+ "Could not find DNA shared config DN in entry: %s" %
+ dna_config_base)
+ return
+
+ #
+ # Update the shared config entry related to that host
+ #
+ # If the shared config entry already exists (like upgrade)
+ # the update occurs immediately without sleep.
+ #
+ # If the shared config entry does not exist (fresh install)
+ # DS server waits for 30s after its startup to create it.
+ # Startup likely occurred few sec before this function is
+ # called so this loop will wait for 30s max.
+ #
+ # In case the server is not able to create the entry
+ # The loop gives a grace period of 60s before logging
+ # the failure to update the shared config entry and return
+ #
+ max_wait = 30
+ for i in range(0, max_wait + 1):
+ try:
+ entries = conn.get_entries(
+ sharedcfgdn, scope=ldap.SCOPE_ONELEVEL,
+ filter='dnaHostname=%s' % self.fqdn
+ )
+ break
+ except errors.NotFound:
+ root_logger.debug(
+ "Unable to find DNA shared config entry for "
+ "dnaHostname=%s (under %s) so far. Retry in 2 sec." %
+ (self.fqdn, sharedcfgdn)
+ )
+ time.sleep(2)
+ else:
+ root_logger.error(
+ "Could not get dnaHostname entries in {} seconds".format(
+ max_wait * 2)
+ )
+ return
+
+ # If there are several entries, all of them will be updated
+ # just log a debug msg. This is likely the result of #5510
+ if len(entries) != 1:
+ root_logger.debug(
+ "%d entries dnaHostname=%s under %s. One expected" %
+ (len(entries), self.fqdn, sharedcfgdn)
+ )
+
+ # time to set the bind method and the protocol in the
+ # shared config entries
+ for entry in entries:
+ mod = []
+ if entry.single_value.get(dna_bind_method) != method:
+ mod.append((ldap.MOD_REPLACE, dna_bind_method, method))
+
+ if entry.single_value.get(dna_conn_protocol) != method:
+ mod.append((ldap.MOD_REPLACE, dna_conn_protocol, protocol))
+
+ if mod:
+ try:
+ conn.modify_s(entry.dn, mod)
+ except Exception as e:
+ root_logger.error(
+ "Failed to set SASL/GSSAPI bind method/protocol "
+ "in entry {}: {}".format(entry, e)
+ )
diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 362b99f..82802b8 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -987,6 +987,10 @@ def install(installer):
service.print_msg("Restarting the web server")
http.restart()
+ # update DNA shared config entry is done as far as possible
+ # from restart to avoid waiting for its creation
+ ds.update_dna_shared_config()
+
# Set the admin user kerberos password
ds.change_admin_password(admin_password)
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index 3a3bbc0..e3052c1 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -881,6 +881,10 @@ def install(installer):
ds.replica_populate()
+ # update DNA shared config entry is done as far as possible
+ # from restart to avoid waiting for its creation
+ ds.update_dna_shared_config()
+
# Everything installed properly, activate ipa service.
services.knownservices.ipa.enable()
@@ -1457,6 +1461,10 @@ def promote(installer):
ds.replica_populate()
+ # update DNA shared config entry is done as far as possible
+ # from restart to avoid waiting for its creation
+ ds.update_dna_shared_config()
+
custodia.import_dm_password(config.master_host_name)
promote_sssd(config.host_name)
diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 455b28a..866eaf9 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -1524,6 +1524,7 @@ def upgrade_configuration():
ds.ldap_connect()
ds_enable_sidgen_extdom_plugins(ds)
+ ds.update_dna_shared_config()
ds.ldap_disconnect()
# Now 389-ds is available, run the remaining http tasks
--
2.5.0
--
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
