On 05/09/14 10:59, Martin Kosek wrote:
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
I found a problem during upgrade
https://fedorahosted.org/freeipa/ticket/4529
--
Martin Basti
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
