[SPARK-22479][SQL] Exclude credentials from SaveintoDataSourceCommand.simpleString by onursatici · Pull Request #19708 · apache/spark

onursatici · 2017-11-09T14:53:32Z

What changes were proposed in this pull request?

Do not include jdbc properties which may contain credentials in logging a logical plan with SaveIntoDataSourceCommand in it.

How was this patch tested?

building locally and trying to reproduce (per the steps in https://issues.apache.org/jira/browse/SPARK-22479):

== Parsed Logical Plan ==
SaveIntoDataSourceCommand org.apache.spark.sql.execution.datasources.jdbc.JdbcRelationProvider@570127fa, Map(dbtable -> test20, driver -> org.postgresql.Driver, url -> *********(redacted), password -> *********(redacted)), ErrorIfExists
+- Range (0, 100, step=1, splits=Some(8))
== Analyzed Logical Plan ==
SaveIntoDataSourceCommand org.apache.spark.sql.execution.datasources.jdbc.JdbcRelationProvider@570127fa, Map(dbtable -> test20, driver -> org.postgresql.Driver, url -> *********(redacted), password -> *********(redacted)), ErrorIfExists
+- Range (0, 100, step=1, splits=Some(8))
== Optimized Logical Plan ==
SaveIntoDataSourceCommand org.apache.spark.sql.execution.datasources.jdbc.JdbcRelationProvider@570127fa, Map(dbtable -> test20, driver -> org.postgresql.Driver, url -> *********(redacted), password -> *********(redacted)), ErrorIfExists
+- Range (0, 100, step=1, splits=Some(8))
== Physical Plan ==
Execute SaveIntoDataSourceCommand
+- SaveIntoDataSourceCommand org.apache.spark.sql.execution.datasources.jdbc.JdbcRelationProvider@570127fa, Map(dbtable -> test20, driver -> org.postgresql.Driver, url -> *********(redacted), password -> *********(redacted)), ErrorIfExists
+- Range (0, 100, step=1, splits=Some(8))

ash211 · 2017-11-09T19:05:32Z

Jenkins, this is ok to test

SparkQA · 2017-11-09T21:57:44Z

Test build #83652 has finished for PR 19708 at commit 04aa9f0.

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

gatorsmile · 2017-11-10T04:11:09Z

 Seq.empty[Row]
 }
+
+ override def simpleString: String = s"SaveIntoDataSourceCommand ${dataSource}, ${mode}"


https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/util/Utils.scala#L2631-L2638
Reuse spark.redaction.regex?

I can use that, but I would need to expand the default for spark.redaction.regex as the user and url (some drivers allow credentials passed in the connection url) fields might also contain sensitive data. We should also change JDBCRelation::toString to include the redaction regex to be consistent.
I would argue not showing the jdbc properties at all because they provide little value and wrong redaction regex configuration could cause leaks to downstream log collection systems.
let me know if that makes sense, and I can modify this accordingly

SaveIntoDataSourceCommand is not being used for JDBC only.
JDBCRelation::toString was already fixed in #15975 ?

SparkQA · 2017-11-13T18:52:56Z

Test build #83799 has finished for PR 19708 at commit a0d15df.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-11-13T20:19:21Z

Test build #83810 has finished for PR 19708 at commit ae091ec.

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

gatorsmile · 2017-11-13T21:22:40Z

Just want to confirm whether the examples in the PR description are the ones based on the latest updates?

gatorsmile · 2017-11-13T21:26:48Z

Generally, this looks good to me.

cc @cloud-fan @hvanhovell

hvanhovell · 2017-11-13T21:31:37Z

This looks good. I was wondering if we shouldn't also take a look at data source operations like InsertIntoDataSourceCommand?

Could you also add a test?

SparkQA · 2017-11-13T23:38:04Z

Test build #83811 has finished for PR 19708 at commit 56f48f3.

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

onursatici · 2017-11-14T21:37:20Z

Yeah PR description reflects the latest changes.
InsertIntoDataSourceCommand could log credentials via the JDBCRelation it contains, but that class already properly redacts

SparkQA · 2017-11-15T00:46:58Z

Test build #83864 has finished for PR 19708 at commit db565f6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class SaveIntoDataSourceCommandSuite extends SharedSQLContext

ash211 · 2017-11-15T01:10:39Z

+ override protected def sparkConf: SparkConf = super.sparkConf
+ .set("spark.redaction.string.regex", "(?i)password|url")
+
+ test("treeString is redacted") {


old test name? we're not modifying the treeString anymore, it's just the SaveIntoDataSourceCommand

I followed the naming convention here:
spark/sql/core/src/test/scala/org/apache/spark/sql/execution/DataSourceScanExecRedactionSuite.scala
Line 33 in e9f983d
test("treeString is redacted") {
we are essentially redacting SaveIntoDataSourceCommand::simpleString which is called in SaceIntoDataSourceCommand::treeString

That is not really a convention. Can you just call it simpleString is redacted?

jiangxb1987 · 2017-11-15T13:47:00Z

+class SaveIntoDataSourceCommandSuite extends SharedSQLContext {
+
+ override protected def sparkConf: SparkConf = super.sparkConf
+ .set("spark.redaction.string.regex", "(?i)password|url")


Shoundn't this be spark.redaction.regex instead of spark.redaction.string.regex?

jiangxb1987

LGTM pending jenkins

gatorsmile · 2017-11-15T16:57:47Z

+class SaveIntoDataSourceCommandSuite extends SharedSQLContext {
+
+ override protected def sparkConf: SparkConf = super.sparkConf
+ .set("spark.redaction.regex", "(?i)password|url")


Nit: indents.

SparkQA · 2017-11-15T17:15:30Z

Test build #83895 has finished for PR 19708 at commit 2a1204b.

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

gatorsmile · 2017-11-15T17:27:15Z

LGTM

SparkQA · 2017-11-15T17:34:04Z

Test build #83896 has finished for PR 19708 at commit 2983bba.

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

ash211

LGTM

SparkQA · 2017-11-15T20:41:44Z

Test build #83901 has finished for PR 19708 at commit 2942477.

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

gatorsmile · 2017-11-15T22:12:08Z

Thanks! Merged to master.

@onursatici Could you submit a separate PR for 2.2?

do not log properties on SaveintoDataSourceCommand.simpleString
04aa9f0

gatorsmile reviewed Nov 10, 2017
View reviewed changes

redact all maps in treenode
a0d15df

onursatici changed the title ~~[SPARK-22479][SQL] Exclude credentials from SaveintoDataSourceCommand.simpleString~~[WIP][SPARK-22479][SQL] Exclude credentials from SaveintoDataSourceCommand.simpleStringNov 13, 2017

redact options for SaveIntoDataSourceCommand
56f48f3

onursatici force-pushed the os/redact-jdbc-creds branch from ae091ec to 56f48f3Compare November 13, 2017 20:22

test redaction for saveintodatasourcecommand
db565f6

onursatici changed the title ~~[WIP][SPARK-22479][SQL] Exclude credentials from SaveintoDataSourceCommand.simpleString~~[SPARK-22479][SQL] Exclude credentials from SaveintoDataSourceCommand.simpleStringNov 14, 2017

ash211 reviewed Nov 15, 2017
View reviewed changes

jiangxb1987 reviewed Nov 15, 2017
View reviewed changes

change correct redact conf key
2a1204b

jiangxb1987 approved these changes Nov 15, 2017
View reviewed changes

rename test
2983bba

gatorsmile reviewed Nov 15, 2017
View reviewed changes

indent
2942477

ash211 approved these changes Nov 15, 2017
View reviewed changes

asfgit closed this in 2014e7aNov 15, 2017

onursatici mentioned this pull request Nov 15, 2017
[SPARK-22479][SQL][BRANCH-2.2] Exclude credentials from SaveintoDataSourceCommand.simpleString #19761
Closed

Uh oh!

Conversation

onursatici commented Nov 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ash211 commented Nov 9, 2017

Uh oh!

SparkQA commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 13, 2017

Uh oh!

SparkQA commented Nov 13, 2017

Uh oh!

gatorsmile commented Nov 13, 2017

Uh oh!

gatorsmile commented Nov 13, 2017

Uh oh!

hvanhovell commented Nov 13, 2017

Uh oh!

SparkQA commented Nov 13, 2017

Uh oh!

onursatici commented Nov 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

gatorsmile commented Nov 15, 2017

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

ash211 left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 15, 2017

Uh oh!

gatorsmile commented Nov 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

onursatici commented Nov 9, 2017•
edited
Loading

onursatici commented Nov 14, 2017•
edited
Loading