Updating workflow collection to allow for use of API Token validation #1141

RyanLettieri · 2023-08-30T19:17:09Z

Description

Updated the service collection for workflow in the .NET SDK to allow for the use of api token authentication. Since the durable task framework allows for the passing in of a gRPC channel when configuring the gRPC client, we now create a gRPC channel object to have the API Token headers as well as the address we want to use and pass that through to the DTFx.

Issue reference

The issue this PR will close: #[1134]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

[x ] Code compiles correctly
Created/updated tests
Extended the documentation

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

codecov · 2023-08-30T19:19:48Z

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.02% 🎉

Comparison is base (fd7168f) 66.45% compared to head (5704c07) 66.47%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1141      +/-   ##
==========================================
+ Coverage   66.45%   66.47%   +0.02%     
==========================================
  Files         171      171              
  Lines        5750     5754       +4     
  Branches      624      624              
==========================================
+ Hits         3821     3825       +4     
  Misses       1782     1782              
  Partials      147      147

Flag	Coverage Δ
net6	`66.47% <80.00%> (+0.02%)`	⬆️
net7	`66.47% <80.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/Shared/DaprDefaults.cs	`66.66% <80.00%> (+5.12%)`	⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

DeepanshuA · 2023-08-31T11:18:11Z

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

+
+        public static IServiceCollection ConfigureDurableGrpcClient(this IServiceCollection services, string apiToken)
+        {
+            services.AddHttpClient(ClientName)


For my understanding: Why do we need to add HttpClient while configuring Grpc Client?

Under the hood, the gRPC client uses the HttpClient. It's all HTTP/2 for transport iirc.

halspang

Can you update the example readme with how to use the API token?

halspang · 2023-09-05T19:27:03Z

examples/Workflow/WorkflowConsoleApp/Program.cs

+DaprClient daprClient;
+string apiToken = Environment.GetEnvironmentVariable("DAPR_API_TOKEN");
+if (!string.IsNullOrEmpty(apiToken))
+{
+    daprClient = new DaprClientBuilder().UseDaprApiToken(apiToken).Build();
+}
+else
+{
+    daprClient = new DaprClientBuilder().Build();
+}


We still need the using. After the initialization, you can just wrap the while in this:

using (daprClient) { // Loop here. }

halspang · 2023-09-05T19:27:41Z

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

@@ -57,7 +59,19 @@ public static class WorkflowServiceCollectionExtensions

                if (TryGetGrpcAddress(out string address))
                {
-                    builder.UseGrpc(address);
+                    string? apiToken = Environment.GetEnvironmentVariable("DAPR_API_TOKEN");


For consistency, please use var where you can.

halspang · 2023-09-05T19:35:09Z

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

+                        serviceCollection.ConfigureDurableGrpcClient(apiToken);
+                        HttpClient client = new HttpClient();
+                        client.DefaultRequestHeaders.Add("Dapr-Api-Token", apiToken);
+                        builder.UseGrpc(CreateChannel(client));


This seems to repeat the code in the ConfigureDurableGrpcClient do we need both methods?

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

…flow-api-token-fix

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

yaron2 · 2023-09-06T00:29:59Z

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

+                        // serviceCollection.ConfigureDurableGrpcClient(apiToken);
+                        HttpClient client = new HttpClient();
+                        client.DefaultRequestHeaders.Add("Dapr-Api-Token", apiToken);
+                        builder.UseGrpc(CreateChannel(client));


The address variable should be passed to the builder, otherwise if users configured Dapr to use a non-localhost endpoint, this will not be propagated correctly to the underlying Durable Task client. Good find by @alicejgibbons

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

halspang

One question and a few nits. Did you ensure that the various places where we call the DaprDefault methods all handle an empty/null case instead of just a null case? It shouldn't be too many places to check.

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs

Updating workflow collection to allow for use of API Token validation

ee2e3ee

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

RyanLettieri requested review from a team as code owners August 30, 2023 19:17

DeepanshuA reviewed Aug 31, 2023

View reviewed changes

halspang requested changes Sep 5, 2023

View reviewed changes

RyanLettieri added 2 commits September 5, 2023 13:55

Addressing review comments

bad483a

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

Merge branch 'master' of https://github.com/dapr/dotnet-sdk into work…

b4a1095

…flow-api-token-fix

RyanLettieri force-pushed the workflow-api-token-fix branch from 8723c3b to b4a1095 Compare September 5, 2023 19:56

Cleaning up variables and removing unneeded fn

db364f1

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

yaron2 requested changes Sep 6, 2023

View reviewed changes

RyanLettieri added 5 commits September 5, 2023 18:36

Potential fix for grpc workflow issue

daaeaec

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

Potential fix for grpc workflow issue pt2

980017f

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

Cleaning up workflow service collection code

fa5ab83

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

Using and fixing daprdefaults for workflow api token default

bfa2b86

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

more feedback

5704c07

Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

famarting reviewed Sep 7, 2023

View reviewed changes

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

mukundansundar mentioned this pull request Sep 7, 2023

v1.12 Endgame dapr/dapr#6899

Closed

halspang requested changes Sep 7, 2023

View reviewed changes

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

src/Dapr.Workflow/WorkflowServiceCollectionExtensions.cs Outdated Show resolved Hide resolved

halspang approved these changes Sep 7, 2023

View reviewed changes

yaron2 approved these changes Sep 7, 2023

View reviewed changes

halspang merged commit 87329f6 into dapr:master Sep 7, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating workflow collection to allow for use of API Token validation #1141

Updating workflow collection to allow for use of API Token validation #1141

RyanLettieri commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading

DeepanshuA Aug 31, 2023

halspang Sep 5, 2023

halspang left a comment

halspang Sep 5, 2023

halspang Sep 5, 2023

halspang Sep 5, 2023

yaron2 Sep 6, 2023

halspang left a comment

Updating workflow collection to allow for use of API Token validation #1141

Updating workflow collection to allow for use of API Token validation #1141

Conversation

RyanLettieri commented Aug 30, 2023 • edited Loading

Description

Issue reference

Checklist

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

DeepanshuA Aug 31, 2023

Choose a reason for hiding this comment

halspang Sep 5, 2023

Choose a reason for hiding this comment

halspang left a comment

Choose a reason for hiding this comment

halspang Sep 5, 2023

Choose a reason for hiding this comment

halspang Sep 5, 2023

Choose a reason for hiding this comment

halspang Sep 5, 2023

Choose a reason for hiding this comment

yaron2 Sep 6, 2023

Choose a reason for hiding this comment

halspang left a comment

Choose a reason for hiding this comment

RyanLettieri commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading