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

[HttpFoundation] Support for RFC9213 Targeted HTTP Cache Control #47288

Open
alexander-schranz opened this issue Aug 16, 2022 · 23 comments
Open

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Aug 16, 2022

Description

There is currently a new RFC published for cache-control headers specific for CDN's. It was created by 3 authors working at cloudflare, akamai and fastly.

https://datatracker.ietf.org/doc/rfc9213/

I think it would be great if the HttpFoundation make it easier to control this headers so we have the same functionality like we have now for the Cache-Control header in the Response Class.

Some more links beside the rfc above:

Example

I think the following methods of the HeaderBag would be effect by an "optional" targeted header variable (CDN-Cache-Control, Fastly-Cache-Control, Cloudflare-Cache-Control):

  • addCacheControlDirective
  • removeCacheControlDirective
  • getCacheControlDirective
  • hasCacheControlDirective

The following method on the Response would also be effected by "optional" header name:

  • setMaxAge
  • setStaleIfError
  • setStaleWhileRevalidate
  • setCache
  • setPrivate
  • setPublic
  • isCacheable
  • setImmutable?

Why at first way it looks like it make sense the following I think have no effect on -cache-control header:

  • setSharedMaxAge (cas aching is set via setMaxAge)

Also the Symfony Cache itself should keep the "-Cache-Control" header in mind. In general it is mostly a CDN-Cache-Control and an own name in case of Symfony it would make sense to call it Symfony-Cache-Control.

This headers make it clearer if the response itself is cachable by a server side cache but maybe not by a client side cache (including private ESI's) currently this kind of controlling was not very transparent and FOSHttpCache did workaround by a custom X-Reverse-Proxy-TTL which did not support other things like stale-if-error, statle-while-revalidate, ... Original discussion started there some time ago before the RFC was available: FriendsOfSymfony/FOSHttpCache#455.

PS: As seen above the RFC goes with <Targeted>-Cache-Control while it maybe would first make sense to give only <targeted> into the methods above. It would be better go give the whole header name into it like CDN-Cache-Control this make it possible to support none standardized headers like Surrogate-Control (fastly) and Edge-Control (akamai) also.

Mention some cache related persons: @dbu, @andrerom, @Toflar

@Toflar
Copy link
Contributor

Toflar commented Aug 16, 2022

Looks like cache control header handling should become its own class with an optional prefix. So you can work with multiple CacheControl objects, one having no prefix (default) and others having the vendor prefix (Fastly, Cloudflare...). The Response should then accept and work with a collection of those CacheControl objects. Probably?

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Aug 16, 2022

@Toflar would be possible to move it to such an object but also array $this->cacheControls[] would work. If we go with an object I personally would keep it @internal and not expose it. But this are implementation details, in general I think you agree on adding RFC9213 functionality to Symfony HttpFoundation itself :)

@Toflar
Copy link
Contributor

Toflar commented Aug 16, 2022

If that RFC becomes a standard, then yes, Symfony should support having vendor specific cache control headers. However, we should try to keep the current API as simple as it is now.

@dbu
Copy link
Contributor

dbu commented Aug 16, 2022

according to https://datatracker.ietf.org/doc/html/rfc7127, a "proposed standard" level RFC is already pretty solid.

i prefer Toflars suggestion to extract the CacheControl into its own class. the HeaderBag mixes the 2 concerns of being a container for all headers and of managing the cache control header. if we separate that, i think it would be cleaner already for the existing functionality. and would allow to handle the targeted cache control headers nicely.

and HttpCache should also implement support for targeted cache control headers, as it is a surrogate cache and not a generic cache.

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@dbu
Copy link
Contributor

dbu commented Mar 3, 2023

according to the akamai blog, the reason for RFC9213 was that edge architecture was not precise enough and incompatible implementations exist. the main difference seems to be that the targetting in edge-arch is with a ;[target] suffix to the value.

i wonder if we should offer both models. for example fastly uses surrogate-control (see also). it seems to me the semantic information will be the same, only the rendering is different (prefixed cache-control headers vs a surrogate-control header with potentially suffixed values when targets are used). we could have a flag on the cache control class to decide which spec to follow, so that it can be globally set by symfony rather than on each response individually.

maintainers, how do you think about this discussion? should we do a pull request to extract response cache control into its own class, and then add things to support RFC9213 / edge-arch?

@carsonbot carsonbot removed the Stalled label Mar 3, 2023
@alexander-schranz
Copy link
Contributor Author

As the value of that headers look the same I think it should not be too much work to support both Surrogate-Control and <targeted>-Cache-Control? The RFC9213 is also supported in souin which is the cache middleware of Roadrunner and can also be used with traefik and caddy webserver (which can make sense for FrankenPHP).

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@alexander-schranz
Copy link
Contributor Author

This is not resolved yet. Would be nice to get some feedback out of the Symfony Core Team about what they think about this.

@carsonbot carsonbot removed the Stalled label Sep 25, 2023
@dbu
Copy link
Contributor

dbu commented Sep 25, 2023

@nicolas-grekas you and I quickly discussed about this last week. if there is general interest in exploring this, alexander and me could have a look and propose changes, but it would be good to hear from the core team if you are ok with adding such functionality to the symfony Response object.

technically its possible to do all this by setting headers manually, or with a third party response listener. but the first does not give the logic we have for the Cache-Control header, and the second would mean installing an additional tool and indirection, making it less accessible than if its directly on the response.

@andrerom
Copy link
Contributor

andrerom commented Sep 28, 2023

+1 on adding support for this in Response/HeaderBag objects, makes it easier to standardize this more widely if Symfony and other frameworks makes use of this.

( Side: Suggesting the same over in YARP, which builds on Asp.Net, so adding this natively in Response objects there as well makes a lot of sense: microsoft/reverse-proxy#2235 )

So to avoid subtle misunderstandings, including what headers Symfony Reverse Proxy would use, is something like this the direction people agree on here?

diff --git a/src/Symfony/Component/HttpFoundation/HeaderBag.php b/src/Symfony/Component/HttpFoundation/HeaderBag.php
index 9a3d5549ff..39bfbe4c14 100644
--- a/src/Symfony/Component/HttpFoundation/HeaderBag.php
+++ b/src/Symfony/Component/HttpFoundation/HeaderBag.php
@@ -28,6 +28,16 @@ class HeaderBag implements \IteratorAggregate, \Countable, \Stringable
      */
     protected $headers = [];
     protected $cacheControl = [];
+    /**
+     * RFC9213: Targeted HTTP Cache Control (Dedicated Cache-Control for CDNs & ReverseProxies)
+     *
+     * Also allows "Surrogate-Control" (Fastly) and "Edge-Control" (Akamai).
+     * For Symfony reverse proxy, supported headers should be in order of preference:
+     *          Symfony-Cache-Control, ReverseProxy-Cache-Control, Cache-Control (s-maxage, ..)
+     *
+     * @var array<string, list<string|null>>
+     */
+    protected $targetedCacheControl = [];
 
     public function __construct(array $headers = [])
     {
@@ -155,6 +165,8 @@ class HeaderBag implements \IteratorAggregate, \Countable, \Stringable
 
         if ('cache-control' === $key) {
             $this->cacheControl = $this->parseCacheControl(implode(', ', $this->headers[$key]));
+        } else if ('surrogate-control' === $key || 'edge-control' === $key || str_ends_with($key, '-cache-control')) {
+            $this->targetedCacheControl[$key] = $this->parseCacheControl(implode(', ', $this->headers[$key]));
         }
     }
 
@@ -187,6 +199,8 @@ class HeaderBag implements \IteratorAggregate, \Countable, \Stringable
 
         if ('cache-control' === $key) {
             $this->cacheControl = [];
+        } else if ('surrogate-control' === $key || 'edge-control' === $key || str_ends_with($key, '-cache-control')) {
+            $this->targetedCacheControl[$key] = [];
         }
     }
 
@@ -215,27 +229,42 @@ class HeaderBag implements \IteratorAggregate, \Countable, \Stringable
      *
      * @return void
      */
-    public function addCacheControlDirective(string $key, bool|string $value = true)
+    public function addCacheControlDirective(string $key, bool|string $value = true, string $targetHeader = null)
     {
-        $this->cacheControl[$key] = $value;
-
-        $this->set('Cache-Control', $this->getCacheControlHeader());
+        if ($targetHeader === null) {
+            $this->cacheControl[$key] = $value;
+            $this->set('Cache-Control', $this->getCacheControlHeader());
+        } else {
+            $target = strtr($targetHeader, self::UPPER, self::LOWER);
+            $this->targetedCacheControl[$target][$key] = $value;
+            $this->set($targetHeader, $this->getCacheControlHeader($targetHeader));
+        }
     }

@dbu
Copy link
Contributor

dbu commented Sep 28, 2023

i agree on the direction of your draft, yes.

i was thinking of refactoring the cache control into its own class out of HeaderBag because HeaderBag is growing too big. then we could forward the addCacheControlDirective to the right instance of a cache control etc.

@andrerom
Copy link
Contributor

andrerom commented Oct 1, 2023

One thing (mentioned inline in diff) for Symfony's builtin ReverseProxy:

There are traditionally some cases where you'd use both ReverseProxy and a CDN, for slightly different parts of your responses. For Symfony + FosHttpCache setups in the typically semi personalized content in reverse proxy and static assets in CDN.

That is why it might make sense to not support CDN-Cache-Control which are meant for CDNs, but rather support fallbacks ala:
Symfony-Cache-Control, ReverseProxy-Cache-Control, Cache-Control (s-maxage, ..)

Might be this is less relevant today where most CDN has edge computing capabilities (so could contain the semi personalized content), but despite that might be use cases for several cache layers.

@dbu
Copy link
Contributor

dbu commented Oct 3, 2023

that topic would be covered by the RFC with targeting: https://www.rfc-editor.org/rfc/rfc9213.html#name-cache-behavior

it would need some design decisions how to do that in the application however, as it would not be elegant if the targets are hardcoded all through the application. probably we should have a way to set semantic names and link those in configuration with the correct target header names.

we could have a listener that inserts the mapping of semantic names ot target header names into the response.

@DemigodCode
Copy link
Contributor

Don't think that edge-computing is the way to go for every website.
In my case the edge computing will cause very high costs and it's not an option for me.

I was thinking about a Cache-Control object which can addionally to the default "Cache-Control"-Object be added to the Response object by using a name.

That would support an unlimited number of cache controls, which can be helpful while migrating from one CDN to another CDN and would also allow to stack CDNs / Caches.
I'm struggling with that currently and therefore the suggestion ;-)

@nicolas-grekas
Copy link
Member

I don't think we should support Surrogate-Control. It didn't get traction and the RFC is better aligned to current practices.

Could we have this with "just" an extra $target argument to addCacheControl (and all related methods, isCacheable, setMaxAge, etc) ? Then we'd send $target."-Cache-Control" as needed.

@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Oct 25, 2023

I don't see the value of using target here and concat it with -Cache-Control it would limit us and make searching for a specific header difficult and untransparent in the code bases. So I personally would prefer go with $header = 'Cache-Control' and so header: 'Varnish-Cache-Control' or header: 'Fastly-Cache-Control' and so on can be added as additional param to the cache calls. This is more transparent and make searching for such headers in the code a lot easier.

@DemigodCode
Copy link
Contributor

I don't think we should support Surrogate-Control. It didn't get traction and the RFC is better aligned to current practices.

Could we have this with "just" an extra $target argument to addCacheControl (and all related methods, isCacheable, setMaxAge, etc) ? Then we'd send $target."-Cache-Control" as needed.

Why do you think it didn't get traction? It's supported by CloudFlare and Fastly now.

I don't see the value of using target here and concat it with -Cache-Control it would limit us and make searching for a specific header difficult and untransparent in the code bases. So I personally would prefer go with $header = 'Cache-Control' and so header: 'Varnish-Cache-Control' or header: 'Fastly-Cache-Control' and so on can be added as additional param to the cache calls. This is more transparent and make searching for such headers in the code a lot easier.

Thinking about a configuration parameter like "CDN" which uses the Surrogate-Headers instead of "Cache-Control" and an additional response method like "setBrowserCacheDirective(CacheDirective $directive)" and setCacheDirective(CacheDirective $directive).

Then you can exchance Cache-Control with Surrogate-Control in the response header and use The BrowserCacheDirective for Cache-Control-Header.

That would prevent us from specifying the header name on each cache directive method call like public(), private() and so on.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 25, 2023

It's supported by CloudFlare and Fastly now

They also support the RFC so we don't need to support edge-arch.

Another argument is that edge-arch and the RFC don't support the same set of cache attributes.

We don't need to handle everything in core. If one really wants edge-arch, then fine, do it. But still no need for this in core. It's been around since 2001 and nobody asked for it. We're not looking for nice-to-haves :)

the value of using target here and concat it with -Cache-Control

It's to skip repeating the part that's already in the RFC, and also to reason about "targets". I don't see how this would make searching the code more difficult.

Note that I'm driven by the "let's do the least possibles changes" in my reasoning. We already have a bunch of CC-related methods, they all just need a target argument to handle this concept. That might be worth reusing. Anything more complex might not belong to core since it can be done outside of it.

@alexander-schranz
Copy link
Contributor Author

@nicolas-grekas For me its more about the developer experience. Realistically the devs working will not know about RFC <target>-cache-control they probablay mostly just get task like we added Fastly and we need to replace the Symfony-Cache-Control header with Fastly-Cache-Control header, or searching where the Fastly-Cache-Control header is coming from to network headers. But they will when we go with just the $target parameter not be able to find the code parts setting that headers they first would need to know to search for 'Fastly' instead of 'Fastly-Cache-Control' or 'Symfony' instead of 'Symfony-Cache-Control'.

Note that I'm driven by the "let's do the least possibles changes" in my reasoning. We already have a bunch of CC-related methods, they all just need a target argument to handle this concept. That might be worth reusing. Anything more complex might not belong to core since it can be done outside of it.

Sure I understand to keep the changes as less as possible but still think from the changes point of view it just avoid the concation so using target: 'Fastly-Cache-Control' instead of target: 'Fastly' would be more transparent and less magic to the CC related methods, make thins easier to find in my opinion.

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@carsonbot
Copy link

Could I get a reply or should I close this?

@alexander-schranz
Copy link
Contributor Author

Think this issue make still sense to discuss :)

@carsonbot carsonbot removed the Stalled label May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants