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

Added dynamic routing and redesigned API #46

Merged
merged 52 commits into from
Sep 22, 2017

Conversation

mkosieradzki
Copy link
Contributor

@mkosieradzki mkosieradzki commented Feb 24, 2017

Added dynamic routing and redesigned API
Fixed cookie support

This PR addresses:
#37, #35, #34 ?, #27, #10, #43, #47

NOTES:

  • There are no tests for new use-cases I will create them if you accept the new API.
  • I will need to add one more callback (probably not to options but to one of the .ForwardRequest overloads to allow modifying response headers before sending back content).

If you are not convinced about merging QueryString parameters - this is an extremely useful feature when forwarding to the Service Fabric Reverse Proxy. You are often adding query string parameters like PartitionKey=.

@dnfclas
Copy link

dnfclas commented Feb 24, 2017

@mkosieradzki,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@mkosieradzki
Copy link
Contributor Author

Redesign in-progress.

Expose utility methods for advanced scenarios using ProxyUtils
@@ -0,0 +1,34 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Need a copyright and license header here

Copy link
Contributor

@mikaelm12 mikaelm12 left a comment

Choose a reason for hiding this comment

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

Looks good overall. After we get Travis passing this should be good to merge.

@mkosieradzki
Copy link
Contributor Author

@mikaelm12 Thanks, any idea why is Travis failing? Current source code is failing as well. This specific PR is not introducing any new failed test.

Shouldn't we fix travis in a separate PR, because this one is huge enough ;).

It looks like a problem with ClientWebSocket on non-windows platform. I was trying to check other WebSocket-related repos but they have even AppVeyor failing.

@mikaelm12
Copy link
Contributor

Any ideas @davidfowl @anurse?
We're getting an exception in System.Net.Websockets saying that its unable to connect
https://travis-ci.org/aspnet/Proxy/jobs/209152222#L1859

@mkosieradzki
Copy link
Contributor Author

@mikaelm12 Just letting you know this is ready to merge.

@mikaelm12
Copy link
Contributor

@mkosieradzki I haven't forgotten about this 😄
I need to investigate the flaky WebSocket test before merging this.

@mkosieradzki
Copy link
Contributor Author

@mikaelm12 But this one has nothing to do with the WebSocket test, WebSockets were in the previous commit (but the test didn't fail because travis was down).

However I can try to work on the websockets issue.

@KasperHoldum
Copy link

Any progress on dynamic routing?

@mkosieradzki
Copy link
Contributor Author

I have updated this PR and fixed one more issue (cookie support - 1 liner).

@mikaelm12 - please merge if possible (again this PR has nothing to do with the flaky WebSockets test).

BTW. I can try to investigate this flaky test (but it is unrelated to this PR).

@davidfowl
Copy link
Member

Why is there a public ProxyService class?

@mkosieradzki
Copy link
Contributor Author

@davidfowl
I honestly don't know. I did it becase @Tratcher explictly asked me to ;). My wild guess is to be able to add remove service manually from DI container. It is registered directly under its type (not interface). So if it was internal there will be no means to "address" this service. But that's only my guess.

@davidfowl
Copy link
Member

@Tratcher can you clarify?

@mkosieradzki It doesn't look as though it's required at all. Also, you didn't rename SharedProxyOptions to ProxyOptions

@mkosieradzki
Copy link
Contributor Author

@davidfowl Please see (Feb 28):

I will need to revive the ProxyMiddleware for this and the ProxyOptions. But meanwhile current ProxyOptions will become SharedProxyOptions to avoid naming clash.

@Tratcher
Copy link
Member

I don't recall the reasoning for using a service. The only discussion I can find for it is here: #46 (comment)

You should be able to remove the service, inject the options into the middleware, and flow them from there.

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.

None yet

7 participants