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

[Jan Cholasta]

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