durable task scheduler auth extension support #362

YunchuWang · 2025-01-09T02:31:59Z

This pull request introduces several significant changes, primarily focusing on the setup of new projects and the integration of Azure Managed extensions for the Durable Task Framework client. The most important changes include the addition of new projects to the solution, updates to the build configuration, and the implementation of new client extensions and options for Azure Managed services.

New Projects and Build Configuration:

Added new projects Worker.AzureManaged, Client.AzureManaged, Shared.AzureManaged.Tests, Client.AzureManaged.Tests, and Worker.AzureManaged.Tests to the solution file Microsoft.DurableTask.sln.
Updated the build configuration in Microsoft.DurableTask.sln to include the newly added projects. [1] [2]
Set up .NET 6.0 in the GitHub Actions workflow file .github/workflows/validate-build.yml.

Azure Managed Client Implementation:

Created the Client.AzureManaged.csproj project file with necessary package references and project dependencies.
Implemented DurableTaskSchedulerClientExtensions to provide extension methods for configuring Durable Task clients to use the Azure Durable Task Scheduler service.
Defined DurableTaskSchedulerClientOptions to configure the Durable Task Scheduler, including methods for creating gRPC channels and handling connection strings.
Added AccessTokenCache to manage and refresh Azure access tokens.
Implemented DurableTaskSchedulerConnectionString to parse and handle connection strings for the Durable Task Scheduler service.

save initial

cgillum

Looking great! I added some comments.

src/Extensions/Azure/DurableTaskSchedulerConnectionString.cs

src/Extensions/Azure/DurableTaskSchedulerOptions.cs

src/Extensions/Azure/DurableTaskSchedulerExtensions.cs

cgillum · 2025-01-09T03:35:41Z

@jviau would you mind taking a look at this as well?

The main feedback I'm interested in from you is the package name/organization. The idea behind this PR is to introduce a mechanism for configuring the gRPC channels to work with the Durable Task Scheduler (DTS). This includes setting up the appropriate auth and some other service-specific headers (like the task hub name). I figured this might be the right time to introduce an "Extensions" concept and make "Azure" the extension consumers should use for talking to DTS. There could be others in the future. WDYT?

This reverts commit 131c575.

src/Extensions/Azure/DurableTaskSchedulerExtensions.cs

Microsoft.DurableTask.sln

src/Extensions/Azure/Azure.csproj

src/Extensions/Azure/DurableTaskSchedulerOptions.cs

src/Extensions/Azure/DurableTaskSchedulerExtensions.cs

src/Extensions/Azure/DurableTaskSchedulerOptions.cs

jviau · 2025-01-10T15:59:34Z

I figured this might be the right time to introduce an "Extensions" concept and make "Azure" the extension consumers should use for talking to DTS. There could be others in the future. WDYT?

I wonder if we want a name other that Extensions for backends? I am worried this may get confused with a backend vs helpers for Azure / a specific Azure service. Say we wanted to port an AzureStorage backend hook to durable - where would that go? Or if we wanted to introduce helpers (not backends) for interacting with Azure services (Compute, Storage, CosmosDB, etc). Where would those go?

There is the Worker and Client naming for this already. Worker's are backends, Clients are for interacting with a backend. Would separate Worker.AzureManaged and Client.AzureManaged (or just "Azure") be better?

This also brings up the point of keeping a separate Worker and Client package. While it is more setup work on our end, the benefit is customers can bring in just the clients for their API projects and not need the entire worker.

sample fix

This reverts commit 14b94eb.

YunchuWang · 2025-01-11T21:49:49Z

I figured this might be the right time to introduce an "Extensions" concept and make "Azure" the extension consumers should use for talking to DTS. There could be others in the future. WDYT?

I wonder if we want a name other that Extensions for backends? I am worried this may get confused with a backend vs helpers for Azure / a specific Azure service. Say we wanted to port an AzureStorage backend hook to durable - where would that go? Or if we wanted to introduce helpers (not backends) for interacting with Azure services (Compute, Storage, CosmosDB, etc). Where would those go?

There is the Worker and Client naming for this already. Worker's are backends, Clients are for interacting with a backend. Would separate Worker.AzureManaged and Client.AzureManaged (or just "Azure") be better?

This also brings up the point of keeping a separate Worker and Client package. While it is more setup work on our end, the benefit is customers can bring in just the clients for their API projects and not need the entire worker.

any advice on jacob's comment @cgillum ?

YunchuWang · 2025-01-11T21:50:38Z

@jviau thanks for all the comments! i updated the pr and its ready for another review, added more tests and tested it manually with sample and working

cgillum · 2025-01-12T01:07:47Z

There is the Worker and Client naming for this already. Worker's are backends, Clients are for interacting with a backend. Would separate Worker.AzureManaged and Client.AzureManaged (or just "Azure") be better?

This also brings up the point of keeping a separate Worker and Client package. While it is more setup work on our end, the benefit is customers can bring in just the clients for their API projects and not need the entire worker.

@jviau this makes sense to me. How about this - there will be two new nuget packages:

Microsoft.DurableTask.Client.AzureManaged
Microsoft.DurableTask.Worker.AzureManaged

I prefer "AzureManaged" over "Azure" because "Azure" becomes ambiguous if we ever wanted to introduce "AzureStorage" or something similar as a backend option for this SDK in the future.

@YunchuWang in terms of project structure, how about this?

/src/Client/AzureManaged/Client.AzureManaged.csproj
/src/Worker/AzureManaged/Worker.AzureManaged.csproj
/src/Shared/AzureManaged/*.cs

The /src/Shared/AzureManaged directory would just be shared .cs files that get imported into both projects, similar to /src/Shared/Grpc.

YunchuWang · 2025-01-12T22:04:50Z

split into separate packages and split tests, also add @cgillum 's sample to here using split packages and tested working
, thanks, can you review again?

cgillum · 2025-01-13T00:30:11Z

src/Shared/AzureManaged/Shared.AzureManaged.csproj

@@ -0,0 +1,25 @@
+<Project Sdk="Microsoft.NET.Sdk">


There should not be a shared project. Rather, we should important the code directly into the both the client and worker projects. The other directories under /src/Shared as examples.

The reason for this is that we don't want to have to publish and maintain a "Shared" nuget package.

Correct - move these files to src/Shared/AzureManaged, then include them via:

<SharedSection Include="AzureManaged" />

There are some public types in the AzureManaged shared files right now, which we want to avoid. Options are:

Introduce another package Microsoft.DurableTask.Abstractions.AzureManaged to contain the common files.

this makes 3 packages for 1 feature. Once set up it isn't that hard to maintain, but I would understand if there is pushback on it being cumbersome to need 3 packages per backend.

Consolidate all the packages into 1, Microsoft.DurableTask.Backend.AzureManaged or Microsoft.DurableTask.AzureManaged

i like number 2, 3 packages seem a bit cumbersome, basically we do a rename and go back to original approach before/ @cgillum for advice

as discussed with Chris offline, he suggested to keep two packages and use differently named option types to separate shared ones

samples/AspNetWebApp/AspNetWebApp.csproj

samples/AspNetWebApp/Program.cs

src/Shared/AzureManaged/DurableTaskSchedulerOptions.cs

src/Worker/AzureManaged/DurableTaskSchedulerWorkerExtensions.cs

samples/AspNetWebApp/Utils.cs

samples/AspNetWebApp/appsettings.Production.json

jviau · 2025-01-13T19:24:52Z

src/Shared/AzureManaged/Shared.AzureManaged.csproj

@@ -0,0 +1,25 @@
+<Project Sdk="Microsoft.NET.Sdk">


There are some public types in the AzureManaged shared files right now, which we want to avoid. Options are:

Introduce another package Microsoft.DurableTask.Abstractions.AzureManaged to contain the common files.

this makes 3 packages for 1 feature. Once set up it isn't that hard to maintain, but I would understand if there is pushback on it being cumbersome to need 3 packages per backend.

Consolidate all the packages into 1, Microsoft.DurableTask.Backend.AzureManaged or Microsoft.DurableTask.AzureManaged

src/Shared/AzureManaged/Shared.AzureManaged.csproj

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientExtensionsTests.cs

Co-authored-by: Jacob Viau <javia@microsoft.com>

YunchuWang · 2025-01-14T05:02:57Z

thanks, updated to two packages. can i get another review @jviau @cgillum？

cgillum

Just a few minor things.

cgillum · 2025-01-15T00:12:27Z

.github/workflows/validate-build.yml

+    - name: Setup .NET 6.0
+      uses: actions/setup-dotnet@v3
+      with:
+        dotnet-version: '6.0.x'


Why is this step needed? I would expect that the step below would take care of everything?

cgillum · 2025-01-15T00:17:25Z

src/Client/AzureManaged/DurableTaskSchedulerClientExtensions.cs

+    /// using the provided Durable Task Scheduler options.
+    /// </summary>
+    /// <param name="schedulerOptions">Monitor for accessing the current scheduler options configuration.</param>
+    internal class ConfigureGrpcChannel(IOptionsMonitor<DurableTaskSchedulerClientOptions> schedulerOptions) :


Should this be a private class?

Suggested change

internal class ConfigureGrpcChannel(IOptionsMonitor<DurableTaskSchedulerClientOptions> schedulerOptions) :

class ConfigureGrpcChannel(IOptionsMonitor<DurableTaskSchedulerClientOptions> schedulerOptions) :

cgillum · 2025-01-15T00:18:53Z

src/Client/AzureManaged/DurableTaskSchedulerClientOptions.cs

+    /// Gets or sets the worker ID used to identify the worker instance.
+    /// The default value is a string containing the machine name, process ID, and a unique identifier.
+    /// </summary>
+    public string WorkerId { get; set; } = $"{Environment.MachineName},{Environment.ProcessId},{Guid.NewGuid():N}";


Client options don't need a worker ID. We only need this for Worker options.

cgillum · 2025-01-15T00:19:13Z

src/Client/AzureManaged/DurableTaskSchedulerClientOptions.cs

+
+    /// <summary>
+    /// Gets or sets a value indicating whether to allow insecure channel credentials.
+    /// This should only be set to true in development/testing scenarios.


Suggested change

/// This should only be set to true in development/testing scenarios.

/// This should only be set to true in local development/testing scenarios.

cgillum · 2025-01-15T00:22:00Z

src/Client/AzureManaged/DurableTaskSchedulerClientOptions.cs

+        string authType = connectionString.Authentication;
+
+        // Parse the supported auth types, in a case-insensitive way and without spaces
+        switch (authType.ToLower(CultureInfo.InvariantCulture).Replace(" ", string.Empty))


Let's not allow spaces. I'd like to reduce the test matrix that we have to worry about for all language SDKs. Also, I think it's more natural to use .ToLowerInvariant() instead of .ToLower(CultureInfo.Invariant)

Suggested change

switch (authType.ToLower(CultureInfo.InvariantCulture).Replace(" ", string.Empty))

switch (authType.ToLowerInvariant)

cgillum · 2025-01-15T00:55:43Z

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientExtensionsTests.cs

+
+public class DurableTaskSchedulerClientExtensionsTests
+{
+    private const string ValidEndpoint = "myaccount.westus3.durabletask.io";


nit: please update the code in these files to match the C# style used by this repo. Your editor should be getting hints about this because of the .editorconfig file that exists in this repo. In particular, we don't use default visibility modifiers (like private here) and we generally don't use var.

cgillum · 2025-01-15T00:57:26Z

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientExtensionsTests.cs

+        // Assert
+        var provider = services.BuildServiceProvider();
+        var options = provider.GetService<IOptions<GrpcDurableTaskClientOptions>>();
+        options.Should().NotBeNull();


I'm curious why we don't validate the properties on the options in this test?

cgillum · 2025-01-15T00:59:24Z

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientExtensionsTests.cs

+
+        // Assert
+        action.Should().Throw<ArgumentNullException>()
+            .WithMessage("Value cannot be null. (Parameter 'The connection string is missing the required 'Endpoint' property.')");


This doesn't look correct. It should be Parameter '{parameterName}' but here you have it showing the message. Are we constructing exceptions incorrectly in the validation code?

cgillum · 2025-01-15T01:00:33Z

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientExtensionsTests.cs

+
+        // Act & Assert
+        var action = () => mockBuilder.Object.UseDurableTaskScheduler(connectionString);
+        action.Should().Throw<ArgumentException>();


The correct exception to throw in this case is ArgumentNullException.

cgillum · 2025-01-15T01:03:20Z

test/Client/AzureManaged.Tests/DurableTaskSchedulerClientOptionsTests.cs

+        };
+
+        // Act
+        var channel = options.CreateChannel();


Should we dispose this channel to avoid leaking resources?

durable task scheduler auth extension

fc053ec

save initial

cgillum reviewed Jan 9, 2025

View reviewed changes

YunchuWang added 14 commits January 8, 2025 22:46

support local conn with no auth and via http

6f1d2bc

be consistent with azuremanaged targetversion

51e39bd

clean

6987f1b

namespace

e162815

fix

e19de44

fix

3a6dc52

doc

65dfb85

ppl

29445a0

remove

e63f12a

test proj to sln

5c72ee7

fix warning

c6e42c5

remove dup

6a66aaa

fix

131c575

Revert "fix"

65fa607

This reverts commit 131c575.

YunchuWang requested a review from cgillum January 9, 2025 10:05

jviau reviewed Jan 9, 2025

View reviewed changes

YunchuWang added 10 commits January 10, 2025 12:52

save

4f45ec5

fix

d4607e4

save

552a9c8

save

54dba76

some fb

5e97555

fix

9c890c5

update

49c6282

update tests

1adf7cc

sample

14b94eb

sample fix

save

5696b2f

YunchuWang added 3 commits January 11, 2025 11:04

Revert "sample"

0d2a20c

This reverts commit 14b94eb.

fix tests

f4f03fc

test accesstokencache

59c5e9c

YunchuWang requested a review from jviau January 11, 2025 21:48

YunchuWang added 11 commits January 12, 2025 02:59

split

3dfbb72

fix shared

5083703

save

a505004

fix client managed

443613e

fix worker

8744f8a

fix shared tests

37d3d5e

fix client tests

c7f499a

fix worker tests

f077e98

fix tests path

89b7071

update sln

77cd2d9

add sample

3af8b62

cgillum reviewed Jan 13, 2025

View reviewed changes

jviau reviewed Jan 13, 2025

View reviewed changes

YunchuWang requested a review from cgillum January 13, 2025 22:44

YunchuWang and others added 3 commits January 13, 2025 14:58

Update src/Shared/AzureManaged/Shared.AzureManaged.csproj

549e105

Co-authored-by: Jacob Viau <javia@microsoft.com>

fix compile

3a40d09

refactor

c697fbc

YunchuWang force-pushed the wangbill/auth branch from 821acdd to c697fbc Compare January 14, 2025 04:50

fix

55eea54

YunchuWang requested a review from jviau January 14, 2025 05:02

cgillum reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

durable task scheduler auth extension support #362

durable task scheduler auth extension support #362

YunchuWang commented Jan 9, 2025 •

edited

Loading

cgillum left a comment

cgillum commented Jan 9, 2025

jviau commented Jan 10, 2025

YunchuWang commented Jan 11, 2025

YunchuWang commented Jan 11, 2025

cgillum commented Jan 12, 2025 •

edited

Loading

YunchuWang commented Jan 12, 2025 •

edited

Loading

cgillum Jan 13, 2025

jviau Jan 13, 2025

jviau Jan 13, 2025

YunchuWang Jan 13, 2025

YunchuWang Jan 13, 2025

jviau Jan 13, 2025

YunchuWang commented Jan 14, 2025 •

edited

Loading

cgillum left a comment

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

cgillum Jan 15, 2025

	internal class ConfigureGrpcChannel(IOptionsMonitor<DurableTaskSchedulerClientOptions> schedulerOptions) :
	class ConfigureGrpcChannel(IOptionsMonitor<DurableTaskSchedulerClientOptions> schedulerOptions) :

	/// This should only be set to true in development/testing scenarios.
	/// This should only be set to true in local development/testing scenarios.

	switch (authType.ToLower(CultureInfo.InvariantCulture).Replace(" ", string.Empty))
	switch (authType.ToLowerInvariant)

durable task scheduler auth extension support #362

Are you sure you want to change the base?

durable task scheduler auth extension support #362

Conversation

YunchuWang commented Jan 9, 2025 • edited Loading

New Projects and Build Configuration:

Azure Managed Client Implementation:

cgillum left a comment

Choose a reason for hiding this comment

cgillum commented Jan 9, 2025

jviau commented Jan 10, 2025

YunchuWang commented Jan 11, 2025

YunchuWang commented Jan 11, 2025

cgillum commented Jan 12, 2025 • edited Loading

YunchuWang commented Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YunchuWang commented Jan 14, 2025 • edited Loading

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YunchuWang commented Jan 9, 2025 •

edited

Loading

cgillum commented Jan 12, 2025 •

edited

Loading

YunchuWang commented Jan 12, 2025 •

edited

Loading

YunchuWang commented Jan 14, 2025 •

edited

Loading