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 _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
