Skip to content
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

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

Merged
merged 10 commits into from
Sep 29, 2023

Conversation

ArtRoman
Copy link
Contributor

@ArtRoman ArtRoman commented Sep 1, 2023

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.

@raman-m raman-m added bug Identified as a potential bug needs feedback Issue is waiting on feedback before acceptance Websockets Ocelot feature: Websockets labels Sep 2, 2023
@raman-m raman-m changed the title Update WebSocketsProxyMiddleware.cs Replace non-WS protocols for the 'ClientWebSocket' in WebSocketsProxyMiddleware Sep 2, 2023
@raman-m raman-m linked an issue Sep 2, 2023 that may be closed by this pull request
@ArtRoman
Copy link
Contributor Author

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
Copy link
Member

raman-m commented Sep 15, 2023

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
Copy link
Member

raman-m commented Sep 15, 2023

@ArtRoman commented on Sep 14

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

@raman-m
Copy link
Member

raman-m commented Sep 15, 2023

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

Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

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

@raman-m raman-m changed the title Replace non-WS protocols for the 'ClientWebSocket' in WebSocketsProxyMiddleware #1509 #1683 Replace non-WS protocols for the 'ClientWebSocket' in WebSocketsProxyMiddleware Sep 15, 2023
@ArtRoman
Copy link
Contributor Author

ArtRoman commented Sep 15, 2023

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
Copy link
Member

raman-m commented Sep 19, 2023

@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.

@raman-m
Copy link
Member

raman-m commented Sep 20, 2023

@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
Copy link
Member

raman-m commented Sep 28, 2023

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

Done! See commit 5d9c214
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
Copy link
Member

raman-m commented Sep 28, 2023

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

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs feedback Issue is waiting on feedback before acceptance labels Sep 29, 2023
@raman-m raman-m merged commit ab29442 into ThreeMammals:develop Sep 29, 2023
2 checks passed
@raman-m
Copy link
Member

raman-m commented Sep 29, 2023

@ArtRoman Congrats! 🥳

@JHennerley
Copy link

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
Copy link
Member

raman-m commented Sep 29, 2023

@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
Copy link
Contributor Author

ArtRoman commented Oct 1, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug Websockets Ocelot feature: Websockets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WebSocket exception Upstream Scheme is not applied to DownstreamScheme, if latter is not specified
4 participants