add support for Azure SQL by adding Active Directory authentication methods #53
Conversation
| if self.windows_login is True: | ||
| self.authentication = "Windows Login" | ||
|
|
||
|
|
||
| return 'server', 'database', 'schema', 'port', 'UID', \ | ||
| 'authentication', 'encrypt' |
There was a problem hiding this comment.
So instead of displaying windows_login as True in dbt debug output , this changes would display authentication: Windows Login
We could have added windows auth as an Authentication value within the profiles config but
to have backward compatibility for Windows Auth with the config property : windows_login otherwise dbt debug window would display authentication=sql
I have tested out both the Windows auth and SQL auth part for an On Prem SQL server with this branch |
|
@mikaelene this PR is all that is needed to enable Azure SQL, which would pretty cool! Like @NandanHegde15 said, we can authenticate and found all the what do you think is a good roll-out strategy for this? Make a release cut and have people (besides us) test it before squashing to master? |
| con_str.append(f"PWD={{{credentials.PWD}}}") | ||
|
|
||
| if not getattr(credentials, 'encrypt', False): | ||
| con_str.append(f"Encrypt={credentials.encrypt}") | ||
|
|
||
| con_str_concat = ';'.join(con_str) | ||
| logger.debug(f'Using connection string: {con_str_concat}') |
There was a problem hiding this comment.
This code logger.debug(f'Using connection string: {con_str_concat}') displays the credentials in the dbt logs without any encryption. So we would prefer this line be commented out and can be used only during debugging purposes.
This fix can be made in a separate PR but wanted to highlight you this scenario.
Added a description of Azure SQL requiring tires greater than S2 for column store index to be created in README file
There was a problem hiding this comment.
This looks good @NandanHegde15 ! I'll merge it to master and see if I can try it out before pushing to pipi. Feel free to submit at PR for the credentials debug log as well so we solve that security hole.
rehash of #49 but to master branch now. @NandanHegde15 will add some more documentation and test this PR does not break the existing on-prem SQL Server connection methods