[SPARK-14025][STREAMING][WEBUI] Fix streaming job descriptions on the event timeline #11845

lw-lin · 2016-03-19T12:47:03Z

What changes were proposed in this pull request?

Removed the extra <a href=...>...</a> for each streaming job's description on the event timeline.

[Before]

[After]

How was this patch tested?

test suits, manual checks (see screenshots above)

lw-lin · 2016-03-19T12:48:26Z

@andrewor14 @zsxwing would you mind taking a look at this when you have time? Thanks!

srowen · 2016-03-19T13:21:55Z

I agree it's a problem, but this seems like a hacky way to band-aid it, with a second version of the description and a flag passed around. Is the problem not just that this is HTML, but not being interpreted as such?

Is it necessary that this description have HTML to begin with?

lw-lin · 2016-03-19T13:56:39Z

@srowen thanks for looking at this!

I believe job descriptions were intended to contains only plain texts at first, but HTMLs were introduced in for streaming jobs by #8791. Hyperlinks in the CompletedJobs/CompletedStages tables are proven quite useful, so I think maybe we'll need one more version of the job description(In most cases the two versions are the same, though)? Any thoughts on how to make this elegant would be appreciated :)

lw-lin · 2016-03-19T14:02:26Z

Besides, the blue/green bar in the event timeline itself is clickable, linking to the specific job page. The <a href> thing is superfluous, let's figure out how to remove it.

srowen · 2016-03-19T14:06:50Z

Ah I thought this was a tooltip, where HTML can't render, but it's not. I wonder, is the problem just that this is rendered as text and not HTML? if it's controlled by the UI, it seems safe to just let it render as HTML if possible?

lw-lin · 2016-03-19T14:17:15Z

Actually we've intentionally escaped the description for the event timeline, so that it will be rendered as plain texts; please see https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala#L85.

So I think it was TD's intention to add hyperlinks to the text tables(where descriptions are not escaped), but the same description would be escaped and used in the tooltip too.

lw-lin · 2016-03-19T14:42:10Z

Maybe let's summarize a little bit:

job/stage descriptions are used at 2 places: the Event Timeline and the text tables (CompletedJobs/CompletedStages)
job/stage descriptions were intended to contains only texts at first, so everything's well
[SPARK-10652][SPARK-10742][Streaming] Set meaningful job descriptions for all streaming jobs #8791 introduced HTML in job/stage description for streaming jobs
- the text tables works well, because it renders HTML links directly, without escaping the description
- the event timeline escapes the description intentionally, and then displays the description, leading to the plain <a href></a> thing

So this PR adds a second version of the description, which will used for the event timeline. For non-streaming jobs, they are the same.

srowen · 2016-03-19T14:46:14Z

My question is, why escape it? the library will accept and render HTML and that is desirable here right?

lw-lin · 2016-03-19T15:00:44Z

the original tooltip library does accept and render HTML, but I guess we Spark developers decide we only need plain texts here:

for the blue box in the event timeline, itself is clickable both for streaming/non-streaming jobs, so it's unnecessary to contain any more HTML like <a href></a>
for the black tooltip, it is designed to contain only plain texts. The tooltip would just move with the cursor, so even it does contain a link, no one can manage to click on it.

So we'll escape things to prevent from malicious codes. However then HTMLs(<a href></a>) slipped in along with PR#8791 :)

lw-lin · 2016-03-19T15:03:32Z

The escape part can be traced at:

andrewor14 commented on an outdated diff on 23 Sep 2015 in [SPARK-10652][SPARK-10742][Streaming] Set meaningful job descriptions for all streaming jobs #8791: can you add a paragraph on the security implications here? (What happens if I put in a <script> tag or something, or if I reference a malicious link)
TD's commit addressing andrew's comment: 606b284

Maybe @andrewor14 will explain better :)

andrewor14 · 2016-03-21T17:41:48Z

ok to test

SparkQA · 2016-03-21T17:56:08Z

Test build #53688 has finished for PR 11845 at commit 0f32845.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-03-21T17:57:14Z

@lw-lin thanks for summarizing the issue. Yes, we don't want to process arbitrary HTML so we don't have to worry about potential XSS threats. However, I agree with @srowen that the approach you chose here seems kind of arbitrary. Instead, a better place to do this might be in UIUtils.makeDescription. There we already parse the description for anchor tags, so we can just pass in an additional flag there to optionally remove the tags.

lw-lin · 2016-03-22T01:57:16Z

@andrewor14 thank you for the informative review. Will soon update this PR accordingly.

This reverts commit 0f32845.

SparkQA · 2016-03-22T07:34:15Z

Test build #53755 has finished for PR 11845 at commit 22ac2d9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

lw-lin · 2016-03-22T09:55:21Z

Updated with new commits.

Tests passed on my local machine; I'm not sure why scalastyle checks failed (this PR doesn't touch this code snippet):

Scalastyle checks failed at following occurrences:
[error] /home/jenkins/workspace/SparkPullRequestBuilder/core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala:64:0: Whitespace at end of line
[error] (core/test:scalastyle) errors exist

Can we get a retest, please? :)

srowen · 2016-03-22T10:51:02Z

It failed on a line you modified so it is due to this change. The error message explains what you need to change.

SparkQA · 2016-03-22T15:29:48Z

Test build #53770 has finished for PR 11845 at commit ebe1182.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-03-22T22:45:34Z

core/src/main/scala/org/apache/spark/ui/UIUtils.scala

-              val fullUri = s"${basePathUri.stripSuffix("/")}/${relativePath.stripPrefix("/")}"
-              e % Attribute(null, "href", fullUri, Null)
-            case _ => n
+      val rule = if (plainText) {


style:

val rule = if (plainText) { ... } else { ... }

@andrewor14 sure; updated with the new commit, thanks!

andrewor14 · 2016-03-22T22:46:21Z

Looks pretty good. @zsxwing can you have a look?

SparkQA · 2016-03-23T04:23:22Z

Test build #53869 has finished for PR 11845 at commit 0761e5f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2016-03-23T22:15:34Z

LGTM. Merging to master. Thanks, @lw-lin

lw-lin · 2016-03-23T22:50:22Z

Thank you all for your kind review & comments, @srowen @andrewor14 @zsxwing !

Add job/stage descriptionPlain for event-line

0f32845

srowen mentioned this pull request Mar 19, 2016

[SPARK-10652][SPARK-10742][Streaming] Set meaningful job descriptions for all streaming jobs #8791

Closed

lw-lin changed the title ~~[SPARK-14025][STREAMING][WEBUI] Fix streaming job descriptions on the event line~~ [SPARK-14025][STREAMING][WEBUI] Fix streaming job descriptions on the event timeline Mar 21, 2016

lw-lin added 3 commits March 22, 2016 09:59

Revert "Add job/stage descriptionPlain for event-line"

3c4178b

This reverts commit 0f32845.

fix job description for the event timeline

7b08f95

fix style

22ac2d9

fix style

ebe1182

andrewor14 reviewed Mar 22, 2016
View reviewed changes

address andrew's comments

0761e5f

asfgit closed this in de4e48b Mar 23, 2016

lw-lin deleted the description-event-line branch March 25, 2016 01:21

[SPARK-14025][STREAMING][WEBUI] Fix streaming job descriptions on the event timeline #11845

[SPARK-14025][STREAMING][WEBUI] Fix streaming job descriptions on the event timeline #11845

Uh oh!

Conversation

lw-lin commented Mar 19, 2016

What changes were proposed in this pull request?

[Before]

[After]

How was this patch tested?

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

srowen commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

srowen commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

srowen commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

lw-lin commented Mar 19, 2016

Uh oh!

andrewor14 commented Mar 21, 2016

Uh oh!

SparkQA commented Mar 21, 2016

Uh oh!

andrewor14 commented Mar 21, 2016

Uh oh!

lw-lin commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 22, 2016

Uh oh!

lw-lin commented Mar 22, 2016

Uh oh!

srowen commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 22, 2016

Uh oh!

andrewor14 Mar 22, 2016

Choose a reason for hiding this comment

Uh oh!

lw-lin Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Mar 22, 2016

Uh oh!

SparkQA commented Mar 23, 2016

Uh oh!

zsxwing commented Mar 23, 2016

Uh oh!

lw-lin commented Mar 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants