Skip to content
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.

Sign up for GitHub

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

Fix to (re)enable lower-privilege connection strings #234

Closed
wants to merge 4 commits into from
Closed

Fix to (re)enable lower-privilege connection strings #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fc69eb
Fix to (re)enable lower-privilege connection strings
cgillum Sep 13, 2024
5b1997d
Additional updates after testing
cgillum Oct 9, 2024
94413a9
Merge branch 'main' into cgillum/master-db-access
cgillum Oct 9, 2024
f437232
Update CHANGELOG
cgillum Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Updates

* Fix SQL retry logic to open a new connection if a previous failure closed the connection ([#221](https://github.com/microsoft/durabletask-mssql/pull/221)) - contributed by [@microrama](https://github.com/microrama)
* Fix to (re)enable lower-privilege connection strings ([#234](https://github.com/microsoft/durabletask-mssql/pull/234))

## v1.3.0

Expand Down
1 change: 1 addition & 0 deletions src/DurableTask.SqlServer/DTUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ public static bool HasPayload(HistoryEvent e)
return historyEvent.EventType switch
{
EventType.ExecutionStarted => ((ExecutionStartedEvent)historyEvent).Version,
EventType.SubOrchestrationInstanceCreated => ((SubOrchestrationInstanceCreatedEvent)historyEvent).Version,
cgillum marked this conversation as resolved.
Show resolved Hide resolved
EventType.TaskScheduled => ((TaskScheduledEvent)historyEvent).Version,
_ => null,
};
Expand Down
2 changes: 1 addition & 1 deletion src/DurableTask.SqlServer/Scripts/logic.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,7 +1444,7 @@ BEGIN
-- Instance IDs can be overwritten only if the orchestration is in a terminal state
IF @existingStatus NOT IN ('Failed')
BEGIN
DECLARE @msg nvarchar(4000) = FORMATMESSAGE('Cannot rewing instance with ID ''%s'' because it is not in a ''Failed'' state, but in ''%s'' state.', @InstanceID, @existingStatus);
DECLARE @msg nvarchar(4000) = FORMATMESSAGE('Cannot rewind instance with ID ''%s'' because it is not in a ''Failed'' state, but in ''%s'' state.', @InstanceID, @existingStatus);
cgillum marked this conversation as resolved.
Show resolved Hide resolved
THROW 50001, @msg, 1;
END

Expand Down
16 changes: 14 additions & 2 deletions src/DurableTask.SqlServer/SqlDbManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,22 @@ async Task<DatabaseLock> AcquireDatabaseLockAsync(bool createDatabaseIfNotExists

async Task EnsureDatabaseExistsAsync()
{
// 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;
}
Comment on lines -180 to +195
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!await this.DoesDatabaseExistAsync(this.settings.DatabaseName, connection))
{
Expand Down
Loading