On 09/04/2014 03:09 PM, Jan Cholasta wrote: > Dne 4.9.2014 v 13:40 Martin Kosek napsal(a): >> On 09/04/2014 01:19 PM, Jan Cholasta wrote: >>> Dne 4.9.2014 v 12:31 David Kupka napsal(a): >>>> On 09/03/2014 04:45 PM, Jan Cholasta wrote: >>>>> Dne 3.9.2014 v 16:25 David Kupka napsal(a): >>>>>> On 09/03/2014 04:05 PM, Jan Cholasta wrote: >>>>>>> Dne 3.9.2014 v 12:37 David Kupka napsal(a): >>>>>>>> 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. >>>>>>> >>>>>>> There is a little bug in ipa-upgradeconfig in the 4.0 version of the >>>>>>> patch. This is wrong: >>>>>>> >>>>>>> for request in requests: >>>>>>> nss_dir, nickname, ca_name, pre_command, post_command, profile >>>>>>> = request >>>>>>> criteria = { >>>>>>> 'cert-database': nss_dir, >>>>>>> 'cert-nickname': nickname, >>>>>>> 'ca-name': ca_name, >>>>>>> 'template-profile': profile, >>>>>>> } >>>>>>> >>>>>>> It should be: >>>>>>> >>>>>>> for nss_dir, nickname, ca_name, pre_command, post_command in >>>>>>> requests: >>>>>>> criteria = { >>>>>>> 'cert-database': nss_dir, >>>>>>> 'cert-nickname': nickname, >>>>>>> 'ca-name': ca_name, >>>>>>> } >>>>>>> >>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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(). >>>>>>> >>>>>>> You no longer check if the profile argument is set in modify() - either >>>>>>> re-introduce the check, or remove the default value of the argument to >>>>>>> make it mandatory. >>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> 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. >>>>>>> >>>>>>> I forgot about ipaserver.install.plugins.ca_renewal_master (sorry), it >>>>>>> should be fixed as well. >>>>>>> >>>>>> I need to check my patches more carefully, thank for the patience. >>>>>> >>>>> >>>>> And I need to do my reviews more carefully: in ca_renewal_master, the >>>>> "cert-storage" criterium should in fact be "cert-database". >>>>> >>>>> ACK after you fix the above. >>>>> >>>> Fixed together with other issues discussed on IRC. Please review it one >>>> more time. >>> >>> Thanks, ACK. >>> >> >> Thanks! But as I just found out, neither patch applies to ipa-4-1 branch. And >> as the merge is not straightforward, I would leave the backport rather either >> to Jan or David. >> >> Martin >> > > Here. >
Thanks, I just had to remove one pylint bug: ipaserver/install/cainstance.py:324: [E0601(used-before-assignment), stop_tracking_certificates] Using variable 'e' before assignment) Pushed to master, ipa-4-1, ipa-4-0. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel