[Freeipa-devel] [PATCH 0286] baseldap: Handle missing parent objects properly in *-find

[Jan Cholasta]

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