[caddy] proxy requests to port locations through ws proxy

[akosyakov]

What it does

• fix Websocket connections no longer work #4047: It is a work-around, ws-proxy does low level manipulations to web socket headers to align them with most common usages.

How to test

• Start a workspace for https://github.com/vaadin/skeleton-starter-flow-spring
• When the server is ready, try to create a web socket for port 35729 and connect to it from devtools. You can verify it in network tab.
  • Notice that live reloading won't work since they don't respect our preview env, only gitpod.io

[aledbf]

why are you removing the mapping for workspace ports?

[akosyakov]

ws-proxy does it after adjusting headers: https://github.com/gitpod-io/gitpod/pull/4419/files#diff-39f2e37e657f269523bb53adff2f549f05b1af3db45432b89daa1c2068847ef3R269

[akosyakov]

As far as i understood without this change it does not go through ws-proxy at least in prev env?

[aledbf]

Correct.

Just in case, I copied this configuration from what we had in NGINX previously.

[akosyakov]

I think in nginx we also delegated to ws-proxy:

gitpod/chart/config/proxy/vhost.server.conf

Lines 160 to 192 in a0a5017

	server {
	listen {{ $listen }};
	# Matches:
	# - (webview-|browser-|extensions-)?+ for now, we only support Theia webviews here! (TODO is there a - meaningful - way to generalize this?)
	# - (?<port>[0-9]{2,5})- port to forward to
	# - (?<wsid>[a-z][0-9a-z\-]+) workspace Id
	# - \.ws(-[a-z0-9]+)? workspace base domain
	# "" needed because of {} (nginx syntax wart)
	server_name "~^(webview-|browser-|extensions-)?+(?<port>[0-9]{2,5})-(?<wsid>[a-z0-9][0-9a-z\-]+)\.ws-{{ .Values.components.proxy.withWsCluster.shortname }}\.${PROXY_DOMAIN_REGEX}$";
	{{- if $useHttps }}
	include lib.ssl.conf;
	include lib.https_redirect.conf;
	{{- end }}
	# include lib.workspace-port-locations.conf;
	include lib.region-headers.conf;
	include lib.resolver.conf;
	location / {
	include lib.proxy.conf;
	include lib.ws-sse.conf;
	# Increase connect timeout
	proxy_connect_timeout 10s;
	# Set max body size to make big uploads work
	client_max_body_size 2048m;
	# disable the error log to not spam our logs when the kube DNS doesn't know about the service yet
	error_log off;
	proxy_pass http://ws-proxy.{{ .Values.components.proxy.withWsCluster.namespace }}.svc.cluster.local:8080$request_uri;

but moved it to caddy while switching to nginx on purpose. I am not sure.

[aledbf]

/werft run

👍 started the job as gitpod-build-ak-fix-web-sockets.5

[aledbf]

@akosyakov this change LGTM but can you split the change in two different PRs?
One for the Caddyfile, to route everything through ws-proxy and other change headers in ws-proxy?

[akosyakov]

Do you mean commits? I’m offline till tomorrow morning if it is critical and should go in tomorrow deployment. Feel free to split, rebase and merge yourself.

[aledbf]

Do you mean commits?

No, two PRs. The caddy change has no relation with the one in ws-proxy. Just to not assume in the future that are related in any way.

[svenefftinge]

Aren't those changes related, i.e. would the fix work without the caddy change?

[aledbf]

would the fix work without the caddy change?

Yes.

The change to Caddy is for core-dev to behave similarly to production (all traffic through ws-proxy)

[csweichel]

Those two changes are separate indeed. One is to bring core-dev closer to ws-proxy, the other is fixing ws-proxy itself.
While we indeed have two separate changes robed into one commit - which might create confusion in the future - I'd hope that this PR will act as a reminder that those changes should not have been conflated in the first place.

All that said, I'd propose we merge those changes because they make a whole lot of sense :)

[svenefftinge]

Thanks for the clarification, Alejandro. So I guess my confusion is good proof of your concern. That said, I agree we should merge it as is to be able to get it in front of users ASAP.

[akosyakov]

I split into 2 commits with detailed explanation from this PR.

[aledbf]

LGTM