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. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
