-
Notifications
You must be signed in to change notification settings - Fork 35
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
Suggestions for Microsoft.ServiceFabric.AspNetCore.Gateway #4
Comments
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: [email protected] Thanks, |
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. |
Hi Christian, Please see my comments inline. CommunicationClient: [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: [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: [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: [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. |
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. |
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:
CommunicationClientFactory:
Request forwarding:
Via
,X-Forwarded-*
andForwarded
headers to allow the services to get the original informationX-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 complexOptions:
You can find some more information in the project wiki
The text was updated successfully, but these errors were encountered: