On 03/13/2014 01:33 PM, Alexander Bokovoy wrote:
> On Thu, 13 Mar 2014, Petr Spacek wrote:
>> On 13.3.2014 13:20, Martin Kosek wrote:
>>> On 03/13/2014 01:10 PM, Alexander Bokovoy wrote:
>>>> On Thu, 13 Mar 2014, Martin Kosek wrote:
>>>>> On 03/13/2014 01:01 PM, Alexander Bokovoy wrote:
>>>>>> On Thu, 13 Mar 2014, Martin Kosek wrote:
>>>>>>> On 03/13/2014 12:45 PM, Tomas Babej wrote:
>>>>>>>> Hi,
>>>>>>>> Changes the code in the idrange_del method to not only check for
>>>>>>>> the root domains that match the SID in the IDRange, but for the
>>>>>>>> SIDs of subdomains of trusts as well.
>>>>>>>> https://fedorahosted.org/freeipa/ticket/4247
>>>>>>> This is a very complicated validation procedure IMO. Lot of subcommands,
>>>>>>> lot of
>>>>>>> LDAP searches.
>>>>>>> Why can't we do just one LDAP search with
>>>>>>> - base api.env.container_trusts
>>>>>>> - scope SUB
>>>>>>> - filter
>>>>>>> (&(objectclass=ipaNTTrustedDomain)(ipanttrusteddomainsid=range_sid))
>>>>>>> When errors.NotFound is raised, we are OK. When it is not raised, we 
>>>>>>> have a
>>>>>>> problem.
>>>>>>> Wouldn't it be simpler?
>>>>>> No. Please do not do optimization here. It is a code that is called very
>>>>>> rarely and expressiveness is more important here than optimizing access
>>>>>> to couple of entries in LDAP.
>>>>> I am not optimizing - I am actually making the validation much simpler.
>>>>> What is
>>>>> more simple and straightforward?
>>>>> A) One ldap.find_entries call
>>>>> B) A loop, numerous subcommands and LDAP searches
>>>> So far I've been successful in keeping details on how trust objects are
>>>> represented in LDAP hidden from the rest of the framework code by
>>>> encapsulating it all in trust.py. The change you propose will
>>>> make it leaking to idrange.py. If we start changing the structure (which
>>>> is maintained by ipasam module, not the framework), we will have more
>>>> maintenance problems with the code spread out.
>>> Ok, I can see your point, but I am still not sure it warrants that 
>>> complicated
>>> validation and a new dependency between commands. You cannot that easily 
>>> change
>>> the structure of the subdomains anyway as it would break all older servers
>>> which expect the subdomains to be where they are.
>>> In plugins, we do LDAP searches in cases like this one quite regularly IMO, 
>>> it
>>> is not something unprecendented. And there is a good reason, simple LDAP 
>>> call
>>> is much easier and less error prone to changes in our frameworks than 
>>> calling
>>> subcommands. One glitch or a change in the subcommand can break not only the
>>> subcommand, but it's all callers.
>> Note that you can encapsulate the search proposed by Martin in a function
>> defined in trusts.py so it doesn't need to be implemented idrange.py.
>> It is just a matter of finding the right name for it.
> I wanted to propose that as well.
> We already have conditional import of ipaserver.dcerpc, we can use
> ipalibs.plugins.trust import for that helper, just don't export it
> through API.

This may work as well, we just need to be cautious in the idrange plugin so
that the idrange-del works even when ipa-server-trust-ad package is not
installed on the system (which would probably break current patch too, when I
am thinking about it).


Freeipa-devel mailing list

Reply via email to