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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel