refactor: swap mssql driver to 'microsoft' driver #29694

cwegener · 2023-12-07T20:43:08Z

Description:

The old driver ('denisenkom') is only receiving limited attention. The
new driver ('microsoft') is maintained by the vendor of the actual RDBMS
now. And is also the driver listed on golang.org

Link to tracking Issue:

#27200

Testing:

No integration tests exist for MSSQL, only for Oracle, Postgres and MySQL

TODO: add MSSQL integration tests.

Documentation:

Revelant documentation is available upstream:
https://github.com/microsoft/go-mssqldb/blob/main/README.md

linux-foundation-easycla · 2023-12-07T20:43:13Z

The committers listed above are authorized under a signed CLA.

✅ login: cwegener (6679d91)

andrzej-stencel · 2023-12-08T09:32:01Z

It's shame we don't have integration tests for MS SQL Server, they would be ideal here to verify that the feature still works. 😅
@cwegener did you run any manual tests?

andrzej-stencel · 2023-12-08T11:43:30Z

@cwegener I allowed myself to push the make gotidy commit to your branch, hope you don't mind 🙏

codeboten

Thanks for the PR!

TODO: add MSSQL integration tests.

Is the TODO something you want to add to this PR or separately? If it's separate, please file an issue

cwegener · 2023-12-08T20:16:10Z

It's shame we don't have integration tests for MS SQL Server, they would be ideal here to verify that the feature still works. 😅 @cwegener did you run any manual tests?

This patch has been used in production for a few months at 3 different customers.

EDIT: Furthermore, the connection strings from the legacy driver are compatible with the fork. And this change is not a breaking change.

cwegener · 2023-12-08T20:17:10Z

Thanks for the PR!

TODO: add MSSQL integration tests.

Is the TODO something you want to add to this PR or separately? If it's separate, please file an issue

There is a separate issues for this that is linked to this PR.

#29695

cwegener · 2023-12-08T20:23:33Z

@cwegener I allowed myself to push the make gotidy commit to your branch, hope you don't mind 🙏

Thanks! I only ran gotidy in the sqlqueryreceiver folder. my bad. 😄

cwegener · 2023-12-13T05:48:55Z

@astencel-sumo @codeboten This patch is now rebased for v0.91.0 updates

andrzej-stencel

Thank you!

atoulme · 2023-12-14T17:07:22Z

Please fix new conflicts, sorry.

cwegener · 2023-12-14T20:47:19Z

@atoulme All done.

dmitryax · 2023-12-14T23:37:08Z

receiver/sqlqueryreceiver/db_client.go

 	_ "github.com/go-sql-driver/mysql"
 	_ "github.com/lib/pq"
+	_ "github.com/microsoft/go-mssqldb"
+	_ "github.com/microsoft/go-mssqldb/integratedauth/krb5"


Why do we need the second import?

This is what the change is - Swapping the old 'denisenkom' driver for the new microsoft driver.

With regards to the github.com/microsoft/go-mssqldb/integratedauth/krb5 module, this is required to be able to use kerberos authentication mechanism.

Why only this one? Other authentication mechanisms also have init() func https://github.com/microsoft/go-mssqldb/tree/main/integratedauth

Good point.

The reason that krb5 is added is because that is actually the auth mechanism that I have used so far myself.

We may actually need the other additional ones as well!

I will investigate and come back shortly.

This is the source:
https://github.com/microsoft/go-mssqldb/blob/c902b674869e3f177b723cf2a5b1364c14efda47/README.md?plain=1#L93-L125

Basically, the summary is:

The driver has support to perform three different "integrated auth" mechanism, depending on which platform the driver is executing on:

https://github.com/microsoft/go-mssqldb/blob/main/README.md?plain=1#L129

On Windows, the default auth mechanism is something called SSPI, which is only available when building the driver for Windows https://github.com/microsoft/go-mssqldb/tree/main/integratedauth/winsspi (which basically automatically will chose either kerberos or ntlm)

On Linux, the two options are either ntlm or krb5 explicitely, with ntlm being the default in the driver

I do not know why the krb5 package was designed in such way that it needs an explicit import.

There is some dicsussion on the original thread when the feature was implemented, but I can't quickly glean why this kerberos features was designed in this particular way.

microsoft/go-mssqldb#35

Why only this one? Other authentication mechanisms also have init()

In summary. The Kerberos on Non-Windows integratedauth package is special compared to the winsspi and ntlm packages in that it is required to be imported explicitly, but I have no understanding of why the krb5 package was designed in this way.

Ok. Thanks. I see that other mechanisms are imported already https://github.com/microsoft/go-mssqldb/blob/main/auth_windows.go but krb5 is not

github-actions · 2023-12-30T05:19:11Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

cwegener · 2024-01-02T01:50:31Z

Rebased and force pushed again.

**Description:** The old driver ('denisenkom') is only receiving limited attention. The new driver ('microsoft') is maintained by the vendor of the actual RDBMS now. And is also the driver listed on golang.org **Link to tracking Issue:** open-telemetry#27200 **Testing:** No integration tests exist for MSSQL, only for Oracle, Postgres and MySQL TODO: add MSSQL integration tests. **Documentation:** Revelant documentation is available upstream: https://github.com/microsoft/go-mssqldb/blob/main/README.md make gotidy Signed-off-by: Christoph Wegener <cwegener@users.noreply.github.com>

cwegener · 2024-01-03T21:16:46Z

Rebased and force pushed again.

And one more time ..

dmitryax · 2024-01-03T22:58:30Z

receiver/sqlqueryreceiver/db_client.go

 	_ "github.com/go-sql-driver/mysql"
 	_ "github.com/lib/pq"
+	_ "github.com/microsoft/go-mssqldb"
+	_ "github.com/microsoft/go-mssqldb/integratedauth/krb5"


Ok. Thanks. I see that other mechanisms are imported already https://github.com/microsoft/go-mssqldb/blob/main/auth_windows.go but krb5 is not

…emetry#29694) **Description:** The old driver ('denisenkom') is only receiving limited attention. The new driver ('microsoft') is maintained by the vendor of the actual RDBMS now. And is also the driver listed on golang.org **Link to tracking Issue:** open-telemetry#27200 **Documentation:** Revelant documentation is available upstream: https://github.com/microsoft/go-mssqldb/blob/main/README.md Signed-off-by: Christoph Wegener <cwegener@users.noreply.github.com>

cwegener requested a review from dmitryax as a code owner December 7, 2023 20:43

cwegener requested a review from a team December 7, 2023 20:43

github-actions bot assigned jpkrohling Dec 7, 2023

github-actions bot added the receiver/sqlquery SQL query receiver label Dec 7, 2023

cwegener mentioned this pull request Dec 7, 2023

sqlqueryreceiver lacks integration tests for mssql #29695

Open

cwegener requested a review from mx-psi as a code owner December 8, 2023 10:23

github-actions bot added cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command labels Dec 8, 2023

codeboten reviewed Dec 8, 2023

View reviewed changes

cwegener requested a review from codeboten December 12, 2023 03:55

cwegener force-pushed the main branch from ace649a to 53eed2f Compare December 13, 2023 05:47

andrzej-stencel approved these changes Dec 13, 2023

View reviewed changes

cwegener force-pushed the main branch from 7a482ba to 2d38750 Compare December 14, 2023 20:47

dmitryax reviewed Dec 14, 2023

View reviewed changes

cwegener requested a review from dmitryax December 16, 2023 03:48

github-actions bot added the Stale label Dec 30, 2023

cwegener force-pushed the main branch from 2d38750 to 4834dc6 Compare January 2, 2024 01:49

github-actions bot removed the Stale label Jan 2, 2024

cwegener force-pushed the main branch from 4834dc6 to 6679d91 Compare January 3, 2024 21:11

dmitryax approved these changes Jan 3, 2024

View reviewed changes

dmitryax merged commit 0c27a3b into open-telemetry:main Jan 3, 2024
85 checks passed

github-actions bot added this to the next release milestone Jan 3, 2024

andrzej-stencel mentioned this pull request Jan 23, 2024

swap sqlserver drivers: denisenkom/go-mssqldb -> microsoft/go-mssqldb #27200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: swap mssql driver to 'microsoft' driver #29694

refactor: swap mssql driver to 'microsoft' driver #29694

cwegener commented Dec 7, 2023

linux-foundation-easycla bot commented Dec 7, 2023 •

edited

Loading

andrzej-stencel commented Dec 8, 2023

andrzej-stencel commented Dec 8, 2023

codeboten left a comment

cwegener commented Dec 8, 2023 •

edited

Loading

cwegener commented Dec 8, 2023

cwegener commented Dec 8, 2023

cwegener commented Dec 13, 2023

andrzej-stencel left a comment

atoulme commented Dec 14, 2023

cwegener commented Dec 14, 2023

dmitryax Dec 14, 2023 •

edited

Loading

cwegener Dec 15, 2023

dmitryax Dec 15, 2023

cwegener Dec 15, 2023

cwegener Dec 15, 2023

cwegener Dec 15, 2023

dmitryax Jan 3, 2024

github-actions bot commented Dec 30, 2023

cwegener commented Jan 2, 2024

cwegener commented Jan 3, 2024

dmitryax Jan 3, 2024

refactor: swap mssql driver to 'microsoft' driver #29694

refactor: swap mssql driver to 'microsoft' driver #29694

Conversation

cwegener commented Dec 7, 2023

linux-foundation-easycla bot commented Dec 7, 2023 • edited Loading

andrzej-stencel commented Dec 8, 2023

andrzej-stencel commented Dec 8, 2023

codeboten left a comment

Choose a reason for hiding this comment

cwegener commented Dec 8, 2023 • edited Loading

cwegener commented Dec 8, 2023

cwegener commented Dec 8, 2023

cwegener commented Dec 13, 2023

andrzej-stencel left a comment

Choose a reason for hiding this comment

atoulme commented Dec 14, 2023

cwegener commented Dec 14, 2023

dmitryax Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

cwegener Dec 15, 2023

Choose a reason for hiding this comment

dmitryax Dec 15, 2023

Choose a reason for hiding this comment

cwegener Dec 15, 2023

Choose a reason for hiding this comment

cwegener Dec 15, 2023

Choose a reason for hiding this comment

cwegener Dec 15, 2023

Choose a reason for hiding this comment

dmitryax Jan 3, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 30, 2023

cwegener commented Jan 2, 2024

cwegener commented Jan 3, 2024

dmitryax Jan 3, 2024

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Dec 7, 2023 •

edited

Loading

cwegener commented Dec 8, 2023 •

edited

Loading

dmitryax Dec 14, 2023 •

edited

Loading