sunchao commented on code in PR #58243:
URL: https://github.com/apache/spark/pull/58243#discussion_r3889865003
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -222,6 +243,14 @@ private[spark] object JettyUtils extends Logging {
val newHeader = createProxyLocationHeader(headerValue, clientRequest,
serverResponse.getRequest().getURI())
if (newHeader != null) {
+ if (reverseProxyUrl.nonEmpty) {
+ val scheme = clientRequest.getScheme
+ val host = Option(clientRequest.getHeader("host")).getOrElse("")
+ val rootProxyPrefix = s"$scheme://$host/proxy/"
+ if (newHeader.startsWith(rootProxyPrefix)) {
+ return reverseProxyUrl + "/proxy/" +
newHeader.substring(rootProxyPrefix.length)
Review Comment:
[P2] Normalize the proxy URL before appending the redirect path
`MasterWebUI.addProxy` passes the configured URL here without stripping its
trailing slash, although server-relative URLs and trailing slashes are
supported. With `spark.ui.reverseProxy=true` and
`spark.ui.reverseProxyUrl="/"`, a proxied backend redirect now produces
`Location: //proxy/<id>/...`, which browsers interpret as a URL on host `proxy`
instead of the current public host. A real Jetty proxy probe produced
`//proxy/worker-1/target` on this revision, while the parent retained the
master host. Absolute URLs ending in `/` also acquire an extra slash before
`/proxy/`. Please normalize the URL consistently with Master, Worker, and
SparkContext before this concatenation.
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/MasterWebUI.scala:
##########
@@ -64,10 +65,15 @@ class MasterWebUI(
addStaticHandler(MasterWebUI.STATIC_RESOURCE_DIR)
addRenderLogHandler(this, master.conf)
if (killEnabled) {
+ val killRedirectTarget = master.conf.get(UI_REVERSE_PROXY_URL)
Review Comment:
[P2] Honor reverseProxy=false when choosing the kill redirect
This reads `spark.ui.reverseProxyUrl` without checking whether
`spark.ui.reverseProxy` is enabled. With reverse proxy disabled but a URL
retained in a shared configuration, POSTs to either kill endpoint now redirect
to that inactive proxy instead of the directly accessed master's root. An HTTP
probe with `reverseProxy=false` and
`reverseProxyUrl=https://public.example/cluster-a` confirmed the changed
destination; the parent redirected to the local master. The configuration
documentation says `reverseProxyUrl` is effective only when reverse proxy is
enabled, and the other Master configuration paths retain that guard. Please
apply the same guard here.
##########
core/src/main/scala/org/apache/spark/SparkContext.scala:
##########
@@ -636,7 +636,9 @@ class SparkContext(config: SparkConf) extends Logging {
}
if (_conf.get(UI_REVERSE_PROXY)) {
- val proxyUrl =
_conf.get(UI_REVERSE_PROXY_URL).getOrElse("").stripSuffix("/")
+ val proxyUrl = _conf.get(UI_REVERSE_PROXY_URL)
+ .getOrElse(sys.props.getOrElse("spark.ui.proxyBase", ""))
Review Comment:
[P2] Avoid reusing the previous SparkContext's derived proxy root
With `spark.ui.reverseProxy=true` and no `spark.ui.reverseProxyUrl`,
creating, stopping, and recreating a SparkContext in the same JVM now reads the
property written by the first context and appends the second application's ID.
A two-context runtime probe produced `/proxy/<app1>/proxy/<app2>` here, versus
`/proxy/<app2>` on the parent. `stop()` does not clear or restore this
property, and `UIUtils.uiRoot` prioritizes it for navigation and
static-resource URLs, so the second application's links target the stopped
application. Please keep the externally supplied base separate from the
application-specific value, or restore the original value when the context
stops.
##########
core/src/main/scala/org/apache/spark/deploy/master/ui/MasterWebUI.scala:
##########
@@ -64,10 +65,15 @@ class MasterWebUI(
addStaticHandler(MasterWebUI.STATIC_RESOURCE_DIR)
addRenderLogHandler(this, master.conf)
if (killEnabled) {
+ val killRedirectTarget = master.conf.get(UI_REVERSE_PROXY_URL)
+ .map(_.stripSuffix("/") + "/")
Review Comment:
[P2] Preserve compatibility with the existing proxy redirect wrapper
With `spark.ui.reverseProxy=true`, `spark.ui.reverseProxyUrl=/cluster-a`,
and `spark.ui.proxyRedirectUri=https://public.example`, `Master.onStart` sets
`spark.ui.proxyBase=/cluster-a`, and the existing `ProxyRedirectHandler`
already prefixes that base to redirect destinations. Supplying `/cluster-a/`
here therefore adds it twice: an HTTP probe of the kill endpoint returned
`https://public.example/cluster-a/cluster-a/` on this revision, versus
`https://public.example/cluster-a/` on the parent. Both kill endpoints inherit
this behavior, breaking navigation after a kill. Please avoid adding the proxy
prefix again when the redirect wrapper already supplies it.
--
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]
