[SPARK-52381][CORE] JsonProtocol: Only accept subclasses of SparkListenerEvent #51061
Conversation
| case other => mapper.readValue(json.toString, Utils.classForName(other)) | ||
| .asInstanceOf[SparkListenerEvent] | ||
| case other => | ||
| if (other.startsWith("org.apache.spark")) { |
There was a problem hiding this comment.
is it possible to have an allowList ? downstream projects may have custom events, i.e. Apache Kyuubi has custom events under org.apache.kyuubi package
There was a problem hiding this comment.
Thanks @pan3793, I've removed the check for the package name and now just check if the class is a subclass of SparkListenerEvent.
pjfanning
changed the title
pjfanning
changed the title
|
Test failure looks related, otherwise LGTM cc @LuciferYang |
|
It looks like the discussion thread is not accessible ? |
| @@ -935,8 +935,9 @@ class AdaptiveQueryExecSuite | |||
| val error = intercept[SparkException] { | |||
| joined.collect() | |||
| } | |||
| assert((Seq(error) ++ Option(error.getCause) ++ error.getSuppressed()).exists( | |||
| e => e.getMessage() != null && e.getMessage().contains("coalesce test error"))) | |||
| val exceptionMessages = (Seq(error) ++ Option(error.getCause) ++ error.getSuppressed()) | |||
There was a problem hiding this comment.
Is this change necessary? The issue "SPARK-47148: AQE should avoid submitting a shuffle job upon cancellation" is associated with a flaky test.
There was a problem hiding this comment.
I can undo this. The test failed during one run for me and I made this change to try to debug it.
This change makes it easier for Scalatest to infer a test failure message because the collection being asserted on is a Seq[String] instead of Seq[Exception].
Uninformative message from recent test failure:
#51061 (comment)
There was a problem hiding this comment.
@LuciferYang do you still want me to revert this? It is a test and the existing code made it quite hard to debug it while the newer code makes it easier.
There was a problem hiding this comment.
If this change holds significance, I recommend submitting it in a standalone pr as it's not relevant to the theme of the current pr, correct?
@pan3793 that test seems to have passed on the last build for this PR. I have modified the test in this PR to make the assertion easier for ScalaTest to infer informative failure messages for - but the slow sql tests passed so the change didn't help. |
Friendly ping @mridulm, What do you think about this PR? |
|
IMO this is not a security issue - as discussed in the thread, users need too much access to begin with in order to leverage the "exploit" (pls do let me know if I am missing some nuance here where this can be a problem !). I am not strongly in favor of, or against the change @LuciferYang - will let you drive it. |
…t subclasses extra check scalastyle Update JsonProtocolSuite.scala
LuciferYang
changed the title
What changes were proposed in this pull request?
JsonProtocol tidy up. Only parse JSON relating to Spark events.
https://issues.apache.org/jira/browse/SPARK-52381
Why are the changes needed?
Tidier code and https://lists.apache.org/thread/9zwkdo85wcdfppgqvbhjly8wdgf595yp
Does this PR introduce any user-facing change?
No
How was this patch tested?
Unit test
Was this patch authored or co-authored using generative AI tooling?
No