nicolasazrak / caddy-cache

Caching middleware for caddy
Mozilla Public License 2.0
109 stars 30 forks source link

Cache should honor `Surrogate-Control` response headers as well #24

Open ryanschneider opened 6 years ago

ryanschneider commented 6 years ago

The logic to support Surrogate-Control would basically the same as Cache-Control, but the header is specifically meant for reverse proxy <-> origin server controls (Cache-Control is meant for end browsers)

See: https://www.w3.org/TR/edge-arch/

Often used by CDNs: https://docs.fastly.com/guides/tutorials/cache-control-tutorial https://www.nuevocloud.com/documentation/getting-started/cache-headers-cache-control-surrogate-control-and-expires

One caveat: It's not clear to me if it'd the cache plugins job to strip the Surrogate-Control header from the response, or if that should be handled by the proxy plugin.

ryanschneider commented 6 years ago

Testing some more, I think cache does need to remove the Surrogate-Control header, since if proxy removes it the cache doesn't see it.

nicolasazrak commented 6 years ago

To be honest I didn't knew it existed. It shouldn't be hard to implement, it's just a change in the rules logic (https://github.com/nicolasazrak/caddy-cache/blob/master/rules.go). I don't have much time right now but I'll try to implement it in a few days if I can.

ryanschneider commented 6 years ago

Me neither until I came across it :) I probably won't have time this week, but if you haven't gotten to it by next week I should have time to help with a PR.

Thanks!

On Mon, Apr 16, 2018 at 3:58 PM, Nicolas notifications@github.com wrote:

To be honest I didn't knew it existed. It shouldn't be hard to implement, it's just a change in the rules logic (https://github.com/ nicolasazrak/caddy-cache/blob/master/rules.go). I don't have much time right now but I'll try to implement it in a few days if I can.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nicolasazrak/caddy-cache/issues/24#issuecomment-381776040, or mute the thread https://github.com/notifications/unsubscribe-auth/AADREH_4aZL3layEZ8vTwxgGW_TrZDKiks5tpSIfgaJpZM4TUett .

ryanschneider commented 6 years ago

I finally spent some time looking at this more, and realized it actually needs to be added to the cachecontrol package upstream, so I'll open an issue there and see if I can add it.

pquerna commented 6 years ago

(upstream package here)

BTW, I'm generally fine adding support for this in cachecontrol, but as someone who has been doing http caching for a long time, I wasn't familiar with Surrogate-Control. Its not supported by nginx/apache/etc.

So, I'm not opposed to supporting it, I was just wondering if there is a real world origin you are trying to support that outputs these?

Yes, I see that varnish & fastly have been promoting more use of it, but besides a 2001 w3c note, it is notability absent from things like rfc7234

I wonder if @mnot has commentary about Surrogate-Control?

ryanschneider commented 6 years ago

Our in-house origin does, because we added it for our CDN, which I'm now looking to replace with our own edge servers, and was building a proof of concept with Caddy.

It is nice in that it's not passed to end-user browsers, so you can have separate CDN and end-user caching strategies, but I agree 100% it's pretty niche.

nicolasazrak commented 6 years ago

I'm in favor of adding any feature as long as it doesn't add complexity to the project, which seems to be the case. For this package, I think it's just a matter of upgrading the cache-control version when the support for Surrogate-Control is implemented and removing the header from the response if it's present. So unless I'm missing something this should be a pretty trivial change.

mnot commented 6 years ago

It's also supported by Akamai and other CDNs. That is not one of my favourite specs (sorry), but it does see deployment.

I'm half-tempted to write an updated spec and come up with some tests.

It would be good for frameworks to support it, so that they can interop with CDNs better; I've been trying to get CDNs to become more standardised in how they handle HTTP, so this would help (I think).