-
Notifications
You must be signed in to change notification settings - Fork 29
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
fix(pgs): cache custom domains #174
Conversation
} | ||
reverse_proxy web:3000 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
#tls internal { | ||
# on_demand | ||
#} |
There was a problem hiding this comment.
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.
*.{$APP_DOMAIN}, {$APP_DOMAIN} { | ||
reverse_proxy web:3000 | ||
log { | ||
format append { | ||
server_id {$APP_DOMAIN} | ||
} | ||
} | ||
|
||
#tls internal |
There was a problem hiding this comment.
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.
exclude /check | ||
} | ||
key { | ||
template {http.request.method}-{http.request.path} |
There was a problem hiding this comment.
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}
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closing in favor of #175 |
The main change here was changing the cache key to remove the host.
Reference: caddyserver/cache-handler#115 (comment)
CC @mac-chaffee