Add .NET 8.0 and .NET 10.0 support to DurableTask.AzureServiceFabric#1305
Open
krishankumar95 wants to merge 1 commit intoAzure:mainfrom
Open
Add .NET 8.0 and .NET 10.0 support to DurableTask.AzureServiceFabric#1305krishankumar95 wants to merge 1 commit intoAzure:mainfrom
krishankumar95 wants to merge 1 commit intoAzure:mainfrom
Conversation
Multi-target the Service Fabric provider to support net48, net472, net8.0, and net10.0. On modern .NET, the OWIN/Web API HTTP layer is replaced with ASP.NET Core Controllers + Kestrel via the official Microsoft.ServiceFabric.AspNetCore.Kestrel integration package. Key changes: - Update SF SDK from 6.4.617 to 8.3.475/11.3.475 - Add conditional compilation (#if NETFRAMEWORK / NET6_0_OR_GREATER) for framework-specific code paths - Replace OWIN + Web API with ASP.NET Core on modern .NET: - KestrelCommunicationListener replaces OwinCommunicationListener - ASP.NET Core ControllerBase replaces Web API ApiController - ActivityLoggingMiddleware replaces ActivityLoggingMessageHandler - Built-in DI replaces DefaultDependencyResolver - Fix cross-platform API usage: - HttpUtility.UrlEncode -> WebUtility.UrlEncode - SHA256Managed -> SHA256.Create() - JsonMediaTypeFormatter -> direct Newtonsoft.Json serialization - Update test projects to also target net10.0 - Bump Newtonsoft.Json to 13.0.3 for ASP.NET Core compatibility Existing net48/net472 behavior is fully preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }; | ||
| var jsonSettings = new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.All }; | ||
| var jsonContent = new StringContent( | ||
| JsonConvert.SerializeObject(@object, jsonSettings), |
Check warning
Code scanning / CodeQL
Cross-site scripting Medium
Contributor
There was a problem hiding this comment.
Pull request overview
Multi-targets the DurableTask Service Fabric provider to support modern .NET (net8.0/net10.0) while preserving .NET Framework behavior, including replacing the legacy OWIN/Web API proxy surface with ASP.NET Core + Kestrel for newer TFMs.
Changes:
- Add multi-targeting (net48/net472/net8.0/net10.0) and update central package versions for newer Service Fabric SDK and Newtonsoft.Json.
- Introduce ASP.NET Core hosting path (KestrelCommunicationListener, ControllerBase, middleware) for non-NETFRAMEWORK builds while keeping OWIN/Web API for NETFRAMEWORK.
- Update remote client and helper utilities for cross-platform APIs (UrlEncode, SHA256.Create, JSON serialization without Http.Formatting).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/TestFabricApplication/TestApplication.Common/TestApplication.Common.csproj | Adds net10.0 target and makes OWIN hosting dependency conditional to .NET Framework TFMs. |
| test/DurableTask.AzureServiceFabric.Tests/DurableTask.AzureServiceFabric.Tests.csproj | Multi-targets tests for net48 + net10.0 and aligns test package refs with central management. |
| test/DurableTask.AzureServiceFabric.Integration.Tests/DurableTask.AzureServiceFabric.Integration.Tests.csproj | Multi-targets integration tests for net48 + net10.0; makes System.Web/WebApi refs net48-only. |
| test/DurableTask.AzureServiceFabric.Integration.Tests/DeploymentUtil/DeploymentHelper.cs | Suppresses deprecation warning for SF SDK RemoveReplicaAsync overload. |
| src/DurableTask.AzureServiceFabric/Service/TaskHubProxyListener.cs | Switches proxy listener to KestrelCommunicationListener on modern .NET; retains OWIN on .NET Framework. |
| src/DurableTask.AzureServiceFabric/Service/Startup.cs | Adds ASP.NET Core Startup (controllers + NewtonsoftJson) for modern TFMs while preserving OWIN Startup for NETFRAMEWORK. |
| src/DurableTask.AzureServiceFabric/Service/ProxyServiceExceptionLogger.cs | NETFRAMEWORK-only compilation for Web API exception logger. |
| src/DurableTask.AzureServiceFabric/Service/ProxyServiceExceptionHandler.cs | NETFRAMEWORK-only compilation for Web API exception handler. |
| src/DurableTask.AzureServiceFabric/Service/OwinCommunicationListener.cs | NETFRAMEWORK-only compilation for OWIN listener. |
| src/DurableTask.AzureServiceFabric/Service/IOwinAppBuilder.cs | NETFRAMEWORK-only compilation for OWIN Startup interface. |
| src/DurableTask.AzureServiceFabric/Service/FabricOrchestrationServiceController.cs | Adds ASP.NET Core controller implementation for modern TFMs while keeping Web API controller for NETFRAMEWORK. |
| src/DurableTask.AzureServiceFabric/Service/DefaultDependencyResolver.cs | NETFRAMEWORK-only compilation for Web API dependency resolver. |
| src/DurableTask.AzureServiceFabric/Service/ActivityLoggingMiddleware.cs | Adds ASP.NET Core middleware equivalent to the Web API DelegatingHandler logging. |
| src/DurableTask.AzureServiceFabric/Service/ActivityLoggingMessageHandler.cs | NETFRAMEWORK-only compilation for Web API activity logging handler. |
| src/DurableTask.AzureServiceFabric/Remote/RemoteOrchestrationServiceClient.cs | Removes Http.Formatting usage; switches to explicit Newtonsoft.Json serialization and cross-platform UrlEncode. |
| src/DurableTask.AzureServiceFabric/Remote/DefaultStringPartitionHashing.cs | Replaces SHA256Managed with SHA256.Create for cross-platform support. |
| src/DurableTask.AzureServiceFabric/Extensions.cs | Replaces HttpUtility.UrlEncode with WebUtility.UrlEncode for cross-platform support. |
| src/DurableTask.AzureServiceFabric/DurableTask.AzureServiceFabric.csproj | Adds net8.0/net10.0 targets and conditionalizes framework-specific dependencies (OWIN vs ASP.NET Core). |
| Directory.Packages.props | Updates central versions (Service Fabric SDK, Newtonsoft.Json) and adds conditional versions for modern TFMs. |
Comments suppressed due to low confidence (4)
src/DurableTask.AzureServiceFabric/DurableTask.AzureServiceFabric.csproj:43
Microsoft.Extensions.DependencyInjectionis referenced unconditionally, but the centrally-managed version is very old (2.1.1). Fornet8.0/net10.0builds (which also referenceMicrosoft.AspNetCore.App), this can pin/compile against an incompatible DI assembly version and cause runtime assembly load failures. Make this PackageReference conditional tonet48/net472(or add a conditional central version for modern TFMs).
<ItemGroup>
<PackageReference Include="ImpromptuInterface" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" />
<PackageReference Include="Microsoft.ServiceFabric.Data" />
<PackageReference Include="Microsoft.ServiceFabric.Services" />
<PackageReference Include="Microsoft.ServiceFabric" />
<PackageReference Include="System.Collections.Immutable" />
src/DurableTask.AzureServiceFabric/DurableTask.AzureServiceFabric.csproj:6
- Targeting
net10.0will require the build environment/CI to have a .NET 10 SDK installed. This repo doesn’t appear to pin SDK versions (noglobal.json), and the existing GitHub workflow doesn’t set up modern SDKs. Consider adding/updating tooling (e.g.,global.jsonor pipeline setup) to ensurenet10.0builds reliably.
<TargetFrameworks>net48;net472;net8.0;net10.0</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
test/DurableTask.AzureServiceFabric.Tests/DurableTask.AzureServiceFabric.Tests.csproj:6
- The AzureServiceFabric test suite now targets
net48;net10.0, but the product project also targetsnet8.0. As-is, the newnet8.0target won’t be exercised by these tests. Consider addingnet8.0to the testTargetFrameworks(or otherwise ensuringnet8.0is built/tested in CI).
<TargetFrameworks>net48;net10.0</TargetFrameworks>
<Platforms>AnyCPU;x64</Platforms>
test/DurableTask.AzureServiceFabric.Integration.Tests/DurableTask.AzureServiceFabric.Integration.Tests.csproj:8
- The integration tests now target
net48;net10.0, but the product project also targetsnet8.0. Consider addingnet8.0to the integration testTargetFrameworksso the new modern runtime path is covered beyond a single TFM.
<TargetFrameworks>net48;net10.0</TargetFrameworks>
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
<Platforms>AnyCPU;x64</Platforms>
</PropertyGroup>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+62
| context.Response.Headers[Constants.ActivityIdHeaderName] = activityId; | ||
| } |
There was a problem hiding this comment.
ActivityId response header is set after the request pipeline runs. In ASP.NET Core the response may have already started (either in the normal path or after writing the error body), which can throw and/or silently drop the header. Register the header via context.Response.OnStarting(...) (or set it before calling next) so it’s guaranteed to be included.
| ServiceFabricProviderEventSource.Tracing.LogProxyServiceError(activityId, requestPath, requestMethod, exception); | ||
| context.Response.StatusCode = (int)HttpStatusCode.InternalServerError; | ||
| context.Response.ContentType = "application/json"; | ||
| await context.Response.WriteAsync(JsonConvert.SerializeObject(exception)); |
There was a problem hiding this comment.
On exception, the middleware serializes and returns the raw Exception object to the caller. This leaks stack traces / internal details and can produce very large or non-serializable payloads. Consider returning a sanitized error shape (e.g., message + activityId) and keep full exception details only in logs.
Suggested change
| await context.Response.WriteAsync(JsonConvert.SerializeObject(exception)); | |
| var errorResponse = new | |
| { | |
| message = "An internal server error occurred.", | |
| activityId = activityId | |
| }; | |
| await context.Response.WriteAsync(JsonConvert.SerializeObject(errorResponse)); |
Comment on lines
+78
to
+82
| services.AddControllers() | ||
| .AddNewtonsoftJson(options => | ||
| { | ||
| options.SerializerSettings.TypeNameHandling = TypeNameHandling.All; | ||
| }); |
There was a problem hiding this comment.
TypeNameHandling.All is enabled for ASP.NET Core JSON (applies to request deserialization as well as responses). With untrusted clients this is a well-known JSON.NET deserialization risk. If the proxy endpoint can be reached by untrusted callers, consider restricting allowed types via a custom ISerializationBinder (or avoid TypeNameHandling.All for input models).
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Multi-target the Service Fabric provider to support net48, net472, net8.0, and net10.0. On modern .NET, the OWIN/Web API HTTP layer is replaced with ASP.NET Core Controllers + Kestrel via the official Microsoft.ServiceFabric.AspNetCore.Kestrel integration package.
Key changes:
Existing net48/net472 behavior is fully preserved.