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.

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

durable task scheduler auth extension support #362

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

YunchuWang
Copy link
Member

This pull request introduces a new set of Azure extensions for the Durable Task Framework, including project setup, implementation of core classes and methods, and unit tests. The most important changes include adding new projects to the solution, defining the core classes and methods for Azure integration, and creating unit tests to ensure the functionality of the new extensions.

Project Setup:

  • Added new projects Extensions and Azure to the solution Microsoft.DurableTask.sln and configured their build settings. [1] [2] [3]

Core Implementation:

  • Created the Azure.csproj file to define the Azure extensions project, including dependencies and target frameworks.
  • Implemented the DurableTaskSchedulerConnectionString class to parse and manage connection strings for the Durable Task Scheduler service.
  • Added DurableTaskSchedulerExtensions class with methods to configure Durable Task Worker and Client to use the Durable Task Scheduler service.
  • Defined DurableTaskSchedulerOptions class to encapsulate configuration options for the Durable Task Scheduler.

Unit Tests:

  • Created unit tests in DurableTaskSchedulerConnectionStringTests.cs to validate the parsing and handling of connection strings.

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Looking great! I added some comments.

@cgillum
Copy link
Member

cgillum commented Jan 9, 2025

@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?

@YunchuWang YunchuWang requested a review from cgillum January 9, 2025 10:05
src/Extensions/Azure/DurableTaskSchedulerExtensions.cs Outdated Show resolved Hide resolved
src/Extensions/Azure/DurableTaskSchedulerExtensions.cs Outdated Show resolved Hide resolved
return new ArgumentException(message: $"Required option '{optionName}' was not provided.");
}

sealed class TokenCache(TokenCredential credential, TokenRequestContext context, TimeSpan margin)
Copy link
Member

Choose a reason for hiding this comment

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

Did you look into Azure.Identity library to see if they already do token caching?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@YunchuWang YunchuWang Jan 10, 2025

Choose a reason for hiding this comment

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

https://github.com/Azure/azure-sdk-for-net/blob/8992dc64edcedebcbde48e33f376d8489e4a8d19/sdk/identity/Azure.Identity/samples/TokenCache.md
https://azuresdkdocs.blob.core.windows.net/%24web/dotnet/Azure.Identity/1.13.1/index.html?utm_source=chatgpt.com

{34383AAB-75E3-480E-8EE7-28B3F7CC9DD5}

yes i checked azure.identity 1.31.1 doc mentions
Thread safety
We guarantee that all credential instance methods are thread-safe and independent of each other (guideline). This ensures that the recommendation of reusing credential instances is always safe, even across threads.

also default it use in memory cache. our current implementation dont need reimplement these.

AzureCliCredential does not support that but i believe running from local does not need high load, correct me if i am wrong

src/Extensions/Azure/DurableTaskSchedulerExtensions.cs Outdated Show resolved Hide resolved
src/Extensions/Azure/DurableTaskSchedulerExtensions.cs Outdated Show resolved Hide resolved
Microsoft.DurableTask.sln Outdated Show resolved Hide resolved
src/Extensions/Azure/Azure.csproj Show resolved Hide resolved
src/Extensions/Azure/DurableTaskSchedulerOptions.cs Outdated Show resolved Hide resolved
src/Extensions/Azure/DurableTaskSchedulerExtensions.cs Outdated Show resolved Hide resolved
src/Extensions/Azure/DurableTaskSchedulerOptions.cs Outdated Show resolved Hide resolved
@jviau
Copy link
Member

jviau commented Jan 10, 2025

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.

@YunchuWang YunchuWang requested a review from jviau January 11, 2025 21:48
@YunchuWang
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member

cgillum commented Jan 12, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants