Copy Accept-Encoding normalisation from EC2 Varnish #379
Conversation
vcl_templates/www.vcl.erb
Outdated
| @@ -202,6 +202,22 @@ sub vcl_recv { | |||
| } | |||
| <% end %> | |||
|
|
|||
| # Normalise Accept-Encoding header | |||
There was a problem hiding this comment.
Fastly already does some similar-looking stuff as standard - have you checked that this isn't replicating the snippet that Fastly adds itself? (if you view the full VCL in the Fastly UI, you should see an extra stanza or two of VCL that isn't coming from our templates - I think it's in there).
https://developer.fastly.com/reference/http/http-headers/Accept-Encoding/#normalization
There was a problem hiding this comment.
Ah, I wasn't aware of that. We can keep the bit that strips the header if the content is already compressed, but I don't know if that's worth bothering with
There was a problem hiding this comment.
Ah, sorry - I was partly mistaken - the config I was thinking of is actually ours.
Leads me to another question though, which is: have you tested that this doesn't break Brotli?
There was a problem hiding this comment.
I still have a hunch that we're duplicating Fastly functionality but I'm less sure. It's important that we don't break Brotli though, since that would be a regression.
There was a problem hiding this comment.
Sorry yes, this does seem to be replicating what fastly is already doing. I copied this directly from the EC2 Varnish VCL file, so we can definitely get rid of the bits that check for gzip and deflate. We can probably keep the bit that checks if the content is already compressed, though.
The config that I copied and that which Fastly already does shouldn't work with brotli at all, but it shouldn't break anything. We're manipulating a request header here before it gets to a backend server that performs the compression, so if a browser says it accepts brotli, the server will never see that and will only serve an uncompressed file or one done with gzip/deflate
There was a problem hiding this comment.
Oh, are you saying the Brotli support has never worked? 😅
There was a problem hiding this comment.
As far as I can tell, that should never have worked because an Accept-Encoding: br header would be stripped out in EC2's Varnish: https://github.com/alphagov/govuk-puppet/blob/56c3da336aadb68d7dfbf6b0db2e748b18221bbc/modules/varnish/templates/default.vcl.erb#L49-L63
There was a problem hiding this comment.
I've updated this PR to only remove the Accept-Encoding header if the content is already compressed. The functionality Fastly provides should do the rest.
There was a problem hiding this comment.
I'm pretty sure Brotli worked before. https://trello.com/c/u9qiWQr1/13-3-enable-brotli-compression-in-fastly-for-production
Bear in mind that it's Fastly that serves the Brotli-compressed objects, not us. So as long as we're not breaking the header in vcl_recv (i.e. the procedure that runs before Fastly processes the incoming request) we should be ok. Please do test that we haven't broken it though.
There was a problem hiding this comment.
Actually now I'm not sure that I'm not just getting confused here. Anyway, please test for regressions! Code looks good to me.
samsimpson1
force-pushed
the
header_normalise
branch
2 times, most recently
from
691b6dd to
a3e7619
Compare
samsimpson1
force-pushed
the
header_normalise
branch
from
a3e7619 to
c5ba0e9
Compare
This is copied from the EC2-based Varnish config. This is required for replatforming work, since we will be retiring the EC2-based Varnish services.
I have tested it using Fastly Fiddle.
This is integration-only at this point to test before deploying everywhere.
Trello: https://trello.com/c/88bU7Qmm/854-normalise-accept-encoding-header-at-fastly