-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-19220][UI] Make redirection to HTTPS apply to all URIs. #16582
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.
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
Changes from all commits
983f490
67df755
eb0fcb7
5b65c69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,9 @@ import org.apache.spark.util.Utils | |
| */ | ||
| private[spark] object JettyUtils extends Logging { | ||
|
|
||
| val SPARK_CONNECTOR_NAME = "Spark" | ||
| val REDIRECT_CONNECTOR_NAME = "HttpsRedirect" | ||
|
|
||
| // Base type for a function that returns something based on an HTTP request. Allows for | ||
| // implicit conversion from many types of functions to jetty Handlers. | ||
| type Responder[T] = HttpServletRequest => T | ||
|
|
@@ -274,25 +277,28 @@ private[spark] object JettyUtils extends Logging { | |
| conf: SparkConf, | ||
| serverName: String = ""): ServerInfo = { | ||
|
|
||
| val collection = new ContextHandlerCollection | ||
| addFilters(handlers, conf) | ||
|
|
||
| val gzipHandlers = handlers.map { h => | ||
| h.setVirtualHosts(Array("@" + SPARK_CONNECTOR_NAME)) | ||
|
|
||
| val gzipHandler = new GzipHandler | ||
| gzipHandler.setHandler(h) | ||
| gzipHandler | ||
| } | ||
|
|
||
| // Bind to the given port, or throw a java.net.BindException if the port is occupied | ||
| def connect(currentPort: Int): (Server, Int) = { | ||
| def connect(currentPort: Int): ((Server, Option[Int]), Int) = { | ||
| val pool = new QueuedThreadPool | ||
| if (serverName.nonEmpty) { | ||
| pool.setName(serverName) | ||
| } | ||
| pool.setDaemon(true) | ||
|
|
||
| val server = new Server(pool) | ||
| val connectors = new ArrayBuffer[ServerConnector] | ||
| val connectors = new ArrayBuffer[ServerConnector]() | ||
| val collection = new ContextHandlerCollection | ||
|
|
||
| // Create a connector on port currentPort to listen for HTTP requests | ||
| val httpConnector = new ServerConnector( | ||
| server, | ||
|
|
@@ -306,26 +312,33 @@ private[spark] object JettyUtils extends Logging { | |
| httpConnector.setPort(currentPort) | ||
| connectors += httpConnector | ||
|
|
||
| sslOptions.createJettySslContextFactory().foreach { factory => | ||
| // If the new port wraps around, do not try a privileged port. | ||
| val securePort = | ||
| if (currentPort != 0) { | ||
| (currentPort + 400 - 1024) % (65536 - 1024) + 1024 | ||
| } else { | ||
| 0 | ||
| } | ||
| val scheme = "https" | ||
| // Create a connector on port securePort to listen for HTTPS requests | ||
| val connector = new ServerConnector(server, factory) | ||
| connector.setPort(securePort) | ||
|
|
||
| connectors += connector | ||
|
|
||
| // redirect the HTTP requests to HTTPS port | ||
| collection.addHandler(createRedirectHttpsHandler(securePort, scheme)) | ||
| val httpsConnector = sslOptions.createJettySslContextFactory() match { | ||
| case Some(factory) => | ||
| // If the new port wraps around, do not try a privileged port. | ||
| val securePort = | ||
| if (currentPort != 0) { | ||
| (currentPort + 400 - 1024) % (65536 - 1024) + 1024 | ||
| } else { | ||
| 0 | ||
| } | ||
| val scheme = "https" | ||
| // Create a connector on port securePort to listen for HTTPS requests | ||
| val connector = new ServerConnector(server, factory) | ||
| connector.setPort(securePort) | ||
| connector.setName(SPARK_CONNECTOR_NAME) | ||
| connectors += connector | ||
|
|
||
| // redirect the HTTP requests to HTTPS port | ||
| httpConnector.setName(REDIRECT_CONNECTOR_NAME) | ||
| collection.addHandler(createRedirectHttpsHandler(securePort, scheme)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed one point.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually that is caused by my change, so let me fix it. |
||
| Some(connector) | ||
|
|
||
| case None => | ||
| // No SSL, so the HTTP connector becomes the official one where all contexts bind. | ||
| httpConnector.setName(SPARK_CONNECTOR_NAME) | ||
| None | ||
| } | ||
|
|
||
| gzipHandlers.foreach(collection.addHandler) | ||
| // As each acceptor and each selector will use one thread, the number of threads should at | ||
| // least be the number of acceptors and selectors plus 1. (See SPARK-13776) | ||
| var minThreads = 1 | ||
|
|
@@ -337,17 +350,20 @@ private[spark] object JettyUtils extends Logging { | |
| // The number of selectors always equals to the number of acceptors | ||
| minThreads += connector.getAcceptors * 2 | ||
| } | ||
| server.setConnectors(connectors.toArray) | ||
| pool.setMaxThreads(math.max(pool.getMaxThreads, minThreads)) | ||
|
|
||
| val errorHandler = new ErrorHandler() | ||
| errorHandler.setShowStacks(true) | ||
| errorHandler.setServer(server) | ||
| server.addBean(errorHandler) | ||
|
|
||
| gzipHandlers.foreach(collection.addHandler) | ||
| server.setHandler(collection) | ||
|
|
||
| server.setConnectors(connectors.toArray) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you move
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. O.K. That's what I thought. |
||
| try { | ||
| server.start() | ||
| (server, httpConnector.getLocalPort) | ||
| ((server, httpsConnector.map(_.getLocalPort())), httpConnector.getLocalPort) | ||
| } catch { | ||
| case e: Exception => | ||
| server.stop() | ||
|
|
@@ -356,13 +372,16 @@ private[spark] object JettyUtils extends Logging { | |
| } | ||
| } | ||
|
|
||
| val (server, boundPort) = Utils.startServiceOnPort[Server](port, connect, conf, serverName) | ||
| ServerInfo(server, boundPort, collection) | ||
| val ((server, securePort), boundPort) = Utils.startServiceOnPort(port, connect, conf, | ||
| serverName) | ||
| ServerInfo(server, boundPort, securePort, | ||
| server.getHandler().asInstanceOf[ContextHandlerCollection]) | ||
| } | ||
|
|
||
| private def createRedirectHttpsHandler(securePort: Int, scheme: String): ContextHandler = { | ||
| val redirectHandler: ContextHandler = new ContextHandler | ||
| redirectHandler.setContextPath("/") | ||
| redirectHandler.setVirtualHosts(Array("@" + REDIRECT_CONNECTOR_NAME)) | ||
| redirectHandler.setHandler(new AbstractHandler { | ||
| override def handle( | ||
| target: String, | ||
|
|
@@ -442,7 +461,23 @@ private[spark] object JettyUtils extends Logging { | |
| private[spark] case class ServerInfo( | ||
| server: Server, | ||
| boundPort: Int, | ||
| rootHandler: ContextHandlerCollection) { | ||
| securePort: Option[Int], | ||
| private val rootHandler: ContextHandlerCollection) { | ||
|
|
||
| def addHandler(handler: ContextHandler): Unit = { | ||
| handler.setVirtualHosts(Array("@" + JettyUtils.SPARK_CONNECTOR_NAME)) | ||
| rootHandler.addHandler(handler) | ||
| if (!handler.isStarted()) { | ||
| handler.start() | ||
| } | ||
| } | ||
|
|
||
| def removeHandler(handler: ContextHandler): Unit = { | ||
| rootHandler.removeHandler(handler) | ||
| if (handler.isStarted) { | ||
| handler.stop() | ||
| } | ||
| } | ||
|
|
||
| def stop(): Unit = { | ||
| server.stop() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this code here?
setVirtualHostsshould always be called inaddHandlerfor each handler right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContextHandlerCollection.addHandlerdoes not callsetVirtualHosts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addHandlerhere meansServerInfo#addHandler, sorry for confusing.And I noticed
serverInfoinWebUIcan beNoneandserverInfo.foreach(_.addHandler(handler))is not called withinWebUI#attachHandlerin that case. So I understand it's reasonable change.Can you add a test case for justification of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean. Without this change the UI does not work at all. The test I added already covers it.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it's funny. I commented out this change and ran the test case you added (UISuite and UISeleniumSuite) but it passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course I understand this change is needed. I've confirmed it manually.