Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/21/2014 01:46 PM, Jan Cholasta wrote: Dne 21.11.2014 v 11:28 Tomas Babej napsal(a): On 11/20/2014 04:01 PM, Jan Cholasta wrote: Dne 19.11.2014 v 15:12 Tomas Babej napsal(a): On 11/19/2014 02:03 PM, Jan Cholasta wrote: Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. Why are you special casing base scope search? Since base search is performed only on the entry specified by the DN, there is no need to differentiate between ContainerNotFound and NotFound. So the logic is as follows: subtree search - ContainerNotFound is raised when container does not exist, NotFound if search provided no results onelevel search - the same base search - NotFound is raised if the specified DN does not exist There is a difference between a search on a non-existent entry and a search on an existent entry with a non-matching filter. This difference exists on LDAP level and applies to all search scopes, not just the base scope. I don't think hiding this difference is useful at all. Remember that this bug exists because we were hiding the difference in the first place. Also, after giving this some thought, I think it would be better to create a new error for the case where there is no match instead of the case where the entry does not exist. NotFound pretty much corresponds to LDAP's NO_SUCH_OBJECT, something like NoMatchingEntry or EmptyResult would fit the no-match result better. Sorry for the delay, I thought this was on review. Attaching the updated patch. -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From f9852aa80e831728ccb76389356a0f5e671cf1f2 Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Wed, 19 Nov 2014 12:00:07 +0100 Subject: [PATCH] baseldap: Handle missing parent objects properly in *-find commands The find_entries function in ipaldap does not differentiate between a LDAP search that returns error code 32 (No such object) and LDAP
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
Dne 13.1.2015 v 16:04 Tomas Babej napsal(a): On 11/21/2014 01:46 PM, Jan Cholasta wrote: Dne 21.11.2014 v 11:28 Tomas Babej napsal(a): On 11/20/2014 04:01 PM, Jan Cholasta wrote: Dne 19.11.2014 v 15:12 Tomas Babej napsal(a): On 11/19/2014 02:03 PM, Jan Cholasta wrote: Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. Why are you special casing base scope search? Since base search is performed only on the entry specified by the DN, there is no need to differentiate between ContainerNotFound and NotFound. So the logic is as follows: subtree search - ContainerNotFound is raised when container does not exist, NotFound if search provided no results onelevel search - the same base search - NotFound is raised if the specified DN does not exist There is a difference between a search on a non-existent entry and a search on an existent entry with a non-matching filter. This difference exists on LDAP level and applies to all search scopes, not just the base scope. I don't think hiding this difference is useful at all. Remember that this bug exists because we were hiding the difference in the first place. Also, after giving this some thought, I think it would be better to create a new error for the case where there is no match instead of the case where the entry does not exist. NotFound pretty much corresponds to LDAP's NO_SUCH_OBJECT, something like NoMatchingEntry or EmptyResult would fit the no-match result better. Sorry for the delay, I thought this was on review. Attaching the updated patch. Thanks, ACK. Pushed to: master: e11e8235ac9af09a587262368ef795cddbdd0e4e ipa-4-1: 44134460b6545b51a17884ce353e556bd8cd753f -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/20/2014 04:01 PM, Jan Cholasta wrote: Dne 19.11.2014 v 15:12 Tomas Babej napsal(a): On 11/19/2014 02:03 PM, Jan Cholasta wrote: Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. Why are you special casing base scope search? Since base search is performed only on the entry specified by the DN, there is no need to differentiate between ContainerNotFound and NotFound. So the logic is as follows: subtree search - ContainerNotFound is raised when container does not exist, NotFound if search provided no results onelevel search - the same base search - NotFound is raised if the specified DN does not exist -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
Dne 21.11.2014 v 11:28 Tomas Babej napsal(a): On 11/20/2014 04:01 PM, Jan Cholasta wrote: Dne 19.11.2014 v 15:12 Tomas Babej napsal(a): On 11/19/2014 02:03 PM, Jan Cholasta wrote: Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. Why are you special casing base scope search? Since base search is performed only on the entry specified by the DN, there is no need to differentiate between ContainerNotFound and NotFound. So the logic is as follows: subtree search - ContainerNotFound is raised when container does not exist, NotFound if search provided no results onelevel search - the same base search - NotFound is raised if the specified DN does not exist There is a difference between a search on a non-existent entry and a search on an existent entry with a non-matching filter. This difference exists on LDAP level and applies to all search scopes, not just the base scope. I don't think hiding this difference is useful at all. Remember that this bug exists because we were hiding the difference in the first place. Also, after giving this some thought, I think it would be better to create a new error for the case where there is no match instead of the case where the entry does not exist. NotFound pretty much corresponds to LDAP's NO_SUCH_OBJECT, something like NoMatchingEntry or EmptyResult would fit the no-match result better. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
Dne 19.11.2014 v 15:12 Tomas Babej napsal(a): On 11/19/2014 02:03 PM, Jan Cholasta wrote: Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. Thanks for the suggestsions. Attached is a new patch which hooks into find_entries method and differentiates between the cases. Why are you special casing base scope search? -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From 7a279fc809f812a6c7a91ed4a54550ea6589f1d3 Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Wed, 19 Nov 2014 12:00:07 +0100 Subject: [PATCH] baseldap: Handle missing parent objects properly in *-find commands When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 --- ipalib/plugins/baseldap.py | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index 375441c0fd55efe70d5a6f5bed6700e618874082..e4cc699ee0be29c184e35b510c7a10c5ff3d5c07 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -1934,7 +1934,11 @@ class LDAPSearch(BaseLDAPCommand, crud.Search): term = args[-1] if self.obj.parent_object: -base_dn = self.api.Object[self.obj.parent_object].get_dn(*args[:-1]) +api_parent_obj = self.api.Object[self.obj.parent_object] +try: +base_dn = api_parent_obj.get_dn_if_exists(*args[:-1]) +except errors.NotFound: +api_parent_obj.handle_not_found(*args[:-1]) else: base_dn = DN(self.obj.container_dn, api.env.basedn) assert isinstance(base_dn, DN) -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, If there is an extra search, will it be done on the same connection/bind ? Testing that an entry exists, even if no attribute are requested, loads the entry in the cache and evaluate the aci. If the entry does not exist, I do not think there is a gain between search(base) and search(subtree). If the entry exists, it will add the overhead of first search (connect/bind/aci) and the benefit of the first search depends if this entry matches the filter. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. +1 But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 01:44 PM, Tomas Babej wrote: On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. I understood that. But it does not make idview-* or dnsrecord-* any faster :-) Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. Right. To avoid potential issues, the ParentNotFound would need to be a subclass to NotFoundError so that except NotFound still works. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
Dne 19.11.2014 v 13:44 Tomas Babej napsal(a): On 11/19/2014 12:51 PM, Martin Kosek wrote: On 11/19/2014 12:41 PM, Tomas Babej wrote: On 11/19/2014 12:24 PM, Martin Kosek wrote: On 11/19/2014 12:03 PM, Tomas Babej wrote: Hi, When constructing a parent DN in LDAPSearch, we should always check that the parent object exists (hence use get_dn_if_exists), rather than search on unexistant containers (which can happen with get_dn). Replaces get_dn calls with get_dn_if_exists in *-find commands and makes sure proper error message is raised. https://fedorahosted.org/freeipa/ticket/4659 Doesn't it produce extra LDAP search thus making all our search commands slower? Is that what we want? No it does not make all of our LDAP search slower. It only happens for the objects that have parent objects, such as idoverrides or dnsrecords. ... and makes them slower. What I was pointing out here is that this is not a issue for ALL *-find commands (e.g user-find, group-find will not suffer from it), as you incorrectly stated. Wouldn't it be better to distinguish between LDAP search with no results and LDAP search with missing parent DN? The reply looks different, at least in CLI: Up to discussion. We would probably need to introduce a new exception, like ParentObjectNotFound. # search result search: 4 result: 0 Success # search result search: 4 result: 32 No such object matchedDN: cn=accounts,dc=mkosek-f20,dc=test Also, I do not think you can just stop using get_dn(), some commands override this call to get more complex searches (like host-find searching for shortname). Look into the get_dn_if_exists, it just wraps around get_dn, so no issue here. Any custom behaviour is preserved. Ah, ok, thanks for info. To sum up, I think this is worth changing this behaviour by default, ignoring a non-matching value of the parent object is not a correct general approach in my opinion. Well, that's the question. Whether we would leave DS to validate the search itself or do all the pre-check ourselves. To me, doing just one LDAP search and processing the error correctly looks better. But I can live even with your version then, I will leave the framework guardians like Honza or Petr3 to decide. +1 on single LDAP search and proper error processing. I see now what you're trying to suggest. However, the reason boils down to ipaldap.find_entries method not differentiating between a LDAP search that returns error code 32 (No such object) and LDAP search returning error code 0 (Success), but returning no results. In both cases errors.NotFound is raised. The reason I did not go this way is that changing the find_entries method is quite more invasive as this is the method subsenqently called by almost any command. You can always derive the new error (ParentNotFound or whatever) on NotFound, so old code won't break. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find
On 11/19/2014 02:03 PM, Jan Cholasta wrote:
Dne 19.11.2014 v 13:44 Tomas Babej napsal(a):
On 11/19/2014 12:51 PM, Martin Kosek wrote:
On 11/19/2014 12:41 PM, Tomas Babej wrote:
On 11/19/2014 12:24 PM, Martin Kosek wrote:
On 11/19/2014 12:03 PM, Tomas Babej wrote:
Hi,
When constructing a parent DN in LDAPSearch, we should always
check that the parent object exists (hence use get_dn_if_exists),
rather than search on unexistant containers (which can happen
with get_dn).
Replaces get_dn calls with get_dn_if_exists in *-find commands
and makes sure proper error message is raised.
https://fedorahosted.org/freeipa/ticket/4659
Doesn't it produce extra LDAP search thus making all our search
commands
slower? Is that what we want?
No it does not make all of our LDAP search slower. It only happens for
the objects that have parent objects, such as idoverrides or
dnsrecords.
... and makes them slower.
What I was pointing out here is that this is not a issue for ALL *-find
commands (e.g user-find, group-find will not suffer from it), as you
incorrectly stated.
Wouldn't it be better to distinguish between LDAP
search with no results and LDAP search with missing parent DN? The
reply looks
different, at least in CLI:
Up to discussion. We would probably need to introduce a new exception,
like ParentObjectNotFound.
# search result
search: 4
result: 0 Success
# search result
search: 4
result: 32 No such object
matchedDN: cn=accounts,dc=mkosek-f20,dc=test
Also, I do not think you can just stop using get_dn(), some
commands override
this call to get more complex searches (like host-find searching
for shortname).
Look into the get_dn_if_exists, it just wraps around get_dn, so no
issue
here. Any custom behaviour is preserved.
Ah, ok, thanks for info.
To sum up, I think this is worth changing this behaviour by default,
ignoring a non-matching value of the parent object is not a correct
general approach in my opinion.
Well, that's the question. Whether we would leave DS to validate the
search
itself or do all the pre-check ourselves. To me, doing just one LDAP
search and
processing the error correctly looks better. But I can live even
with your
version then, I will leave the framework guardians like Honza or
Petr3 to decide.
+1 on single LDAP search and proper error processing.
I see now what you're trying to suggest. However, the reason boils
down to ipaldap.find_entries method not differentiating between a
LDAP search that returns error code 32 (No such object) and LDAP
search returning error code 0 (Success), but returning no results.
In both cases errors.NotFound is raised.
The reason I did not go this way is that changing the find_entries
method
is quite more invasive as this is the method subsenqently called by
almost
any command.
You can always derive the new error (ParentNotFound or whatever) on
NotFound, so old code won't break.
Thanks for the suggestsions.
Attached is a new patch which hooks into find_entries method and
differentiates between the cases.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
From 2de4f0020a2a09b6328018652a49d8e4bf6c2c20 Mon Sep 17 00:00:00 2001
From: Tomas Babej tba...@redhat.com
Date: Wed, 19 Nov 2014 12:00:07 +0100
Subject: [PATCH] baseldap: Handle missing parent objects properly in *-find
commands
The find_entries function in ipaldap does not differentiate between
a LDAP search that returns error code 32 (No such object) and LDAP
search returning error code 0 (Success), but returning no results.
In both cases errors.NotFound is raised. In turn, LDAPSearch
commands interpret NotFound exception as no results.
To differentiate between the cases, a new error ContainerNotFound
was added, which inherits from NotFound to preserve the compatibility
with the new code.
This error is raised by ipaldap.find_entries in case it is performing
a search with onelevel or subtree scope, and the target dn does not
exist.
https://fedorahosted.org/freeipa/ticket/4659
---
ipalib/errors.py | 16
ipalib/plugins/baseldap.py | 2 ++
ipapython/ipaldap.py | 12 +---
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/ipalib/errors.py b/ipalib/errors.py
index f0426583dae2982b661d8c6d1540fe4acea83cef..7436b5e4bce2c024ab7b686a94b6aa124ebb2840 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1329,6 +1329,22 @@ class PosixGroupViolation(ExecutionError):
errno = 4030
format = _('This is already a posix group and cannot be converted to external one')
+class ContainerNotFound(NotFound):
+
+**4031** Raised when a container upon which a subtree or one-level search
+ is performed is not found.
+
+For example:
+
+ raise ContainerNotFound(reason='no such entry')
+Traceback (most recent call last):
+ ...
+ContainerNotFound: no such entry
+
+
+
+
