[SPARK-19750][UI][branch-2.1] Fix redirect issue from http to https #17083

jerryshao · 2017-02-27T10:48:12Z

What changes were proposed in this pull request?

If spark ui port (4040) is not set, it will choose port number 0, this will make https port to also choose 0. And in Spark 2.1 code, it will use this https port (0) to do redirect, so when redirect triggered, it will point to a wrong url:

like:

/tmp/temp$ wget http://172.27.25.134:55015
--2017-02-23 12:13:54--  http://172.27.25.134:55015/
Connecting to 172.27.25.134:55015... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://172.27.25.134:0/ [following]
--2017-02-23 12:13:54--  https://172.27.25.134:0/
Connecting to 172.27.25.134:0... failed: Can't assign requested address.
Retrying.

--2017-02-23 12:13:55--  (try: 2)  https://172.27.25.134:0/
Connecting to 172.27.25.134:0... failed: Can't assign requested address.
Retrying.

--2017-02-23 12:13:57--  (try: 3)  https://172.27.25.134:0/
Connecting to 172.27.25.134:0... failed: Can't assign requested address.
Retrying.

--2017-02-23 12:14:00--  (try: 4)  https://172.27.25.134:0/
Connecting to 172.27.25.134:0... failed: Can't assign requested address.
Retrying.

So instead of using 0 to do redirect, we should pick a bound port instead.

This issue only exists in Spark 2.1-, and can be reproduced in yarn cluster mode.

How was this patch tested?

Current redirect UT doesn't verify this issue, so extend current UT to do correct verification.

Change-Id: I5306a7553c230811dcada4d9c205b82b2af77c6e

srowen · 2017-02-27T10:56:08Z

Was this fixed otherwise in master, or did some other change make it obsolete? just trying to link this to whatever reason it's only a problem in 2.1, for the record.

jerryshao · 2017-02-27T11:24:38Z

Due to the change of (#16625), the issue is obsolete. So it effects spark 2.1 and 2.0.

jerryshao · 2017-02-27T11:37:32Z

Not sure why Jenkins test cannot be started automatically.

vanzin · 2017-02-27T18:38:10Z

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

@@ -378,7 +378,8 @@ private[spark] object JettyUtils extends Logging {
      server.getHandler().asInstanceOf[ContextHandlerCollection])
  }

-  private def createRedirectHttpsHandler(securePort: Int, scheme: String): ContextHandler = {
+  private def createRedirectHttpsHandler(
+      httpsConnector: ServerConnector, scheme: String): ContextHandler = {


nit: one argument per line when using multiple lines.

But instead of changing this, why not pass the correct port from the caller in the first place?

vanzin · 2017-02-27T18:39:37Z

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

@@ -267,8 +267,11 @@ class UISuite extends SparkFunSuite {
          s"$scheme://localhost:$port/test1/root",
          s"$scheme://localhost:$port/test2/root")
        urls.foreach { url =>
-          val rc = TestUtils.httpResponseCode(new URL(url))
-          assert(rc === expected, s"Unexpected status $rc for $url")
+          val rc = TestUtils.httpResponseCodeAndURL(new URL(url))


val (rc, redirectUrl) = ...

SparkQA · 2017-02-27T19:27:27Z

Test build #73498 has finished for PR 17083 at commit 5408005.

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

Change-Id: I9d10be4fa9d6bc932422ba97574f561d3b0bb430

SparkQA · 2017-02-28T04:35:24Z

Test build #73551 has finished for PR 17083 at commit 9ee6a09.

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

jerryshao · 2017-03-02T07:44:53Z

Ping @vanzin , do you have any further comments? Thanks a lot.

vanzin · 2017-03-02T18:40:39Z

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

@@ -330,7 +330,7 @@ private[spark] object JettyUtils extends Logging {

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


So is there a reason why you didn't make this a single line change to pass the correct port?

@vanzin , AFAIK in this code, at that time when we create a redirect handler, the https ServerConnector hasn't yet started, so we couldn't get the real port from it, it will return -1 as we call getLocalPort.

Ah, good point. I guess I was thinking about the code in master which starts the connector right away.

vanzin · 2017-03-03T01:18:30Z

Merging to 2.1 / 2.0.

## What changes were proposed in this pull request? If spark ui port (4040) is not set, it will choose port number 0, this will make https port to also choose 0. And in Spark 2.1 code, it will use this https port (0) to do redirect, so when redirect triggered, it will point to a wrong url: like: ``` /tmp/temp$ wget http://172.27.25.134:55015 --2017-02-23 12:13:54-- http://172.27.25.134:55015/ Connecting to 172.27.25.134:55015... connected. HTTP request sent, awaiting response... 302 Found Location: https://172.27.25.134:0/ [following] --2017-02-23 12:13:54-- https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:13:55-- (try: 2) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:13:57-- (try: 3) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:14:00-- (try: 4) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. ``` So instead of using 0 to do redirect, we should pick a bound port instead. This issue only exists in Spark 2.1-, and can be reproduced in yarn cluster mode. ## How was this patch tested? Current redirect UT doesn't verify this issue, so extend current UT to do correct verification. Author: jerryshao <sshao@hortonworks.com> Closes #17083 from jerryshao/SPARK-19750.

vanzin · 2017-03-03T01:19:25Z

@jerryshao I think you'll need to manually close the PR.

## What changes were proposed in this pull request? If spark ui port (4040) is not set, it will choose port number 0, this will make https port to also choose 0. And in Spark 2.1 code, it will use this https port (0) to do redirect, so when redirect triggered, it will point to a wrong url: like: ``` /tmp/temp$ wget http://172.27.25.134:55015 --2017-02-23 12:13:54-- http://172.27.25.134:55015/ Connecting to 172.27.25.134:55015... connected. HTTP request sent, awaiting response... 302 Found Location: https://172.27.25.134:0/ [following] --2017-02-23 12:13:54-- https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:13:55-- (try: 2) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:13:57-- (try: 3) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. --2017-02-23 12:14:00-- (try: 4) https://172.27.25.134:0/ Connecting to 172.27.25.134:0... failed: Can't assign requested address. Retrying. ``` So instead of using 0 to do redirect, we should pick a bound port instead. This issue only exists in Spark 2.1-, and can be reproduced in yarn cluster mode. ## How was this patch tested? Current redirect UT doesn't verify this issue, so extend current UT to do correct verification. Author: jerryshao <sshao@hortonworks.com> Closes #17083 from jerryshao/SPARK-19750. (cherry picked from commit 3a7591a) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

jerryshao · 2017-03-03T01:20:29Z

OK, Thanks @vanzin .

Fix redirect issue for https

5408005

Change-Id: I5306a7553c230811dcada4d9c205b82b2af77c6e

vanzin reviewed Feb 27, 2017

View reviewed changes

Address the style issue

9ee6a09

Change-Id: I9d10be4fa9d6bc932422ba97574f561d3b0bb430

vanzin reviewed Mar 2, 2017

View reviewed changes

jerryshao closed this Mar 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19750][UI][branch-2.1] Fix redirect issue from http to https #17083

[SPARK-19750][UI][branch-2.1] Fix redirect issue from http to https #17083

jerryshao commented Feb 27, 2017 •

edited

Loading

srowen commented Feb 27, 2017

jerryshao commented Feb 27, 2017

jerryshao commented Feb 27, 2017

vanzin Feb 27, 2017

vanzin Feb 27, 2017

SparkQA commented Feb 27, 2017

SparkQA commented Feb 28, 2017

jerryshao commented Mar 2, 2017

vanzin Mar 2, 2017

jerryshao Mar 3, 2017

vanzin Mar 3, 2017

vanzin commented Mar 3, 2017

vanzin commented Mar 3, 2017

jerryshao commented Mar 3, 2017

[SPARK-19750][UI][branch-2.1] Fix redirect issue from http to https #17083

[SPARK-19750][UI][branch-2.1] Fix redirect issue from http to https #17083

Conversation

jerryshao commented Feb 27, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

srowen commented Feb 27, 2017

jerryshao commented Feb 27, 2017

jerryshao commented Feb 27, 2017

vanzin Feb 27, 2017

Choose a reason for hiding this comment

vanzin Feb 27, 2017

Choose a reason for hiding this comment

SparkQA commented Feb 27, 2017

SparkQA commented Feb 28, 2017

jerryshao commented Mar 2, 2017

vanzin Mar 2, 2017

Choose a reason for hiding this comment

jerryshao Mar 3, 2017

Choose a reason for hiding this comment

vanzin Mar 3, 2017

Choose a reason for hiding this comment

vanzin commented Mar 3, 2017

vanzin commented Mar 3, 2017

jerryshao commented Mar 3, 2017

jerryshao commented Feb 27, 2017 •

edited

Loading