-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
refactor: swap mssql driver to 'microsoft' driver #29694
Conversation
|
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 I allowed myself to push the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
There is a separate issues for this that is linked to this PR. |
Thanks! I only ran gotidy in the sqlqueryreceiver folder. my bad. 😄 |
@astencel-sumo @codeboten This patch is now rebased for v0.91.0 updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Please fix new conflicts, sorry. |
@atoulme All done. |
_ "github.com/go-sql-driver/mysql" | ||
_ "github.com/lib/pq" | ||
_ "github.com/microsoft/go-mssqldb" | ||
_ "github.com/microsoft/go-mssqldb/integratedauth/krb5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the second import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only this one? Other authentication mechanisms also have init()
func https://github.com/microsoft/go-mssqldb/tree/main/integratedauth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orkrb5
explicitely, withntlm
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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>
And one more time .. |
_ "github.com/go-sql-driver/mysql" | ||
_ "github.com/lib/pq" | ||
_ "github.com/microsoft/go-mssqldb" | ||
_ "github.com/microsoft/go-mssqldb/integratedauth/krb5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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