Re: [Freeipa-devel] DN patch and documentation
On 08/09/2012 03:34 PM, Rich Megginson wrote: On 08/09/2012 01:31 PM, John Dennis wrote: On 08/09/2012 02:08 PM, Rob Crittenden wrote: What is the significance of L here: '2.16.840.1.113730.3.8.L.8' : DN, # ipaTemplateRef These came from: install/share/60policyv2.ldif I didn't notice the .L in the OID which isn't legal (correct?). So beats me, I can't explain why the OID is specified that way. hmm - did you see this comment at the top of the file? # Policy related schema. # This file should not be loaded. # Remove this comment and assign right OIDs when time comes to do something # about this functionality. Ah, good catch Rich! No I didn't see that comment. What I did was grep'ed for all dn syntax attributes in all our ldiff files so I didn't see the comment. I'll remove these as they are obviously incorrect. Thank you both Rob and Rich for the sharp eyes. -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] DN patch and documentation
On 08/09/2012 02:08 PM, Rob Crittenden wrote:
I've been going through the diffs and have some questions in ldap2.py,
these are primarily pedantic:
Some of your questions can be answered by:
http://jdennis.fedorapeople.org/dn_summary.html
What is the significance of L here:
'2.16.840.1.113730.3.8.L.8' : DN, # ipaTemplateRef
These came from:
install/share/60policyv2.ldif
I didn't notice the .L in the OID which isn't legal (correct?). So beats
me, I can't explain why the OID is specified that way.
There are quite a few assert isinstance(dn, DN). I assume this is mainly
meant for developers, we aren't expecting to handle that gracefully at
runtime?
Correct. They are there to prevent future use of dn strings by
developers whose habits die hard. The goal is 100% DN usage 100% of the
time.
If we allow strings some of the time we're on a slippery slope. Think of
it as type enforcement meant to protect ourselves.
The assertions also proved valuable in finding a number of places where
functions failed to return values or correct values. This showed up a
lot in the pre and post callbacks whose signature specifies a return
value but the developer forgot to return a value. Apparently pylint does
not pick these things up.
In production we should disable assertions, we should open a ticket to
disable assertions in production.
It seems to me that allowing a DN passed in as a string would be nice in
some cases. Calling bind, for example, looks very awkward and isn't as
readable as cn=directory manager, IMHO. What is the downside of having a
converter for these?
See the above documentation for the rationale. In particular the section
called "Why did I use tuples instead of strings when initializing DN's?"
search_ext() has an extra embedded line which makes the function a bit
confusing to read.
O.K. you lost me on that one :-)
Do we need to keep _debug_log_ldap?
I don't think it hurts. I found it very useful to see the actual LDAP
data when debugging.
In search_ext_s() (and a few others) should we just return
self.convert_result() directly?
Petr asked this question too. I don't have strong feelings either way.
The reason it's stored in another variable is it's easy to add a print
statement or stop in the debugger and examine it when need be. It also
makes it clear it's the IPA formatted results. I don't think there is
much cost to using the variable. I'm not attached to it, it can be changed.
In ldap2::__init__ what would raise an AttributeError and would we want
to hide that fact with an empty base_dn? Is this attempting to allow the
use of ldap2.ldap2 in cases where the api is not initialized?
Beats me, that's been in the code for a long time. An empty DN is the
same as an empty string. Maybe we should set it to None instead so we
know base_dn was never initialized properly.
In ipaserver/ipaldap.py there is a continuance of the assertions, some
of which don't seem to be needed. For example, in toDict() it shouldn't
be possible to create an Entry object without having self.dn be a DN
object, so is this assertion necessary?
Many objects now enforce DN's for their dn attribute. A dn attribute may
only ever be None or an instance of a DN. This is implemented with
dn = ipautil.dn_attribute_property('_dn')
In objects which define their dn property this way an assert
isinstance(self.dn, DN) is not necessary because the dn property
enforces it. So you're correct, those particular asserts could be
removed. They were added before dn type enforcement via object property
was added. I could go through and clean those up, but perhaps we should
open a separate ticket for that.
--
John Dennis
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] DN patch and documentation
On 08/09/2012 01:31 PM, John Dennis wrote:
On 08/09/2012 02:08 PM, Rob Crittenden wrote:
I've been going through the diffs and have some questions in ldap2.py,
these are primarily pedantic:
Some of your questions can be answered by:
http://jdennis.fedorapeople.org/dn_summary.html
What is the significance of L here:
'2.16.840.1.113730.3.8.L.8' : DN, # ipaTemplateRef
These came from:
install/share/60policyv2.ldif
I didn't notice the .L in the OID which isn't legal (correct?). So
beats me, I can't explain why the OID is specified that way.
hmm - did you see this comment at the top of the file?
# Policy related schema.
# This file should not be loaded.
# Remove this comment and assign right OIDs when time comes to do something
# about this functionality.
There are quite a few assert isinstance(dn, DN). I assume this is mainly
meant for developers, we aren't expecting to handle that gracefully at
runtime?
Correct. They are there to prevent future use of dn strings by
developers whose habits die hard. The goal is 100% DN usage 100% of
the time.
If we allow strings some of the time we're on a slippery slope. Think
of it as type enforcement meant to protect ourselves.
The assertions also proved valuable in finding a number of places
where functions failed to return values or correct values. This showed
up a lot in the pre and post callbacks whose signature specifies a
return value but the developer forgot to return a value. Apparently
pylint does not pick these things up.
In production we should disable assertions, we should open a ticket to
disable assertions in production.
It seems to me that allowing a DN passed in as a string would be nice in
some cases. Calling bind, for example, looks very awkward and isn't as
readable as cn=directory manager, IMHO. What is the downside of having a
converter for these?
See the above documentation for the rationale. In particular the
section called "Why did I use tuples instead of strings when
initializing DN's?"
search_ext() has an extra embedded line which makes the function a bit
confusing to read.
O.K. you lost me on that one :-)
Do we need to keep _debug_log_ldap?
I don't think it hurts. I found it very useful to see the actual LDAP
data when debugging.
In search_ext_s() (and a few others) should we just return
self.convert_result() directly?
Petr asked this question too. I don't have strong feelings either way.
The reason it's stored in another variable is it's easy to add a print
statement or stop in the debugger and examine it when need be. It also
makes it clear it's the IPA formatted results. I don't think there is
much cost to using the variable. I'm not attached to it, it can be
changed.
In ldap2::__init__ what would raise an AttributeError and would we want
to hide that fact with an empty base_dn? Is this attempting to allow the
use of ldap2.ldap2 in cases where the api is not initialized?
Beats me, that's been in the code for a long time. An empty DN is the
same as an empty string. Maybe we should set it to None instead so we
know base_dn was never initialized properly.
In ipaserver/ipaldap.py there is a continuance of the assertions, some
of which don't seem to be needed. For example, in toDict() it shouldn't
be possible to create an Entry object without having self.dn be a DN
object, so is this assertion necessary?
Many objects now enforce DN's for their dn attribute. A dn attribute
may only ever be None or an instance of a DN. This is implemented with
dn = ipautil.dn_attribute_property('_dn')
In objects which define their dn property this way an assert
isinstance(self.dn, DN) is not necessary because the dn property
enforces it. So you're correct, those particular asserts could be
removed. They were added before dn type enforcement via object
property was added. I could go through and clean those up, but perhaps
we should open a separate ticket for that.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] DN patch and documentation
Martin Kosek wrote: On 08/08/2012 11:02 PM, John Dennis wrote: All the issues Martin discovered (except for the ip-address parameter) are now fixed and pushed to the dn repo. Also now the dn repo is fully rebased against master (except for one commit for ticket 2954 which I had to revert, see ticket for details). Thank you for the continued testing. FYI: to use the dn repo: git clone git://fedorapeople.org/~jdennis/freeipa.dn.git git checkout dn Good. I see that all issues except the ipa-replica-prepare are now fixed. I verified that this is indeed an issue caused by the DN refactoring, I am attaching a patch fixing the issue. With this patch, ipa-replica-prepare issue disappears. Petr Vobornik also noticed an issue with trust-show command. I am attaching a patch with fix as well. We push the patches when we are pushing your work. I have not found any more show-stopping issues, so I will just continue my testing, and also give space to other testers in case they discover something else. Martin I've been going through the diffs and have some questions in ldap2.py, these are primarily pedantic: What is the significance of L here: '2.16.840.1.113730.3.8.L.8' : DN, # ipaTemplateRef There are quite a few assert isinstance(dn, DN). I assume this is mainly meant for developers, we aren't expecting to handle that gracefully at runtime? It seems to me that allowing a DN passed in as a string would be nice in some cases. Calling bind, for example, looks very awkward and isn't as readable as cn=directory manager, IMHO. What is the downside of having a converter for these? search_ext() has an extra embedded line which makes the function a bit confusing to read. Do we need to keep _debug_log_ldap? In search_ext_s() (and a few others) should we just return self.convert_result() directly? In ldap2::__init__ what would raise an AttributeError and would we want to hide that fact with an empty base_dn? Is this attempting to allow the use of ldap2.ldap2 in cases where the api is not initialized? In ipaserver/ipaldap.py there is a continuance of the assertions, some of which don't seem to be needed. For example, in toDict() it shouldn't be possible to create an Entry object without having self.dn be a DN object, so is this assertion necessary? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] DN patch and documentation
On 08/08/2012 11:02 PM, John Dennis wrote:
> All the issues Martin discovered (except for the ip-address parameter) are now
> fixed and pushed to the dn repo. Also now the dn repo is fully rebased against
> master (except for one commit for ticket 2954 which I had to revert, see
> ticket
> for details).
>
> Thank you for the continued testing.
>
> FYI: to use the dn repo:
>
> git clone git://fedorapeople.org/~jdennis/freeipa.dn.git
> git checkout dn
>
Good. I see that all issues except the ipa-replica-prepare are now fixed. I
verified that this is indeed an issue caused by the DN refactoring, I am
attaching a patch fixing the issue. With this patch, ipa-replica-prepare issue
disappears.
Petr Vobornik also noticed an issue with trust-show command. I am attaching a
patch with fix as well. We push the patches when we are pushing your work.
I have not found any more show-stopping issues, so I will just continue my
testing, and also give space to other testers in case they discover something
else.
Martin
>From 693366a012179dc393687d0bd6b7e39ea967265c Mon Sep 17 00:00:00 2001
From: Martin Kosek
Date: Thu, 9 Aug 2012 07:01:04 -0400
Subject: [PATCH 1/2] Fix DN usage in adtrust-show command
---
ipalib/plugins/trust.py |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ipalib/plugins/trust.py b/ipalib/plugins/trust.py
index 87c40c861fd6a91151863769811eecdcd9122c42..b2facd8eb5cb6003ce4c542a4853dbd52277f469 100644
--- a/ipalib/plugins/trust.py
+++ b/ipalib/plugins/trust.py
@@ -109,7 +109,7 @@ class trust(LDAPObject):
def make_trust_dn(env, trust_type, dn):
if trust_type in trust.trust_types:
container_dn = DN(('cn', trust_type), env.container_trusts, env.basedn)
-return unicode(DN(DN(dn)[0], container_dn))
+return DN(DN(dn)[0], container_dn)
return dn
class trust_add(LDAPCreate):
--
1.7.10.4
>From 3ac25f77a7b98b32117a3eda78c56db7e9b8d711 Mon Sep 17 00:00:00 2001
From: Martin Kosek
Date: Thu, 9 Aug 2012 08:34:32 -0400
Subject: [PATCH 2/2] Do not crash in ipa-replica-prepare DNS phase
ipa-replica-prepare was not able to create a replica info file
when DNS records were required. The problem was in incorrect
extraction of master's FQDN in get_dns_masters function in DNS
plugin and incorrect ip_address unicode conversion in bindinstance.
Both issues were fixed.
---
ipalib/plugins/dns.py |2 +-
ipaserver/install/bindinstance.py | 10 --
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index e70b36c29abcc422ac15e734ca0ad549b2a07e68..991eb0164710c9023a6e7f13f18447a70de3c66b 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2067,7 +2067,7 @@ class dnsrecord(LDAPObject):
master_dn = entry[0]
assert isinstance(master_dn, DN)
try:
-master = master_dn[0]['cn']
+master = master_dn[1]['cn']
dns_masters.append(master)
except (IndexError, KeyError):
pass
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index b93a32f53e91fe9a724bc20ee30bc924aed9fcd4..2e00f70b1e5530e1b243a2df6e1214bea30eb1ba 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -291,11 +291,14 @@ def add_zone(name, zonemgr=None, dns_backup=None, ns_hostname=None, ns_ip_addres
ns_main = ns_hostname
ns_replicas = []
+if ns_ip_address is not None:
+ns_ip_address = unicode(ns_ip_address)
+
try:
api.Command.dnszone_add(unicode(name),
idnssoamname=unicode(ns_main+'.'),
idnssoarname=unicode(zonemgr),
-ip_address=unicode(ns_ip_address),
+ip_address=ns_ip_address,
idnsallowdynupdate=True,
idnsupdatepolicy=unicode(update_policy),
idnsallowquery=u'any',
@@ -332,11 +335,14 @@ def add_reverse_zone(zone, ns_hostname=None, ns_ip_address=None,
ns_main = ns_hostname
ns_replicas = []
+if ns_ip_address is not None:
+ns_ip_address = unicode(ns_ip_address)
+
try:
api.Command.dnszone_add(unicode(zone),
idnssoamname=unicode(ns_main+'.'),
idnsallowdynupdate=True,
-ip_address=unicode(ns_ip_address),
+ip_address=ns_ip_address,
idnsupdatepolicy=unicode(update_policy),
idnsallowquery=u'any',
idnsallowtransfer=u'none',)
--
1.7.10.4
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/list
