[SPARK-31272][SQL] Support DB2 Kerberos login in JDBC connector #28215

gaborgsomogyi · 2020-04-14T13:34:08Z

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 DB2 support (other supported databases will come in later PRs).

What this PR contains:

Added DB2ConnectionProvider
Added DB2ConnectionProviderSuite
Added DB2KrbIntegrationSuite docker integration test
Changed DB2 JDBC driver to use the latest (test scope only)
Changed test table data type to a type which is supported by all the databases
Removed double connection creation on test side
Increased connection timeout in docker tests because DB2 docker takes quite a time to start

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 DB2 using kerberos.

How was this patch tested?

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

gaborgsomogyi · 2020-04-14T13:35:26Z

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


-
 @DockerTest
-@Ignore // AMPLab Jenkins needs to be updated before shared memory works on docker


Not relevant change. Since docker tests are not integrated into jenkins we can turn this on.

I'm seeing that other test suites for other DBMS don't have this, so good to remove to make it consistent.

Not really following the discussion; are you guys saying this line should be removed? Because there's nothing changing here.

Yes, as other tests don't have this. It's not a kind of "should be", but "can be".

And it's removed in code diff as of now.

I don't see it removed. It's still there.

Sorry for the confusion, I should be more clearer - the change is removed, in other words, rolled back. No change.

gaborgsomogyi · 2020-04-14T13:36:09Z

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

+  /**
+   * Parameter whether the container should run privileged.
+   */
+  val privileged: Boolean = false


DB2 docker requires privileged run.

gaborgsomogyi · 2020-04-14T13:36:43Z

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

-        val conn = java.sql.DriverManager.getConnection(jdbcUrl)
-        conn.close()
+      var conn: Connection = null
+      eventually(timeout(2.minutes), interval(1.second)) {


Single connection simplification + timeout increase.

gaborgsomogyi · 2020-04-14T13:37:07Z

...tegration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerKrbJDBCIntegrationSuite.scala


  override def dataPreparation(conn: Connection): Unit = {
-    conn.prepareStatement("CREATE TABLE bar (c0 text)").executeUpdate()
+    conn.prepareStatement("CREATE TABLE bar (c0 VARCHAR(8))").executeUpdate()


DB2 doesn't support text.

I'd be surprised if this change affects others, but it may be worth to test others manually and mention the result.

When doing modifications I'm always re-executing all of them. This has happened here too.

And not to avoid the question all passed :)

gaborgsomogyi · 2020-04-14T13:39:12Z

external/docker-integration-tests/src/test/resources/db2_krb_setup.sh

+USERPROFILE=/database/config/db2inst1/sqllib/userprofile
+echo "export DB2_KRB5_PRINCIPAL=db2/__IP_ADDRESS_REPLACE_ME__@EXAMPLE.COM" >> $USERPROFILE
+echo "export KRB5_KTNAME=/var/custom/db2.keytab" >> $USERPROFILE
+su - db2inst1 -c "db2set DB2ENVLIST=KRB5_KTNAME"


This trick is needed because DB2 forwards environment variables automatically only if it's starting with DB2 (KRB5_KTNAME doesn't fit).

It would be nice to add this as "comment" to reduce the hops to finally reach this comment on understanding this trick.

Good idea, added.

gaborgsomogyi · 2020-04-14T13:43:06Z

cc @HeartSaVioR

SparkQA · 2020-04-14T16:27:18Z

Test build #121279 has finished for PR 28215 at commit 59d34fa.

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

gaborgsomogyi · 2020-04-14T17:36:48Z

Looks unrelated.

gaborgsomogyi · 2020-04-14T17:37:03Z

Retest this please.

SparkQA · 2020-04-14T20:19:36Z

Test build #121287 has finished for PR 28215 at commit 59d34fa.

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

HeartSaVioR

The code change looks great in overall. As I commented in earlier PRs, still not be able to run the tests (and it's tricky to modify my local env. only for running tests), so just assumed you've run the tests manually.

HeartSaVioR · 2020-04-15T07:21:34Z

external/docker-integration-tests/src/test/resources/db2_krb_setup.sh

+USERPROFILE=/database/config/db2inst1/sqllib/userprofile
+echo "export DB2_KRB5_PRINCIPAL=db2/__IP_ADDRESS_REPLACE_ME__@EXAMPLE.COM" >> $USERPROFILE
+echo "export KRB5_KTNAME=/var/custom/db2.keytab" >> $USERPROFILE
+su - db2inst1 -c "db2set DB2ENVLIST=KRB5_KTNAME"


It would be nice to add this as "comment" to reduce the hops to finally reach this comment on understanding this trick.

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

...scala/org/apache/spark/sql/execution/datasources/jdbc/connection/DB2ConnectionProvider.scala

HeartSaVioR · 2020-04-15T09:00:31Z

...tegration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerKrbJDBCIntegrationSuite.scala


  override def dataPreparation(conn: Connection): Unit = {
-    conn.prepareStatement("CREATE TABLE bar (c0 text)").executeUpdate()
+    conn.prepareStatement("CREATE TABLE bar (c0 VARCHAR(8))").executeUpdate()


I'd be surprised if this change affects others, but it may be worth to test others manually and mention the result.

gaborgsomogyi · 2020-04-16T14:51:35Z

Re-executed the docker tests and passed.

SparkQA · 2020-04-16T17:33:47Z

Test build #121369 has finished for PR 28215 at commit eb4878c.

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

gaborgsomogyi · 2020-04-16T18:20:15Z

retest this, please

SparkQA · 2020-04-16T22:17:04Z

Test build #121374 has finished for PR 28215 at commit eb4878c.

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

HeartSaVioR

LGTM assuming manual tests were passed

gaborgsomogyi · 2020-04-17T10:40:26Z

cc @dongjoon-hyun @vanzin since you know this area well

dongjoon-hyun · 2020-04-18T20:44:16Z

Hi, @gaborgsomogyi . It would be great if you proceed Updating and enabling DB2IntegrationSuite in a separate PR first.

dongjoon-hyun · 2020-04-18T20:46:32Z

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

-@Ignore // AMPLab Jenkins needs to be updated before shared memory works on docker
 class DB2IntegrationSuite extends DockerJDBCIntegrationSuite {
  override val db = new DatabaseOnDocker {
    override val imageName = "lresende/db2express-c:10.5.0.5-3.10.0"


Please update this image to the official one like ibmcom/db2:11.5.0.0a. It would be great if we use the same DB2 version in both DB2IntegrationSuite and DB2KrbIntegrationSuite.

The above comment is for the new PR.

It worth a separate PR because it's a non-trivial change.

SparkQA · 2020-04-20T14:55:46Z

Test build #121526 has finished for PR 28215 at commit e56c7bd.

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

vanzin · 2020-04-22T03:07:12Z

The check failure is valid, it's failing to compile in that environment.

HeartSaVioR · 2020-04-22T03:16:47Z

Oh, nice catch. That compilation error seems to only come with JDK11.

That said, Spark PR builder may need to run at least two different environments; that's lucky we found the issue from Github Action, but it has been the source of false alarm hence not considered seriously in many case. E.g. we may not able to find this if we see failure in Linter build side on Github Action, as building Spark will be cancelled if Linter build fails.

gaborgsomogyi · 2020-04-22T10:50:37Z

Ah gosh! Fixed it.

SparkQA · 2020-04-22T13:45:25Z

Test build #121616 has finished for PR 28215 at commit 1ecacec.

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

vanzin · 2020-04-23T00:09:52Z

Merging to master.

dongjoon-hyun · 2020-04-25T00:55:15Z

Is there additional required for this? All the other test (including new DB2IntegreationSuite in #28325) passed, but this one fail.

...
DB2KrbIntegrationSuite:
org.apache.spark.sql.jdbc.DB2KrbIntegrationSuite *** ABORTED ***
  Exception encountered when invoking run on a nested suite - The code passed to eventually never returned normally. Attempted 128 times over 2.016876443166667 minutes. Last failure message: Login failure for db2/10.0.0.6@EXAMPLE.COM from keytab /Users/dongjoon/PRS/SPARK-PR-28325/external/docker-integration-tests/target/tmp/spark-35512019-33ef-4cad-a54a-2bec69a3d4c2/db2.keytab. (DockerJDBCIntegrationSuite.scala:158)
...
Run completed in 4 minutes, 27 seconds.
Total number of tests run: 42
Suites: completed 8, aborted 1
Tests: succeeded 42, failed 0, canceled 0, ignored 0, pending 0
*** 1 SUITE ABORTED ***

… the DB2 docker inside ### What changes were proposed in this pull request? This is a followup PR discussed [here](#28215 (comment)). ### Why are the changes needed? It would be good to re-enable `DB2IntegrationSuite` and upgrade the docker image inside to use the latest. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing docker integration tests. Closes #28325 from gaborgsomogyi/SPARK-31533. Authored-by: Gabor Somogyi <gabor.g.somogyi@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2020-04-25T21:45:45Z

Hi, Guys.
Currently, all the other suites including DB2IntegrationSuite passed . Only DB2KrbIntegrationSuite fails. This PR seems to need more verification. Although this was verified manually, the newly added integration test should be valid.

How can we handle this commit?

gaborgsomogyi · 2020-04-26T04:50:04Z

Is it fine to check it on Monday as a start?

gaborgsomogyi · 2020-04-26T04:52:01Z

Logs would be helpful from test side and from docker instance side to see what have gone wrong.

dongjoon-hyun · 2020-04-26T05:48:33Z

Sure, no rush for this because this is an integration test we will not revert this urgently. I just wondered this IT was tested or not when this PR was merged.

gaborgsomogyi · 2020-04-26T06:39:22Z

I'm re-executing them after each change because it's only couple of minutes. I've just tried it out and works on my machine. On Monday I'll put it into a loop but logs would speed up the process because such case we dont have to spend time on reproduction.

dongjoon-hyun · 2020-04-26T18:38:22Z

Got it. Could you share your work environment? Then, I can try your way if possible. I tried master branch in my two separate machines, but there was no luck. I'm using Docker Desktop community (2.3.0.0) on both MacBook Pro and MacPro.

For the record, in two machines, this test suite consecutively fails several times (I also increased the resources up to 10 cores and 22GB memory) and never succeeds in this environment.

dongjoon-hyun · 2020-04-26T19:28:12Z

Hi, @gatorsmile and @maropu .
Could you verify this new JDBC integration test suite additionally, please?
I'm asking your help since it might be a dev environment issue and I failed so far.

maropu · 2020-04-27T05:45:31Z

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

+  override protected def setAuthentication(keytabFile: String, principal: String): Unit = {
+    val config = new SecureConnectionProvider.JDBCConfiguration(
+      Configuration.getConfiguration, "JaasClient", keytabFile, principal)
+    Configuration.setConfiguration(config)


Is this safe when scanning tables in different secure databases ?

Nice catch! I'll create a separate jira to handle config synchronisation globally...

Spark can scan different JDBC relations concurrently though, could we synchronized them easily?

I think the solution shouldn't be complicated but it effects all other providers which change the configuration (not just DB2).

I've filed https://issues.apache.org/jira/browse/SPARK-31575 to handle the issue.

gaborgsomogyi · 2020-04-27T08:08:19Z

@dongjoon-hyun here is my environment (just checked the latest master and still works):

MacBook Pro
macOS Catalina 10.15.3 (19D76)
openjdk version "1.8.0_242"
Docker desktop community 2.1.0.5 (40693)

gaborgsomogyi · 2020-04-27T08:12:49Z

That said maybe my environment is just a lucky one and would be good to take a look at the logs from test side and from container side as well.

maropu · 2020-04-27T11:35:15Z

I run it by myself and I checked it passed on my MacOS (Sierra, Docker Desktop community v2.3.0.0).

dongjoon-hyun · 2020-04-27T15:59:44Z

Thank you, @maropu !

dongjoon-hyun · 2020-04-27T16:00:14Z

Thank you, @gaborgsomogyi .

[SPARK-31272][SQL] Support DB2 Kerberos login in JDBC connector

59d34fa

probot-autolabeler bot added BUILD SQL labels Apr 14, 2020

gaborgsomogyi commented Apr 14, 2020

View reviewed changes

HeartSaVioR reviewed Apr 15, 2020

View reviewed changes

Review fix

eb4878c

HeartSaVioR approved these changes Apr 17, 2020

View reviewed changes

dongjoon-hyun reviewed Apr 18, 2020

View reviewed changes

Rollback DB2IntegrationSuite ignore

e56c7bd

Java11 fix

1ecacec

vanzin closed this in c619990 Apr 23, 2020

gaborgsomogyi mentioned this pull request Apr 24, 2020

[SPARK-31533][SQL][TESTS] Enable DB2IntegrationSuite test and upgrade the DB2 docker inside #28325

Closed

maropu reviewed Apr 27, 2020

View reviewed changes



		@DockerTest
		@Ignore // AMPLab Jenkins needs to be updated before shared memory works on docker

[SPARK-31272][SQL] Support DB2 Kerberos login in JDBC connector #28215

[SPARK-31272][SQL] Support DB2 Kerberos login in JDBC connector #28215

Uh oh!

Conversation

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

Uh oh!

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!

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!

HeartSaVioR Apr 22, 2020 • 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!

gaborgsomogyi Apr 14, 2020 • 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!

gaborgsomogyi Apr 14, 2020 • 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!

gaborgsomogyi commented Apr 14, 2020

Uh oh!

SparkQA commented Apr 14, 2020

Uh oh!

gaborgsomogyi commented Apr 14, 2020

Uh oh!

gaborgsomogyi commented Apr 14, 2020

Uh oh!

SparkQA commented Apr 14, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborgsomogyi commented Apr 16, 2020

Uh oh!

SparkQA commented Apr 16, 2020

Uh oh!

gaborgsomogyi commented Apr 16, 2020

Uh oh!

SparkQA commented Apr 16, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

gaborgsomogyi commented Apr 17, 2020

gaborgsomogyi commented Apr 14, 2020 •

edited

Loading

HeartSaVioR Apr 22, 2020 •

edited

Loading

gaborgsomogyi Apr 14, 2020 •

edited

Loading

gaborgsomogyi Apr 14, 2020 •

edited

Loading

dongjoon-hyun commented Apr 26, 2020 •

edited

Loading

maropu Apr 27, 2020 •

edited

Loading