On 17.10.2013 18:01, Petr Viktorin wrote:
On 10/17/2013 02:21 PM, Jan Cholasta wrote:
Hi,

this patchset contains refactoring of the certificate renewal code,
which will be the base for CA certificate renewal.

The biggest change is a new certmonger CA helper
dogtag-ipa-ca-renew-agent, which replaces
dogtag-ipa-retrieve-agent-submit as well as parts of certmonger
post-commands used in certificate renewal. It provides more flexibility
when doing renewals and allows unified certmonger configuration on both
CA master and clones.

How to test: Test both CA-ful and CA-less server and replica installs
and upgrades, check that certmonger is configured properly and
certificate renewal works (see
https://fedorahosted.org/freeipa/ticket/2803#comment:17 for details).

Dependencies:
freeipa-jcholast-161.3-Fix-certificate-renewal-scripts-to-work-with-separat.patch.


Thanks!
Here are my comments from reading the patches; I haven't tested yet.

161. Looks good
172. Looks good

173.
The second hunk removes `pem_cert` that is later used for
upload_ca_dercert()

Right, will fix.


174. Looks good

175.
This line in upload_ca_cert() seems redundant:
     name = certdb.cacert_name

It indeed is in this particular patch.


176. Looks good
177. Looks good
178. Looks good

179.
Note: look if the other calls don't rely on this (replica-install,
ca-install)

I didn't see any failures.


180. Looks good

181.
What are those constants you define in the beginning? Why are most not
used? I think you should add a link to some reference.

They are certmonger specific:

https://git.fedorahosted.org/cgit/certmonger.git/tree/doc/submit.txt
https://git.fedorahosted.org/cgit/certmonger.git/tree/src/submit-e.h

Will add the links.

Nitpick - PEP8 explicitly says to avoid aligning with extra spaces:
http://www.python.org/dev/peps/pep-0008/#pet-peeves

OK.


Please use a docstring for documenting what request_cert() does, and
describe the return value.

OK. Return value is based on information in submit.txt (see above for link).

I don't see the purpose of the `if rc == WAIT_WITH_DELAY` block since
`delay, cookie` gets joined again by the caller.
Wouldn't it be cleaner to always return (rc, output) instead of doing
that [1:] dance?

In the context of this single patch, yes. It the context of the whole patchset, no.


Did you consider using `traceback.format_exc` for logging errors? Full
tracebacks tend to be more useful than just type & message.

I did, but was not sure I want to spam syslog with full tracebacks.


182.
I don't understand why you reordered get_subjectaltname & get_subject,
that makes it hard to see what changed.

It felt more natural to me this way.


183. Looks good

184. Looks OK

185.
ldap_connect: conn.disconnect() should be in a finally clause

Right.


retrieve_cert:
- Please use docstrings for documenting functions
- The try: block is too long, NotFound only needs to be handled for
conn.get_entry()
- The `except Exception` block should be removed, or it should just log
and re-raise; the error handling is done in main().

The body of this function is mostly copy-paste from dogtag-ipa-retrieve-agent-submit. Yes, it could use some cleaning up.


main:
Shouldn't we rather raise an error if CERTMONGER_CA_PROFILE has some
unknown value?

No, it might be a Dogtag profile name.



186.
I'm getting lost in all the arguments to dogtag_start_tracking(). Could
you name them when calling it?

OK.


187.
I think one changelog entry for all these patches is enough.

OK.


188.
Please use docstrings for documenting functions.

I think even with OPERATION_NOT_SUPPORTED_BY_HELPER this should display
some output, like "unknown operation %s".

This is handled by certmonger.


When store_cert() runs out of attempts, it should not return ISSUED. The
exception should be re-raised so it's logged.

If it didn't return ISSUED, the certificate would not be saved by certmonger and you would not be able to retry the storage attempt. How much of a problem is this I don't know, the behavior is taken from renew_ra_cert.


189.
Again, naming arguments in dogtag_start_tracking calls would make them
clearer

190.
Instead of dogtag.install_constants, this should use
configured_constants().

I didn't actually look at what's inside the function. Will fix.


191.
Again, please use docstrings for documenting functions.
Or just do the `if` right in main(), I don't think a separate function
is worth it here.

OK, it probably isn't.


192.
Again, naming dogtag_start_tracking args would make the code clearer.

193.
Same here

194.
I don't see why you reorder the methods, it makes the changes harder to
spot.

To have them in a single spot instead of all over the place.


195. Looks OK

196.
Again, please use docstrings for documenting functions.



--
Jan Cholasta

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to