Upgrade sqlserver driver to version 12.8.1 #24686

lukmanulhakkeem · 2025-03-07T04:49:19Z

Description

NTLM authentication is not supported in the current driver. We will get below error if we specify NTLM in the connection URL
integratedSecurity=true;authenticationScheme=NTLM;

We need to upgrade the driver to get this support

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

SQL Server Connector Changes
* Upgrade sqlserver driver to version 12.8.1, to include NTLM authentication support. See :ref:`connector/sqlserver:authentication`.

imjalpreet · 2025-03-07T07:12:18Z

pom.xml

@@ -1385,7 +1385,7 @@
            <dependency>
                <groupId>com.microsoft.sqlserver</groupId>
                <artifactId>mssql-jdbc</artifactId>
-                <version>7.0.0.jre8</version>
+                <version>8.2.1.jre8</version>


Can we try to use the latest stable release 12.8.1.jre8?

We will have to upgrade it again to use the jre11+ dependency once the Java Upgrade PR is merged.

My bad, actually i meant for 12.8.1. I will change it

@imjalpreet when are we planning for the java upgrade?

We hope to be running mostly java 17 bytecode by end of H1/beginning of H2. For now, we will be on java 8

@lukmanulhakkeem can you please look into the CI failures?

@ZacBlanco So this driver needs to be updated with the java 17 upgrade right? How can we ensure that? Is there any parent ticket that track all the dependencies for the java 17 upgrade?

@lukmanulhakkeem you might find this helpful - https://github.com/orgs/prestodb/projects/31/views/1

lukmanulhakkeem · 2025-03-12T06:06:39Z

@imjalpreet ci failures are fixed. could you review it again?

pratyakshsharma

thank you for the fix, looks good.

steveburnett · 2025-03-12T13:16:39Z

Consider if we should add a mention in the SQL Server Connector doc that NTLM is supported. There isn't a mention in the doc of the current absence of NTLM as a limitation.

imjalpreet

@lukmanulhakkeem Thanks for the fix, in addition to what Steve suggested can we also update the documentation to highlight that the default value of encrypt is true after the driver upgrade?

lukmanulhakkeem · 2025-03-13T04:33:08Z

@steveburnett @imjalpreet I will update

lukmanulhakkeem · 2025-03-13T05:18:32Z

@lukmanulhakkeem Thanks for the fix, in addition to what Steve suggested can we also update the documentation to highlight that the default value of encrypt is true after the driver upgrade?

This default value is already updated in the documentation by mistake. So I can update only the NTLM auth support

lukmanulhakkeem · 2025-03-13T06:18:43Z

Microsoft supports multiple authentication mechanisms, including NTLM, and has provided documentation for each

https://learn.microsoft.com/en-us/sql/connect/jdbc/using-kerberos-integrated-authentication-to-connect-to-sql-server?view=sql-server-ver16

It would be difficult to include all of them in the Presto documentation. I don't think it’s ideal to document only NTLM authentication while leaving out other popular ones like Kerberos.

@imjalpreet @steveburnett Please suggest an approach here

steveburnett · 2025-03-13T13:41:36Z

If the current documentation describes the default value of encrypt as true, and that is how it will be after this PR is merged, then I think no doc change for this is needed.
I agree that it would be awkward to mention only NTLM and not the other authentications, and that it is outside the scope of this PR to ask you to add documention for how to use all the other auth mechanisms that are supported.

I am okay with not mentioning authentication in the documentation in the SQL Server page, but I would ask that you consider mentioning NTLM as the motivation for the upgrade in a release note. Maybe something like this:

SQL Server Connector Changes

* Upgrade sqlserver driver to version 12.8.1, to include NTLM authentication support.

This way, NTLM is at least in the Presto documentation as a searchable term for people to find if they search for it, and in the context of the SQL Server connector.

Maybe we could add a heading Authentication to the SQL Server page. In it list the authentication methods that we know to work. Something as short as this:

Authentication

The following authentication methods are supported:
* NTLM
* Kerberos
* (others)

If you feel comfortable with doing this third ask in this PR, please do. You don't have to be complete, just list the ones you know to work successfully. The doc can be iterated on and expanded later.

If you do not feel comfortable with doing this third ask in this PR, I can open a new issue about adding an Authentication topic to the SQL Server page and the issue can be addressed in a later PR. In that case I would welcome your input as a contributor and reviewer.

lukmanulhakkeem · 2025-03-13T16:52:09Z

@steveburnett I added authentication properties in brief. Could you please review

lukmanulhakkeem · 2025-03-13T17:00:01Z

Should I edit any files to add it to the release note?

steveburnett

Thanks for this! I think this is perfect.

I really appreciate you providing an example of a sample connection URL with the NTLM authentication.

Other authentication method examples can be added in future documentation PRs, and this new Authentication topic gives a place for that new doc to be added.

steveburnett · 2025-03-13T17:17:33Z

Should I edit any files to add it to the release note?

I would suggest that you link to the new documentation from the release note. I tested this link (linking to a page is easy, linking to a heading within a page is a little trickier) and confirmed that it works in a local doc build.

== RELEASE NOTES ==

SQL Server Connector Changes
* Upgrade sqlserver driver to version 12.8.1, to include NTLM authentication support. See :ref:`connector/sqlserver:authentication`.

imjalpreet · 2025-03-14T04:51:59Z

This default value is already updated in the documentation by mistake.

@lukmanulhakkeem I believe we should add a note stating that the default value of encrypt changed from false to true starting in Presto 0.292. Some users may not explicitly set encrypt to false, assuming the default was still false, just as we didn’t set it to false in our Product Tests.

lukmanulhakkeem · 2025-03-14T05:05:20Z

@imjalpreet Can i add the below note?

Starting from release 0.292, the default value of encrypt has changed from false to true.

imjalpreet · 2025-03-14T05:27:57Z

@lukmanulhakkeem Yes, looks good. @steveburnett can also review it once added.

You can use a note similar to the one used in this overview section: https://prestodb.github.io/docs/0.291/connector/druid.html.

lukmanulhakkeem · 2025-03-14T05:56:05Z

Updated the doc.

imjalpreet

@lukmanulhakkeem just a small nit, otherwise LGTM, can you please squash your commits too? Thanks.

imjalpreet · 2025-03-14T06:00:33Z

presto-docs/src/main/sphinx/connector/sqlserver.rst

@@ -62,6 +62,46 @@ A connection string using a truststore would be similar to the following example

    connection-url=jdbc:sqlserver://<host>:<port>;databaseName=<databaseName>;encrypt=true;trustServerCertificate=false;trustStoreType=PEM;hostNameInCertificate=hostname;trustStore=path/to/truststore.pem;trustStorePassword=password

+.. note::


nit: let's move this to above the current documentation of the encrypt property.

Above this line:

presto/presto-docs/src/main/sphinx/connector/sqlserver.rst

Line 32 in 7fb09c9

To disable encryption in the connection string, use the ``encrypt`` property:

Done

Could you check this move of text? I'm not seeing it in the current branch.

@steveburnett You can check this - https://github.com/prestodb/presto/compare/7fb09c9fec2312f1252cd98f7bef4ed1ca83d793..2354bc0c1a63a14e94fb3bdd83a7f1255024c246

lukmanulhakkeem · 2025-03-14T06:31:25Z

Extra space added with the commit. I am fixing

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good.

lukmanulhakkeem · 2025-03-17T15:32:25Z

Thanks @steveburnett

lukmanulhakkeem requested a review from a team as a code owner March 7, 2025 04:49

lukmanulhakkeem requested a review from jaystarshot March 7, 2025 04:49

imjalpreet reviewed Mar 7, 2025

View reviewed changes

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch 2 times, most recently from 15c74ab to 22c1bbe Compare March 10, 2025 09:28

lukmanulhakkeem changed the title ~~Upgrade sqlserver driver to version 8.2.1~~ Upgrade sqlserver driver to version 12.8.1 Mar 10, 2025

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch 2 times, most recently from 442f6e1 to 19aa45e Compare March 12, 2025 04:58

lukmanulhakkeem requested a review from imjalpreet March 12, 2025 06:04

pratyakshsharma previously approved these changes Mar 12, 2025

View reviewed changes

imjalpreet requested changes Mar 13, 2025

View reviewed changes

lukmanulhakkeem dismissed pratyakshsharma’s stale review via 7105ccd March 13, 2025 16:49

lukmanulhakkeem requested review from steveburnett and elharo as code owners March 13, 2025 16:49

steveburnett previously approved these changes Mar 13, 2025

View reviewed changes

lukmanulhakkeem requested a review from imjalpreet March 13, 2025 17:09

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch 2 times, most recently from b194ab3 to d971bd8 Compare March 14, 2025 04:25

lukmanulhakkeem dismissed steveburnett’s stale review via 7fb09c9 March 14, 2025 05:36

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch from d971bd8 to 7fb09c9 Compare March 14, 2025 05:36

imjalpreet reviewed Mar 14, 2025

View reviewed changes

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch from 7fb09c9 to 2354bc0 Compare March 14, 2025 06:23

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch from 2354bc0 to 797ebff Compare March 14, 2025 09:33

Upgrade sqlserver driver to version 12.8.1

2b835a0

lukmanulhakkeem force-pushed the sqlserver-driver-upgrade branch from 797ebff to 2b835a0 Compare March 15, 2025 05:59

lukmanulhakkeem requested a review from imjalpreet March 15, 2025 06:02

steveburnett approved these changes Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade sqlserver driver to version 12.8.1 #24686

Upgrade sqlserver driver to version 12.8.1 #24686

lukmanulhakkeem commented Mar 7, 2025 •

edited

Loading

imjalpreet Mar 7, 2025

lukmanulhakkeem Mar 7, 2025

lukmanulhakkeem Mar 7, 2025

ZacBlanco Mar 7, 2025

imjalpreet Mar 8, 2025

lukmanulhakkeem Mar 10, 2025

pratyakshsharma Mar 12, 2025

lukmanulhakkeem commented Mar 12, 2025

pratyakshsharma left a comment

steveburnett commented Mar 12, 2025

imjalpreet left a comment

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

steveburnett commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

steveburnett left a comment

steveburnett commented Mar 13, 2025

imjalpreet commented Mar 14, 2025

lukmanulhakkeem commented Mar 14, 2025

imjalpreet commented Mar 14, 2025

lukmanulhakkeem commented Mar 14, 2025

imjalpreet left a comment

imjalpreet Mar 14, 2025

imjalpreet Mar 14, 2025

lukmanulhakkeem Mar 14, 2025

lukmanulhakkeem Mar 14, 2025

steveburnett Mar 17, 2025

lukmanulhakkeem Mar 17, 2025

lukmanulhakkeem commented Mar 14, 2025

steveburnett left a comment

lukmanulhakkeem commented Mar 17, 2025

		@@ -62,6 +62,46 @@ A connection string using a truststore would be similar to the following example

		connection-url=jdbc:sqlserver://<host>:<port>;databaseName=<databaseName>;encrypt=true;trustServerCertificate=false;trustStoreType=PEM;hostNameInCertificate=hostname;trustStore=path/to/truststore.pem;trustStorePassword=password

		.. note::

Upgrade sqlserver driver to version 12.8.1 #24686

Are you sure you want to change the base?

Upgrade sqlserver driver to version 12.8.1 #24686

Conversation

lukmanulhakkeem commented Mar 7, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukmanulhakkeem commented Mar 12, 2025

pratyakshsharma left a comment

Choose a reason for hiding this comment

steveburnett commented Mar 12, 2025

imjalpreet left a comment

Choose a reason for hiding this comment

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

steveburnett commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

lukmanulhakkeem commented Mar 13, 2025

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented Mar 13, 2025

imjalpreet commented Mar 14, 2025

lukmanulhakkeem commented Mar 14, 2025

imjalpreet commented Mar 14, 2025

lukmanulhakkeem commented Mar 14, 2025

imjalpreet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukmanulhakkeem commented Mar 14, 2025

steveburnett left a comment

Choose a reason for hiding this comment

lukmanulhakkeem commented Mar 17, 2025

lukmanulhakkeem commented Mar 7, 2025 •

edited

Loading