[SPARK-22791] [SQL] [SS] Redact Output of Explain #19985

gatorsmile · 2017-12-15T07:04:13Z

What changes were proposed in this pull request?

When calling explain on a query, the output can contain sensitive information. We should provide an admin/user to redact such information.

Before this PR, the plan of SS is like this

== Physical Plan ==
*HashAggregate(keys=[value#6], functions=[count(1)], output=[value#6, count(1)#12L])
+- StateStoreSave [value#6], state info [ checkpoint = file:/private/var/folders/vx/j0ydl5rn0gd9mgrh1pljnw900000gn/T/temporary-91c6fac0-609f-4bc8-ad57-52c189f06797/state, runId = 05a4b3af-f02c-40f8-9ff9-a3e18bae496f, opId = 0, ver = 0, numPartitions = 5], Complete, 0
   +- *HashAggregate(keys=[value#6], functions=[merge_count(1)], output=[value#6, count#18L])
      +- StateStoreRestore [value#6], state info [ checkpoint = file:/private/var/folders/vx/j0ydl5rn0gd9mgrh1pljnw900000gn/T/temporary-91c6fac0-609f-4bc8-ad57-52c189f06797/state, runId = 05a4b3af-f02c-40f8-9ff9-a3e18bae496f, opId = 0, ver = 0, numPartitions = 5]
         +- *HashAggregate(keys=[value#6], functions=[merge_count(1)], output=[value#6, count#18L])
            +- Exchange hashpartitioning(value#6, 5)
               +- *HashAggregate(keys=[value#6], functions=[partial_count(1)], output=[value#6, count#18L])
                  +- *SerializeFromObject [staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, input[0, java.lang.String, true], true, false) AS value#6]
                     +- *MapElements <function1>, obj#5: java.lang.String
                        +- *DeserializeToObject value#30.toString, obj#4: java.lang.String
                           +- LocalTableScan [value#30]

After this PR, we can get the following output if users set spark.redaction.string.regex to file:/[\\w_]+

== Physical Plan ==
*HashAggregate(keys=[value#6], functions=[count(1)], output=[value#6, count(1)#12L])
+- StateStoreSave [value#6], state info [ checkpoint = *********(redacted)/var/folders/vx/j0ydl5rn0gd9mgrh1pljnw900000gn/T/temporary-e7da9b7d-3ec0-474d-8b8c-927f7d12ed72/state, runId = 8a9c3761-93d5-4896-ab82-14c06240dcea, opId = 0, ver = 0, numPartitions = 5], Complete, 0
   +- *HashAggregate(keys=[value#6], functions=[merge_count(1)], output=[value#6, count#32L])
      +- StateStoreRestore [value#6], state info [ checkpoint = *********(redacted)/var/folders/vx/j0ydl5rn0gd9mgrh1pljnw900000gn/T/temporary-e7da9b7d-3ec0-474d-8b8c-927f7d12ed72/state, runId = 8a9c3761-93d5-4896-ab82-14c06240dcea, opId = 0, ver = 0, numPartitions = 5]
         +- *HashAggregate(keys=[value#6], functions=[merge_count(1)], output=[value#6, count#32L])
            +- Exchange hashpartitioning(value#6, 5)
               +- *HashAggregate(keys=[value#6], functions=[partial_count(1)], output=[value#6, count#32L])
                  +- *SerializeFromObject [staticinvoke(class org.apache.spark.unsafe.types.UTF8String, StringType, fromString, input[0, java.lang.String, true], true, false) AS value#6]
                     +- *MapElements <function1>, obj#5: java.lang.String
                        +- *DeserializeToObject value#27.toString, obj#4: java.lang.String
                           +- LocalTableScan [value#27]

How was this patch tested?

Added a test case

gatorsmile · 2017-12-15T07:08:10Z

cc @marmbrus @hvanhovell @zsxwing @cloud-fan

gatorsmile · 2017-12-15T07:09:00Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala

+ override protected def sparkConf: SparkConf = super.sparkConf
+ .set("spark.redaction.string.regex", "file:/[\\w_]+")
+
+ test("explain - redaction") {


This is just a SS example. I believe we have more such cases.

Is it because you want to use explainInternal, so you write a SS test?

Just because I found a potential security issue in SS.

SparkQA · 2017-12-15T08:05:01Z

Test build #84946 has finished for PR 19985 at commit 8f44edf.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-15T11:15:59Z

Test build #84949 has finished for PR 19985 at commit 8f44edf.

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

gatorsmile · 2017-12-15T17:06:03Z

retest this please

zsxwing · 2017-12-15T18:44:18Z

sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala

+ * Redact the sensitive information in the given string.
+ */
+ private def withRedaction(message: => String): String = {
+ Utils.redact(SparkSession.getActiveSession.map(_.sparkContext.conf).orNull, message)


Why not make Utils.redact accept a regex parameter so that we can use session conf?

A different query may need to use a different regex, a session conf will be better and the user can change it during runtime.

zsxwing · 2017-12-15T18:49:57Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala

+
+ val explainWithoutExtended = q.explainInternal(false)
+ assert(explainWithoutExtended.contains(replacement))
+ assert(explainWithoutExtended.contains("StateStoreRestore"))


nit: add assert(!explainWithoutExtended.contains("file:/")) to verify it does replace the correct content.

zsxwing · 2017-12-15T18:50:02Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala

+
+ val explainWithExtended = q.explainInternal(true)
+ assert(explainWithExtended.contains(replacement))
+ assert(explainWithExtended.contains("StateStoreRestore"))


zsxwing · 2017-12-15T18:50:49Z

Looks good. My major comment is using a session conf instead.

SparkQA · 2017-12-15T19:54:24Z

Test build #84968 has finished for PR 19985 at commit 8f44edf.

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

SparkQA · 2017-12-16T11:42:15Z

Test build #85002 has finished for PR 19985 at commit e246096.

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

gatorsmile · 2017-12-16T17:27:02Z

retest this please

SparkQA · 2017-12-16T20:50:49Z

Test build #85011 has finished for PR 19985 at commit e246096.

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

gatorsmile · 2017-12-18T22:55:54Z

retest this please

gatorsmile · 2017-12-18T22:56:01Z

cc @zsxwing

zsxwing · 2017-12-18T23:01:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+ .doc("Regex to decide which parts of strings produced by Spark contain sensitive " +
+ "information. When this regex matches a string part, that string part is replaced by a " +
+ "dummy value. This is currently used to redact the output of SQL explain commands. " +
+ "When this conf is not set, the value from `spark.sql.redaction.string.regex` is used.")


nit: spark.sql.redaction.string.regex -> spark.redaction.string.regex

zsxwing

LGTM except one nit

SparkQA · 2017-12-19T02:29:03Z

Test build #85077 has finished for PR 19985 at commit d50d594.

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

cloud-fan · 2017-12-19T02:34:48Z

sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala

+ /**
+ * Redact the sensitive information in the given string.
+ */
+ private def withRedaction(message: => String): String = {


=> String looks not very useful here, we need to materialize anyway when calling Utils.redact

cloud-fan · 2017-12-19T02:35:57Z

sql/core/src/test/scala/org/apache/spark/sql/execution/DataSourceScanExecRedactionSuite.scala

+ assert(isIncluded(df.queryExecution, replacement))
+
+ assert(isIncluded(df.queryExecution, "FileScan"))
+ assert(!isIncluded(df.queryExecution, "file:/"))


what the difference between these 3 lines and https://github.com/apache/spark/pull/19985/files#diff-0c515221ed6e6eadcec71b3b9ad3a3e1R70 ?

SparkQA · 2017-12-19T08:02:53Z

Test build #85090 has finished for PR 19985 at commit 75c1479.

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

cloud-fan · 2017-12-19T14:12:33Z

thanks, merging to master!

fix.

8f44edf

gatorsmile commented Dec 15, 2017

View reviewed changes

cloud-fan approved these changes Dec 15, 2017

View reviewed changes

zsxwing reviewed Dec 15, 2017

View reviewed changes

address comments.

e246096

zsxwing reviewed Dec 18, 2017

View reviewed changes

gatorsmile added 2 commits December 18, 2017 15:07

fix.

b813b4d

clean

d50d594

cloud-fan reviewed Dec 19, 2017

View reviewed changes

address comments.

75c1479

asfgit closed this in 2831571 Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22791] [SQL] [SS] Redact Output of Explain #19985

[SPARK-22791] [SQL] [SS] Redact Output of Explain #19985

gatorsmile commented Dec 15, 2017 •

edited

Loading

gatorsmile commented Dec 15, 2017 •

edited

Loading

gatorsmile Dec 15, 2017

zsxwing Dec 15, 2017

gatorsmile Dec 16, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 15, 2017

gatorsmile commented Dec 15, 2017

zsxwing Dec 15, 2017

zsxwing Dec 15, 2017

zsxwing Dec 15, 2017

gatorsmile Dec 16, 2017

zsxwing Dec 15, 2017

zsxwing commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 16, 2017

gatorsmile commented Dec 16, 2017

SparkQA commented Dec 16, 2017

gatorsmile commented Dec 18, 2017

gatorsmile commented Dec 18, 2017

zsxwing Dec 18, 2017

zsxwing left a comment

SparkQA commented Dec 19, 2017

cloud-fan Dec 19, 2017

cloud-fan Dec 19, 2017

gatorsmile Dec 19, 2017

SparkQA commented Dec 19, 2017

cloud-fan commented Dec 19, 2017

[SPARK-22791] [SQL] [SS] Redact Output of Explain #19985

[SPARK-22791] [SQL] [SS] Redact Output of Explain #19985

Conversation

gatorsmile commented Dec 15, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

gatorsmile commented Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 15, 2017

SparkQA commented Dec 15, 2017

gatorsmile commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsxwing commented Dec 15, 2017

SparkQA commented Dec 15, 2017

SparkQA commented Dec 16, 2017

gatorsmile commented Dec 16, 2017

SparkQA commented Dec 16, 2017

gatorsmile commented Dec 18, 2017

gatorsmile commented Dec 18, 2017

Choose a reason for hiding this comment

zsxwing left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 19, 2017

cloud-fan commented Dec 19, 2017

gatorsmile commented Dec 15, 2017 •

edited

Loading

gatorsmile commented Dec 15, 2017 •

edited

Loading