Suggestions for Microsoft.ServiceFabric.AspNetCore.Gateway

[cwe1ss]

Hi,

I created a very similar Gateway. You can find the code here: https://github.com/c3-ls/ServiceFabric-HttpServiceGateway

If I understood correctly, you are working on the Service Fabric team. It would be great if you could consider my code in case you plan to move this project into the SDK. It would also be great if you could keep it an open source project even though it moves into the SDK. I'd be happy to retire my project and help you with this one in that case!

these are the main differences:

CommunicationClient:

• CommunicationClient holds a reference to HttpClient -> so it's more similar to the current WCF solution
• HttpClientHandler.AllowAutoRedirect = false -> this is very important. The gateway should pass responses to the client 1:1 and not follow redirects.
• OperationTimeout for HttpClient -> You don't have any timout logic yet

CommunicationClientFactory:

• Logging -> this is critical and one of the main pain points in the SDK. we need to know when retries happen.
• I already implemented some exception/retry handling. This is a hard and very opinionated topic! there should be an easy hook for devs to change this behavior. I currently only have an option for setting whether valid (but semantically failed) responses (404, 500, etc.) should be retried. A more general hook would be better I guess.

Request forwarding:

• the original request body is a forward-only stream - you can't use it within the InvokeWithRetry loop, it will throw an Exception on the second attempt.
• the gateway sets Via, X-Forwarded-* and Forwarded headers to allow the services to get the original information
• the gateway also adds the path of the gateway as a non-standard X-Forwarded-BasePath header. this allows services to adjust their base path in responses (e.g. gateway has path /SampleService but service is hosted on localhost:30001/). it would be better if the gateway could rewrite URLs in the response but this is quite complex
• the gateway doesn't forward headers that are not valid for forwarding (the current asp.net proxy sample just forwards everything.) E.g. it is not allowed to forward the "Host" header. (you overwrite it correctly though)

Options:

• I structured them a little bit different but both contain a way to pass the PartitionKey-resolver.

You can find some more information in the project wiki

[weidazhao]

Hi Christian,

I'm working on Azure Developer Experience team that belongs to Visual Studio team in Microsoft. We're partnering with Service Fabric team to improve developer experience for the platform. Thank you for your valuable feedback! I will review the technical details above and reply later on. Yes, the intention is to eventually bake both Hosting and Gateway into the Service Fabric SDK as they are very fundamental building blocks for the ASP.NET Core apps running on Service Fabric. For open source, I need to discuss with the team internally to see how to make this happen, but I personally think that's definitely the way to go. I'll be happy to see we merging the goodness in both implementations into one place.

Besides here, you also can reach me via my company mail: dazhao@microsoft.com

Thanks,
-David

[cwe1ss]

Hi David, that's great to hear! Getting the Service Fabric client libraries open source would be awesome. It's amazing to see what you guys have already achieved with .NET Core, roslyn, ASP.NET Core, ADAL, ...

I just made some further changes in my project and I improved the wiki. There is now a separate library for the HTTP communication (C3.ServiceFabric.HttpCommunication) - this means, you can now use the retry logic outside of the gateway for reliable service to service communication with HTTP. I have a few sample projects that show the usage.

[weidazhao]

Hi Christian,

Please see my comments inline.

CommunicationClient:
•CommunicationClient holds a reference to HttpClient -> so it's more similar to the current WCF solution

[David] HttpClient is thread-safe, so I want to reuse the same instance as much as possible, unless we find there are bottlenecks.

•HttpClientHandler.AllowAutoRedirect = false -> this is very important. The gateway should pass responses to the client 1:1 and not follow redirects.

[David] Agree.

•OperationTimeout for HttpClient -> You don't have any timout logic yet

[David] The default timeout of HttpClient is 100 seconds if you don't set anything. But yes, it's fair to expose the parameter so people to configure it.

CommunicationClientFactory:
•Logging -> this is critical and one of the main pain points in the SDK. we need to know when retries happen.

[David] Agree. The code in this repo is a prototype (or you could consider it as a minimum viable product (MVP)), so no logging was added. Logging is definitely mandatory to add when we productizing the component.

•I already implemented some exception/retry handling. This is a hard and very opinionated topic! there should be an easy hook for devs to change this behavior. I currently only have an option for setting whether valid (but semantically failed) responses (404, 500, etc.) should be retried. A more general hook would be better I guess.

[David] Yep, exception handling is hard. Making it configurable to fit different needs would be the right way to go.

Request forwarding:
•the original request body is a forward-only stream - you can't use it within the InvokeWithRetry loop, it will throw an Exception on the second attempt.

[David] In my code, I created a new HttpRequestMessage instance and copied the steam to it every retry. So the problem shouldn't exist, unless I missed something.

•the gateway sets Via , X-Forwarded-* and Forwarded headers to allow the services to get the original information

•the gateway also adds the path of the gateway as a non-standard X-Forwarded-BasePath header. this allows services to adjust their base path in responses (e.g. gateway has path /SampleService but service is hosted on localhost:30001/). it would be better if the gateway could rewrite URLs in the response but this is quite complex

•the gateway doesn't forward headers that are not valid for forwarding (the current asp.net proxy sample just forwards everything.) E.g. it is not allowed to forward the "Host" header. (you overwrite it correctly though)

[David] For header overwriting, there is another middleware at https://github.com/aspnet/BasicMiddleware/tree/dev/src/Microsoft.AspNetCore.HttpOverrides. I was aware of the middleware when I created the prototype so I didn't add anything in the prototype and planned to review how to reuse the HttpOverrides middleware when productizing the gateway.

Options:
•I structured them a little bit different but both contain a way to pass the PartitionKey-resolver.

[David] Yep, partition resolver is one of the key value propositions of this middleware. It can provide developers an extensibility point at server side to implement partition resolving logic so that the logic can be hidden from clients.

[rfcdejong]

Sorry if it's a bit offtopic but I cannot wait to see the hosting package released. For what matters: Christian Weiss his code looks good as well, but I haven't tried it. Even tho i'm not using the gateway part.

I was able to convert the hosting part of our ASP.NET Core 1.0 RC2 to Azure AppFabric using this hosting package (even tho it's MVP) and it seems to be working.