[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
David Kupka
dkupka at redhat.com
Wed Sep 3 10:37:06 UTC 2014
On 09/02/2014 01:56 PM, Jan Cholasta wrote:
> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
>> 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.
>
> I didn't notice the patch is targeted for 4.0. Can you please provide
> patches for both ipa-4-0 and ipa-4-1/master?
>
Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.
>>>
>>>
>>> 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
>
> You removed the comments, but left the attributes there. You should
> remove them as well, they are not necessary, as you set them all in
> __init__().
>
Removed.
>>>
>>>
>>> 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.
>
> OK, no need to include the TODO comment though.
>
>>
>>>
>>> I haven't done functional testing yet, expect more comments later.
>>>
>>> Honza
>>>
>>
>
> 12) ipa-client-install still uses ipa-getcert instead of certmonger API
> to request the host certificate, but since we plan on stopping doing
> that (https://fedorahosted.org/freeipa/ticket/4449), I guess you don't
> have to do anything about it.
Ok. I will leave it on you since you are owner of this ticket.
>
>
> 13) You require dict for criteria in get_request_id, but you pass tuples
> to it in ipa-upgradeconfig, ipa-cacert-manage and ipa-certupdate, which
> makes them fail.
Good point, fixed.
>
>
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-5-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 32035 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140903/04f63305/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-5-ipa40-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 29313 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140903/04f63305/attachment-0001.bin>
More information about the Freeipa-devel
mailing list