You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Venki Korukanti <ve...@gmail.com> on 2015/09/02 02:56:51 UTC
Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/
-----------------------------------------------------------
Review request for drill and Jacques Nadeau.
Bugs: DRILL-3725
https://issues.apache.org/jira/browse/DRILL-3725
Repository: drill-git
Description
-------
See DRILL-3725 for details.
Diffs
-----
pom.xml 8d4b318f26c0d9723cc1bd8d842d38e8fa7f9cea
Diff: https://reviews.apache.org/r/38036/diff/
Testing
-------
Manual testing. Working on adding tests for Web interface (DRILL-2965).
Thanks,
Venki Korukanti
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Venki Korukanti <ve...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/
-----------------------------------------------------------
(Updated Sept. 30, 2015, 7:59 a.m.)
Review request for drill and Jacques Nadeau.
Changes
-------
Addressed review comments:
1) Converted RestServerHelper to an instance class WebServer
2) Added an option to enable/disable SSL for web interface (disabled by default)
Bugs: DRILL-3725
https://issues.apache.org/jira/browse/DRILL-3725
Repository: drill-git
Description
-------
See DRILL-3725 for details.
Diffs (updated)
-----
distribution/src/resources/drill-override-example.conf 805d6e9
exec/java-exec/pom.xml cb48567
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java d54a777
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java a3f17e9
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/WebServer.java PRE-CREATION
exec/java-exec/src/main/resources/drill-module.conf dbe449a
pom.xml bc5af74
Diff: https://reviews.apache.org/r/38036/diff/
Testing
-------
Manual testing. Working on adding tests for Web interface (DRILL-2965).
Thanks,
Venki Korukanti
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Jacques Nadeau <ja...@gmail.com>.
On Sept. 30, 2015, 1:49 a.m., Venki Korukanti wrote:
> > In addition to comment above, can you confirm what will happen if a user tries to go to http://drillbit:8047?
> >
> > We should try to make that experience seamless. Is there a way to do that (basically redirect them to https://drillbit:8047)
> >
> > Beyond those changes, +1
>
> Venki Korukanti wrote:
> Problem is we can't have two protocols on the same port with support provided by jetty. If we are using the default ports (80 for http and 443 for https) we could have http://drillbit and https://drillbit working. From stackoverflow [1] there is a way to implement by overriding connector and looking at the first byte etc. This depends on some low level details of the protocol and java version.
>
> For the above reason and warning displayed for self-signed certifcates by browser, is it better to disable the https by default and provide an option?
>
> Venki Korukanti wrote:
> Forgot to add reference:
> [1] http://stackoverflow.com/questions/11182192/how-do-i-serve-https-and-http-for-jetty-from-one-port/
Yeah, I suppose we should then disable by default.
- Jacques
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/#review101062
-----------------------------------------------------------
On Sept. 2, 2015, 12:57 a.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38036/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 12:57 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3725
> https://issues.apache.org/jira/browse/DRILL-3725
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> See DRILL-3725 for details.
>
>
> Diffs
> -----
>
> distribution/src/resources/drill-override-example.conf 805d6e9
> exec/java-exec/pom.xml 601f5ca
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 140e9a8
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 3862cea
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java PRE-CREATION
> pom.xml 8d4b318
>
> Diff: https://reviews.apache.org/r/38036/diff/
>
>
> Testing
> -------
>
> Manual testing. Working on adding tests for Web interface (DRILL-2965).
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Venki Korukanti <ve...@gmail.com>.
On Sept. 29, 2015, 6:49 p.m., Venki Korukanti wrote:
> > In addition to comment above, can you confirm what will happen if a user tries to go to http://drillbit:8047?
> >
> > We should try to make that experience seamless. Is there a way to do that (basically redirect them to https://drillbit:8047)
> >
> > Beyond those changes, +1
Problem is we can't have two protocols on the same port with support provided by jetty. If we are using the default ports (80 for http and 443 for https) we could have http://drillbit and https://drillbit working. From stackoverflow [1] there is a way to implement by overriding connector and looking at the first byte etc. This depends on some low level details of the protocol and java version.
For the above reason and warning displayed for self-signed certifcates by browser, is it better to disable the https by default and provide an option?
- Venki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/#review101062
-----------------------------------------------------------
On Sept. 1, 2015, 5:57 p.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38036/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 5:57 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3725
> https://issues.apache.org/jira/browse/DRILL-3725
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> See DRILL-3725 for details.
>
>
> Diffs
> -----
>
> distribution/src/resources/drill-override-example.conf 805d6e9
> exec/java-exec/pom.xml 601f5ca
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 140e9a8
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 3862cea
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java PRE-CREATION
> pom.xml 8d4b318
>
> Diff: https://reviews.apache.org/r/38036/diff/
>
>
> Testing
> -------
>
> Manual testing. Working on adding tests for Web interface (DRILL-2965).
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Venki Korukanti <ve...@gmail.com>.
On Sept. 29, 2015, 6:49 p.m., Venki Korukanti wrote:
> > In addition to comment above, can you confirm what will happen if a user tries to go to http://drillbit:8047?
> >
> > We should try to make that experience seamless. Is there a way to do that (basically redirect them to https://drillbit:8047)
> >
> > Beyond those changes, +1
>
> Venki Korukanti wrote:
> Problem is we can't have two protocols on the same port with support provided by jetty. If we are using the default ports (80 for http and 443 for https) we could have http://drillbit and https://drillbit working. From stackoverflow [1] there is a way to implement by overriding connector and looking at the first byte etc. This depends on some low level details of the protocol and java version.
>
> For the above reason and warning displayed for self-signed certifcates by browser, is it better to disable the https by default and provide an option?
Forgot to add reference:
[1] http://stackoverflow.com/questions/11182192/how-do-i-serve-https-and-http-for-jetty-from-one-port/
- Venki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/#review101062
-----------------------------------------------------------
On Sept. 1, 2015, 5:57 p.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38036/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2015, 5:57 p.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3725
> https://issues.apache.org/jira/browse/DRILL-3725
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> See DRILL-3725 for details.
>
>
> Diffs
> -----
>
> distribution/src/resources/drill-override-example.conf 805d6e9
> exec/java-exec/pom.xml 601f5ca
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 140e9a8
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 3862cea
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java PRE-CREATION
> pom.xml 8d4b318
>
> Diff: https://reviews.apache.org/r/38036/diff/
>
>
> Testing
> -------
>
> Manual testing. Working on adding tests for Web interface (DRILL-2965).
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/#review101062
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java (line 59)
<https://reviews.apache.org/r/38036/#comment158376>
Can we change this to be an instance class. Let's call it DrillRestServer and make it AutoCloseable. Then all state can be internal. We just need to pass in the required configuration information. That way org.eclipse.jetty.server.Server is only held internally.
In addition to comment above, can you confirm what will happen if a user tries to go to http://drillbit:8047?
We should try to make that experience seamless. Is there a way to do that (basically redirect them to https://drillbit:8047)
Beyond those changes, +1
- Jacques Nadeau
On Sept. 2, 2015, 12:57 a.m., Venki Korukanti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38036/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2015, 12:57 a.m.)
>
>
> Review request for drill and Jacques Nadeau.
>
>
> Bugs: DRILL-3725
> https://issues.apache.org/jira/browse/DRILL-3725
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> See DRILL-3725 for details.
>
>
> Diffs
> -----
>
> distribution/src/resources/drill-override-example.conf 805d6e9
> exec/java-exec/pom.xml 601f5ca
> exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 140e9a8
> exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 3862cea
> exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java PRE-CREATION
> pom.xml 8d4b318
>
> Diff: https://reviews.apache.org/r/38036/diff/
>
>
> Testing
> -------
>
> Manual testing. Working on adding tests for Web interface (DRILL-2965).
>
>
> Thanks,
>
> Venki Korukanti
>
>
Re: Review Request 38036: DRILL-3725: Add HTTPS support for Drill web
interface
Posted by Venki Korukanti <ve...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38036/
-----------------------------------------------------------
(Updated Sept. 1, 2015, 5:57 p.m.)
Review request for drill and Jacques Nadeau.
Bugs: DRILL-3725
https://issues.apache.org/jira/browse/DRILL-3725
Repository: drill-git
Description
-------
See DRILL-3725 for details.
Diffs (updated)
-----
distribution/src/resources/drill-override-example.conf 805d6e9
exec/java-exec/pom.xml 601f5ca
exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java 140e9a8
exec/java-exec/src/main/java/org/apache/drill/exec/server/Drillbit.java 3862cea
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/RestServerHelper.java PRE-CREATION
pom.xml 8d4b318
Diff: https://reviews.apache.org/r/38036/diff/
Testing
-------
Manual testing. Working on adding tests for Web interface (DRILL-2965).
Thanks,
Venki Korukanti