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

[Martin Kosek]

On 01/15/2014 03:10 PM, Alexander Bokovoy wrote:
> 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.

Thanks for explanation, but it still does not make sense to me and is IMO a
wrong and confusing statement. We also want to drop out in
errors.DatabaseError, errors.DatabaseTimeOut, errors.LimitsExceeded and we do
not add a special except statement for it.

If you really want to add a note about the NotFound case, I would suggest using
rather comment which would be less confusing and more informative.

Martin