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

[David Kupka]

Hope, I've addressed all the issues (except 9 and 11, inline). Let's go 
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:
> 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().

Not sure, it is used in self-test although I doubt anyone ever ran it.

>
>
> 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).
>

This patch is ugly enough. I will create a separate one for this.

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

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-4-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 32144 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140829/031f0b67/attachment.bin>