> On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > >
Thanks for the feedback. > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java > > Lines 462 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113971#file2113971line462> > > > > s/System/Java system Done > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > 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/diff/4/?file=2113972#file2113972line342> > > > > 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. Thanks for pointing this out. That's true, I do see this as a potential performance regression. Though as you had previously identified, the database password is also being read in the same manner during every new connection initialization. I've created HIVE-21047 to address this. > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 343 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line343> > > > > may be rename this to configureSSLDeprecated Done > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 346 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line346> > > > > 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. Removed > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 354 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line354> > > > > 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. Removed > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 356 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line356> > > > > This comment is unclear. The getPassword method will get the encrypted > > password if configured right? Correct. I've updated the comment to better reflect what getPassword() does. > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 367-369 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line367> > > > > Can you define constants for javax.net.ssl* property keys at the class > > level? Done > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 371 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line371> > > > > Redundant log, please remove. Removed > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java > > Lines 373 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113972#file2113972line373> > > > > Don't see the code where we disable and continue. Am I missing > > something? I don't think I worded that exception correctly. I more of meant to say that SSL encrypted communication to the DB will not be occuring. But after looking at the code again, this exception being thrown will terminate the connection request either way. I removed the bit where it says "Disabling SSL and continuing" > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > 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/diff/4/?file=2113972#file2113972line398> > > > > 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." Done > On Dec. 14, 2018, 10:43 p.m., Vihang Karajgaonkar wrote: > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > > Lines 1069 (patched) > > <https://reviews.apache.org/r/69534/diff/4/?file=2113973#file2113973line1069> > > > > suggest rename to testDeprecatedConfigIsOverriden() Done - Morio ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69534/#review211343 ----------------------------------------------------------- On Dec. 16, 2018, 3:11 a.m., Morio Ramdenbourg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69534/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2018, 3:11 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. > > > 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 > 3fa21b768cd120cd89343c9ccd142d5e2ccdef2e > > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java > 0cf113c927f2274d085e07cd72921fb35227e1f3 > > > Diff: https://reviews.apache.org/r/69534/diff/5/ > > > 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 > >
