[GitHub] spark pull request #21523: [SPARK-24506][UI] Add UI filters also to thriftse...
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/21523#discussion_r194722157
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -91,7 +91,12 @@ private[spark] abstract class WebUI(
/** Attach a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
-serverInfo.foreach(_.addHandler(handler))
+serverInfo.foreach { sInfo =>
--- End diff --
I see, @vanzin , thanks for checking this. Do you have this WIP as a PR? If
so, should I close this or updating according to your suggestion?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21523: [SPARK-24506][UI] Add UI filters also to thriftse...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21523#discussion_r194490802
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -91,7 +91,12 @@ private[spark] abstract class WebUI(
/** Attach a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
-serverInfo.foreach(_.addHandler(handler))
+serverInfo.foreach { sInfo =>
--- End diff --
I have a slight preference for doing this in the `ServerInfo` class, so
that we're sure we're covering all paths.
I have a WIP patch for another thing where I did that, something like this:
```
@@ -507,17 +517,19 @@ private[spark] case class ServerInfo(
server: Server,
boundPort: Int,
securePort: Option[Int],
+private val conf: SparkConf,
private val rootHandler: ContextHandlerCollection) {
- def addHandler(handler: ContextHandler): Unit = {
+ def addHandler(handler: ServletContextHandler): Unit = {
handler.setVirtualHosts(JettyUtils.toVirtualHosts(JettyUtils.SPARK_CONNECTOR_NAME))
+JettyUtils.addFilters(Seq(handler), conf)
rootHandler.addHandler(handler)
if (!handler.isStarted()) {
handler.start()
}
}
```
It also avoids a race where an already started handler would be added
before filters are installed. Extremely unlikely that would be a problem, but
well.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21523: [SPARK-24506][UI] Add UI filters also to thriftse...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21523#discussion_r194488465
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -91,7 +91,12 @@ private[spark] abstract class WebUI(
/** Attach a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
-serverInfo.foreach(_.addHandler(handler))
+serverInfo.foreach { sInfo =>
+ sInfo.addHandler(handler)
+ // If the UI has already been bound, we need to add the filters to
the newly attached
+ // handlers. Otherwise, they will be attached when binding.
+ addFilters(Seq(handler), conf)
--- End diff --
Nevermind you already removed it.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21523: [SPARK-24506][UI] Add UI filters also to thriftse...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21523#discussion_r194488116
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -91,7 +91,12 @@ private[spark] abstract class WebUI(
/** Attach a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
-serverInfo.foreach(_.addHandler(handler))
+serverInfo.foreach { sInfo =>
+ sInfo.addHandler(handler)
+ // If the UI has already been bound, we need to add the filters to
the newly attached
+ // handlers. Otherwise, they will be attached when binding.
+ addFilters(Seq(handler), conf)
--- End diff --
There's a call to `addFilters` in `HistoryServer.scala` that becomes
redundant with this code, I think.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21523: [SPARK-24506][UI] Add UI filters also to thriftse...
GitHub user mgaido91 opened a pull request: https://github.com/apache/spark/pull/21523 [SPARK-24506][UI] Add UI filters also to thriftserver tab ## What changes were proposed in this pull request? Currently, `spark.ui.filters` are not applied to the pages added in the thriftserver tab. This happens because the tab is attached after the UI is started. This can allow unauthorized access to the pages. The PR adds the filters also to the pages which are added by the thriftserver tab. ## How was this patch tested? manual tests (without the patch, starting the thriftserver with `--conf spark.ui.filters=org.apache.hadoop.security.authentication.server.AuthenticationFilter --conf spark.org.apache.hadoop.security.authentication.server.AuthenticationFilter.params="type=simple"` you can access `http://localhost:4040/sqlserver`; with the patch, 401 is the response as for the other pages). You can merge this pull request into a Git repository by running: $ git pull https://github.com/mgaido91/spark SPARK-24506 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21523.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21523 commit 086a4f62b95b455e9e789ea820b0e1f8b7e8d643 Author: Marco Gaido Date: 2018-06-10T10:44:01Z [SPARK-24506][UI] Add UI filters also to thriftserver tab --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
