[SPARK-31337][SQL]Support MS SQL Kerberos login in JDBC connector #28635

gaborgsomogyi · 2020-05-25T10:42:00Z

What changes were proposed in this pull request?

When loading DataFrames from JDBC datasource with Kerberos authentication, remote executors (yarn-client/cluster etc. modes) fail to establish a connection due to lack of Kerberos ticket or ability to generate it.

This is a real issue when trying to ingest data from kerberized data sources (SQL Server, Oracle) in enterprise environment where exposing simple authentication access is not an option due to IT policy issues.

In this PR I've added MS SQL support.

What this PR contains:

Added MSSQLConnectionProvider
Added MSSQLConnectionProviderSuite
Changed MS SQL JDBC driver to use the latest (test scope only)
Changed MsSqlServerIntegrationSuite docker image to use the latest
Added a version comment to MariaDBConnectionProvider to increase trackability

Why are the changes needed?

Missing JDBC kerberos support.

Does this PR introduce any user-facing change?

Yes, now user is able to connect to MS SQL using kerberos.

How was this patch tested?

Additional + existing unit tests
Existing integration tests
Test on cluster manually

gaborgsomogyi · 2020-05-25T10:50:49Z

Apart from manual testing I've tried to add docker integration test (failed) and tried out the following:

Set up an active directory docker => not yet supported, please see this link for details.
Use Hadoop MiniKdc => simply never authenticated, furthermore MS never claimed it is working

If there will be a working docker image with an Active Directory instance we can try it again. In the meantime if somebody has an idea how to overcome this feel free to add.

SparkQA · 2020-05-25T10:51:27Z

Test build #123081 has finished for PR 28635 at commit 3c30c4a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

gaborgsomogyi

Couple of pointers to help reviewers.

There is a driver wrapper here which achieves more or less the same behaviour but with an external jar. The blogpost can be found here.

gaborgsomogyi · 2020-05-25T11:30:28Z

...integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MsSqlServerIntegrationSuite.scala

 class MsSqlServerIntegrationSuite extends DockerJDBCIntegrationSuite {
  override val db = new DatabaseOnDocker {
-    override val imageName = "mcr.microsoft.com/mssql/server:2017-GA-ubuntu"
+    override val imageName = "mcr.microsoft.com/mssql/server:2019-GA-ubuntu-16.04"


This is not absolutely necessary, if we think we can extract it into a new PR. Thought it would be overkill.

gaborgsomogyi · 2020-05-25T11:33:54Z

...ala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala

+    val configName = "jaasConfigurationName"
+    val appEntryDefault = "SQLJDBCDriver"
+
+    val parseURL = try {


There are basically 2 approaches to parse the URL to get jaasConfigurationName:

Try to call private parseAndMergeProperties => tried it first

Parse the URL manually => used as fallback

Both way has been tested in MSSQLConnectionProviderSuite.

gaborgsomogyi · 2020-05-25T11:40:20Z

...ala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala

+
+import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions
+
+private[sql] class MSSQLConnectionProvider(


The implementation is based on this.

gaborgsomogyi · 2020-05-25T11:46:43Z

...ala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala

+    result.put("integratedSecurity", "true")
+    result.put("authenticationScheme", "JavaKerberos")


These props needed to reach kerberos authentication case:
https://github.com/microsoft/mssql-jdbc/blob/0d4e97f401dc0e55779460d9709dd7ee399246be/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java#L3771-L3772

It'd be nice to add the background information on comment, either the code side or the class doc.

Added an inline comment.

gaborgsomogyi · 2020-05-25T11:54:54Z

...rg/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProviderSuite.scala

+  }
+
+  test("setAuthenticationConfigIfNeeded custom parser must set authentication if not set") {
+    val parserMethod = "IntentionallyNotExistingMethod"


This simulates a driver where the private parser method doesn't exist. Such case manual parsing takes place and the code goes forward without issue.

gaborgsomogyi · 2020-05-25T12:15:11Z

cc @HeartSaVioR

SparkQA · 2020-05-25T14:00:48Z

Test build #123082 has finished for PR 28635 at commit 88306f0.

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

...rg/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProviderSuite.scala

SparkQA · 2020-05-25T18:55:21Z

Test build #123090 has finished for PR 28635 at commit 16687d6.

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

dev/deps/spark-deps-hadoop-3.2-hive-2.3

SparkQA · 2020-05-27T10:25:25Z

Test build #123177 has finished for PR 28635 at commit 587a502.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ClusterStats(featureSum: Vector, squaredNormSum: Double, weightSum: Double)
class ClusteringEvaluator(JavaEvaluator, HasPredictionCol, HasFeaturesCol, HasWeightCol,
case class Log(child: Expression) extends UnaryLogExpression(StrictMath.log, \"LOG\")
case class CurrentCatalog() extends LeafExpression with Unevaluable
case class GetCurrentDatabaseAndCatalog(catalogManager: CatalogManager) extends Rule[LogicalPlan]

gaborgsomogyi · 2020-05-27T10:33:27Z

retest this, please

SparkQA · 2020-05-27T13:35:52Z

Test build #123184 has finished for PR 28635 at commit 587a502.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ClusterStats(featureSum: Vector, squaredNormSum: Double, weightSum: Double)
class ClusteringEvaluator(JavaEvaluator, HasPredictionCol, HasFeaturesCol, HasWeightCol,
case class Log(child: Expression) extends UnaryLogExpression(StrictMath.log, \"LOG\")
case class CurrentCatalog() extends LeafExpression with Unevaluable
case class GetCurrentDatabaseAndCatalog(catalogManager: CatalogManager) extends Rule[LogicalPlan]

HeartSaVioR

I've gone through the general review but I'm afraid I'm not qualified to review in details.

...ala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala

HeartSaVioR · 2020-06-01T03:52:42Z

...ala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala

+    result.put("integratedSecurity", "true")
+    result.put("authenticationScheme", "JavaKerberos")


It'd be nice to add the background information on comment, either the code side or the class doc.

gaborgsomogyi · 2020-06-04T11:08:21Z

cc @vanzin

SparkQA · 2020-06-04T13:48:22Z

Test build #123530 has finished for PR 28635 at commit 0a71849.

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

vanzin · 2020-06-16T02:09:39Z

retest this please

SparkQA · 2020-06-16T05:45:03Z

Test build #124083 has finished for PR 28635 at commit 0a71849.

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

vanzin · 2020-06-17T01:22:06Z

Merging to master.

thereverand · 2020-06-20T22:44:38Z

I could be wrong, but it doesn't look like this fixes this issue completely.
It doesn't support username/password and Kerberos, which the driver supports, and if the proper krb5.conf and JAAS config are in place does work on the driver node of a Databricks cluster, but then fails on the remote executors in the cluster with the same error being described here. So it looks to me to be the same issue.

gaborgsomogyi · 2020-06-30T14:06:59Z

@thereverand just seen your comment. It has never stated that username/password is working.
Propagating u/p through the whole cluster will enlarge the attack surface.
This supports keytab which can be better protected.

As a resolution we're planning to add JDBC connection provider API in SPARK-32001 where any custom provider can be added.
This allows each provider to add custom authentication possibilities.

[SPARK-31337][SQL]Support MS SQL Kerberos login in JDBC connector

3c30c4a

probot-autolabeler bot added BUILD SQL labels May 25, 2020

Fix deps

88306f0

gaborgsomogyi commented May 25, 2020

View reviewed changes

seanli-rallyhealth reviewed May 25, 2020

View reviewed changes

...rg/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProviderSuite.scala Outdated Show resolved Hide resolved

Review fix

16687d6

dongjoon-hyun reviewed May 26, 2020

View reviewed changes

dev/deps/spark-deps-hadoop-3.2-hive-2.3 Outdated Show resolved Hide resolved

gaborgsomogyi mentioned this pull request May 26, 2020

[SPARK-31821][BUILD] Remove mssql-jdbc dependencies from Hadoop 3.2 profile #28640

Closed

3 tasks

Merge branch 'master' into SPARK-31337

587a502

HeartSaVioR reviewed Jun 1, 2020

View reviewed changes

Review fix

0a71849

vanzin closed this in eeb8120 Jun 17, 2020


		import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions

		private[sql] class MSSQLConnectionProvider(

		result.put("integratedSecurity", "true")
		result.put("authenticationScheme", "JavaKerberos")

[SPARK-31337][SQL]Support MS SQL Kerberos login in JDBC connector #28635

[SPARK-31337][SQL]Support MS SQL Kerberos login in JDBC connector #28635

Uh oh!

Conversation

gaborgsomogyi commented May 25, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

gaborgsomogyi commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 25, 2020

Uh oh!

gaborgsomogyi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

gaborgsomogyi commented May 25, 2020

Uh oh!

SparkQA commented May 25, 2020

Uh oh!

Uh oh!

SparkQA commented May 25, 2020

Uh oh!

Uh oh!

SparkQA commented May 27, 2020

Uh oh!

gaborgsomogyi commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 27, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborgsomogyi commented Jun 4, 2020

Uh oh!

SparkQA commented Jun 4, 2020

Uh oh!

vanzin commented Jun 16, 2020

Uh oh!

SparkQA commented Jun 16, 2020

Uh oh!

vanzin commented Jun 17, 2020

Uh oh!

thereverand commented Jun 20, 2020

Uh oh!

gaborgsomogyi commented Jun 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gaborgsomogyi commented May 25, 2020 •

edited

Loading

gaborgsomogyi left a comment •

edited

Loading

gaborgsomogyi commented May 27, 2020 •

edited

Loading