[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Jan Cholasta]

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):
> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
>> David Kupka wrote:
>>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>>>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>>>
>>>>> This change should prevent hard-to-reproduce bugs like
>>>>> https://fedorahosted.org/freeipa/ticket/4280
>>>>
>>>> Thanks for this effort, the updated certmonger module looks much
>>>> better! This
>>>> will help us get rid of the non-standard communication with certmonger.
>>>>
>>>> Just couple initial comments from me by reading the code:
>>>>
>>>> 1) Testing needs fixed version of certmonger, right? This needs to be
>>>> spelled
>>>> out right with the patch.
>>> Yes, certmonger 0.75.13 and above should be fine according ticket
>>> https://fedorahosted.org/certmonger/ticket/36. Added to patch
>>> description.
>>
>> You should update the spec to set the minimum version as well.
> Sure, thanks.
>>
>>>>
>>>> 2) Description text in patches is cheap, do not be afraid to use it and
>>>> describe what you did and why. Link to the ticket is missing in the
>>>> description
>>>> as well:
>>> Ok, increased verbosity a bit :-)
>>>>
>>>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with its
>>>>> files.
>>>>>
>>>>> ---
>>>>
>>>> 3) get_request_id API:
>>>>
>>>>>            criteria = (
>>>>> -            ('cert_storage_location', dogtag_constants.ALIAS_DIR,
>>>>> -             certmonger.NPATH),
>>>>> -            ('cert_nickname', nickname, None),
>>>>> +            ('cert_storage_location', dogtag_constants.ALIAS_DIR),
>>>>> +            ('cert_nickname', nickname),
>>>>>            )
>>>>>            request_id = certmonger.get_request_id(criteria)
>>>>
>>>> Do we want to continue using the "criteria" object or should we rather
>>>> switch
>>>> to normal function options? I.e. rather using
>>>>
>>>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>>>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>>>
>>>> ? It would look more consistent with other calls. I am just asking,
>>>> not insisting.
>>> I've no preference here. It seems to be a very small change. Has anyone
>>> a reason to do it one way and not the other?
>>
>> I think I used this criteria thing to avoid having a bazillion optional
>> parameters and for future-proofing. I think at this point the list is
>> probably pretty stable, so I'd base it on whether you care about having
>> a whole ton of optional parameters or not (it has the advantage of
>> self-documenting itself).
>>
> The list is probably stable but also really excessive. I don't think it
> would help to have more than dozen optional parameters. So I prefer to
> leave as-is and change it in future if it is wanted.
>>>>
>>>> 3) Starting function:
>>>>
>>>>> +    try:
>>>>> +        ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>>>> skip_output=True)
>>>>> +    except Exception, e:
>>>>> +        root_logger.error('Failed to start certmonger: %s' % e)
>>>>> +        raise e
>>>>
>>>> I see 2 issues related to this code:
>>>> a) Do not call SYSTEMCTL directly. To be platform independent,
>>>> rather use
>>>> services.knownservices.messagebus.start() that is overridable by
>>>> someone else
>>>> porting to non-systemd platforms.
>>> Is there anything that can't be done using ipalib/ipapython/ipaplatform?
>>
>> It can't make coffee (yet).
>>
>>>> b) In this case, do not use "raise e", but just "raise" to keep the
>>>> exception
>>>> stack trace intact for better debugging.
>>> Every day there's something new to learn about python or FreeIPA.
>>>>
>>>> Both a) and b) should be fixed in other occasions and places.
>>> I found only one occurence of a) issue. Is there some hidden or are you
>>> talking about the whole FreeIPA project?
>>>>
>>>> 4) Feel free to add yourself to Authors section of this module. You
>>>> refactored
>>>> it greatly to earn it :-)
>>> Done.
>>
>> You already import dbus, why also separately import DBusException?
>>
> Removed, thanks for noticing.
>> rob
>>

1) The patch needs to be rebased.


2) Please try to follow PEP8, at least in certmonger.py.


3) In certificate_renewal_update() in ipa-upgradeconfig you removed 
ca_name from criteria.


4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and 
stop_tracking() is unnecessary. We can keep calling request nicknames 
"request_id" and certificate nicknames "nickname" in our APIs.


5) I think it would be better to add a docstring to 
_cm_dbus_object.__init__() instead of doing this:

     # object is accesible over this DBus bus instance
     bus = None
     # DBus object path
     path = None
     # the actual DBus object
     obj = None
     # object interface name
     obj_dbus_if = None
     # object parent interface name
     parent_dbus_if = None
     # object inteface
     obj_if = None
     # property interface
     prop_if = None


6) In _start_certmonger(), please check if certmonger is already running 
before starting it, what applies to systemd might not apply to other 
init systems.


7) Do we ever need to connect to certmonger on the session bus? If not, 
there is no need to support it in _connect_to_certmonger().


8) You should not ignore other criteria when 'nickname' is specified in 
_get_requests(). Use find_request_by_nickname only if 'nickname' is the 
only criterion, otherwise filter the result of get_requests, including 
'nickname'.


9) IMO you can indeed remove request_cert().


10) You forgot to port modify() and resubmit().


11) As for get_pin(), it should be moved to ipapython.dogtag, because it 
is Dogtag related (you don't need to do it in this patch).


I haven't done functional testing yet, expect more comments later.

Honza

-- 
Jan Cholasta