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

[Jan Cholasta]

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