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

[proxy] Replace nginx with Caddy #3964

Merged
merged 4 commits into from
May 19, 2021
Merged

[proxy] Replace nginx with Caddy #3964

merged 4 commits into from
May 19, 2021

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Apr 15, 2021

Changes:

  • Migrate proxy from NGINX to Caddy
  • Remove bash script to build configuration files
  • Remove chart configuration files
  • Cleanup chart variables
  • Remove the option to use HTTP or HTTPS and make HTTPS mandatory.
  • Migrate lua scripts to Caddy middleware (Go)

TODO:
- make minio and docker-registry optional
- Private workspace ports

  • Custom 404 error pages?

fixes #3611

@aledbf aledbf force-pushed the aledbf/proxy2caddy branch 11 times, most recently from 34ea1b2 to 05c5763 Compare April 16, 2021 15:12
@aledbf
Copy link
Member Author

aledbf commented Apr 16, 2021

/werft run

👍 started the job as gitpod-build-aledbf-proxy2caddy.27

@aledbf aledbf force-pushed the aledbf/proxy2caddy branch 12 times, most recently from 52bcadd to 139e88f Compare April 17, 2021 02:34
@aledbf
Copy link
Member Author

aledbf commented Apr 17, 2021

/werft run

👍 started the job as gitpod-build-aledbf-proxy2caddy.55

@aledbf aledbf force-pushed the aledbf/proxy2caddy branch 5 times, most recently from 5a54824 to 57106ea Compare April 17, 2021 22:51
@aledbf
Copy link
Member Author

aledbf commented Apr 23, 2021

It works very nice now! What was the issue?

There is an issue in Go related to http/2 golang/go#42534 (comment)
(temporal workaround is to disable http/2). TODO: check if this affects ws-proxy when is exposed directly (pluggable workspaces)

@akosyakov
Copy link
Member

There is an issue in Go related to http/2 golang/go#42534 (comment)
(temporal workaround is to disable http/2). TODO: check if this affects ws-proxy when is exposed directly (pluggable workspaces)

I see, wonder whether it can affect end users? e.g. if one tests an app loading time on a port location then he can get wrong impression because of disabled http2, i.e. resources will be loaded one after another, not parallel? or it does not affect them?

@aledbf
Copy link
Member Author

aledbf commented Apr 24, 2021

I see, wonder whether it can affect end users?

Maybe? I can perceive a difference only opening something like vscode (~2000 request every time)

resources will be loaded one after another, not parallel?

There is a default of 6 parallel downloads. This is really old but the values are similar https://www.stevesouders.com/blog/2008/03/20/roundup-on-parallel-connections/

@akosyakov akosyakov marked this pull request as ready for review April 24, 2021 19:57
@akosyakov akosyakov marked this pull request as draft April 24, 2021 19:57
@akosyakov
Copy link
Member

Unfortunately it makes really huge difference from perf perspective. Our website loaded with slow 3g with disabled caching in production vs the PR:
Screenshot 2021-04-24 at 22 12 15
Screenshot 2021-04-24 at 22 13 01

The thing is that optimisation techniques with http2 and without are quite different, and not having http2 on port location will harm usefulness of Gitpod in this sense.

@aledbf
Copy link
Member Author

aledbf commented May 4, 2021

/werft run

👍 started the job as gitpod-build-aledbf-proxy2caddy.142

@aledbf aledbf force-pushed the aledbf/proxy2caddy branch from 5765334 to cdba43c Compare May 5, 2021 11:58
@geropl
Copy link
Member

geropl commented May 10, 2021

The thing is that optimisation techniques with http2 and without are quite different, and not having http2 on port location will harm usefulness of Gitpod in this sense.

Hm. I'd really love to see us move away from nginx. But it'd would also nice to not have to compromise. @aledbf Would it make sense to wait just a little longer and check if either:
a) golang/go#42534 gets fixed upstream
b) caddy uses the fixed net/http code (this looks like it already does...?) But the question is if we really want to go with untested code here.

Personally I'm inclined to wait: As nice as a switch would be it's not blocking us AFAIK, and the nginx mess stuff is stable atm. Also, the PR is not likely to rot fast. WDYT?

@aledbf
Copy link
Member Author

aledbf commented May 10, 2021

a) golang/go#42534 gets fixed upstream

Agree.

b) caddy uses the fixed net/http code

I tried the proposed fix without success.

Personally I'm inclined to wait:

Yes, I agree with that. That is why this PR is still a draft.

Edit: after all these comments, I updated the PR with a fix that seems to work 🙃

@aledbf
Copy link
Member Author

aledbf commented May 10, 2021

@akosyakov please try again

Screenshot from 2021-05-10 10-59-08

@aledbf
Copy link
Member Author

aledbf commented May 10, 2021

Screenshot from 2021-05-10 11-08-17

@aledbf aledbf requested a review from akosyakov May 10, 2021 15:33
@aledbf aledbf force-pushed the aledbf/proxy2caddy branch 2 times, most recently from 22a14d8 to e9b76ba Compare May 11, 2021 15:09
chart/templates/proxy-configmap.yaml Outdated Show resolved Hide resolved
components/proxy/conf/Caddyfile Outdated Show resolved Hide resolved
components/proxy/conf/Caddyfile Outdated Show resolved Hide resolved
components/proxy/plugins/workspace/go.mod Outdated Show resolved Hide resolved
@aledbf aledbf force-pushed the aledbf/proxy2caddy branch from e9b76ba to 8e56e65 Compare May 13, 2021 11:11
@aledbf aledbf marked this pull request as ready for review May 13, 2021 13:18
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

works fast and stable for my use cases, but i did not check code

@akosyakov
Copy link
Member

@aledbf one thing which seems to be wrong it is how long does it take to download global.css with new PR in our website on slow 3g connection without caching again the prod. It is still slow compare to prod with recent changes.

FROM openresty/openresty:1.19.3.1-3-alpine

ENV TRIGGER_REBUILD 1
FROM aledbf/caddy-http2:0.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we build this caddy image as part of the Gitpod repo, or is this a fork of caddy?

Copy link
Member Author

Choose a reason for hiding this comment

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

This image is a fork of Caddy with the updated packages aledbf/caddy@68fbb8e
I can do the same thing in the gitpod org, forking caddy and adding the commit, or maintain the code in my fork until the fix land in Caddy.

components/proxy/conf/Caddyfile Outdated Show resolved Hide resolved
components/proxy/plugins/workspace/download_workspace.go Outdated Show resolved Hide resolved
@aledbf aledbf force-pushed the aledbf/proxy2caddy branch from 8e56e65 to d98dc05 Compare May 18, 2021 13:15
@aledbf
Copy link
Member Author

aledbf commented May 19, 2021

/werft run

👍 started the job as gitpod-build-aledbf-proxy2caddy.155

@aledbf aledbf merged commit dd0826c into main May 19, 2021
@aledbf aledbf deleted the aledbf/proxy2caddy branch May 19, 2021 23:46
@aledbf aledbf mentioned this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: proxy language: go type: improvement Improves an existing feature or existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxy seem to drop some requests
5 participants