Nflrijal commented on code in PR #59089:
URL: https://github.com/apache/spark/pull/59089#discussion_r4435151936
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -315,6 +325,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:
`MasterWebUI.addProxy` now passes `spark.ui.proxyRedirectUri` to
`JettyUtils.createProxyHandler`, and `filterServerResponseHeader` only re-roots
proxied `Location` headers onto `spark.ui.reverseProxyUrl` when
`proxyRedirectUri.isEmpty`. Also updated `docs/configuration.md` and `UI.scala`
to document redirect rewriting responsibilities.
##########
core/src/test/scala/org/apache/spark/ui/JettyUtilsSuite.scala:
##########
@@ -68,4 +69,37 @@ class JettyUtilsSuite extends SparkFunSuite {
assert(!JettyUtils.isValidCsrfToken(newRequest(csrfTokenParam = ""),
"abc"))
assert(!JettyUtils.isValidCsrfToken(newRequest(csrfTokenParam = "abc"),
""))
}
+
+ test("SPARK-58893: reverse proxy addProxyHeaders and
filterServerResponseHeader rewriting") {
+ val normalizedProxyUrl = "http://proxyhost:8080/myprefix";
+ // Create an instance of the ProxyServlet created in
JettyUtils.createProxyHandler
+ val handler = JettyUtils.createProxyHandler(
+ id => if (id == "app-1") Some("http://target:8080";) else None,
+ normalizedProxyUrl)
+
+ // Test reverse-proxy URL normalization and location header filter
+ val clientReq = mock(classOf[HttpServletRequest])
+ when(clientReq.getScheme).thenReturn("http")
+ when(clientReq.getHeader("host")).thenReturn("master:8080")
+ when(clientReq.getHeader("X-Forwarded-Context")).thenReturn(null)
+ when(clientReq.getContextPath).thenReturn("")
+ when(clientReq.getPathInfo).thenReturn("/app-1/jobs")
+
+ val serverResp = mock(classOf[CResponse])
+ val cReq = mock(classOf[CRequest])
+ when(serverResp.getRequest()).thenReturn(cReq)
+ when(cReq.getURI()).thenReturn(new java.net.URI("http://target:8080/jobs";))
+
+ // Verify location header rewrite logic from JettyUtils
+ val originalLocation = "http://master:8080/proxy/app-1/jobs/redirect";
+ val scheme = clientReq.getScheme
+ val host = clientReq.getHeader("host")
+ val rootProxyPrefix = s"$scheme://$host/proxy/"
+ assert(originalLocation.startsWith(rootProxyPrefix))
+
+ val rest = originalLocation.substring(rootProxyPrefix.length)
+ val rewrittenLocation =
s"${normalizedProxyUrl.stripSuffix("/")}/proxy/$rest"
Review Comment:
Removed the mock-only test in `JettyUtilsSuite` and added real HTTP proxy
tests in `UISuite` (`SPARK-58893: createProxyHandler X-Forwarded-Context and
Location with reverseProxyUrl`) and `MasterWorkerUISuite` (asserting the 301
trailing-slash redirect `Location` on `/proxy/<workerId>/json`).
--
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]
