Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 25, 2019, 7:59 p.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Changes
---

Made a few phrasing changes based on Vihang's feedback


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description (updated)
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore and directly adding the trusted certificates to it. The 
previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
- Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
- Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
- Removed the unit test that checks for an empty truststore path.


Diffs (updated)
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/4/

Changes: https://reviews.apache.org/r/69834/diff/3-4/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Morio Ramdenbourg via Review Board 


> On Jan. 25, 2019, 7:41 p.m., Vihang Karajgaonkar wrote:
> > Some minor comments related to logs. Rest looks good.

Thanks for the feedback


> On Jan. 25, 2019, 7:41 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Lines 472 (patched)
> > 
> >
> > Nit, It think it would be useful to say "Defaults to jssecacerts, if it 
> > exists, otherwise uses cacerts"

Done


> On Jan. 25, 2019, 7:41 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Lines 362 (patched)
> > 
> >
> > May be its useful to specify what is the default. So a message like ".. 
> > has not been set. Defaulting to jssecacerts, if it exists. Otherwise, 
> > cacerts."

Done


> On Jan. 25, 2019, 7:41 p.m., Vihang Karajgaonkar wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 369 (original), 371 (patched)
> > 
> >
> > nit, instead of defaulting to default .. may be just say Using default 
> > Java truststore password.

Done


- Morio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212345
---


On Jan. 25, 2019, 7:22 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 25, 2019, 7:22 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
> 
> Modified the TLS properties in the following ways:
>  - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
>  - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
>  - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/3/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Vihang Karajgaonkar via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212345
---


Fix it, then Ship it!




Some minor comments related to logs. Rest looks good.


standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Lines 472 (patched)


Nit, It think it would be useful to say "Defaults to jssecacerts, if it 
exists, otherwise uses cacerts"



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Lines 362 (patched)


May be its useful to specify what is the default. So a message like ".. has 
not been set. Defaulting to jssecacerts, if it exists. Otherwise, cacerts."



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 369 (original), 371 (patched)


nit, instead of defaulting to default .. may be just say Using default Java 
truststore password.


- Vihang Karajgaonkar


On Jan. 25, 2019, 7:22 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 25, 2019, 7:22 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
> 
> Modified the TLS properties in the following ways:
>  - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
>  - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
>  - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/3/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Morio Ramdenbourg via Review Board 


> On Jan. 25, 2019, 5:08 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
> > Line 1082 (original)
> > 
> >
> > Add a test to make sure that we don't throw an exception if the 
> > truststore path and password is empty.

Done


- Morio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212335
---


On Jan. 25, 2019, 1:38 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 25, 2019, 1:38 a.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
>   
> Modified the TLS properties in the following ways:
> - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
> - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
> - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/2/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 25, 2019, 7:22 p.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Changes
---

Added a unit test to ensure that an empty truststore path/password does not 
throw an exception based on Karthik's feedback, and improved the comments


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description (updated)
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore and directly adding the trusted certificates to it. The 
previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
 - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
 - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
 - Removed the unit test that checks for an empty truststore path.


Diffs (updated)
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/3/

Changes: https://reviews.apache.org/r/69834/diff/2-3/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-25 Thread Karthik Manamcheri via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212335
---




standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
Line 1082 (original)


Add a test to make sure that we don't throw an exception if the truststore 
path and password is empty.


- Karthik Manamcheri


On Jan. 25, 2019, 1:38 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 25, 2019, 1:38 a.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
>   
> Modified the TLS properties in the following ways:
> - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
> - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
> - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/2/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 


> On Jan. 24, 2019, 11:21 p.m., Karthik Manamcheri wrote:
> >

Thanks for the feedback


> On Jan. 24, 2019, 11:21 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Line 465 (original), 466 (patched)
> > 
> >
> > The statement about exposing it in a way to be discovered by others is 
> > confusing. Consider rephrasing. Instead of providing an inadvisable 
> > statement, provide an advice instead on what the user should be doing.

Done


> On Jan. 24, 2019, 11:21 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
> > Line 470 (original), 472 (patched)
> > 
> >
> > Can you just say, "Defaults to the default Java truststore file..." 
> > similar to the pattern of other properties.

Done


> On Jan. 24, 2019, 11:21 p.m., Karthik Manamcheri wrote:
> > standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
> > Line 360 (original), 360 (patched)
> > 
> >
> > Can we do a LOG.info here stating that the truststore path has not been 
> > set and we will default to the Java truststore file?
> > 
> > Same for the truststore password.

Done


- Morio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212295
---


On Jan. 25, 2019, 1:38 a.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 25, 2019, 1:38 a.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
>   
> Modified the TLS properties in the following ways:
> - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
> - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
> - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/2/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 25, 2019, 1:38 a.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description (updated)
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore and directly adding the trusted certificates to it. The 
previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
- Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
- Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
- Removed the unit test that checks for an empty truststore path.


Diffs
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/2/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 25, 2019, 1:38 a.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Changes
---

Made changes to phrasing and logging


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description (updated)
---

Modified phrasing and added logs for default behavior


Diffs (updated)
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/2/

Changes: https://reviews.apache.org/r/69834/diff/1-2/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Karthik Manamcheri via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/#review212295
---




standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Line 465 (original), 466 (patched)


The statement about exposing it in a way to be discovered by others is 
confusing. Consider rephrasing. Instead of providing an inadvisable statement, 
provide an advice instead on what the user should be doing.



standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
Line 470 (original), 472 (patched)


Can you just say, "Defaults to the default Java truststore file..." similar 
to the pattern of other properties.



standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
Line 360 (original), 360 (patched)


Can we do a LOG.info here stating that the truststore path has not been set 
and we will default to the Java truststore file?

Same for the truststore password.


- Karthik Manamcheri


On Jan. 24, 2019, 11:16 p.m., Morio Ramdenbourg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69834/
> ---
> 
> (Updated Jan. 24, 2019, 11:16 p.m.)
> 
> 
> Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
> Karajgaonkar.
> 
> 
> Bugs: HIVE-21083
> https://issues.apache.org/jira/browse/HIVE-21083
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> It was identified that a valid way of configuring TLS is by using the Java 
> default truststore and directly adding the trusted certificates to it. The 
> previous HMS implementation did not support this.
> 
> Modified the TLS properties in the following ways:
> - Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
> user does not specify a custom one, then it will default to the Java 
> truststore.
> - Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
> This used to generate a lot of noise if the user did not provide one. Also, 
> the contents of the truststore is certificates, which is public information 
> and doesn't require strict security.
> - Removed the unit test that checks for an empty truststore path.
> 
> 
> Diffs
> -
> 
>   
> standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
>  75f0c0a356f3b894408aa54b9cce5220d47d7f26 
>   
> standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
>  9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
>   
> standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
>  29738ba19b0d5ed9ec224d2288c0c1c922d0674c 
> 
> 
> Diff: https://reviews.apache.org/r/69834/diff/1/
> 
> 
> Testing
> ---
> 
> - Existing unit test coverage
> - Manual testing by verifying that these properties can configure TLS to a 
> MySQL DB
> 
> 
> Thanks,
> 
> Morio Ramdenbourg
> 
>

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 24, 2019, 11:16 p.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description (updated)
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore and directly adding the trusted certificates to it. The 
previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
- Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
- Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
- Removed the unit test that checks for an empty truststore path.


Diffs
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/1/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Re: Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

(Updated Jan. 24, 2019, 11:13 p.m.)


Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Bugs: HIVE-21083
https://issues.apache.org/jira/browse/HIVE-21083


Repository: hive-git


Description
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore. The previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
- Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
- Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
- Removed the unit test that checks for an empty truststore path.


Diffs
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/1/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg

Review Request 69834: HIVE-21083: Removed the truststore location property requirement and removed the warnings on the truststore password property

2019-01-24 Thread Morio Ramdenbourg via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69834/
---

Review request for hive, Adam Holley, Karthik Manamcheri, Na Li, and Vihang 
Karajgaonkar.


Repository: hive-git


Description
---

It was identified that a valid way of configuring TLS is by using the Java 
default truststore. The previous HMS implementation did not support this.

Modified the TLS properties in the following ways:
- Removed the requirement for metastore.dbaccess.ssl.truststore.path. If the 
user does not specify a custom one, then it will default to the Java truststore.
- Removed the logs / warnings on metastore.dbaccess.ssl.truststore.password. 
This used to generate a lot of noise if the user did not provide one. Also, the 
contents of the truststore is certificates, which is public information and 
doesn't require strict security.
- Removed the unit test that checks for an empty truststore path.


Diffs
-

  
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java
 75f0c0a356f3b894408aa54b9cce5220d47d7f26 
  
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
 9f721243c94d48eef35acdcbd0c2e143ab6d23ec 
  
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
 29738ba19b0d5ed9ec224d2288c0c1c922d0674c 


Diff: https://reviews.apache.org/r/69834/diff/1/


Testing
---

- Existing unit test coverage
- Manual testing by verifying that these properties can configure TLS to a 
MySQL DB


Thanks,

Morio Ramdenbourg