[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 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.
Well, I tried to explain it :)

You can drop that bit if you want.

-- 
/ Alexander Bokovoy