----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69534/#review211343 -----------------------------------------------------------
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java Lines 462 (patched) <https://reviews.apache.org/r/69534/#comment296327> s/System/Java system standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Line 328 (original), 342 (patched) <https://reviews.apache.org/r/69534/#comment296334> This patch may introduce performance regression in the setConf method which is called for every new connection. MetastoreConf.getPassword is expensive since it needs to decrypt the truststore password. See HIVE-20740 which has more details. In most common cases, these configuration properties almost never change once they are set. But they are being read again and again at every new connection initialization time. I think we can improve this by caching the db and truststore password and reading it once when HMS starts. But I guess this could be separate from this JIRA. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 343 (patched) <https://reviews.apache.org/r/69534/#comment296336> may be rename this to configureSSLDeprecated standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 346 (patched) <https://reviews.apache.org/r/69534/#comment296328> This is a redundant log. If ssl is configured we have other logs below to tell us that. Note that logs printed in this method are going to be very frequent for each new HMS connection. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 354 (patched) <https://reviews.apache.org/r/69534/#comment296329> The exception message suggests taht it Disables SSL and continues, but actually, the exception is uncaught and it will terminate the connection request (and infact HMS coming up). I think you can remove "Disabling SSL and continuing." if thats not needed. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 356 (patched) <https://reviews.apache.org/r/69534/#comment296330> This comment is unclear. The getPassword method will get the encrypted password if configured right? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 367-369 (patched) <https://reviews.apache.org/r/69534/#comment296331> Can you define constants for javax.net.ssl* property keys at the class level? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 371 (patched) <https://reviews.apache.org/r/69534/#comment296332> Redundant log, please remove. standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Lines 373 (patched) <https://reviews.apache.org/r/69534/#comment296333> Don't see the code where we disable and continue. Am I missing something? standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java Line 332 (original), 398 (patched) <https://reviews.apache.org/r/69534/#comment296335> I would suggest change this log to say "Configuring SSL using a deprecated key " + ConfVars.DBACCESS_SSL_PROPS.toString() + ". This may be removed in the future. See HIVE-20992 for more details." standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java Lines 1069 (patched) <https://reviews.apache.org/r/69534/#comment296337> suggest rename to testDeprecatedConfigIsOverriden() - Vihang Karajgaonkar On Dec. 14, 2018, 1:26 a.m., Morio Ramdenbourg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69534/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2018, 1:26 a.m.) > > > Review request for hive, Adam Holley, Karthik Manamcheri, Peter Vary, and > Vihang Karajgaonkar. > > > Bugs: HIVE-20992 > https://issues.apache.org/jira/browse/HIVE-20992 > > > Repository: hive-git > > > Description > ------- > > The following new properties were added: > > 1. metastore.dbaccess.use.SSL (hive.metastore.dbaccess.use.SSL) > 2. javax.net.ssl.trustStore > 3. javax.net.ssl.trustStorePassword > 4. javax.net.ssl.trustStoreType > > This was in an effort to guide the user towards an easier SSL > configuration experience. This is the minimum requirement to set up SSL > encryption to the HMS backend store. > > This also solves the issue of the truststore password being stored in > plain text. It can now be encrypted by default and loaded through the > MetastoreConf.getPassword() method which handles secure password access > > The property "hive.metastore.dbaccess.ssl.properties" is now > deprecated, but it will still be kept for backwards-compatibility purposes. > > Modified comments to clearly reflect what is / is not deprecated > > > Diffs > ----- > > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > e25a8cf9a19d78c0cc00bb2e5e0abee4d851ad98 > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > e598a43e4dc2d2a2c25886ae7cbafd29b47c1f24 > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > 0cf113c927f2274d085e07cd72921fb35227e1f3 > > > Diff: https://reviews.apache.org/r/69534/diff/4/ > > > Testing > ------- > > Tests: > 1. Unit tests were added to cover the functionality of configuring the Java > system properties. > 2. Performed some manual and sanity tests to ensure that SSL was still > configurable to a remote DB. I performed these on MySQL, PostgreSQL, Oracle, > and Derby DB by creating generic DB hosts and setting them up with SSL. Once > SSL was set up, I triggered the metastore to perform database calls, and > captured packets using tcpdump. I then uploaded my packet captures to > Wireshark, and ensured that none of the data was human-readable. > > I plan to upload a document to our Wiki explaining the process of enabling > TLS to these databases. > > > Thanks, > > Morio Ramdenbourg > >
