search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.38k stars 1.64k forks source link

#1509 #1683 Replace non-WS protocols for the 'ClientWebSocket' in WebSocketsProxyMiddleware #1689

Closed ArtRoman closed 1 year ago

ArtRoman commented 1 year ago

Fixes #1509 #1683

Fix WebSocket for SignalR

Proposed Changes

An ugly fix of ArgumentException: Only Uris starting with 'ws://' or 'wss://' are supported. (Parameter 'uri') exception from downstream host because 'uri' values starts from http, but this isn't allowed in ClientWebSocket.ConnectAsync. This will return Expected HTTP 101 response but was '500 Internal Server Error' error to upstream host.

ArtRoman commented 1 year ago

Any progress? 1 test fails but it's not connected with my changes:

Application startup exception: System.AggregateException: One or more errors occurred. (Unable to start Ocelot, errors are: Unable to start Ocelot because either a Route or GlobalConfiguration are using QoSOptions but no QosDelegatingHandlerDelegate has been registered in dependency injection container. Are you missing a package like Ocelot.Provider.Polly and services.AddPolly()?)

raman-m commented 1 year ago

Sorry, but the last build has failed in one test only which is Ocelot.AcceptanceTests.ConfigurationReloadTests.should_reload_config_on_change This is unstable test. It fails in 1 of 2 cases or even more.

Which test did you mean in previous message? I have no idea.

raman-m commented 1 year ago

@ArtRoman commented on Sep 14

Don't worry! The build is green now! ✅ Congrats! 🎉

raman-m commented 1 year ago

@abhiphirke @zhaoyongjie183 Hello! As an issue raisers, Could you test this fix please, and confirm that your issue is gone with this fix?

ArtRoman commented 1 year ago

Probably we need to update configuration validators also to stop app forcibly if such WebSocket-routes with wrong scheme exist.

It's not the route scheme issue. WebSocket connection is usual HTTP connection with Upgrade header, it can be done with both "http://" or "ws://" endpoints in URL. For example, Android okhttp library makes "http://" endpoint connection for WebSocket, even when "ws://" was in Uri, and that's fatal for System.Net.ClientWebSocket class. So replacing protocol for real WebSocket connection resolves the issue.

raman-m commented 1 year ago

@ArtRoman Could you Sync fork please? We have the diff since yesterday: Comparing ArtRoman:develop...ThreeMammals:develop OR Add me as collaborator to your forked repo please? I will update develop branch.

ArtRoman commented 1 year ago

What about unit test for Invoke and Proxy methods?

Ok, I will try to add tests.

raman-m commented 1 year ago

@ArtRoman commented on Sep 19 Ok, I will try to add tests.

Perfect! After addition of unit tests this PR will be accepted. Let me know if you need a help.

raman-m commented 1 year ago

@ArtRoman commented on Sep 19 Ok, I will try to add tests.

Done! See commit https://github.com/ThreeMammals/Ocelot/pull/1689/commits/5d9c214c67d51dbe9a1f598b63c50a76f2ef8cb0 It was not easy to write them because there was no WebSocketsProxyMiddlewareTests class for unit tests, and WebSocketsProxyMiddleware is implemented using a lot of framework sealed classes without interfaces! So, the WebSocketsProxyMiddleware class was not ready for unit testing at all. I've prepared it for testing in PR #1377 where these unit tests were introduced.

raman-m commented 1 year ago

@RaynaldM Put your eye on this plz! One more approval please! 😉

raman-m commented 1 year ago

@ArtRoman Congrats! 🥳

JHennerley commented 1 year ago

Any idea on time to release this fix?

We're having some trouble handling websocket requests through Ocelot, it has to go through a few layers of netscaler -> nginx -> ocelot, but even when we try nginx -> ocelot we receive the above error.

raman-m commented 1 year ago

@JHennerley commented on Sep 29

Hi J! I'm planning to release this Sunday, October 1. If something goes wrong, the late date is Monday, October 2.

We're having some trouble handling websocket requests through Ocelot, it has to go through a few layers of netscaler -> nginx -> ocelot, but even when we try nginx -> ocelot we receive the above error.

Bad! What Ocelot version do you use currently? Based on .NET 7 ? Inbound Ocelot traffic cannot be managed by the C# code, it is question of Ocelot app hosting. Do you see? By the code and configs we can control & manage downstream connections and behavior. And a little bit, upstream connections. But entire upstream environment is Ocelot ASP.NET app hosting environment. Anyway, after release just try once again, and let me know testing results please.

ArtRoman commented 1 year ago

Done! See commit 5d9c214 It was not easy to write them because there was no WebSocketsProxyMiddlewareTests class for unit tests

Yes, I'm stuck here. Thank you!