Massive refactor of helix.vcl #7
Conversation
layouts/fastly/helix.vcl
Outdated
| unset req.http.Cookie; | ||
|
|
||
| # Sanitize user input. `urlencode` leaves alphanumeric and `-._~` | ||
| set req.http.X-Strain = regsuball(urlencode(req.http.Foo), {"%.."}, "_"); |
layouts/fastly/helix.vcl
Outdated
| /** | ||
| * Set the `X-From-Edge` header that the above sub checks. | ||
| * | ||
| * Should be called from the top of `vcl_recv`/`vcl_miss` |
There was a problem hiding this comment.
Function accesses bereq. Should this be vcl_miss/vcl_pass?
layouts/fastly/helix.vcl
Outdated
| if (!bereq.http.X-From-Edge) { | ||
| declare local var.data STRING; | ||
| set var.data = strftime({"%s"}, now) + "," + server.datacenter; | ||
| set bereq.http.Secure-From-Edge = |
| * | ||
| * Should be called from the top of `vcl_recv`/`vcl_miss` | ||
| */ | ||
| sub hlx_set_from_edge { |
There was a problem hiding this comment.
In fact, I'm not sure this is ever called?
There was a problem hiding this comment.
@drwilco can we delete this function?
There was a problem hiding this comment.
It's not superfluous, we just need to reference it.
I believe it should be called here:
helix-publish/layouts/fastly/helix.vcl
Line 870 in c9c4a0f
| * | ||
| * Should be called from the top of `vcl_recv`/`vcl_miss` | ||
| */ | ||
| sub hlx_set_from_edge { |
There was a problem hiding this comment.
@drwilco can we delete this function?
layouts/fastly/helix.vcl
Outdated
| unset req.http.Cookie; | ||
|
|
||
| # Sanitize user input. `urlencode` leaves alphanumeric and `-._~` | ||
| set req.http.X-Strain = regsuball(urlencode(req.http.Foo), {"%.."}, "_"); |
|
@tripodsan I spoke with @ejthurgo last week and he said that he wasn't entirely done with his review. |
| # object is in the cache. | ||
| # | ||
| # So `req.url` is what should be left after filtering query string parameters, | ||
| # doing normalizations, etc. Then `req.http.X-Backend-URL` is a transformation of |
There was a problem hiding this comment.
Is it possible that backend transformation could result in any normalization. and therefore cache efficiency?
There was a problem hiding this comment.
It shouldn't. All normalization should be done on req.url and req.http.Host.
| set req.http.X-Trace = req.http.X-Trace + "; vcl_miss"; | ||
| unset bereq.http.X-Orig-Url; | ||
| /** | ||
| * Do all BackEnd REQuest changes. One sub to be called from both vcl_miss and |
There was a problem hiding this comment.
I did that capitalization to explain where bereq comes from :)
| set bereq.http.host = "adobeioruntime.net"; | ||
| } elsif (req.backend == F_GitHub) { | ||
| set bereq.http.host = "raw.githubusercontent.com"; | ||
| set bereq.http.Host = req.http.X-Orig-Host; |
There was a problem hiding this comment.
we stash the original host in req.http.X-Orig-Host, however, we never actually change req.http.host
Is this risk management against future changes to host in vcl_recv, or just an oversight/relic from an older version.
There was a problem hiding this comment.
Risk management vs future changes
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This started out as a refactor to fix the handling of redirects from runtime for static files that are larger than the limit. It then quickly evolved to reworking how restarts are done, which is documented in the large comment at the top.
Other things were fixed along the way, mostly related, so not split out into separate changes:
vcl_missandvcl_passX-URLandX-Orig-URLX-StatictoX-Request-TypeImageout into a separate request type