Repository navigation
[SPARK-60342][CORE][WEBUI] Respect UI root in Master UI links - #59595
Closed
dongjoon-hyun wants to merge 2 commits into
Closed
dongjoon-hyun wants to merge 2 commits into
dongjoon-hyun wants to merge 2 commits into
Conversation
Member
Author
|
Could you review this PR when you have some time, @viirya ? |
viirya
approved these changes
Oct 19, 2026
viirya
left a comment
viirya
left a comment
Member
There was a problem hiding this comment.
+1, LGTM. These were the last site-relative hrefs left in the Master UI. The remaining anchors in deploy/master/ui and deploy/worker/ui already go through makeHref, use absolute URLs, or are page-relative (e.g. app/?appId=). The Master log page's Load More / Load New requests were already fixed by SPARK-60311. With an empty UI root the generated links are unchanged.
I confirmed that each of the three source changes is covered on its own: reverting only LogPage.scala or only EnvironmentPage.scala makes master/worker web ui available behind front-end reverseProxy fail. Widening the proxy check to every site-relative href is a good guard for links added to Master pages later.
Two optional test nits are inline.
FYI, possibly for a separate follow-up in this series: the POST handlers registered with createRedirectHandler (/app/kill, /driver/kill, /app/hold, /app/resume) redirect with new URL(request.getRequestURL, "/"). That resolves against the backend URL and doesn't go through the UI root either. It's a server-side redirect rather than a rendered link, and it predates 4.0, so it doesn't belong in this PR.
| html should not include ("""src="/static""") | ||
| html should include (s"""src="$proxyUrl/static""") | ||
| // other site-relative links, e.g. "Back to Master", must not bypass the proxy either | ||
| (SITE_RELATIVE_LINK_RE findAllMatchIn html).foreach(_.group(1) should startWith (proxyUrl)) |
Member
There was a problem hiding this comment.
nit: every match starts with /, and all callers pass an absolute proxyUrl (http://...), so this effectively asserts that there are no site-relative links at all. Writing it that way states the intent directly and lists every offender in one failure, instead of stopping at the first one:
SITE_RELATIVE_LINK_RE.findAllMatchIn(html).map(_.group(1)).toSeq shouldBe emptyThis also subsumes the html should not include ("""href="/static""") check above.
Member
Author
There was a problem hiding this comment.
Thank you, @viirya. Not all callers pass an absolute proxyUrl, though. master/worker web ui available with reverseProxy calls verifyWorkerUI(html, masterUrl) without reverseProxyUrl, so the worker pages are checked with proxyUrl = "/proxy/<workerId>", and the line above requires href="/proxy/<workerId>/static. So, shouldBe empty would fail there. To list every offender in one failure, I changed it to collect the links that don't start with proxyUrl and check shouldBe empty.
| Seq("environment/", "logPage/?self&logType=out").foreach { path => | ||
| val pageHtml = Utils | ||
| .tryWithResource(Source.fromURL(s"$masterUrl/$path"))(_.getLines().mkString("\n")) | ||
| verifyStaticResourcesServedByProxy(pageHtml, reverseProxyUrl) |
Member
There was a problem hiding this comment.
nit: when this fails, the message is just "/" did not start with substring "http://proxyhost:8080/path/to/spark". The Environment and Log pages both have a / Back link, so it doesn't say which page failed. Maybe wrap the check in withClue(s"$path: ") { ... }?
Member
Author
There was a problem hiding this comment.
Sure. I wrapped it with withClue(s"$path: ").
Member
Author
|
Thank you for the pointer about the redirects, @viirya. For those, |
HyukjinKwon
approved these changes
Oct 19, 2026
Member
Author
|
Thank you, @HyukjinKwon ! |
dongjoon-hyun
added a commit
that referenced
this pull request
Oct 19, 2026
### What changes were proposed in this pull request? This PR aims to respect the UI root (`spark.ui.proxyBase`, `APPLICATION_WEB_PROXY_BASE`, or the Knox `X-Forwarded-Context` header) in the Master UI links, like SPARK-60311 did in the History Server log page. - `MasterPage`: The `Environment` and `Log` links use `UIUtils.prependBaseUri`. - `EnvironmentPage` and `LogPage`: The `Back to Master` links use `UIUtils.prependBaseUri`. ### Why are the changes needed? Under a UI root, e.g., behind a front-end reverse proxy with a path (`spark.ui.reverseProxyUrl`) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark [v4.0.0](https://github.com/apache/spark/releases/tag/v4.0.0) (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894). ### Does this PR introduce _any_ user-facing change? Yes, the `Environment`, `Log`, and `Back to Master` links work under a UI root. There is no change when the UI root is empty (default). ### How was this patch tested? Pass the CIs with the updated `MasterWorkerUISuite`, which fails without this fix. `MasterSuiteBase.verifyStaticResourcesServedByProxy` now checks all site-relative links, not only `/static`, and the test applies it to the Master `Environment` and `Log` pages too. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59595 from dongjoon-hyun/SPARK-60342. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 269a4f7) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun
added a commit
that referenced
this pull request
Oct 19, 2026
### What changes were proposed in this pull request? This PR aims to respect the UI root (`spark.ui.proxyBase`, `APPLICATION_WEB_PROXY_BASE`, or the Knox `X-Forwarded-Context` header) in the Master UI links, like SPARK-60311 did in the History Server log page. - `MasterPage`: The `Environment` and `Log` links use `UIUtils.prependBaseUri`. - `EnvironmentPage` and `LogPage`: The `Back to Master` links use `UIUtils.prependBaseUri`. ### Why are the changes needed? Under a UI root, e.g., behind a front-end reverse proxy with a path (`spark.ui.reverseProxyUrl`) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark [v4.0.0](https://github.com/apache/spark/releases/tag/v4.0.0) (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894). ### Does this PR introduce _any_ user-facing change? Yes, the `Environment`, `Log`, and `Back to Master` links work under a UI root. There is no change when the UI root is empty (default). ### How was this patch tested? Pass the CIs with the updated `MasterWorkerUISuite`, which fails without this fix. `MasterSuiteBase.verifyStaticResourcesServedByProxy` now checks all site-relative links, not only `/static`, and the test applies it to the Master `Environment` and `Log` pages too. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59595 from dongjoon-hyun/SPARK-60342. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 269a4f7) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun
added a commit
that referenced
this pull request
Oct 19, 2026
### What changes were proposed in this pull request? This PR aims to respect the UI root (`spark.ui.proxyBase`, `APPLICATION_WEB_PROXY_BASE`, or the Knox `X-Forwarded-Context` header) in the Master UI links, like SPARK-60311 did in the History Server log page. - `MasterPage`: The `Environment` and `Log` links use `UIUtils.prependBaseUri`. - `EnvironmentPage` and `LogPage`: The `Back to Master` links use `UIUtils.prependBaseUri`. ### Why are the changes needed? Under a UI root, e.g., behind a front-end reverse proxy with a path (`spark.ui.reverseProxyUrl`) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark [v4.0.0](https://github.com/apache/spark/releases/tag/v4.0.0) (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894). ### Does this PR introduce _any_ user-facing change? Yes, the `Environment`, `Log`, and `Back to Master` links work under a UI root. There is no change when the UI root is empty (default). ### How was this patch tested? Pass the CIs with the updated `MasterWorkerUISuite`, which fails without this fix. `MasterSuiteBase.verifyStaticResourcesServedByProxy` now checks all site-relative links, not only `/static`, and the test applies it to the Master `Environment` and `Log` pages too. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59595 from dongjoon-hyun/SPARK-60342. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 269a4f7) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun
added a commit
that referenced
this pull request
Oct 19, 2026
### What changes were proposed in this pull request? This PR aims to respect the UI root (`spark.ui.proxyBase`, `APPLICATION_WEB_PROXY_BASE`, or the Knox `X-Forwarded-Context` header) in the Master UI links, like SPARK-60311 did in the History Server log page. - `MasterPage`: The `Environment` and `Log` links use `UIUtils.prependBaseUri`. - `EnvironmentPage` and `LogPage`: The `Back to Master` links use `UIUtils.prependBaseUri`. ### Why are the changes needed? Under a UI root, e.g., behind a front-end reverse proxy with a path (`spark.ui.reverseProxyUrl`) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark [v4.0.0](https://github.com/apache/spark/releases/tag/v4.0.0) (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894). ### Does this PR introduce _any_ user-facing change? Yes, the `Environment`, `Log`, and `Back to Master` links work under a UI root. There is no change when the UI root is empty (default). ### How was this patch tested? Pass the CIs with the updated `MasterWorkerUISuite`, which fails without this fix. `MasterSuiteBase.verifyStaticResourcesServedByProxy` now checks all site-relative links, not only `/static`, and the test applies it to the Master `Environment` and `Log` pages too. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59595 from dongjoon-hyun/SPARK-60342. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 269a4f7) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun
added a commit
that referenced
this pull request
Oct 19, 2026
### What changes were proposed in this pull request? This PR aims to respect the UI root (`spark.ui.proxyBase`, `APPLICATION_WEB_PROXY_BASE`, or the Knox `X-Forwarded-Context` header) in the Master UI links, like SPARK-60311 did in the History Server log page. - `MasterPage`: The `Environment` and `Log` links use `UIUtils.prependBaseUri`. - `EnvironmentPage` and `LogPage`: The `Back to Master` links use `UIUtils.prependBaseUri`. ### Why are the changes needed? Under a UI root, e.g., behind a front-end reverse proxy with a path (`spark.ui.reverseProxyUrl`) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark [v4.0.0](https://github.com/apache/spark/releases/tag/v4.0.0) (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894). ### Does this PR introduce _any_ user-facing change? Yes, the `Environment`, `Log`, and `Back to Master` links work under a UI root. There is no change when the UI root is empty (default). ### How was this patch tested? Pass the CIs with the updated `MasterWorkerUISuite`, which fails without this fix. `MasterSuiteBase.verifyStaticResourcesServedByProxy` now checks all site-relative links, not only `/static`, and the test applies it to the Master `Environment` and `Log` pages too. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Opus 5.5 Closes #59595 from dongjoon-hyun/SPARK-60342. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 269a4f7) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR aims to respect the UI root (
spark.ui.proxyBase,APPLICATION_WEB_PROXY_BASE, or the KnoxX-Forwarded-Contextheader) in the Master UI links, like SPARK-60311 did in the History Server log page.MasterPage: TheEnvironmentandLoglinks useUIUtils.prependBaseUri.EnvironmentPageandLogPage: TheBack to Masterlinks useUIUtils.prependBaseUri.Why are the changes needed?
Under a UI root, e.g., behind a front-end reverse proxy with a path (
spark.ui.reverseProxyUrl) or Knox, these links go outside of the UI root and don't work. This bug exists since Apache Spark v4.0.0 (2025-05-19), which added the Master log page (SPARK-46870) and environment page (SPARK-47894).Does this PR introduce any user-facing change?
Yes, the
Environment,Log, andBack to Masterlinks work under a UI root. There is no change when the UI root is empty (default).How was this patch tested?
Pass the CIs with the updated
MasterWorkerUISuite, which fails without this fix.MasterSuiteBase.verifyStaticResourcesServedByProxynow checks all site-relative links, not only/static, and the test applies it to the MasterEnvironmentandLogpages too.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5