Skip to content
This repository has been archived by the owner on Dec 31, 2022. It is now read-only.

Use URI form of proxy_pass instead of rewrite #3

Merged
merged 1 commit into from
Jun 16, 2019

Conversation

twifkak
Copy link
Contributor

@twifkak twifkak commented Jun 6, 2019

I believe this fixes an issue where a request for e.g. /index%2ehtml would get proxied as /priv/doc/https://server/index.html, and hence fail to adhere to Google's SXG cache requirements.

If it's easy for you to verify this change on your own stack, I would appreciate it. I've made a similar change on my nginx server, but I'm just hand-editing the config file here, so I may have messed something up. If it's not easy, let me know, and I'll try do so on my own machine. Thanks!

I believe this fixes an issue where a request for e.g. `/index%2ehtml` would get proxied as `/priv/doc/https://server/index.html`, and hence fail to adhere to [Google's SXG cache requirements](https://github.com/ampproject/amppackager/blob/releases/docs/cache_requirements.md).
@Warashi
Copy link
Owner

Warashi commented Jun 16, 2019

Thank you.

Certainly there is a problem you wrote, and this fixes it.
So I merge this.

By the way, I misread the meaning of amppackager's README?

If the URL points to an AMP page and the AMP-Cache-Transform request header is present, rewrite the URL by prepending /priv/doc and forward the request.

https://github.com/ampproject/amppackager/tree/fadd61811d76ebee041ca0707fc36a7f82de0ab7#productionizing

@Warashi Warashi merged commit 9e1aa20 into Warashi:master Jun 16, 2019
@twifkak
Copy link
Contributor Author

twifkak commented Jun 17, 2019

Thanks for the feedback! I only discovered this quirk in nginx's rewrite directive 12 days ago. I will think about how to improve the README here. I don't know what's the best method in e.g. Apache or Varnish, but I can point out the general issue, and call out this one known nginx issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants