Skip to content

[dashboard] Replace nginx with caddy #3851

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

Merged
merged 1 commit into from
Apr 9, 2021
Merged

[dashboard] Replace nginx with caddy #3851

merged 1 commit into from
Apr 9, 2021

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Apr 8, 2021

Reasons:

  • Using nginx for a static server is overkill
  • Caddy is only one static binary without external dependencies
  • Configuration is simpler

@aledbf aledbf force-pushed the aledbf/dashboardcaddy branch from 5cebadb to 355d3ef Compare April 8, 2021 14:26
@aledbf
Copy link
Member Author

aledbf commented Apr 8, 2021

/werft run

👎 cannot start job - please talk to whoever's in charge of your Werft installation

@aledbf
Copy link
Member Author

aledbf commented Apr 8, 2021

/werft run

👍 started the job as gitpod-build-aledbf-dashboardcaddy.3

@aledbf aledbf force-pushed the aledbf/dashboardcaddy branch 3 times, most recently from 6b2c495 to 355fa2a Compare April 8, 2021 15:28
@aledbf aledbf marked this pull request as ready for review April 8, 2021 15:34
@aledbf aledbf mentioned this pull request Apr 8, 2021
24 tasks
@@ -50,8 +50,8 @@ spec:
readinessProbe:
failureThreshold: 3
httpGet:
path: /schemas/gitpod-schema.json
port: 80
path: /ready
Copy link
Member

Choose a reason for hiding this comment

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

that would only mean "the http service is running", the previous check means "the dashboard is served". in-between there might be error cases.

@geropl, do you know why we started to use actual resources from the dashboard to be tested for in the readiness probe?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can imagine several scenarios why that was:

  • convenience: by testing well known dashboard resources, we don't have to think about other paths that we might want to use
  • previous breakage: we may have had some misconfiguration at some point and then moved to checking a dashboard resource

Either way, once caddy is running it would serve the dashboard. It makes sense to me to use a readiness endpoint of caddy here.

@AlexTugarev
Copy link
Member

AlexTugarev commented Apr 9, 2021

/werft run

👍 started the job as gitpod-build-aledbf-dashboardcaddy.12

@aledbf aledbf force-pushed the aledbf/dashboardcaddy branch from 355fa2a to b1aefd3 Compare April 9, 2021 12:01
@aledbf aledbf requested a review from AlexTugarev April 9, 2021 12:09
Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

That worked nicely for me!
I checked that caching works as it used to and known redirects are still in place.
Thanks 🙏🏻

@aledbf aledbf merged commit a5f19d2 into main Apr 9, 2021
@aledbf aledbf deleted the aledbf/dashboardcaddy branch April 9, 2021 13:14
@geropl
Copy link
Member

geropl commented Apr 12, 2021

@aledbf @AlexTugarev @csweichel Did anybody verify that caddy emits "ETag" headers based on content-hash of the static files in this configuration? Something we rely upon here.

}

(caching) {
header Cache-Control "public, max-age=604800, must-revalidate"
Copy link
Member

@geropl geropl Apr 12, 2021

Choose a reason for hiding this comment

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

@aledbf Until now caching was the responsibility of the "front-facing" proxy (cmp. here).

This looks fine generally, but we should double check the results, and ideally either:
a) cross-link with comments, or better
b) merge into one place.

@AlexTugarev
Copy link
Member

@geropl, ups. I've seen the ETags, but didn't check for the implementation. It's indeed based on file size and date.
cf.
https://github.com/caddyserver/caddy/blob/3f6283b385642c56f34b479d1275095379b062d3/modules/caddyhttp/fileserver/staticfiles.go#L545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants