Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01.03.2016 14:50, Martin Babinsky wrote:
On 02/29/2016 05:37 PM, thierry bordaz wrote:
On 02/26/2016 05:48 PM, Martin Babinsky wrote:
On 02/26/2016 04:24 PM, thierry bordaz wrote:
On 02/25/2016 07:17 PM, thierry bordaz wrote:
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:
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/29/2016 05:37 PM, thierry bordaz wrote:
On 02/26/2016 05:48 PM, Martin Babinsky wrote:
On 02/26/2016 04:24 PM, thierry bordaz wrote:
On 02/25/2016 07:17 PM, thierry bordaz wrote:
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/26/2016 05:48 PM, Martin Babinsky wrote:
On 02/26/2016 04:24 PM, thierry bordaz wrote:
On 02/25/2016 07:17 PM, thierry bordaz wrote:
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/26/2016 04:24 PM, thierry bordaz wrote:
On 02/25/2016 07:17 PM, thierry bordaz wrote:
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.)
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/25/2016 07:17 PM, thierry bordaz wrote:
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/25/2016 01:56 PM, Martin Babinsky wrote:
On 02/25/2016 12:17 PM, thierry bordaz wrote:
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.
8-)
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 02/25/2016 12:17 PM, thierry bordaz wrote:
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.
8-)
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"
I did it and fixed most of them but what
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
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.
8-)
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"
I did it and fixed most of them but what remains is long line. I fixed
some of them
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
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
From 391e597b4de93897fb496cc06552cf9e434af3d5 Mon Sep 17 00:00:00 2001
From: Thierry Bordaz
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 03:46 PM, Martin Kosek wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:
Thanks! Couple comments:
I miss ticket number of description.
Thanks Martin for looking at it.
Ouch... the ticket number is https://fedorahosted.org/freeipa/ticket/4026
Does this patch mean that all blocker on DS side preventing remote DNA were
fixed? If yes, it may be worth updating Requires in the spec file in that case
and making sure the backport is in Fedora 23+ or at least FreeIPA 4.3 COPR repo.
It required following 389-ds tickets that are now both fixed in 1.3.4.6
* https://fedorahosted.org/389/ticket/47779
* https://fedorahosted.org/389/ticket/48362
Is it the change to do in the spec file ?
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 899af6c..87e80c9 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
--
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 04:22 PM, thierry bordaz wrote:
> On 01/21/2016 03:46 PM, Martin Kosek wrote:
>> On 01/21/2016 01:37 PM, thierry bordaz wrote:
>> Thanks! Couple comments:
>>
>> I miss ticket number of description.
>
> Thanks Martin for looking at it.
>
> Ouch... the ticket number is https://fedorahosted.org/freeipa/ticket/4026
>>
>> Does this patch mean that all blocker on DS side preventing remote DNA were
>> fixed? If yes, it may be worth updating Requires in the spec file in that
>> case
>> and making sure the backport is in Fedora 23+ or at least FreeIPA 4.3 COPR
>> repo.
> It required following 389-ds tickets that are now both fixed in 1.3.4.6
>
> * https://fedorahosted.org/389/ticket/47779
> * https://fedorahosted.org/389/ticket/48362
>
> Is it the change to do in the spec file ?
>
> diff --git a/freeipa.spec.in b/freeipa.spec.in
> index 899af6c..87e80c9 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
That spec file change should be OK, thanks. I did not do other testing, I would
leave that up to other developers.
--
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 01:37 PM, thierry bordaz wrote: > Thanks! Couple comments: I miss ticket number of description. Does this patch mean that all blocker on DS side preventing remote DNA were fixed? If yes, it may be worth updating Requires in the spec file in that case and making sure the backport is in Fedora 23+ or at least FreeIPA 4.3 COPR repo. -- 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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 04:23 PM, Martin Kosek wrote:
On 01/21/2016 04:22 PM, thierry bordaz wrote:
On 01/21/2016 03:46 PM, Martin Kosek wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:
Thanks! Couple comments:
I miss ticket number of description.
Thanks Martin for looking at it.
Ouch... the ticket number is https://fedorahosted.org/freeipa/ticket/4026
Does this patch mean that all blocker on DS side preventing remote DNA were
fixed? If yes, it may be worth updating Requires in the spec file in that case
and making sure the backport is in Fedora 23+ or at least FreeIPA 4.3 COPR repo.
It required following 389-ds tickets that are now both fixed in 1.3.4.6
* https://fedorahosted.org/389/ticket/47779
* https://fedorahosted.org/389/ticket/48362
Is it the change to do in the spec file ?
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 899af6c..87e80c9 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
That spec file change should be OK, thanks. I did not do other testing, I would
leave that up to other developers.
Thanks for the review. Here the second fix
thierry
From 1682af37327d46205ac3b208b11066f0953a10ef Mon Sep 17 00:00:00 2001
From: Thierry Bordaz
Date: Wed, 20 Jan 2016 15:31: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/server/install.py| 4 ++
ipaserver/install/server/replicainstall.py | 8
ipaserver/install/service.py | 63 ++
ipaserver/install/upgradeinstance.py | 6 ++-
5 files changed, 81 insertions(+), 4 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 899af6c922c0c821d190e3f1c74563b6dede5c03..87e80c933b96dcd4df56696546115f14695c 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/server/install.py b/ipaserver/install/server/install.py
index 49e97eb667a322898acc3a064f4eae5381ded918..64994f3ea676d76e9c58c57fdbbf397711424375 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -955,6 +955,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 41d14f5801ba1cefaca7a27f78d64f45017805a2..1d4e6a23ed443af9963194909d7819076d07d855 100644
--- a/ipaserver/install/server/replicainstall.py
+++ b/ipaserver/install/server/replicainstall.py
@@ -809,6 +809,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()
@@ -1375,6 +1379,10 @@ def promote(installer):
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 05:38 PM, Martin Babinsky wrote:
On 01/21/2016 05:22 PM, Rob Crittenden wrote:
Martin Babinsky wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:
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.
I agree, but note that it is a 60s timeout (30 tries x 2 second sleeps).
Right, looks like I forgot how to math.
I was thinking that this loop could be rewritten in a safer way like
this:
"""
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("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
time.sleep(2)
else:
raise RuntimeError("Could not get dnaHostname entries in
{} seconds".format(max_wait * 2))
"""
the else: branch will be executed only after the iterator (range in
this case because Py3 compatibility) is exhausted thus handling the
timeout.
Hello Martin,
Thanks for this very nice code !
I was a bit afraid of aborting a full install if it was not able to
update the localhost shared config, this is why it just logged a
message. Finally, raising an exception is the thing to do.
This will blow up if something other than NotFound is returned (e.g.
connection error), and maybe that's ok.
Other exceptions do not worry me, only repeated NotFound due to some
bad magic preventing the entries to appear which can lead to unhandled
timeout.
The continue is not needed.
rob
--
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
On 01/21/2016 05:22 PM, Rob Crittenden wrote:
Martin Babinsky wrote:
On 01/21/2016 01:37 PM, thierry bordaz wrote:
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.
I agree, but note that it is a 60s timeout (30 tries x 2 second sleeps).
Right, looks like I forgot how to math.
I was thinking that this loop could be rewritten in a safer way like this:
"""
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("So far enable not find DNA shared
config entry for dnaHostname=%s under %s. Retry in 2sec" % (self.fqdn,
sharedcfgdn))
time.sleep(2)
else:
raise RuntimeError("Could not get dnaHostname entries in {}
seconds".format(max_wait * 2))
"""
the else: branch will be executed only after the iterator (range in this
case because Py3 compatibility) is exhausted thus handling the timeout.
This will blow up if something other than NotFound is returned (e.g.
connection error), and maybe that's ok.
Other exceptions do not worry me, only repeated NotFound due to some bad
magic preventing the entries to appear which can lead to unhandled timeout.
The continue is not needed.
rob
--
Martin^3 Babinsky
--
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
Martin Babinsky wrote:
> On 01/21/2016 01:37 PM, thierry bordaz wrote:
> 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.
I agree, but note that it is a 60s timeout (30 tries x 2 second sleeps).
This will blow up if something other than NotFound is returned (e.g.
connection error), and maybe that's ok.
The continue is not needed.
rob
--
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
Re: [Freeipa-devel] [PATCH] 0017 configure DNA shared config entry to allow connection with GSSAPI
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".
--
Martin^3 Babinsky
--
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
