[SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources #32344

sarutak · 2021-04-26T05:39:00Z

What changes were proposed in this pull request?

This PR proposes to introduce a new JDBC option refreshKrb5Config which allows to reflect the change of krb5.conf.

Why are the changes needed?

In the current master, JDBC datasources can't accept refreshKrb5Config which is defined in Krb5LoginModule.
So even if we change the krb5.conf after establishing a connection, the change will not be reflected.

The similar issue happens when we run multiple *KrbIntegrationSuites at the same time.
MiniKDC starts and stops every KerberosIntegrationSuite and different port number is recorded to krb5.conf.
Due to SecureConnectionProvider.JDBCConfiguration doesn't take refreshKrb5Config, KerberosIntegrationSuites except the first running one see the wrong port so those suites fail.
You can easily confirm with the following command.

build/sbt -Phive Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite"

Does this PR introduce any user-facing change?

Yes. Users can set refreshKrb5Config to refresh krb5 relevant configuration.

How was this patch tested?

New test.

SparkQA · 2021-04-26T06:30:29Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42456/

SparkQA · 2021-04-26T08:11:35Z

Test build #137934 has finished for PR 32344 at commit 1a2c472.

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

dongjoon-hyun · 2021-04-26T14:28:32Z

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

+    Configuration.setConfiguration(null)
+    withTempDir { dir =>
+      val dummyKrb5Conf = File.createTempFile("dummy", "krb5.conf", dir)
+      val origKrb5Conf = sys.props("java.security.krb5.conf")


Shall we use the pre-defined KRB5_CONF_PROP instead of java.security.krb5.conf?

Thank you. I've updated.

dongjoon-hyun

+1, LGTM (with minor comment).
Thank you, @sarutak .

dongjoon-hyun · 2021-04-27T00:54:08Z

Thank you for update.

HyukjinKwon · 2021-04-27T02:11:14Z

cc @gaborgsomogyi FYI

SparkQA · 2021-04-27T02:15:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42494/

SparkQA · 2021-04-27T02:20:01Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42494/

SparkQA · 2021-04-27T04:36:11Z

Test build #137974 has finished for PR 32344 at commit 5df750d.

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

gaborgsomogyi · 2021-04-27T08:22:21Z

@HyukjinKwon thanks for pinging me.

I've had a look at the change and I think it looks good but can introduce a race. Let me share the use-case:

User sets refreshKrb5Config flag with Security context 1
User uses a JDBC connection provider
User modifies krb5.conf but JVM not yet realized that it must be reloaded
Spark authenticates here successfully
JVM loads the Security context 2 from the modified krb5.conf
Spark restores the previously saved Security context 1 here
The modified krb5.conf content just gone

This is just one example, the second one is when config update happens between security context modification and authentication. Such case JDBC authentication will fail temporarily.

I'm not against to add this but I think it's a must to mention somewhere that setting this flag can cause severe issues on running workloads. Seems like the doc change is missing, right?

sarutak · 2021-04-27T21:03:01Z

@gaborgsomogyi Thanks for your suggestion. So, it seems better to note in the doc about the possibility of race condition.

SparkQA · 2021-04-28T06:17:32Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42543/

SparkQA · 2021-04-28T06:22:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42543/

SparkQA · 2021-04-28T08:28:54Z

Test build #138024 has finished for PR 32344 at commit 600598e.

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

gaborgsomogyi

LGTM.

### What changes were proposed in this pull request? This PR proposes to introduce a new JDBC option `refreshKrb5Config` which allows to reflect the change of `krb5.conf`. ### Why are the changes needed? In the current master, JDBC datasources can't accept `refreshKrb5Config` which is defined in `Krb5LoginModule`. So even if we change the `krb5.conf` after establishing a connection, the change will not be reflected. The similar issue happens when we run multiple `*KrbIntegrationSuites` at the same time. `MiniKDC` starts and stops every KerberosIntegrationSuite and different port number is recorded to `krb5.conf`. Due to `SecureConnectionProvider.JDBCConfiguration` doesn't take `refreshKrb5Config`, KerberosIntegrationSuites except the first running one see the wrong port so those suites fail. You can easily confirm with the following command. ``` build/sbt -Phive Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` ### Does this PR introduce _any_ user-facing change? Yes. Users can set `refreshKrb5Config` to refresh krb5 relevant configuration. ### How was this patch tested? New test. Closes #32344 from sarutak/kerberos-refresh-issue. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com> (cherry picked from commit 529b875) Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>

sarutak · 2021-04-29T04:56:47Z

Merged to master and branch-3.1. Thanks all.

…IntegrationSuite ### What changes were proposed in this pull request? This PR fixes an test added in SPARK-35226 (#32344). ### Why are the changes needed? `SELECT 1` seems non-valid query for DB2. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? DB2KrbIntegrationSuite passes on my laptop. I also confirmed all the KrbIntegrationSuites pass with the following command. ``` build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` Closes #32632 from sarutak/followup-SPARK-35226. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…IntegrationSuite ### What changes were proposed in this pull request? This PR fixes an test added in SPARK-35226 (#32344). ### Why are the changes needed? `SELECT 1` seems non-valid query for DB2. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? DB2KrbIntegrationSuite passes on my laptop. I also confirmed all the KrbIntegrationSuites pass with the following command. ``` build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` Closes #32632 from sarutak/followup-SPARK-35226. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 1a43415) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

### What changes were proposed in this pull request? This PR proposes to introduce a new JDBC option `refreshKrb5Config` which allows to reflect the change of `krb5.conf`. ### Why are the changes needed? In the current master, JDBC datasources can't accept `refreshKrb5Config` which is defined in `Krb5LoginModule`. So even if we change the `krb5.conf` after establishing a connection, the change will not be reflected. The similar issue happens when we run multiple `*KrbIntegrationSuites` at the same time. `MiniKDC` starts and stops every KerberosIntegrationSuite and different port number is recorded to `krb5.conf`. Due to `SecureConnectionProvider.JDBCConfiguration` doesn't take `refreshKrb5Config`, KerberosIntegrationSuites except the first running one see the wrong port so those suites fail. You can easily confirm with the following command. ``` build/sbt -Phive Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` ### Does this PR introduce _any_ user-facing change? Yes. Users can set `refreshKrb5Config` to refresh krb5 relevant configuration. ### How was this patch tested? New test. Closes apache#32344 from sarutak/kerberos-refresh-issue. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com> (cherry picked from commit 529b875) Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>

…IntegrationSuite ### What changes were proposed in this pull request? This PR fixes an test added in SPARK-35226 (apache#32344). ### Why are the changes needed? `SELECT 1` seems non-valid query for DB2. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? DB2KrbIntegrationSuite passes on my laptop. I also confirmed all the KrbIntegrationSuites pass with the following command. ``` build/sbt -Phive -Phive-thriftserver -Pdocker-integration-tests "testOnly org.apache.spark.sql.jdbc.*KrbIntegrationSuite" ``` Closes apache#32632 from sarutak/followup-SPARK-35226. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 1a43415) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

sarutak added 2 commits April 26, 2021 12:00

Add refreshKrb5Config parameter.

ef78ae4

Add test.

1a2c472

github-actions bot added the SQL label Apr 26, 2021

dongjoon-hyun reviewed Apr 26, 2021

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-35226][SQL] JDBC datasources should accept refreshKrb5Config parameter~~ [SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources Apr 26, 2021

dongjoon-hyun approved these changes Apr 26, 2021

View reviewed changes

"Replace java.security.krb5.conf" with KRB5_CONF.

5df750d

Add document.

600598e

github-actions bot added the DOCS label Apr 28, 2021

gaborgsomogyi approved these changes Apr 28, 2021

View reviewed changes

sarutak closed this in 529b875 Apr 29, 2021

sarutak mentioned this pull request May 22, 2021

[SPARK-35226][SQL][FOLLOWUP] Fix test added in SPARK-35226 for DB2KrbIntegrationSuite #32632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources #32344

[SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources #32344

sarutak commented Apr 26, 2021 •

edited

Loading

SparkQA commented Apr 26, 2021

SparkQA commented Apr 26, 2021

dongjoon-hyun Apr 26, 2021

sarutak Apr 27, 2021

dongjoon-hyun left a comment

dongjoon-hyun commented Apr 27, 2021

HyukjinKwon commented Apr 27, 2021

SparkQA commented Apr 27, 2021

SparkQA commented Apr 27, 2021

SparkQA commented Apr 27, 2021

gaborgsomogyi commented Apr 27, 2021

sarutak commented Apr 27, 2021 •

edited

Loading

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

gaborgsomogyi left a comment

sarutak commented Apr 29, 2021

[SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources #32344

[SPARK-35226][SQL] Support refreshKrb5Config option in JDBC datasources #32344

Conversation

sarutak commented Apr 26, 2021 • edited Loading

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?

SparkQA commented Apr 26, 2021

SparkQA commented Apr 26, 2021

dongjoon-hyun Apr 26, 2021

Choose a reason for hiding this comment

sarutak Apr 27, 2021

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Apr 27, 2021

HyukjinKwon commented Apr 27, 2021

SparkQA commented Apr 27, 2021

SparkQA commented Apr 27, 2021

SparkQA commented Apr 27, 2021

gaborgsomogyi commented Apr 27, 2021

sarutak commented Apr 27, 2021 • edited Loading

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

SparkQA commented Apr 28, 2021

gaborgsomogyi left a comment

Choose a reason for hiding this comment

sarutak commented Apr 29, 2021

sarutak commented Apr 26, 2021 •

edited

Loading

sarutak commented Apr 27, 2021 •

edited

Loading