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.

Sign up for GitHub

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

fix(pgs): cache custom domains #174

Closed
wants to merge 1 commit into from
Closed

fix(pgs): cache custom domains #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ ssh_data
__debug_bin
.bin
/public/
caddy/caddy
48 changes: 34 additions & 14 deletions caddy/Caddyfile.pgs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,48 @@
}
}

:443 {
reverse_proxy web:3000
log {
format append {
server_id {$APP_DOMAIN}
}
}

#tls internal {
# on_demand
#}
Comment on lines +29 to +31
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 is what I used to test locally.

tls {$APP_EMAIL} {
on_demand
}

encode zstd gzip
route {
cache {
regex {
exclude /check
}
key {
template {http.request.method}-{http.request.path}
Copy link

Choose a reason for hiding this comment

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

See other placeholders here: https://caddyserver.com/docs/caddyfile/concepts#placeholders. I don't recommend to use only method and path in the key. By default it generates a key like that {method}-{scheme}-{host}-{path}-{query}.

Copy link
Contributor

@mac-chaffee mac-chaffee Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I think more accurately, we don't want to share the cache between two different domains. We want to share the cache between two different caddy site blocks, which I'm 99% sure is not possible based on my research here: caddyserver/cache-handler#106 (comment)

Removing the host from the cache key definitely seems bad, since the first person to GET-/index.html on any pgs site would break every other site. Sounded like @darkweak's answer here assumed we meant sharing the cache for two domains within the same site block, which would indeed be possible by removing the host from the key, but it doesn't work for a multi-tenant site block that serves many different websites with name-based virtual hosting.

Copy link
Member Author

@neurosnap neurosnap Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah yes, good call, this will not work with our multitenant setup. For some reason that slipped my mind when looking at this solution and hopeful we could somehow provide placeholders for our particular use case.

Copy link

Choose a reason for hiding this comment

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

AFAIK, if you want to share the same cache between two different caddy site blocks, you'll have use the same storage configuration and it should work then. For this use case, it's better to use redis as storage especially if you deploy multiple caddy instances that have to share the same storage.
To be honest I don't understand well your need and your use case and that's just what I think.

Copy link
Member Author

@neurosnap neurosnap Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah sorry, my original GH issue did not explain the problem well enough, partly because I'm also getting caught up on the particular nuances at play here.

Please allow me to elaborate and @mac-chaffee feel free to correct me if I'm wrong. We use a surrogate key to know which sites to purge the cache. Multiple domains can point to the same site (e.g. https://neovimcraft.com and https://erock-neovimcraft.pgs.sh are the same site, just different domains). When a user uploads a site to us, we need to use the surrogate key to purge both domains from the cache. Because we use (2) different TLS directives in our Caddyfile, we cannot have both domains use the same address block. Further, we need a single Souin API endpoint to perform the purge for any site in our system.

It looks like right now when we have multiple address blocks using the cache directive, Souin creates two instances of the Otter cache. So when we fetch the list of cache keys from the API endpoint, it does not include both the custom domain and the subdomain, it only includes the subdomain cache key.

To make things more complicated, we do not know the custom domain ahead-of-time, so even if we placed the API endpoint on the custom domain address block in Caddy and tried to purge both custom and subdomains (with 2 API calls), we would not have enough information to call the custom domain endpoint.

I'm not against using a different storage service (e.g. redis), however, this feels like overkill considering in-memory storage would otherwise work perfectly for us.

}
}
}
}

*.{$APP_DOMAIN}, {$APP_DOMAIN} {
reverse_proxy web:3000
log {
format append {
server_id {$APP_DOMAIN}
}
}

#tls internal
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 is what I used to test locally.

tls {$APP_EMAIL} {
dns cloudflare {$CF_API_TOKEN}
resolvers 1.1.1.1
}

route {
@souinApi path /souin-api/*
basic_auth @souinApi {
Expand All @@ -38,8 +69,10 @@
regex {
exclude /check
}
key {
template {http.request.method}-{http.request.path}
}
}
reverse_proxy web:3000
Copy link
Member Author

Choose a reason for hiding this comment

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

@mac-chaffee did we need this rever_proxy directive if it is in the parent address already?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I think we don't need both

}

encode zstd gzip
Expand Down Expand Up @@ -117,16 +150,3 @@ monitoring.{$MONITORING_APP_DOMAIN}, prometheus.{$MONITORING_APP_DOMAIN}, grafan
disable_openmetrics
}
}

:443 {
reverse_proxy web:3000
log {
format append {
server_id {$APP_DOMAIN}
}
}
tls {$APP_EMAIL} {
on_demand
}
encode zstd gzip
}