spark-test commented on code in PR #59089:
URL: https://github.com/apache/spark/pull/59089#discussion_r4395105449
##########
core/src/main/scala/org/apache/spark/ui/JettyUtils.scala:
##########
@@ -294,7 +297,14 @@ private[spark] object JettyUtils extends Logging {
}
val existingContext =
Option(clientRequest.getHeader("X-Forwarded-Context")).getOrElse("")
val contextPath = Option(clientRequest.getContextPath).getOrElse("")
- val proxyContext = existingContext + contextPath + prefix
+ val proxyPrefix = if
(normalizedReverseProxyUrl.startsWith("http://";) ||
+ normalizedReverseProxyUrl.startsWith("https://";)) {
+ val uri = new java.net.URI(normalizedReverseProxyUrl)
+ Option(uri.getPath).getOrElse("")
+ } else {
+ normalizedReverseProxyUrl
+ }
+ val proxyContext = existingContext + proxyPrefix + contextPath +
prefix
Review Comment:
**Non-blocking (P2):** This line appends the path of
`spark.ui.reverseProxyUrl` after the incoming `X-Forwarded-Context`, even when
that header already carries the same front-end prefix. Take a Knox-style
gateway that serves the Master under `/path/to/spark`, strips that prefix and
sends `X-Forwarded-Context: /path/to/spark`, with
`spark.ui.reverseProxyUrl=https://gw.example.com/path/to/spark` set as
documented. A request to `/proxy/app-1/jobs/` now forwards
`/path/to/spark/path/to/spark/proxy/app-1`; the merge base forwarded
`/path/to/spark/proxy/app-1`.
Proxied UIs that take their root from this header render every link and
static asset under the doubled path, which 404s at the gateway. This is the
consumer SPARK-58521's chaining was added for. For example, a cluster-mode
driver submitted without `spark.ui.reverseProxy=true` gets only the injected
`-Dspark.ui.reverseProxyUrl`, so it never sets `spark.ui.proxyBase`, and
`uiRoot` falls back to the header. UIs with `spark.ui.proxyBase` set are
unaffected, and gateways that send no header are what this change fixes.
Suggested direction: when an incoming `X-Forwarded-Context` is present,
treat it as the external prefix, and add the reverseProxyUrl path only when no
header is present. Deriving that path once in `createProxyHandler`, from the
raw (still percent-encoded) path, also avoids re-parsing the URL on every
request.
This extends existing discussions
[1](https://github.com/apache/spark/pull/59089#issuecomment-5970307648),
[2](https://github.com/apache/spark/pull/59089#pullrequestreview-5400364129).
**Verification:**
- **Regression:** Send a proxied request whose incoming
`X-Forwarded-Context` equals the reverseProxyUrl path, and assert that the
forwarded header contains that prefix exactly once.
- **Behavior:** Send proxied requests without an incoming header, for both
an absolute and a relative reverseProxyUrl, and assert that the forwarded
header starts with the URL path.
- **Compatibility:** With no reverseProxyUrl configured, assert that the
forwarded header keeps the earlier chaining of the incoming context followed by
`/proxy/<id>`.
##########
core/src/test/scala/org/apache/spark/SparkContextSuite.scala:
##########
@@ -1417,6 +1417,99 @@ class SparkContextSuite extends SparkFunSuite with
LocalSparkContext with Eventu
assert(msg.contains("Cannot use the keyword 'proxy' or 'history' in
reverse proxy URL"))
}
+ test("SPARK-58893: propagate UI_REVERSE_PROXY_URL to spark.ui.proxyBase in
SparkContext") {
+ val conf = new SparkConf().setAppName("testReverseProxyBase")
+ .setMaster("local")
+ .set(UI_REVERSE_PROXY, true)
+ .set(UI_REVERSE_PROXY_URL, "http://proxyhost:8080/myprefix";)
+ sc = new SparkContext(conf)
+ assert(System.getProperty("spark.ui.proxyBase") ===
+ s"http://proxyhost:8080/myprefix/proxy/${sc.applicationId}";)
+ }
+
+ test("SPARK-58893: fallback spark.ui.proxyBase when UI_REVERSE_PROXY_URL is
empty") {
+ val sysProxyBase = "http://proxyhost:8080/sysprefix";
+ System.setProperty("spark.ui.proxyBase", sysProxyBase)
+ try {
+ val conf = new SparkConf().setAppName("testReverseProxyBaseSys")
+ .setMaster("local")
+ .set(UI_REVERSE_PROXY, true)
+ sc = new SparkContext(conf)
+ assert(System.getProperty("spark.ui.proxyBase") ===
+ s"$sysProxyBase/proxy/${sc.applicationId}")
+ } finally {
+ resetSparkContext()
+ System.clearProperty("spark.ui.proxyBase")
+ }
+ }
+
+ test("SPARK-58893: avoid compounding derived proxy root across multiple
SparkContexts") {
+ val conf1 = new SparkConf().setAppName("testApp1")
+ .setMaster("local")
+ .set(UI_REVERSE_PROXY, true)
+ sc = new SparkContext(conf1)
+ val app1Id = sc.applicationId
+ assert(System.getProperty("spark.ui.proxyBase") === s"/proxy/$app1Id")
+ resetSparkContext()
Review Comment:
**Nit (P3):** Stopping the first context here clears `spark.ui.proxyBase`:
no prior value was saved, so the new `stop()` takes its clear branch. The
second context therefore never sees `/proxy/local-<id>`, and the test passes
even with `.filterNot(isAppSpecificProxyBase)` removed. It also passes at the
merge base. The only test that pre-sets an app-specific base uses
`/proxy/application_`, so removing the `/proxy/app-`, `/proxy/spark-` or
`/proxy/local-` alternatives leaves every `SparkContextSuite` test green. The
`local-` check was added in response to review feedback that also asked for a
test pre-setting a `/proxy/local-<id>` base, and that test is still missing.
The test's name also claims more than it checks.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59089#discussion_r4172835143).
**Verification:**
- **Regression:** For each recognized id form, pre-set a stale app-specific
base and assert that the new context derives `/proxy/<new id>` and clears the
stale value on stop, so that removing that form's check fails the test.
##########
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:
**Non-blocking (P2):** `rewrittenLocation` is computed here from the test's
own `normalizedProxyUrl` constant, and the next line asserts it against a
literal. The `handler` and the `clientReq`, `serverResp` and `cReq` mocks are
never passed to `addProxyHeaders` or `filterServerResponseHeader`. So removing
or breaking the `X-Forwarded-Context` composition or the `Location` re-root in
`JettyUtils.createProxyHandler` leaves this test green, despite its name. The
earlier review thread on the proxy handler asked for a test that drives a
proxied request through it, and this test does not do that.
The same change makes the reverse-proxy `MasterWorkerUISuite` test request
`/proxy/<workerId>/json/` instead of `/json`. That skips the worker's
trailing-slash redirect, which was the only end-to-end route through the
re-root with a configured URL. The real-request harness in `UISuite` (the
SPARK-58521 test) uses the default empty URL. So no test exercises either new
branch.
The existing SPARK-58521 harness in `UISuite` already sends real requests
through `createProxyHandler` and can be extended with a configured
reverseProxyUrl. In `MasterWorkerUISuite`, the trailing-slash redirect's
`Location` can be asserted without following it.
This extends existing discussions
[1](https://github.com/apache/spark/pull/59089#issuecomment-5975472018),
[2](https://github.com/apache/spark/pull/59089#pullrequestreview-5400364129),
[3](https://github.com/apache/spark/pull/59089#pullrequestreview-5401989181).
**Verification:**
- **Regression:** Disable the forwarded-context prefix and the `Location`
re-root one at a time, and confirm that a test fails for each.
- **Behavior:** Through real proxied requests, cover an absolute and a
relative reverseProxyUrl, each with and without an incoming
`X-Forwarded-Context`.
##########
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:
**Non-blocking (P2):** This line re-roots every proxied `Location` onto
`spark.ui.reverseProxyUrl`. Two documented layers already apply the front-end
prefix to these redirects, and each now applies it a second time:
- With `spark.ui.proxyRedirectUri` set, `ProxyRedirectHandler` wraps the
whole Master handler collection, including `/proxy`, and rebuilds every
`Location` as `proxyRedirectUri + uiRoot + path`. On the Master, `uiRoot` is
the reverseProxyUrl, because `Master.onStart` sets `spark.ui.proxyBase` to it.
Take `spark.ui.reverseProxyUrl=/path/to/spark` (a documented relative form)
with `spark.ui.proxyRedirectUri=https://mydomain.com`. A driver's redirect to
`/jobs/` now becomes
`https://mydomain.com/path/to/spark/path/to/spark/proxy/app-1/jobs/`; at the
merge base it was `https://mydomain.com/path/to/spark/proxy/app-1/jobs/`.
- The `spark.ui.reverseProxyUrl` docs make the front-end proxy responsible
for rewriting redirects that point at the Spark master. A front end that
preserves `Host` and rewrites `http://<public-host>/` to
`http://<public-host>/path/to/spark/` now also matches the re-rooted
`http://<public-host>/path/to/spark/proxy/...` and prefixes it again.
Every redirect a proxied UI issues is affected, including the
root-to-`/jobs/` redirect, Jetty's trailing-slash redirects, and kill redirects
inside the application UI. Redirects the Master issues itself (kill, hold,
resume) still go to `/` and still depend on those layers, so operators cannot
turn the layers off to compensate.
**Recommended change:** Re-root proxied redirects in the Master proxy only
when Spark has no other redirect rewriter for the Master UI. When
`spark.ui.proxyRedirectUri` is set, leave proxied `Location` headers in their
previous host-rooted form so that `ProxyRedirectHandler` applies the prefix
once. Update the `spark.ui.reverseProxyUrl` documentation to say that the
Master points redirects from proxied worker and application UIs at the
reverseProxyUrl, so front-end redirect rewriting should only handle redirects
that point at the Master itself.
**Why this works:** Each redirect gets exactly one prefixing layer:
`ProxyRedirectHandler` when `spark.ui.proxyRedirectUri` is configured;
otherwise the Master proxy servlet for proxied UIs, with the front end
documented as responsible only for Master-originated redirects.
**Scope:** Make the proxied-redirect re-root conditional on
`spark.ui.proxyRedirectUri` being absent, pass that setting from the Master UI
to the proxy handler, and document the resulting redirect responsibilities for
`spark.ui.reverseProxyUrl`.
**Compatibility:** Master-originated kill, hold and resume redirects, and
all proxied redirects when no reverseProxyUrl is configured, behave as they did
before this PR.
**Risks:** Operators whose front end preserves the Host header and rewrites
redirects keyed on the public host must narrow that rule to Master-pointing
redirects, and the documentation change needs to say so explicitly.
**Constraints:** With `spark.ui.reverseProxyUrl` unset, proxied `Location`
headers must stay exactly as they were before this PR. Master-originated kill,
hold and resume redirects keep their declared `/` target.
**Exclusions:**
- Fixing how `ProxyRedirectHandler` handles an absolute
`spark.ui.proxyBase`, which was already broken before this PR when
`spark.ui.proxyRedirectUri` is combined with an absolute reverseProxyUrl.
**Success:** With `spark.ui.reverseProxyUrl=/path/to/spark` and
`spark.ui.proxyRedirectUri=https://mydomain.com`, a proxied driver redirect to
`/jobs/` resolves to `https://mydomain.com/path/to/spark/proxy/<id>/jobs/`,
with the prefix exactly once. Without `spark.ui.proxyRedirectUri` and with
`spark.ui.reverseProxyUrl` set, proxied redirects point at
`<reverseProxyUrl>/proxy/<id>/...`. With `spark.ui.reverseProxyUrl` unset,
proxied `Location` headers are unchanged from before this PR. The
`spark.ui.reverseProxyUrl` documentation states which redirects Spark rewrites
and which the front end must rewrite.
**Verification:**
- **Regression:** Drive a proxied redirect through the Master proxy with a
relative reverseProxyUrl and `spark.ui.proxyRedirectUri` configured, and assert
that the final `Location` carries the front-end prefix exactly once.
- **Behavior:** Drive a proxied redirect with an absolute reverseProxyUrl
and no `spark.ui.proxyRedirectUri`, and assert that the `Location` is re-rooted
onto the reverseProxyUrl.
- **Compatibility:** With no reverseProxyUrl configured, assert that proxied
`Location` headers keep the host-rooted form they had before this PR, and that
Master kill redirects still target the root.
- **Inspection:** Confirm the `spark.ui.reverseProxyUrl` documentation
describes the Master rewriting proxied redirects, and limits front-end redirect
rewriting to redirects that point at the Master.
--
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]
