gaborgsomogyi commented on a change in pull request #34745:
URL: https://github.com/apache/spark/pull/34745#discussion_r766593758
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala
##########
@@ -68,12 +69,20 @@ class ConnectionProviderSuite
override def canHandle(driver: Driver, options: Map[String, String]):
Boolean = true
override def getConnection(driver: Driver, options: Map[String,
String]): Connection =
throw new RuntimeException()
+ override def needsModifySecurityConfiguration(
Review comment:
Bad idea to add default implementation(at least within the trait). Such
case a user may think it's not important part of the provider and will work
with the default. This is not true for all the secure providers.
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcConnectionProvider.scala
##########
@@ -53,12 +53,25 @@ abstract class JdbcConnectionProvider {
def canHandle(driver: Driver, options: Map[String, String]): Boolean
/**
- * Opens connection toward the database. Since global JVM security
configuration change may needed
- * this API is called synchronized by `SecurityConfigurationLock` to avoid
race.
+ * Opens connection to the database. Since global JVM security configuration
change may be
+ * needed this API is called synchronized by `SecurityConfigurationLock` to
avoid race when
+ * `needsModifySecurityConfiguration` returns true for the given driver with
the given options.
*
* @param driver Java driver which initiates the connection
* @param options Driver options which initiates the connection
* @return a `Connection` object that represents a connection to the URL
*/
def getConnection(driver: Driver, options: Map[String, String]): Connection
+
+ /**
+ * Checks if this connection provider instance needs to modify global
security configuration to
+ * handle authentication and thus should synchronize access to the security
configuration while
+ * the given driver is initiating a connection with the given options.
+ *
+ * @param driver Java driver which initiates the connection
+ * @param options Driver options which initiates the connection
+ * @return True if the connection provider will need to modify the security
configuration when
+ * initiating a connection with the given driver with the given options.
+ */
+ def needsModifySecurityConfiguration(driver: Driver, options: Map[String,
String]): Boolean
Review comment:
Maybe we can call it `modifiesSecurityContext`.
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/BasicConnectionProvider.scala
##########
@@ -48,4 +48,12 @@ private[jdbc] class BasicConnectionProvider extends
JdbcConnectionProvider with
logDebug(s"JDBC connection initiated with URL: ${jdbcOptions.url} and
properties: $properties")
driver.connect(jdbcOptions.url, properties)
}
+
+ override def needsModifySecurityConfiguration(
+ driver: Driver,
+ options: Map[String, String]
+ ): Boolean = {
+ val jdbcOptions = new JDBCOptions(options)
+ jdbcOptions.keytab != null && jdbcOptions.principal != null
Review comment:
`BasicConnectionProvider` is the default unsecure connection provider so
return false is good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
