Skip to main content

[pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates

  • From: Erik Trauschke < >
  • To: Yiteng Zhang < >
  • Cc:
  • Subject: [pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates
  • Date: Fri, 13 Dec 2013 17:13:44 -0800

On 12/13/13 04:38 PM, Yiteng Zhang wrote:
Thank you for your detailed suggestions. In my first version, I supposed
that I have to start another Apache server instead of the one that
HTTPSTestClass uses because I was creating a new certificate that
effects the existing server ca cert file "combined_cas.pem" and caused
that Apache server fail. But I am wrong because I created a new CA
instead of using the existing CA and its private key to create the new
certificate.

So basically I changed the code as what you described, reuse the code
and put them in the right place. It looks more clean and consistent now.
Please see the updated one and let me know if it is on the right track.
Thanks.

https://ips.java.net/webrev/yitezhan/17768096_expiring_2/

Yay, this is like a hundred times better :D And you noticed yourself how much cleaner this now looks and feels.

There are still a bunch of things we can do better, though:

In t_https.py (lines 269++) you are setting up files which need to be set up every time someone wants to create certificates. So this should be moved into the cert generator as some sort of initialization routine.

The current handling of the working directory is kinda ugly. For instance, line 107 of pkg5unittest.py is only relevant for the generate-certs.py script. Lines 110 should probably move into the class itself.
But even after this you still have the problem of where the certs are actually put into the filesystem. It might make sense to pass an argument called base_dir or something to the functions. In any case, your test case shouldn't need to create any directories by itself.

Same problem for the configuration file, openssl.cnf. You could just assume that the cnf file needs to be in your base_dir or you can add another argument for its location. You have to play around with this a little to see what actually looks the best. It might turn out that having the generator functions being static is not working out well and it would be better if you set all the directories and cnf files up in the init function of the class. But as I said you need to do a bit of experimenting.

The goal here is to make creating certs in a test case or the script as easy and straight forward as possible, even if the generator gets more complicated. It's unlikely that the generator needs much work in the future but there might be more test cases which need to generate their own certs. So you want to make that easy.

The test case itself looks good. However, I don't see the need for copying the ta around. Can you not just specify the location of the original ta as parent_loc for make_cs_cert()?

Once you implement the suggestions above, you'll be surprised how short your test case is gonna get. And that is what we want.

Erik





[pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates

Erik Trauschke 12/13/2013

[pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates

Yiteng Zhang 12/14/2013

[pkg-discuss] Re: Code review request 17768096 pkg.client.api.image_create should not raise exception for expiring certificates

Erik Trauschke 12/14/2013
 
 
Close
loading
Please Confirm
Close