[Freeipa-devel] [PATCH 0157] Prohibit deletion of active subdomain range

[Alexander Bokovoy]

On Thu, 13 Mar 2014, Martin Kosek wrote:
>On 03/13/2014 01:36 PM, Martin Kosek wrote:
>> 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).
>>
>> Martin
>
>I take it back - it is not so good idea. You may be running this command on a
>master without ipa-server-trust-ad installed, but which may be however
>installed on other IPA master and thus we do want to check for the trustdomains.
>
>We need to enclose this check to simple LDAP call in idrange.py as I said in
>the beginning.
Ok, this is an argument I agree with.

Tomas, could you please change the code correspondingly?
-- 
/ Alexander Bokovoy