gaborgsomogyi commented on a change in pull request #28635: URL: https://github.com/apache/spark/pull/28635#discussion_r429886742
########## File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.datasources.jdbc.connection + +import java.security.PrivilegedExceptionAction +import java.sql.{Connection, Driver} +import java.util.Properties + +import org.apache.hadoop.security.UserGroupInformation + +import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions + +private[sql] class MSSQLConnectionProvider( + driver: Driver, + options: JDBCOptions, + parserMethod: String = "parseAndMergeProperties" + ) extends SecureConnectionProvider(driver, options) { + override val appEntry: String = { + val configName = "jaasConfigurationName" + val appEntryDefault = "SQLJDBCDriver" + + val parseURL = try { Review comment: There are basically 2 approaches to parse the URL to get `jaasConfigurationName`: * Try to call private [parseAndMergeProperties](https://github.com/microsoft/mssql-jdbc/blob/0d4e97f401dc0e55779460d9709dd7ee399246be/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDriver.java#L831-L854) => tried it first * Parse the URL manually => used as fallback Both way has been tested in `MSSQLConnectionProviderSuite`. ########## File path: external/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MsSqlServerIntegrationSuite.scala ########## @@ -27,7 +27,7 @@ import org.apache.spark.tags.DockerTest @DockerTest 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" Review comment: This is not absolutely necessary, if we think we can extract it into a new PR. Thought it would be overkill. ########## File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.datasources.jdbc.connection + +import java.security.PrivilegedExceptionAction +import java.sql.{Connection, Driver} +import java.util.Properties + +import org.apache.hadoop.security.UserGroupInformation + +import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions + +private[sql] class MSSQLConnectionProvider( Review comment: The implementation is based on [this](https://docs.microsoft.com/en-us/sql/connect/jdbc/using-kerberos-integrated-authentication-to-connect-to-sql-server?view=sql-server-ver15). ########## File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProviderSuite.scala ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.datasources.jdbc.connection + +class MSSQLConnectionProviderSuite extends ConnectionProviderSuiteBase { + test("setAuthenticationConfigIfNeeded default parser must set authentication if not set") { + val driver = registerDriver(MSSQLConnectionProvider.driverClass) + val defaultProvider = new MSSQLConnectionProvider( + driver, options("jdbc:sqlserver://localhost/mssql")) + val customProvider = new MSSQLConnectionProvider( + driver, options(s"jdbc:sqlserver://localhost/mssql;jaasConfigurationName=custommssql")) + + testProviders(defaultProvider, customProvider) + } + + test("setAuthenticationConfigIfNeeded custom parser must set authentication if not set") { + val parserMethod = "IntentionallyNotExistingMethod" Review comment: 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. ########## File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.execution.datasources.jdbc.connection + +import java.security.PrivilegedExceptionAction +import java.sql.{Connection, Driver} +import java.util.Properties + +import org.apache.hadoop.security.UserGroupInformation + +import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions + +private[sql] class MSSQLConnectionProvider( + driver: Driver, + options: JDBCOptions, + parserMethod: String = "parseAndMergeProperties" + ) extends SecureConnectionProvider(driver, options) { + override val appEntry: String = { + val configName = "jaasConfigurationName" + val appEntryDefault = "SQLJDBCDriver" + + val parseURL = try { + val m = driver.getClass.getDeclaredMethod(parserMethod, classOf[String], classOf[Properties]) + m.setAccessible(true) + Some(m) + } catch { + case _: NoSuchMethodException => None + } + + parseURL match { + case Some(m) => + logDebug("Property parser method found, using it") + m.invoke(driver, options.url, null).asInstanceOf[Properties] + .getProperty(configName, appEntryDefault) + + case None => + logDebug("Property parser method not found, using custom parsing mechanism") + options.url.split(';').map(_.split('=')) + .find(kv => kv.length == 2 && kv(0) == configName) + .getOrElse(Array(configName, appEntryDefault))(1) + } + } + + override def getConnection(): Connection = { + setAuthenticationConfigIfNeeded() + UserGroupInformation.loginUserFromKeytabAndReturnUGI(options.principal, options.keytab).doAs( + new PrivilegedExceptionAction[Connection]() { + override def run(): Connection = { + MSSQLConnectionProvider.super.getConnection() + } + } + ) + } + + override def getAdditionalProperties(): Properties = { + val result = new Properties() + result.put("integratedSecurity", "true") + result.put("authenticationScheme", "JavaKerberos") Review comment: 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 ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
