[Freeipa-devel] [PATCH] 0130 -- create missing idranges in trust-fetch-domains

[Alexander Bokovoy]

On Wed, 15 Jan 2014, Martin Kosek wrote:
>On 01/15/2014 02:54 PM, Sumit Bose wrote:
>> On Wed, Jan 15, 2014 at 02:14:08PM +0200, Alexander Bokovoy wrote:
>>> On Tue, 14 Jan 2014, Sumit Bose wrote:
>>>> On Tue, Jan 14, 2014 at 04:03:06PM +0200, Alexander Bokovoy wrote:
>>>>> On Tue, 14 Jan 2014, Martin Kosek wrote:
>>>>>> On 01/14/2014 01:27 PM, Alexander Bokovoy wrote:
>>>>>>> On Tue, 14 Jan 2014, Martin Kosek wrote:
>>>>>>>> On 01/14/2014 01:02 PM, Alexander Bokovoy wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> attached patch implements missing idranges when new child domains
>>>>>>>>> discovered through 'ipa trust-fetch-domains'. This functionality existed
>>>>>>>>> in 'ipa trust-add' but was not exposed in the 'ipa trust-fetch-domains'.
>>>>>>>>>
>>>>>>>>> Additionally, error message is shown in case trust name is incorrect.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4104
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4111
>>>>>>>>>
>>>>>>>>
>>>>>>>> I did not test the patch, just few observations from reading it:
>>>>>>> It is generally wrong to base opinion purely on reading the code (see below
>>>>>>> why) :)
>>>>>>
>>>>>> One does not need to run the code that see the places where it may be rusty :)
>>>>>>
>>>>>>>
>>>>>>>> 1) Why are you moving add_range method from trust object to the module? IMO it
>>>>>>>> could be left there, it belongs to the object. Plus, the patch won't be that
>>>>>>>> big and easier to backport. add_range can be still referred from other commands
>>>>>>>> as "self.obj.add_range", no need to move it.
>>>>>>> It was in trust_add class, not in the object. I need it in the other
>>>>>>> code and without explicit dependency on the object.
>>>>>>
>>>>>> Ok. Though I would still consider having it rather in the trust object to make
>>>>>> it easier accessible from other modules, though our API system.
>>>>> I deliberately don't want that. This is internal API for purposes of
>>>>> trust code -- do you envision any situation when anyone else might want
>>>>> to create idranges programmatically for child domains of the existing
>>>>> trust? Note that you are required to know fairly low-level details of
>>>>> the AD trusts to call it.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> 2) This part looks *very* suspicious:
>>>>>>>>
>>>>>>>> -        trust = self.api.Command.trust_show(keys[0], raw=True)['result']
>>>>>>>> +        try:
>>>>>>>> +            trust = self.api.Command.trust_show(keys[0], raw=True)['result']
>>>>>>>> +        except AssertionError:
>>>>>>>>
>>>>>>>> I do not think that AssertionError should be raised and caught in normal
>>>>>>>> operation, it should be an exceptional exception raised when the world falls
>>>>>>>> apart IMO. I.e. I would rather see some PublicError or Exception based
>>>>>>>> exception to be raised in trust_show in that case...
>>>>>>> It *is* raised and should be caught because this particular snippet is
>>>>>>> to catch situation when wrong trust domain name is passed. Previously
>>>>>>> the code simply generated server-side exception which resulted in
>>>>>>> 'internal error'.
>>>>>>
>>>>>> Ok, understood. My point is, trust_show should not raise AssertException just
>>>>>> because wrong trust domain is passed. There are better means to express that
>>>>>> error - ValidationError or NotFound, based on the situation.
>>>>> Yes, this is due to the way trust.get_dn() is built. Do not commit this
>>>>> patch yet (though building packages for testing would be good), I'm
>>>>> reworking it a bit to move this logic to get_dn() -- otherwise
>>>>> LDAPRetrieve() will always issue AssertError.
>>>>
>>>> I guess I already hit this while testing. trust-fetch-domains does not
>>>> work for valid forest roots anymore :-)
>>>>
>>>> Btw, can you add the forest root check to trustdomain-find as well?
>>> I've fixed it by returning an exception from get_dn(), as it is expected
>>> from other objects' get_dn().
>>>
>>> new version attached.
>>
>> Thanks. I can give functional ACK. All my checks are working as
>> expected, idranges are added if needed and 'ipa: ERROR: no such entry'
>> is displayed if the forest name is invalid. Btw I think you can add
>> https://fedorahosted.org/freeipa/ticket/4095 to the commit message as
>> well.
>>
>> The python code looks good to me as well.
>>
>> bye,
>> Sumit
>
>I have just few of my usual code purity rants :)
>
>What is the meaning of this?
>
>-            except errors.NotFound:
>-                return None
>+            except errors.NotFound, e:
>+                raise e
>
>Looks like NOOP to me, except it reraises the exception and thus hides the origin.
The full try block looks like this:

   try:
      <retrieve from LDAP>
   except errors.NotFound, e:
       raise e
   else:
       <act on the data from LDAP>

The key here is that it is get_dn(). errors.NotFound here has special
meaning and is intercepted by the framework, never checked by the LDAP*
operations (LDAPRetrieve, LDAPSearch, ...). Other exceptions may get
produced and they'll get shown in the logs but errors.NotFound from
get_dn() will never appear in the stacktrace.

The change from 'return None' to re-raising exception is intentional. I
could have dropped the whole 'except ...:' stanza too but this is more of
being explicit in intent here to make clear we want to drop out
exceptionally from get_dn() when entry was not found.

Majority of other get_dn() implementations don't touch LDAP entries at
this point, that makes logical difference here.
-- 
/ Alexander Bokovoy