Fix to (re)enable lower-privilege connection strings #234
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cgillum
commented
Oct 9, 2024
Comment on lines
-180
to
+195
| // Note that we may not be able to connect to the DB, let alone obtain the lock, | ||
| // Note that we may not be able to connect to the target DB, let alone obtain the lock, | ||
| // if the database does not exist yet. So we obtain a connection to the 'master' database for now. | ||
| // If the current connection doesn't have permission to access the master database, this will fail. | ||
| // The exact error we see is hard to predict, so we catch SqlException and log a generic message. | ||
| using SqlConnection connection = this.settings.CreateConnection("master"); | ||
| await connection.OpenAsync(); | ||
| try | ||
| { | ||
| await connection.OpenAsync(); | ||
| } | ||
| catch (SqlException) | ||
| { | ||
| this.traceHelper.GenericInfoEvent( | ||
| "Failed to connect to the master database. Skipping database exists check.", | ||
| instanceId: null); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Skipping the check somewhat reduces the effectiveness of the check. I wonder if there's a bigger refactor here that would allow us to ensure the DB exists without trying to connect to the master DB, which we know may not be allowed by some companies.
If you get my point and think this may exist, but is costly, I'd appreciate if you could add a TODO: comment for future maintenance. But this seems good for now.
You said this probably used to work before - curious if you know what the code used to look like for this. I want to understand if this is essentially reverting back to the old code.
|
After some discussion, it was decided that this fix isn't needed after all. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #233
The fix is to skip the database existence check if the current connection string does not have permission to log into the master database.