Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jerryshao wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5408005
Fix redirect issue for https
jerryshao Feb 27, 2017
9ee6a09
Address the style issue
jerryshao Feb 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions core/src/main/scala/org/apache/spark/TestUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import java.util.Arrays
import java.util.concurrent.{CountDownLatch, TimeUnit}
import java.util.jar.{JarEntry, JarOutputStream}
import javax.net.ssl._
import javax.servlet.http.HttpServletResponse
import javax.tools.{JavaFileObject, SimpleJavaFileObject, ToolProvider}

import scala.collection.JavaConverters._
Expand Down Expand Up @@ -186,12 +187,12 @@ private[spark] object TestUtils {
}

/**
* Returns the response code from an HTTP(S) URL.
* Returns the response code and url (if redirected) from an HTTP(S) URL.
*/
def httpResponseCode(
def httpResponseCodeAndURL(
url: URL,
method: String = "GET",
headers: Seq[(String, String)] = Nil): Int = {
headers: Seq[(String, String)] = Nil): (Int, Option[String]) = {
val connection = url.openConnection().asInstanceOf[HttpURLConnection]
connection.setRequestMethod(method)
headers.foreach { case (k, v) => connection.setRequestProperty(k, v) }
Expand All @@ -210,16 +211,30 @@ private[spark] object TestUtils {
sslCtx.init(null, Array(trustManager), new SecureRandom())
connection.asInstanceOf[HttpsURLConnection].setSSLSocketFactory(sslCtx.getSocketFactory())
connection.asInstanceOf[HttpsURLConnection].setHostnameVerifier(verifier)
connection.setInstanceFollowRedirects(false)
}

try {
connection.connect()
connection.getResponseCode()
if (connection.getResponseCode == HttpServletResponse.SC_FOUND) {
(connection.getResponseCode, Option(connection.getHeaderField("Location")))
} else {
(connection.getResponseCode(), None)
}
} finally {
connection.disconnect()
}
}

/**
* Returns the response code from an HTTP(S) URL.
*/
def httpResponseCode(
url: URL,
method: String = "GET",
headers: Seq[(String, String)] = Nil): Int = {
httpResponseCodeAndURL(url, method, headers)._1
}
}


Expand Down
9 changes: 5 additions & 4 deletions core/src/main/scala/org/apache/spark/ui/JettyUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

@vanzin vanzin Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jerryshao jerryshao Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

@vanzin vanzin Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Some(connector)

case None =>
Expand Down Expand Up @@ -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 = {
Copy link
Contributor

@vanzin vanzin Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

val redirectHandler: ContextHandler = new ContextHandler
redirectHandler.setContextPath("/")
redirectHandler.setVirtualHosts(Array("@" + REDIRECT_CONNECTOR_NAME))
Expand All @@ -391,8 +392,8 @@ private[spark] object JettyUtils extends Logging {
if (baseRequest.isSecure) {
return
}
val httpsURI = createRedirectURI(scheme, baseRequest.getServerName, securePort,
baseRequest.getRequestURI, baseRequest.getQueryString)
val httpsURI = createRedirectURI(scheme, baseRequest.getServerName,
httpsConnector.getLocalPort, baseRequest.getRequestURI, baseRequest.getQueryString)
response.setContentLength(0)
response.encodeRedirectURL(httpsURI)
response.sendRedirect(httpsURI)
Expand Down
7 changes: 5 additions & 2 deletions core/src/test/scala/org/apache/spark/ui/UISuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

@vanzin vanzin Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val (rc, redirectUrl) = ...

assert(rc._1 === expected, s"Unexpected status $rc for $url")
if (rc._1 == HttpServletResponse.SC_FOUND) {
assert(TestUtils.httpResponseCode(new URL(rc._2.get)) === HttpServletResponse.SC_OK)
}
}
}
} finally {
Expand Down