----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14855/#review27334 -----------------------------------------------------------
api/src/com/cloud/network/lb/CertService.java <https://reviews.apache.org/r/14855/#comment53162> Please don't throw Exception. Narrow the throws to what is really needed. api/src/com/cloud/network/lb/CertService.java <https://reviews.apache.org/r/14855/#comment53163> Having a boolean return type is not needed. Just return void. If the validation fails you should just throw an exception that will have a useful error message for the user api/src/com/cloud/network/lb/SslCert.java <https://reviews.apache.org/r/14855/#comment53165> Interfaces for VOs should not have setters. The interfaces are intended to be read only api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java <https://reviews.apache.org/r/14855/#comment53166> I don't understand why a password is needed? engine/schema/src/com/cloud/network/dao/LoadBalancerCertMapVO.java <https://reviews.apache.org/r/14855/#comment53167> Their really should be a state field, not just a simple boolean flag for revoke. It will be easier to reliabily orchestrate if you have states. Maybe different names, but something like "Associating, Associated, Disassociating" and then the final disassociated state is when you just delete the row engine/schema/src/com/cloud/network/dao/SslCertVO.java <https://reviews.apache.org/r/14855/#comment53168> AWS has the following length limits, don't know if there is reason behind them cert = 16384 chain = 2097152 key = 16384 server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java <https://reviews.apache.org/r/14855/#comment53169> why URLDecoder.decode()? server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java <https://reviews.apache.org/r/14855/#comment53170> This is a general comment. You do not need to use the transaction API unless you specifically need to do an atomic commit across multiple rows. Most of this code needs on DB transactions. In this specific case you are not committing the transaction, so I'd be suprised if it works server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java <https://reviews.apache.org/r/14855/#comment53173> It's extremely useful if you generate the certificate fingerprint and store that in the database and include in all responses. The fingerprint is a quick way to determine which cert something is. Here's sample code to do that. String generateFingerPrint(Certificate cert) { StringBuilder buffer = new StringBuilder(60); MessageDigest md = MessageDigest.getInstance("SHA-1"); byte[] data = md.digest(cert.getEncoded()); for ( int i = 0 ; i < data.length ; i++ ) { if ( buffer.length() > 0 ) { buffer.append(":"); } buffer.append(HEX[(0xF0 & data[i]) >>> 4]); buffer.append(HEX[0x0F & data[i]]); } return buffer.toString(); } server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java <https://reviews.apache.org/r/14855/#comment53171> Throw a more approriate exception. - Darren Shepherd On Oct. 22, 2013, 8:19 p.m., Syed Ahmed wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14855/ > ----------------------------------------------------------- > > (Updated Oct. 22, 2013, 8:19 p.m.) > > > Review request for cloudstack, Darren Shepherd, Murali Reddy, and Shengsheng > Huang. > > > Bugs: CLOUDSTACK-4821 > https://issues.apache.org/jira/browse/CLOUDSTACK-4821 > > > Repository: cloudstack-git > > > Description > ------- > > This is the first patch for Ssl termination support in Cloudstack. This patch > is for the certificate management. Basically uploading, validation, deletion, > listing for the certificates. > > The FS for this is at > https://cwiki.apache.org/confluence/display/CLOUDSTACK/SSL+Termination+Support > . I will work on the second patch which will assign the certificates to > loadbalancing rules. > > > Diffs > ----- > > api/src/com/cloud/network/lb/CertService.java PRE-CREATION > api/src/com/cloud/network/lb/SslCert.java PRE-CREATION > api/src/org/apache/cloudstack/api/ApiConstants.java c75e6a0 > > api/src/org/apache/cloudstack/api/command/user/loadbalancer/DeleteSslCertCmd.java > PRE-CREATION > > api/src/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java > PRE-CREATION > > api/src/org/apache/cloudstack/api/command/user/loadbalancer/UploadSslCertCmd.java > PRE-CREATION > api/src/org/apache/cloudstack/api/response/SslCertResponse.java > PRE-CREATION > client/tomcatconf/applicationContext.xml.in 2a3520b > client/tomcatconf/nonossComponentContext.xml.in 9d1da95 > engine/schema/src/com/cloud/network/dao/LoadBalancerCertMapDao.java > PRE-CREATION > engine/schema/src/com/cloud/network/dao/LoadBalancerCertMapDaoImpl.java > PRE-CREATION > engine/schema/src/com/cloud/network/dao/LoadBalancerCertMapVO.java > PRE-CREATION > engine/schema/src/com/cloud/network/dao/SslCertDao.java PRE-CREATION > engine/schema/src/com/cloud/network/dao/SslCertDaoImpl.java PRE-CREATION > engine/schema/src/com/cloud/network/dao/SslCertVO.java PRE-CREATION > server/src/com/cloud/server/ManagementServerImpl.java 699f469 > server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java > PRE-CREATION > server/test/org/apache/cloudstack/lb/ApplicationLoadBalancerTest.java > 9b46e68 > > server/test/org/apache/cloudstack/network/lb/ApplicationLoadBalancerTest.java > PRE-CREATION > server/test/org/apache/cloudstack/network/lb/CertServiceTest.java > PRE-CREATION > server/test/resources/certs/bad_format_cert.crt PRE-CREATION > server/test/resources/certs/dsa_self_signed.crt PRE-CREATION > server/test/resources/certs/dsa_self_signed.key PRE-CREATION > server/test/resources/certs/expired_cert.crt PRE-CREATION > server/test/resources/certs/non_x509_pem.crt PRE-CREATION > server/test/resources/certs/root_chain.crt PRE-CREATION > server/test/resources/certs/rsa_ca_signed.crt PRE-CREATION > server/test/resources/certs/rsa_ca_signed.key PRE-CREATION > server/test/resources/certs/rsa_ca_signed2.crt PRE-CREATION > server/test/resources/certs/rsa_ca_signed2.key PRE-CREATION > server/test/resources/certs/rsa_random_pkey.key PRE-CREATION > server/test/resources/certs/rsa_self_signed.crt PRE-CREATION > server/test/resources/certs/rsa_self_signed.key PRE-CREATION > server/test/resources/certs/rsa_self_signed_with_pwd.crt PRE-CREATION > server/test/resources/certs/rsa_self_signed_with_pwd.key PRE-CREATION > setup/db/db/schema-421to430.sql aaebf96 > > Diff: https://reviews.apache.org/r/14855/diff/ > > > Testing > ------- > > Unittests are done and included in the review. > > > Thanks, > > Syed Ahmed > >
