flow icon indicating copy to clipboard operation
flow copied to clipboard

Websocket in combination with AJP broken behind reverse-proxy in 23.2.

Open knoobie opened this issue 2 years ago • 13 comments

Description of the bug

Previously websocket connections could be forwarded by using a separate location in the httpd configuration like so <Location /web/vaadinServlet/> allowing this connection to be used with http / websockets - after 23.2 the websockets connection is not routed tho /web/vaadinServlet/ anymore, instead it goes straigth to the /web/?v-r=push... where the connection can't be established because /web/** forwards to AJP, which DOES not support websockets.

Example exception:


'HTTP upgrade is not supported by this protocol' Requested resource was: '/web/?v-r=push&v-uiId=1&v-pushId=0ce8ff89-eae1-4612-9a12-3383167a2d5b&X-Atmosphere-tracking-id=0&X-Atmosphere-Framework=3.1.2-javascript&X-Atmosphere-Transport=websocket&X-Atmosphere-TrackMessageSize=true&Content-Type=application/json;%20charset=UTF-8&X-atmo-protocol=true'

Expected behavior

Websockets should be handled by a separat endpoint /web/vaadinServlet/ like done since V8.

Minimal reproducible example

.

Versions

  • Vaadin / Flow version: 23.2
  • Java version: 11
  • OS version: xx
  • Browser version (if applicable):
  • Application Server (if applicable):
  • IDE (if applicable):

knoobie avatar Sep 26 '22 10:09 knoobie

Example httpd configuration can be found in VS-4349

knoobie avatar Sep 26 '22 11:09 knoobie

Vaadin 23.1:

https://github.com/vaadin/flow/blob/c2df9e2a4bfcfdfa692b9f0d83075df312b9b42d/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringBootAutoConfiguration.java#L82-L98

(mapping => /vaadinServlet/* is used for push)

since Vaadin 23.2:

https://github.com/vaadin/flow/blob/1aeafa33b895a22240ecf6374880d84522cb21e6/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringBootAutoConfiguration.java#L81-L106

(pushRegistrationPath => "" is used for push if vaadin is deployed as root mapping)

knoobie avatar Sep 26 '22 17:09 knoobie

One persons bug is always another persons feature...

Is the problem here that you want to forward websockets to another server using an Apache 2 configuration? Isn't that doable now with something like

<Location /web>
  Require expr %{QUERY_STRING} =~ /v-r=push/
</location>

instead of the previous

<Location /web/vaadinServlet/>

Artur- avatar Sep 27 '22 10:09 Artur-

Do you have access to the VS? Otherwise I'm happy to share the configuration. The problem is that we used AJP for all the communication between Apache and the embedded Spring Boot Tomcat for improved performance of the /web location to forward on e.g. port 8009 and have another location for /web/vaadinServlet or with V8 (/web/vaadinServlet/PUSH) which uses websockets on port 8080 as protocol. This is, as far as I know, not possible with only one location as reverse proxy. This can be later done with Apache 2.4.26+ with e.g. a nested if-directive but the RedHat enterprise version we are using, has only a custom patched Apache 2.4.6 version available.

knoobie avatar Sep 27 '22 10:09 knoobie

One persons bug is always another persons feature...

I'm fine with this kind of change, in e.g. V24 and a proper announcement, but the vaadinServlet was literally available for a decade and should not have been removed so lightly in my opinion.

knoobie avatar Sep 27 '22 10:09 knoobie

Would a RewriteRule with [P,L] flags and a RewriteCond help to proxy websocket upgrade requests via ws://....?

mcollovati avatar Sep 27 '22 12:09 mcollovati

I can't really answer that, my knowledge about Apache 2 is limited. The configuration provided in the VS was working for years and dozen of projects and clients - changing this for anyone isn't really feasible at the moment.

knoobie avatar Sep 27 '22 12:09 knoobie

So should we add a "feature" flag to get the old mapping back and then remove the flag in V24, as the main/only reason for returning the old mapping would be compatibility?

Artur- avatar Sep 28 '22 06:09 Artur-

I'm fine with that - but I would also highly suggest to create proper reverse proxy documentation for e.g. Apache 2 and nginx until then for people to migrate (@tarekoraby)

knoobie avatar Sep 28 '22 06:09 knoobie

@mcollovati

Would a RewriteRule with [P,L] flags and a RewriteCond help to proxy websocket upgrade requests via ws://....?

I've talked with our Apache Guru about this, and this would be a performance penalty. We also discussed alternative solutions with nested if within the Location but that is also unsupported by Apache for the ProxyPass.

We couldn't find a proper solution to forward AJP and WS by the same Location or filtering by the QUERY_STRING.

I'm highly interested in a proper solution - meaning, we are also considering our current balance, to get a real solution from Vaadin. We couldn't find one that doesn't decrease performance and works with PUSH and AJP on the same endpoint.

Meaning I'm now against a feature flag and in favor of a proper solution where push is handled by e.g. /vaadinPush/ or some other location below the application or another long-term solution provided by Vaadin.

knoobie avatar Sep 30 '22 11:09 knoobie

@knoobie we're currently considering using dedicated /VAADIN/push mapping path for Push, instead of falling back to /vaadinServlet, because /vaadinServlet has another purpose and semantically isn't related to Push anyhow - it's an internal thing and needed for configuring other Spring services, like for example endpoints. The proposition is to use /VAADIN/push mapping with a feature flag for 23.2 and 23.3, default would be still a root mapping. Then we'll document this change and make this mapping be a default since V24. For your project it means that you would need to change the url template in your proxy config from /web/vaadinServlet/ to /web/VAADIN/push, with no impact on performance. Does it sound acceptable?

mshabarov avatar Oct 04 '22 07:10 mshabarov

@mshabarov that would be awesome! Thanks for reconsidering!

knoobie avatar Oct 04 '22 07:10 knoobie

A separate url for push/websocket would help, thanks for looking into it http://techtuxwords.blogspot.com/2022/10/running-vaadin-tomcat-cluster-behind.html

a-schild avatar Oct 08 '22 21:10 a-schild

@mshabarov do you have an update for me how the progress is going here? We are still on 23.1.x because of this and 23.1.x is falling out of support in a month ;)

knoobie avatar Nov 11 '22 16:11 knoobie

@knoobie we have a solution here and currently making a test coverage for it, but the main obstacle for us is that with Jetty we got ws-disconnect-on-heartbeat errors, not clear why. Need to figure out the root cause before we apply this patch.

mshabarov avatar Nov 14 '22 12:11 mshabarov

Fixed in https://github.com/vaadin/flow/releases/tag/23.3.0.rc1 and will be included into next Vaadin 23.3 release. /VAADIN/push mapping is now used always for push requests.

mshabarov avatar Dec 05 '22 10:12 mshabarov

This ticket/PR has been released with Vaadin 23.3.0.

vaadin-bot avatar Dec 13 '22 18:12 vaadin-bot

This ticket/PR has been released with Vaadin 24.0.0.alpha6 and is also targeting the upcoming stable 24.0.0 version.

vaadin-bot avatar Dec 14 '22 15:12 vaadin-bot