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

Add X-Cache-Enabled HTTP header #1513

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Mar 15, 2024

This PR adds the X-Cache-Enabled HTTP header to nginx responses. It will not be added when upstream cache status is BYPASS as then no caching is used for the request.

WordPress Site Health Page Caching uses specific HTTP header values in the loopback HTTP response in order to detect that page caching is indeed enabled.

The related Fastcgi-Cache HTTP header is added by Trellis, but not used by the WordPress site health check.

Adding this to Trellis by default appears to be a good idea (no obvious side-effects, improving compatibility with WordPress core (site health check)).

See roots discussion: https://discourse.roots.io/t/site-health-check-page-cache-not-detected/25640/2

Note: Should this X-Cache-Enabled HTTP header only be added to loopback requests? On the other hand, the Fastcgi-Cache HTTP header is also already added to all requests.

Possible alternative: A Bedrock MU plugin that uses the Fastcgi-Cache HTTP header to set X-Cache-Enabled in PHP code.

@swalkinshaw
Copy link
Member

Note: Should this X-Cache-Enabled HTTP header only be added to loopback requests? On the other hand, the Fastcgi-Cache HTTP header is also already added to all requests.

If possible (and relatively easy) I might prefer this just to avoid having "duplicate" response headers. In the long run it might be better to unify on the more generic X-Cache-Enabled header but I'd rather not deal with that breaking change.

🤔 X-Cache-Enabled is a bit of a weird name thinking about it more. It says cache is enabled, but not necessarily if its a cache hit? 🤷‍♂️ X-Cache is the more common header for cache status

Another reason to try and limit this to loopback if possible.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 15, 2024

@swalkinshaw: Ideally the X-Cache HTTP header is also checked in WordPress, but until this is added, X-Cache-Enabled appears to be the least intrusive HTTP header.

Yes, constraining the header to loopback requests only would be a good idea, it should be relatively easy using the map.

@strarsis
Copy link
Contributor Author

strarsis commented May 30, 2024

@swalkinshaw: Alternative proposal: Add the X-Cache header name (with check callback) to the checked headers by using the site_status_page_cache_supported_cache_headers filter, as in a Bedrock site mu-plugin.

Related WordPress trac issue (alternative/improved methods of detecting page cache): https://core.trac.wordpress.org/ticket/57349

@swalkinshaw
Copy link
Member

We definitely won't add this in Bedrock by default. I'm not against supporting it in Trellis if we can limit to loopback. I think proxy_set_header is what we want instead of add_header? I'd prefer using X-Cache as well if that works.

@strarsis
Copy link
Contributor Author

strarsis commented Jul 31, 2024

Side note: The already added Fastcgi-Cache header is not added with proxy_set_header because it can be useful to the outside, e.g. reverse proxy frontends?

@strarsis
Copy link
Contributor Author

strarsis commented Aug 1, 2024

proxy_set_header

After some experimentation I noticed that WordPress does not check for the upstream HTTP header, but rather does a loopback HTTP request and checks the HTTP headers from it. So the cache-specific header must be added to the reverse proxy frontend, it could still be added conditionally, for those WordPress loopback requests only, but this would not work as an added upstream HTTP header.

…equests.

Move `X-Cache-Enabled` HTTP header to PHP-specific `location` block.
@strarsis
Copy link
Contributor Author

strarsis commented Aug 2, 2024

@swalkinshaw: It was not as trivial as I thought first, but now the configuration only adds the X-Cache-Enabled header for loopback-requests. Checking for localhost origin (127.0.0.1 or ::1) did not work, as WordPress loopback requests use the public IP address of the web server itself. Luckily comparing server_addr with $remote_addr offers a robust and elegant solution to this issue. At first I had to use volatile in some of the maps, but now it works without it. Apparently the caching works out correctly in the current configuration. In practice this should result in zero performance difference, even at very high loads.

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.

2 participants