Hi Jinmei,
1. Yes, a parallel change would be good for keystore. In both cases, we
need to specify the default. My sample text said empty string ('') -- is
that correct, or is the default JKS?
2 & 3: Good - glad they're correct. I wasn't sure, just noticed the
patterns.
-DaveOn Sat, Aug 5, 2017 at 12:15 PM, Jinmei Liao <jil...@pivotal.io> wrote: > > > > On Aug. 3, 2017, 11:36 p.m., Dave Barnes wrote: > > > 1. The help message for the property should state what it does and > whether it has a default. JKS was our assumption in the past, but with the > introduction of this feature other values (such as 'pkcs12') are possible. > > > Current wording: "For Java truststore file format, this property has > the value jks (or JKS)." > > > Suggested wording: "Specifies the type of the ssl truststore. The > default is '' (an empty string) For Java truststore format, specify 'jks' > or 'JKS'." > > > > > > The following questions come from me reading for patterns, please > forgive if they're stupid ones: > > > > > > 2. In configureLegacyClusterSSL (and ..ServerSSL and ..JMXSSL and > ..ServiceSSL), I see two occurrences of "sslConfig.setTruststoreType( > getDistributionConfig().getClusterSSLKeyStoreType());". Should the second > one in each case be getClusterSSLTrustStoreType() ? > > > > > > 3. In DistributionConfigImpl.java, there's a long list of static > imports for gateway, http, jmx, etc. KEYSTORE_TYPE is included for each > group, but not TRUSTSTORE_TYPE. Is this correct? > > 1. I am simply copying from the ssl-keystore-type, should this suggested > description also apply to the keystore as well? > 2. the truststore type does not exist for legacy ssl specifications, so > there is no getClusterSSLTrustStoreType() method. Here I am just assuming > the keystore and truststore type is the same. > 3. IDEA did that according to the latest import style. > > > - Jinmei > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61417/#review182181 > ----------------------------------------------------------- > > > On Aug. 3, 2017, 9:15 p.m., Jinmei Liao wrote: > > > > ----------------------------------------------------------- > > This is an automatically generated e-mail. To reply, visit: > > https://reviews.apache.org/r/61417/ > > ----------------------------------------------------------- > > > > (Updated Aug. 3, 2017, 9:15 p.m.) > > > > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, > Patrick Rhomberg, and Udo Kohlmeyer. > > > > > > Repository: geode > > > > > > Description > > ------- > > > > GEODE-3328: adding ssl-truststore-type to the config > > > > this is what's requested in the JIRA ticket. This changeset just deals > with adding the property into gemfire properties > > > > > > Diffs > > ----- > > > > > > geode-core/src/main/java/org/apache/geode/distributed/ConfigurationProperties.java > 63f6505101f6edb62167b854d3d16d76d0a893cd > > geode-core/src/main/java/org/apache/geode/distributed/internal/ > AbstractDistributionConfig.java 795f6a5a4a4f42fe065c9ca6013fd5598f9311d8 > > geode-core/src/main/java/org/apache/geode/distributed/ > internal/DistributionConfig.java c2a395de0bfe21ed8efeb6b25e331f7085b3bf4f > > > > geode-core/src/main/java/org/apache/geode/distributed/internal/DistributionConfigImpl.java > fbe894c96447b2e30594eb2ed0652dd08e1028ce > > > > geode-core/src/main/java/org/apache/geode/internal/net/SSLConfigurationFactory.java > f86f07eb5e58b8509e909b7748795530efd65339 > > > > geode-core/src/main/java/org/apache/geode/management/GemFireProperties.java > 08fa9b54ea066b4158478ae89d8295ed0b1a337b > > geode-core/src/main/java/org/apache/geode/management/ > internal/beans/BeanUtilFuncs.java 499ef010f354ebf88765190f1b5eb945da83accc > > geode-core/src/test/java/org/apache/geode/distributed/internal/ > DistributionConfigJUnitTest.java 525f988cd3cb4f19872168df9b7105645f5c0498 > > > > > > Diff: https://reviews.apache.org/r/61417/diff/1/ > > > > > > Testing > > ------- > > > > precheckin running > > > > > > Thanks, > > > > Jinmei Liao > > > > > >
