Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linux/arm64 support, mssql17 -> 18 #1034

Draft
wants to merge 3 commits into
base: 2.0
Choose a base branch
from
Draft

linux/arm64 support, mssql17 -> 18 #1034

wants to merge 3 commits into from

Conversation

k-wolski
Copy link

@k-wolski k-wolski commented Sep 17, 2024

Summary

linux/arm64 support added
msodbcsql17 -> 18, as arm64 packages are available only with v18

Importance

multiplatform support is useful for people using different architecture than amd64

Checklist

This PR:

  • follows the guidelines laid out in CONTRIBUTING.md
  • links relevant issue(s)
  • adds/updates tests (if appropriate)
  • adds/updates docstrings (if appropriate)
  • adds an entry in CHANGELOG.md

@k-wolski k-wolski requested a review from trymzet September 17, 2024 11:40
@k-wolski k-wolski self-assigned this Sep 17, 2024
@k-wolski
Copy link
Author

There are issues on lapp with such changes:

pyodbc.OperationalError: ('08001', '[08001] [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:self signed certificate] (-1) (SQLDriverConnect)')

I will try to figure out proper certificate for it later.

@k-wolski k-wolski marked this pull request as draft September 30, 2024 12:34
@k-wolski
Copy link
Author

k-wolski commented Oct 4, 2024

In ODBC Driver for SQL Server 17, by default there were no certificate checked. They changed it and in 18 it's enabled, that's why there is such error on Lapp:

pyodbc.OperationalError: ('08001', '[08001] [Microsoft][ODBC Driver 18 for SQL Server]SSL Provider: [error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed:self signed certificate] (-1) (SQLDriverConnect)')

It should be possible to set these parameters in pyodbc and then it should work again, but it needs to be tested by someone, eg. @angelika233 before confirming:

    'Encrypt=Yes;'
    'TrustServerCertificate=Yes;'

@trymzet
Copy link
Contributor

trymzet commented Oct 4, 2024

Yeah, we should allow passing extra connection args in SQL https://github.com/dyvenia/viadot/blob/2.0/src/viadot/sources/base.py#L195-L215

Sth like (this is probably buggy but you get the idea 😛) Also, these names are probably case-insensitive so this could be simplified to not bother with capitalization.

@property
def conn_str(self) -> str:
    """Generate a connection string from params or config.

    Note that the user and password are escaped with '{}' characters.

    Returns:
        str: The ODBC connection string.
    """

    def params_dict_to_connection_string(params):
        return ';'.join([f"{key}={value}" for key, value in params.items()]) + ';'

    credentials = copy.deepcopy(self.credentials)

    driver = credentials.pop("driver")
    server = credentials.pop("server")
    db_name = credentials.pop("db_name")
    uid = credentials.pop("user", "")
    pwd = credentials.pop("password", "")

    conn_str = f"DRIVER={{{driver}}};SERVER={server};DATABASE={db_name};UID={uid};PWD={pwd};"

    # Should probably also be handled with the extra_args logic.
    if auth := credentials.pop("authentication", ""):
        conn_str += "Authentication=" + auth + ";"

    extra_args = params_dict_to_connection_string(credentials)

    return conn_str + extra_args

@angelika233
Copy link
Contributor

angelika233 commented Oct 8, 2024

I tested it by changing the SQL source in the following way:

if "trust_server_certificate" in self.credentials:
           conn_str += (
               "TrustServerCertificate="
               + self.credentials["trust_server_certificate"]
               + ";"
           )
if "encrypt" in self.credentials:
           conn_str += "Encrypt=" + self.credentials["encrypt"] + ";"

I tried different combinations of these params but it didn't help with the error. The error message stays the same

@trymzet
Copy link
Contributor

trymzet commented Oct 8, 2024

@angelika233 self.credentials["encrypt"] and self.credentials["trust_server_certificate"] were set to yes, right (I think it's case-insensitive, but it has to be the word "yes", not "true" etc.)? And you verified the final connection string included them? 😅

@trymzet
Copy link
Contributor

trymzet commented Oct 8, 2024

Might be this:

The security strength of SHA1 and MD5 based signatures in TLS has been reduced.

This results in SSL 3, TLS 1.0, TLS 1.1 and DTLS 1.0 no longer working at the default security level of 1 and instead requires security level 0.

Source: https://docs.openssl.org/3.0/man7/migration_guide/#tls-changes, through https://stackoverflow.com/a/77814036/8524524

Here's also a supposedly fully working example: https://techcommunity.microsoft.com/t5/sql-server-blog/odbc-driver-18-0-for-sql-server-released/ba-p/3169228 (comment by user fetzere)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants