Skip to content
This repository has been archived by the owner on Oct 10, 2018. It is now read-only.

PrepareWebSocketClient and GetProxyOptions options. #75

Closed
wants to merge 3 commits into from

Conversation

saithis
Copy link

@saithis saithis commented Nov 27, 2017

We need to modify the headers and the target url of the websocket connection request, but depending on the request the headers and the target url could be different.

With this PR the headers can then be changed with PrepareWebSocketClient (which works similar to PrepareRequest for normal http requests). But the target url can't be set on the ClientWebSocket. Thats why I changed the fixed ProxyOptions in the RunProxy method to a function in the SharedProxyOptions which gets the HttpRequest and returns the ProxyOptions for each request.

@saithis
Copy link
Author

saithis commented Nov 27, 2017

I can't build the tests and samples, I get the following error: "The current .NET SDK does not support targeting .NET Core 2.1. Either target .NET Core 2.0 or lower, or use a version of the .NET SDK that supports .NET Core 2.1."
Any help how I can solve that, so that I can fix the tests and samples? I searched for a .NET SDK 2.1 preview download, but couldn't find one.

@davidfowl
Copy link
Member

@saithis run the build script, it installs things. Then you need to add the %userprofile%.dotnet to your path (in front of the program files location) to make visual studio work.

@dnfclas
Copy link

dnfclas commented Nov 27, 2017

CLA assistant check
All CLA requirements met.

@saithis
Copy link
Author

saithis commented Nov 27, 2017

@davidfowl Thanks :) Visual Studio still doesn't work, but at least I could check my code with the buildscript. I hope the tests succeed now.

/// </summary>
/// <param name="app"></param>
/// <param name="baseUri">Destination base uri</param>
public static void RunProxy(this IApplicationBuilder app, Uri baseUri)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't remove these methods as it would be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Proxy has never shipped, breaking changes aren't a problem.

Copy link
Contributor

@mkosieradzki mkosieradzki left a comment

Choose a reason for hiding this comment

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

I don't get the point of using GetProxyOptions.

Current approach integrates with the middleware pipeline.

Why not to use only the PrepareWebSocketClient?

@saithis
Copy link
Author

saithis commented Nov 27, 2017

@mkosieradzki because the destinationUri must be provided in await client.ConnectAsync(destinationUri, context.RequestAborted);
How would I do that with PrepareWebSocketClient?
Change PrepareWebSocketClient to also optionally return a destinationUri? But that would seem awkward to me.
If you have a good idea, I'll change it.

@mkosieradzki
Copy link
Contributor

@saithis check for conditions (e.g. MapWhen) in the middleware pipeline then run RunProxy with a proper destination url.

I understand that you want to use PrepareWebSocketClient to setup some advanced web socket client options - not the url.

If you can't get it working, I will have a look in couple of days.

@mkosieradzki
Copy link
Contributor

Please forgive me if I am missing something (I am typing from mobile phone), but dynamic routing scenario should be handled by #46 and AFAIR you should be able to use HttpContext.ProxyRequest extension method in your dynamic url scenario.

@saithis
Copy link
Author

saithis commented Nov 27, 2017

Maybe a bit more about my usecase, because i think MapWhen doesn't work for it:
We have a database with webapps, customers and subdomains. Every customer will have a subdomain resolving to the proxy. Every new version release of our product gets deployed as a new webapp and is inserted in this database. Now when our customer wants to use this new version, he clicks a button and the proxy should automatically switch all following requests to the api with the new version. Also the proxy adds some headers to every request, so the api knows to which customer the request belongs.

We have a prototype working for this, but now we wanted to add SingalR to it and couldn't do it with the code from the current dev branch. With the changes in this PR we got it mostly working. Only missing part is now how to switch the websocket over to the new api version.

As far as I know, we couldn't dynamically add/remove/change customers and apis when using MapWhen. Or am I wrong there?

I understand that you want to use PrepareWebSocketClient to setup some advanced web socket client options - not the url.

I only want to set headers and the url, nothing else. But the headers and url can be different for the requests and can change all the time (they are decided depending on the entries in out database)

Please forgive me if I am missing something (I am typing from mobile phone), but dynamic routing scenario should be handled by #46 and AFAIR you should be able to use HttpContext.ProxyRequest extension method in your dynamic url scenario.

Yes I probably could write my own middleware and use HttpContext.ProxyRequest, I just thought this code would be helpful for more people, as some had already asked for this. Also with #46 you have to provide dummy ProxyOptions and then override the destination in PrepareRequest, which works, but isn't that nice.

@mkosieradzki
Copy link
Contributor

Yup, I get the dynamic routing point and this is basically why I have created #46. In that case IMO you should use ProxyRequest and no:

  • you don't need to create a middleware as ProxyRequest just works with any kind of inline middleware like .Use
  • you don't have to provide dumny ProxyOptions as ProxyRequest does not even take ProxyOptions

@saithis
Copy link
Author

saithis commented Nov 27, 2017

Yeah, I meant to "create" a inline middleware like .Use :)

Should I change the PR to only add PrepareWebSocketClient then? (If yes, would a new, clean PR be better?)

@mkosieradzki
Copy link
Contributor

I am presenting only my personal opinion. As the ProxyRequest API is actually undocumented and very often missed. And IMO existence of this API might have some influence on further API changes.

I cannot speak in the name of team.

@saithis
Copy link
Author

saithis commented Nov 28, 2017

@mkosieradzki I think you may be right. Writing my own (inline) middleware is probably the better solution than GetProxyOptions. I'll make a new PR with only the PrepareWebSocketClient option. This PR can then be closed/rejected.

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

Successfully merging this pull request may close these issues.

6 participants