[SPARK-19220][UI] Make redirection to HTTPS apply to all URIs. #16582

vanzin · 2017-01-13T22:43:15Z

The redirect handler was installed only for the root of the server;
any other context ended up being served directly through the HTTP
port. Since every sub page (e.g. application UIs in the history
server) is a separate servlet context, this meant that everything
but the root was accessible via HTTP still.

The change adds separate names to each connector, and binds contexts
to specific connectors so that content is only served through the
HTTPS connector when it's enabled. In that case, the only thing that
binds to the HTTP connector is the redirect handler.

Tested with new unit tests and by checking a live history server.

The redirect handler was installed only for the root of the server; any other context ended up being served directly through the HTTP port. Since every sub page (e.g. application UIs in the history server) is a separate servlet context, this meant that everything but the root was accessible via HTTP still. The change adds separate names to each connector, and binds contexts to specific connectors so that content is only served through the HTTPS connector when it's enabled. In that case, the only thing that binds to the HTTP connector is the redirect handler. Tested with new unit tests and by checking a live history server.

SparkQA · 2017-01-14T00:44:22Z

Test build #71353 has finished for PR 16582 at commit 983f490.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-01-14T03:24:03Z

Test build #71359 has finished for PR 16582 at commit 67df755.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2017-01-14T19:48:20Z

Pinging some (random?) people @ajbozarth @srowen @sarutak

ajbozarth · 2017-01-16T20:15:03Z

The code changes looks good to me, but my experience in code working with SSL is still small so someone with more experience should also double-check.

sarutak · 2017-01-17T12:09:12Z

I'll take a look at this within the weekend.

sarutak · 2017-01-20T02:15:12Z

@vanzin I'm looking into this change and it works well on standalone-mode but doesn't on yarn-mode.
I think it is because ResourceManager's web proxy might not handle https properly.
It seems httpclient in WebAppProxyServlet is not configured for SSL.
Do you have any idea?

The change itself seems good to me.

vanzin · 2017-01-20T02:25:13Z

I think it is because ResourceManager's web proxy might not handle https properly.

Yeah, that's a known issue with enabling SSL for the web UI on YARN with self-signed certificates.

sarutak · 2017-01-20T02:29:29Z

I understand. if there are no additional comments from anyone by tomorrow, I'll merge this.

sarutak · 2017-01-20T06:24:35Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

+
+          // redirect the HTTP requests to HTTPS port
+          httpConnector.setName(REDIRECT_CONNECTOR_NAME)
+          collection.addHandler(createRedirectHttpsHandler(securePort, scheme))


I noticed one point.
If a port is already used, collection.addHandler will take place more than twice leading redirection doesn't work properly.
Of course, it's not your fault. If you fix it in this PR together, it's good but it's a separate issue so I'll fix in another PR otherwise.

Actually that is caused by my change, so let me fix it.

…ful bind.

SparkQA · 2017-01-21T03:08:06Z

Test build #71748 has finished for PR 16582 at commit eb0fcb7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak

It almost LGTM.
I leave some comments and have one question.
Why not simply remove old redirect handler like collection.removeHandler ?

sarutak · 2017-01-24T05:53:03Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

+      gzipHandlers.foreach(collection.addHandler)
      server.setHandler(collection)
+
+      server.setConnectors(connectors.toArray)


Why did you move server.setConnectors(connectors.toArray) and gzipHandlers.foreach(collection.addHandler)?

Mostly for grouping. "This is where all handlers are added to the server."

In any case I have another change (#16625) that kinda moves all this stuff around anyway...

O.K. That's what I thought.

sarutak · 2017-01-24T05:57:17Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

    addFilters(handlers, conf)

    val gzipHandlers = handlers.map { h =>
+      h.setVirtualHosts(Array("@" + SPARK_CONNECTOR_NAME))


Do we need this code here? setVirtualHosts should always be called in addHandler for each handler right?

ContextHandlerCollection.addHandler does not call setVirtualHosts.

addHandler here means ServerInfo#addHandler, sorry for confusing.
And I noticed serverInfo in WebUI can be None and serverInfo.foreach(_.addHandler(handler)) is not called within WebUI#attachHandler in that case. So I understand it's reasonable change.
Can you add a test case for justification of this change?

I don't understand what you mean. Without this change the UI does not work at all. The test I added already covers it.

Without this change the UI does not work at all. The test I added already covers it.

Hmm, it's funny. I commented out this change and ran the test case you added (UISuite and UISeleniumSuite) but it passed.

Of course I understand this change is needed. I've confirmed it manually.

vanzin · 2017-01-24T18:03:54Z

Why not simply remove old redirect handler like collection.removeHandler ?

I find it cleaner to just not do something than to do it then have to undo things when they fail.

sarutak · 2017-01-25T02:51:51Z

O.K, It's reasonable.

sarutak · 2017-01-25T10:33:58Z

core/src/test/scala/org/apache/spark/ui/UISuite.scala

+
+      val (conf, sslOptions) = sslEnabledConf()
+      serverInfo = JettyUtils.startJettyServer(
+        "0.0.0.0", 0, sslOptions, Seq[ServletContextHandler](newContext("/")), conf)


To test the neseccity of setVirtualHost in JettyUtils#StartJettyServer correctly, you might need to add another ServletContextHandler instance like newContext("/test0") and corresponding assertion.

See commit 67df755 (the second commit in this PR). Tests failed before that commit.

SparkQA · 2017-01-25T23:27:27Z

Test build #72000 has finished for PR 16582 at commit 5b65c69.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2017-01-26T08:21:54Z

The latest change LGTM. Merging into master. Thanks @vanzin !

sarutak · 2017-01-26T08:27:39Z

This change cannot be applied to branch-2.0 and branch-2.1 cleanly so please open other PRs for those branches. Thanks.

vanzin · 2017-01-26T16:50:35Z

I wasn't really planning to backport this, unless someone is really interested in this functionality.

The redirect handler was installed only for the root of the server; any other context ended up being served directly through the HTTP port. Since every sub page (e.g. application UIs in the history server) is a separate servlet context, this meant that everything but the root was accessible via HTTP still. The change adds separate names to each connector, and binds contexts to specific connectors so that content is only served through the HTTPS connector when it's enabled. In that case, the only thing that binds to the HTTP connector is the redirect handler. Tested with new unit tests and by checking a live history server. Author: Marcelo Vanzin <[email protected]> Closes apache#16582 from vanzin/SPARK-19220. (cherry picked from commit d3dcb63)

The redirect handler was installed only for the root of the server; any other context ended up being served directly through the HTTP port. Since every sub page (e.g. application UIs in the history server) is a separate servlet context, this meant that everything but the root was accessible via HTTP still. The change adds separate names to each connector, and binds contexts to specific connectors so that content is only served through the HTTPS connector when it's enabled. In that case, the only thing that binds to the HTTP connector is the redirect handler. Tested with new unit tests and by checking a live history server. Author: Marcelo Vanzin <[email protected]> Closes apache#16582 from vanzin/SPARK-19220.

vanzin force-pushed the SPARK-19220 branch from 89d9dc2 to 983f490 Compare January 13, 2017 22:43

Uncomment line I disabled for debugging...

67df755

sarutak reviewed Jan 20, 2017

View reviewed changes

Make sure the handler collection only contains handler from a success…

eb0fcb7

…ful bind.

sarutak reviewed Jan 24, 2017

View reviewed changes

sarutak reviewed Jan 25, 2017

View reviewed changes

Tweak test to fail without setVirtualHosts.

5b65c69

asfgit closed this in d3dcb63 Jan 26, 2017

vanzin deleted the SPARK-19220 branch January 26, 2017 17:29

[SPARK-19220][UI] Make redirection to HTTPS apply to all URIs. #16582

[SPARK-19220][UI] Make redirection to HTTPS apply to all URIs. #16582

Uh oh!

Conversation

vanzin commented Jan 13, 2017

Uh oh!

SparkQA commented Jan 14, 2017

Uh oh!

SparkQA commented Jan 14, 2017

Uh oh!

vanzin commented Jan 14, 2017

Uh oh!

ajbozarth commented Jan 16, 2017

Uh oh!

sarutak commented Jan 17, 2017

Uh oh!

sarutak commented Jan 20, 2017

Uh oh!

vanzin commented Jan 20, 2017

Uh oh!

sarutak commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 21, 2017

Uh oh!

sarutak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarutak Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarutak commented Jan 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 25, 2017

Uh oh!

sarutak commented Jan 26, 2017

Uh oh!

sarutak commented Jan 26, 2017

Uh oh!

vanzin commented Jan 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sarutak Jan 25, 2017 •

edited

Loading

vanzin commented Jan 24, 2017 •

edited

Loading