-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
SQL Server input plugin - Enable Azure Active Directory (AAD) authentication support #8822
Conversation
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.
🤝 ✅ CLA has been signed. Thank you!
I think we should update SQL Server plugin doc (readme) to indicate AAD support using MSI. Also indicate how an AAD login is created on SQL side using TSQL |
…ck for SQL VM, and others
Added content to readme file about its usage and instructions |
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.
Please check my comments in the code. @masree any comments?
Thank you for the review! Valid comments. |
… cleaned up comments for sql server plugin
…ck for SQL VM, and others
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.
I have some comments in the code. The most sever is the potential deadlock when an error occurs in getTokenProvider()
. Please take a look and fix the leaking lock.
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Thank you @srebhan for the review, I've addressed your comments. |
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.
Only some small (cosmetic) things left. Looks good so far.
Thank you @srebhan for quick review, I've addressed your comments. |
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.
Looks like new artifacts were built from this PR. Get them here!
Artifact URLs
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.
Looks good to me.
@avinash-nigam I was also wondering about how you tested this? There aren't any unit tests covering this new auth method, what are your thoughts on including tests? |
@sspaink - This has been manually tested for databases of following variants individually and a combination of multiple variants by deploying the bits on an Ubuntu VM and settings connection strings to use SqlAuth and AADAuth (both as supported cases and not supported cases) - Azure SQL Database - S1, S2 (AAD supported) Currently this plugin does not support integration testing, so there are no tests for authentication yet. Once we build a mechanism to write integration tests, then we would be able to add tests for the same. |
This change appears to break Windows Integrated authentication where no User ID or Password is given in the connection string. If I run telegraf version 1.19.0-rc0 with configuration that works with 1.18.3 I get the following error: This appears to come from the check for Password attribute being used to determine whether to user SQL Server authentication or AAD. |
@cerilewis thanks for reporting this problem! Can you please open an issue and link to this PR. I fear that we need to add an option to specify which auth-scheme to use as the magic here will likely not be able to detect it... :-( |
You already did by mentioning this PR in the issue. :-) Thank you for reporting. |
can anyone please share any example how to enable AAD? |
@arkapravasinha please do not post support questions to old PRs! Nobody will notice them besides me. ;-) Better use the forum or Slack! This being said, how about reading the docs and use |
Required for all PRs:
Associated to feature request - Azure Active Directory (AAD) authentication support in SQL Server input plugin