Add SqlServer MSI Support #591
Conversation
Add MSI Auth option to SQL Server connection. Default if no password is provided in connection string.
update host name parsing to get just the resource endpoint for msi
update go-mssqldb to resolve panic issue referenced here: denisenkom/go-mssqldb#642
switch from deprecated methods. NewServicePrincipalTokenFromManagedIdentity calls the two methods that are deprecated for us
add useMsi param instead of looking for nil password
Update sqlserver readme for msi auth. make useMsi a bit safer
remove comment
Co-authored-by: Keegan Campbell <[email protected]>
refactor resource uri logic into its own method
add tests for msi connection. can only test whether it fails with useMsi= true, or succeeds with useMsi=false
Add SqlServer MSI Support
check msi.EnsureFresh return value
Pull Request Test Coverage Report for Build 487
💛 - Coveralls |
database/sqlserver/sqlserver.go
Outdated
| } | ||
|
|
||
| connector, err := mssql.NewAccessTokenConnector( | ||
| migrate.FilterCustomQuery(purl).String(), tokenProvider) |
There was a problem hiding this comment.
Factor out migrate.FilterCustomQuery(purl).String() to prevent accidental divergence in the future.
e.g. filteredURL := migrate.FilterCustomQuery(purl).String()
| addr := msConnectionStringMsi(ip, port, true) | ||
| p := &SQLServer{} | ||
| _, err = p.Open(addr) | ||
| if err == nil { |
There was a problem hiding this comment.
Is there anyway to test the success case? e.g. are there mock providers available as docker images?
I'm happy to merge this in without the happy case tests but can't provide official support unless there are sufficient tests. e.g. w/o the happy case test, you'll need to mark this parameter as not officially supported in the README
There was a problem hiding this comment.
There doesn't seem to be a way to mock this through container images. The SQL Server container docs only go through password auth and don't mentioned MSI/AAD auth, which kinda makes sense since they're native to Azure, but would be nice if they provided mock images to test auth. We could mock MSI auth in code by returning an empty token, but that would only work if we were mocking the SQL Server instance as well and could control the auth on both sides. For now I will add a disclaimer in the README saying it's not officially supported.
Also, we have tested the happypath manually in our own scenarios and it does work.
| } | ||
|
|
||
| var db *sql.DB | ||
| if useMsi { |
There was a problem hiding this comment.
How would we handle the case when both useMsi and a password are specified?
There was a problem hiding this comment.
In this case I think it makes sense to default to MSI auth and could then also log a message to the console saying something along the lines of "Both MSI Auth and Password Auth were requested, defaulting to MSI Auth. Password will be ignored" (can update the README with this behavior as well). Any sort of fallback to password auth should probably be handled by the caller.
There was a problem hiding this comment.
Any objections to making these options mutually exclusive? e.g. error if both are specified?
There was a problem hiding this comment.
That sounds good to me, made the change.
move migrate.FilterCustomQuery(purl).String() into one line out of if/else. return error if both useMsi=true and password are passed
update readme with warning for useMsi
Update TestMsiFalse test case as now it should fail when useMsi=false and no password is provided
| ErrNoDatabaseName = fmt.Errorf("no database name") | ||
| ErrNoSchema = fmt.Errorf("no schema") | ||
| ErrDatabaseDirty = fmt.Errorf("database is dirty") | ||
| ErrMultipleAuthOptionsPassed = fmt.Errorf("both password and useMsi=true were passed.") |
There was a problem hiding this comment.
Thanks for making this a reference-able error!
Thank you as well! |
Add the option for MSI Auth to Azure SQL Server. Expects a param to be passed to enable MSI Auth, useMsi=true.
PR from denisenkom/go-mssqldb that added msi support: #546