[PATCH] refactor installation code to use python client instead of jython
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython. So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early. Please review, Ade
On 3/19/2013 3:54 PM, Ade Lee wrote:
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython.
So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early.
Please review, Ade
Some comments: 1. Right now the encoder.py has to import other PKI modules in order to construct the TYPES and NOTYPES lists, so if new modules are added we'd need to update the encoder.py. This is not ideal since encoder.py is a common module. It might be better to let the modules register themselves into those lists, for example in encoder.py we can do something like this: import pki.encoder class ConfigurationRequest: ... encoder.TYPES['ConfigurationRequest'] = ConfigurationRequest 2. The SystemCertData is now in NOTYPES list. What if we want to send a SystemCertData object in a future API? It will need to be in TYPES. Maybe instead of using NOTYPES we should check if the object is an array and the elements are in TYPES then we return an array of encoded objects. 3. JSON responses should be converted back into Python object: http://stackoverflow.com/questions/6578986/how-to-convert-json-data-into-a-p... 4. The ConfigurationResponse.getSystemCerts() should return a List instead of a Collection. 5. File access can be written with the 'with' keyword: with open(...) as f: data = f.read() 6. Sometimes OCSP or TKS installation failed: pkispawn : INFO ....... constructing PKI configuration data. pkispawn : INFO ....... configuring PKI configuration data. pkispawn : ERROR ....... Exception from Java Configuration Servlet: [Errno 111] Connection refused Maybe Tomcat is too slow to start? Sometimes it works just fine. -- Endi S. Dewata
I'll file a separate patch to address the issues below. For the last issue, I am attaching a patch to check the server status prior to doing the configuration. This patch is to be applied on top of the previous one. (122) Ade On Wed, 2013-03-20 at 10:19 -0500, Endi Sukma Dewata wrote:
On 3/19/2013 3:54 PM, Ade Lee wrote:
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython.
So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early.
Please review, Ade
Some comments:
1. Right now the encoder.py has to import other PKI modules in order to construct the TYPES and NOTYPES lists, so if new modules are added we'd need to update the encoder.py. This is not ideal since encoder.py is a common module. It might be better to let the modules register themselves into those lists, for example in encoder.py we can do something like this:
import pki.encoder
class ConfigurationRequest: ...
encoder.TYPES['ConfigurationRequest'] = ConfigurationRequest
2. The SystemCertData is now in NOTYPES list. What if we want to send a SystemCertData object in a future API? It will need to be in TYPES.
Maybe instead of using NOTYPES we should check if the object is an array and the elements are in TYPES then we return an array of encoded objects.
3. JSON responses should be converted back into Python object:
http://stackoverflow.com/questions/6578986/how-to-convert-json-data-into-a-p...
4. The ConfigurationResponse.getSystemCerts() should return a List instead of a Collection.
5. File access can be written with the 'with' keyword:
with open(...) as f: data = f.read()
6. Sometimes OCSP or TKS installation failed:
pkispawn : INFO ....... constructing PKI configuration data. pkispawn : INFO ....... configuring PKI configuration data. pkispawn : ERROR ....... Exception from Java Configuration Servlet: [Errno 111] Connection refused
Maybe Tomcat is too slow to start? Sometimes it works just fine.
On 3/20/2013 2:26 PM, Ade Lee wrote:
I'll file a separate patch to address the issues below.
For the last issue, I am attaching a patch to check the server status prior to doing the configuration. This patch is to be applied on top of the previous one. (122)
Ade
OCSP installation is consistently failing. There's a missing <servlet-mapping> element. pkispawn : INFO ....... executing 'systemctl start pki-tomcatd@ocsp-master.service' Traceback (most recent call last): File "/sbin/pkispawn", line 405, in <module> main(sys.argv) File "/sbin/pkispawn", line 389, in main rv = instance.spawn() File "/usr/lib/python2.7/site-packages/pki/deployment/configuration.py", line 98, in spawn status = util.instance.wait_for_startup(60) File "/usr/lib/python2.7/site-packages/pki/deployment/pkihelper.py", line 1026, in wait_for_startup status = self.get_instance_status() File "/usr/lib/python2.7/site-packages/pki/deployment/pkihelper.py", line 1005, in get_instance_status response = client.getStatus() File "/usr/lib/python2.7/site-packages/pki/system.py", line 80, in getStatus self.connection.subsystem + '/getStatus') File "/usr/lib/python2.7/site-packages/pki/client.py", line 54, in get r.raise_for_status() File "/usr/lib/python2.7/site-packages/requests/models.py", line 638, in raise_for_status raise http_error requests.exceptions.HTTPError: 404 Client Error: Not Found -- Endi S. Dewata
Fixed in 124. On Wed, 2013-03-20 at 15:49 -0500, Endi Sukma Dewata wrote:
On 3/20/2013 2:26 PM, Ade Lee wrote:
I'll file a separate patch to address the issues below.
For the last issue, I am attaching a patch to check the server status prior to doing the configuration. This patch is to be applied on top of the previous one. (122)
Ade
OCSP installation is consistently failing. There's a missing <servlet-mapping> element.
pkispawn : INFO ....... executing 'systemctl start pki-tomcatd@ocsp-master.service' Traceback (most recent call last): File "/sbin/pkispawn", line 405, in <module> main(sys.argv) File "/sbin/pkispawn", line 389, in main rv = instance.spawn() File "/usr/lib/python2.7/site-packages/pki/deployment/configuration.py", line 98, in spawn status = util.instance.wait_for_startup(60) File "/usr/lib/python2.7/site-packages/pki/deployment/pkihelper.py", line 1026, in wait_for_startup status = self.get_instance_status() File "/usr/lib/python2.7/site-packages/pki/deployment/pkihelper.py", line 1005, in get_instance_status response = client.getStatus() File "/usr/lib/python2.7/site-packages/pki/system.py", line 80, in getStatus self.connection.subsystem + '/getStatus') File "/usr/lib/python2.7/site-packages/pki/client.py", line 54, in get r.raise_for_status() File "/usr/lib/python2.7/site-packages/requests/models.py", line 638, in raise_for_status raise http_error requests.exceptions.HTTPError: 404 Client Error: Not Found
On 3/20/2013 10:19 AM, Endi Sukma Dewata wrote:
On 3/19/2013 3:54 PM, Ade Lee wrote:
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython.
So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early.
Additional comments for patch #122: 7. All command line arguments for should be quoted in case they contain spaces. This including paths and file names. It would be better to use subprocess.call() instead of os.system(). The arguments can be specified as a list, so they don't need to be quoted. 8. In general a function should not call sys.exit() on error. It would be better to raise an Error and let the main program handle it. 9. Boolean attributes (e.g. isClone, backupKeys, importAdminCert) in Python objects should use real boolean values instead of string. 10. Currently the config_client is instantiated as a global variable in pkihelper.py which may limit its reusability. It would be better to instantiate it where it's actually used (in configuration.py). -- Endi S. Dewata
Attached new patch to be applied on top of 122, 123. The patch also addresses some of the issues in the previous email. See comments below. On Wed, 2013-03-20 at 16:57 -0500, Endi Sukma Dewata wrote:
On 3/20/2013 10:19 AM, Endi Sukma Dewata wrote:
On 3/19/2013 3:54 PM, Ade Lee wrote:
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython.
So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early.
Additional comments for patch #122:
7. All command line arguments for should be quoted in case they contain spaces. This including paths and file names.
It would be better to use subprocess.call() instead of os.system(). The arguments can be specified as a list, so they don't need to be quoted.
Done for code added in this patch. There are other parts of pkispawn that need to be checked for this issue.
8. In general a function should not call sys.exit() on error. It would be better to raise an Error and let the main program handle it.
This is a general problem in pkispawn (not just in the code submitted). In keeping with the behavior in pkijython, this was continued in the code submitted. A ticket should be opened to address this throughout the scriptlets.
9. Boolean attributes (e.g. isClone, backupKeys, importAdminCert) in Python objects should use real boolean values instead of string.
There is already a ticket to make the fields in the java object ConfigurationRequest use real boolean values, slated for 10.1. Python clients should be handled then too.
10. Currently the config_client is instantiated as a global variable in pkihelper.py which may limit its reusability. It would be better to instantiate it where it's actually used (in configuration.py).
Done.
See comments below: On Wed, 2013-03-20 at 10:19 -0500, Endi Sukma Dewata wrote:
On 3/19/2013 3:54 PM, Ade Lee wrote:
This is a pretty big change, but we want to get it into 10.0.2 so that we can eliminate our dependency on jython.
So far, its been tested against a straight CA install. I plan to continue testing against other configurations, but as the code change is quite large, I want to start the review early.
Please review, Ade
Some comments:
1. Right now the encoder.py has to import other PKI modules in order to construct the TYPES and NOTYPES lists, so if new modules are added we'd need to update the encoder.py. This is not ideal since encoder.py is a common module. It might be better to let the modules register themselves into those lists, for example in encoder.py we can do something like this:
import pki.encoder
class ConfigurationRequest: ...
encoder.TYPES['ConfigurationRequest'] = ConfigurationRequest
Fixed in 124.
2. The SystemCertData is now in NOTYPES list. What if we want to send a SystemCertData object in a future API? It will need to be in TYPES.
Maybe instead of using NOTYPES we should check if the object is an array and the elements are in TYPES then we return an array of encoded objects.
Due to time constraints, we will not fix this in this patch. Please open a ticket.
3. JSON responses should be converted back into Python object:
http://stackoverflow.com/questions/6578986/how-to-convert-json-data-into-a-p...
Same as above. Please open a ticket.
4. The ConfigurationResponse.getSystemCerts() should return a List instead of a Collection.
Fixed in 124.
5. File access can be written with the 'with' keyword:
with open(...) as f: data = f.read()
Fixed in 124 for the code added in this patch. There are, however, many other occurences in the python code that should be addressed. Please open a ticket to address these.
6. Sometimes OCSP or TKS installation failed:
pkispawn : INFO ....... constructing PKI configuration data. pkispawn : INFO ....... configuring PKI configuration data. pkispawn : ERROR ....... Exception from Java Configuration Servlet: [Errno 111] Connection refused
Maybe Tomcat is too slow to start? Sometimes it works just fine.
Fixed in 123.
participants (2)
-
Ade Lee -
Endi Sukma Dewata