sunchao commented on code in PR #58243:
URL: https://github.com/apache/spark/pull/58243#discussion_r3926551464
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/MasterWebUI.scala:
##########
@@ -64,10 +63,19 @@ class MasterWebUI(
addStaticHandler(MasterWebUI.STATIC_RESOURCE_DIR)
addRenderLogHandler(this, master.conf)
if (killEnabled) {
+ val isProxy = master.conf.get(UI_REVERSE_PROXY) &&
+ master.conf.get(PROXY_REDIRECT_URI).isEmpty
+ val killRedirectTarget = if (isProxy) {
+ master.conf.get(UI_REVERSE_PROXY_URL).map(_.stripSuffix("/") +
"/").getOrElse("/")
Review Comment:
[P2] Avoid doubling path prefixes in frontend-proxy kill redirects
With `spark.ui.reverseProxy=true`, `spark.ui.reverseProxyUrl=/spark`, and no
`spark.ui.proxyRedirectUri`, this passes `/spark/` to `createRedirectHandler`,
which resolves it to `http://<master>/spark/`. A frontend configured as
`location /spark/ { proxy_pass http://<master>/; }` then applies its normal
redirect rewriting and adds `/spark/` again. An actual nginx probe returned
`/spark/spark/` on this revision, versus `/spark/` on the parent, breaking
navigation after a kill. This affects both kill endpoints and the documented
path-only proxy URL configuration. Please preserve the server-relative public
destination or otherwise avoid embedding the public prefix in an
upstream-absolute redirect that the frontend rewrites.
##########
core/src/test/scala/org/apache/spark/deploy/master/ui/MasterWebUISuite.scala:
##########
@@ -128,6 +128,78 @@ class MasterWebUISuite extends SparkFunSuite {
denyWebUI.stop()
}
}
+
+ test("SPARK-58893: kill application redirect location with reverse proxy") {
+ val reverseProxyConf = new SparkConf()
+ .set(DECOMMISSION_ENABLED, true)
+ .set(UI_REVERSE_PROXY, true)
+ .set(UI_REVERSE_PROXY_URL, "http://proxyhost:8080/myproxy";)
+ val mockMaster = mock(classOf[Master])
+ when(mockMaster.securityMgr).thenReturn(securityMgr)
+ when(mockMaster.conf).thenReturn(reverseProxyConf)
+ when(mockMaster.rpcEnv).thenReturn(rpcEnv)
+ when(mockMaster.self).thenReturn(masterEndpointRef)
+
+ val activeApp = new ApplicationInfo(
+ new Date().getTime, "app-proxy-0", createAppDesc(), new Date(), null,
Int.MaxValue)
+ val appMap = HashMap[String, ApplicationInfo]((activeApp.id, activeApp))
+ when(mockMaster.idToApp).thenReturn(appMap)
+
+ val webUI = new MasterWebUI(mockMaster, 0)
+ try {
+ webUI.bind()
+ val url =
s"http://${Utils.localHostNameForURI()}:${webUI.boundPort}/app/kill/"
+ val body = convPostDataToString(Map(("id", activeApp.id), ("terminate",
"true")))
+ val conn = new
URI(url).toURL.openConnection().asInstanceOf[HttpURLConnection]
+ conn.setInstanceFollowRedirects(false)
+ conn.setRequestMethod("POST")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type",
"application/x-www-form-urlencoded")
+ val out = new DataOutputStream(conn.getOutputStream)
+ out.write(body.getBytes(StandardCharsets.UTF_8))
+ out.close()
+ assert(conn.getResponseCode === 302)
+ assert(conn.getHeaderField("Location") ===
"http://proxyhost:8080/myproxy/";)
+ } finally {
+ webUI.stop()
+ }
+ }
+
+ test("SPARK-58893: honor reverseProxy=false when choosing kill redirect") {
+ val noProxyConf = new SparkConf()
+ .set(DECOMMISSION_ENABLED, true)
+ .set(UI_REVERSE_PROXY, false)
+ .set(UI_REVERSE_PROXY_URL, "http://proxyhost:8080/myproxy";)
+ val mockMaster = mock(classOf[Master])
+ when(mockMaster.securityMgr).thenReturn(securityMgr)
+ when(mockMaster.conf).thenReturn(noProxyConf)
+ when(mockMaster.rpcEnv).thenReturn(rpcEnv)
+ when(mockMaster.self).thenReturn(masterEndpointRef)
+
+ val activeApp = new ApplicationInfo(
+ new Date().getTime, "app-proxy-1", createAppDesc(), new Date(), null,
Int.MaxValue)
+ val appMap = HashMap[String, ApplicationInfo]((activeApp.id, activeApp))
+ when(mockMaster.idToApp).thenReturn(appMap)
+
+ val webUI = new MasterWebUI(mockMaster, 0)
+ try {
+ webUI.bind()
+ val url =
s"http://${Utils.localHostNameForURI()}:${webUI.boundPort}/app/kill/"
+ val body = convPostDataToString(Map(("id", activeApp.id), ("terminate",
"true")))
+ val conn = new
URI(url).toURL.openConnection().asInstanceOf[HttpURLConnection]
+ conn.setInstanceFollowRedirects(false)
+ conn.setRequestMethod("POST")
+ conn.setDoOutput(true)
+ conn.setRequestProperty("Content-Type",
"application/x-www-form-urlencoded")
+ val out = new DataOutputStream(conn.getOutputStream)
+ out.write(body.getBytes(StandardCharsets.UTF_8))
+ out.close()
+ assert(conn.getResponseCode === 302)
+ assert(conn.getHeaderField("Location") === "/")
Review Comment:
[P2] Expect the absolute local root in the disabled-proxy test
`createRedirectHandler` resolves `/` against the request URL before calling
`sendRedirect`, so this response contains `http://<local-host>:<bound-port>/`,
not the literal `/`. The new assertion therefore fails even when disabled-proxy
behavior is correct. This is present in the current CI annotations and
reproduced locally: the suite passed six tests and failed this one with
`http://localhost:<port>/` versus `/`. Please assert the absolute local root,
or compare both the redirect origin and its `/` path.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -2405,6 +2416,12 @@ class SparkContext(config: SparkConf) extends Logging {
ResourceProfile.clearDefaultProfile()
// Unset YARN mode system env variable, to allow switching between cluster
types.
SparkContext.clearActiveContext()
+ if (_conf.get(UI_REVERSE_PROXY)) {
+ _previousProxyBase match {
+ case Some(oldBase) => System.setProperty("spark.ui.proxyBase", oldBase)
+ case None => System.clearProperty("spark.ui.proxyBase")
Review Comment:
[P2] Preserve the proxy base when startup fails before the snapshot
`_previousProxyBase` remains `None` until initialization reaches the proxy
setup block. If construction fails earlier, the exception handler still invokes
`stop()`, and this branch clears an existing `spark.ui.proxyBase` even though
this context never changed it. For example, with `/gateway` already set,
constructing a context with reverse proxy enabled but a missing `spark.master`
clears the property on this revision; the same probe preserves it on the
parent. The JVM consequently loses its externally supplied proxy prefix after
the failed attempt. Please distinguish "not captured/changed" from "captured as
absent", and restore or clear the property only after this context has taken
ownership of it.
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -222,6 +244,15 @@ private[spark] object JettyUtils extends Logging {
val newHeader = createProxyLocationHeader(headerValue, clientRequest,
serverResponse.getRequest().getURI())
if (newHeader != null) {
+ if (normalizedReverseProxyUrl.nonEmpty) {
+ val scheme = clientRequest.getScheme
+ val host = Option(clientRequest.getHeader("host")).getOrElse("")
+ val rootProxyPrefix = s"$scheme://$host/proxy/"
+ if (newHeader.startsWith(rootProxyPrefix)) {
+ val rest = newHeader.substring(rootProxyPrefix.length)
+ return s"$normalizedReverseProxyUrl/proxy/$rest"
Review Comment:
[P2] Update the worker UI fixture for the changed redirect behavior
The existing `MasterWorkerUISuite` test "master/worker web ui available
behind front-end reverseProxy" configures `http://proxyhost:8080/path/to/spark`
but fetches `/proxy/<worker>/json` through the direct master address. The
worker's `/json` to `/json/` redirect now points to that configured external
URL, and `Source.fromURL` follows it to the nonexistent `proxyhost`, timing
out. The focused test passes on the parent and fails on this revision, both
locally and in CI. Please adapt the fixture along with this change, using a
reachable frontend or checking redirects without following them and fetching
content through the local route. The failure reflects the fixture's
assumptions, rather than showing that external redirect rewriting itself should
be removed.
##########
core/src/main/scala/org/apache/spark/ui/UIUtils.scala:
##########
@@ -201,7 +201,14 @@ private[spark] object UIUtils extends Logging {
request: HttpServletRequest,
basePath: String = "",
resource: String = ""): String = {
- uiRoot(request) + basePath + resource
+ val root = uiRoot(request).stripSuffix("/")
+ val cleanBase = if (basePath.startsWith("/")) basePath
+ else if (basePath.nonEmpty) "/" + basePath
Review Comment:
[P2] Update the storage fixture for normalized base paths
`StoragePageSuite` currently mocks `storageTab.basePath` as
`http://localhost:4040`. This normalization changes the generated link to
`/http://localhost:4040/storage/rdd/?id=1`, so its existing `rddTable`
assertions fail. The suite passes 5/5 on the parent and 4/5 on this revision;
the same failure is reported by CI. Production callers supply an empty or
rooted context path, so this fixture does not establish a production
absolute-URL use case. Please update the mock to a realistic context path and
adjust its link expectations, or explicitly preserve and test absolute-base
behavior if that remains part of this helper's contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
