Currently the CDN module will always complain that your server is not a perfect domain if you try to use far future expiration. Not a huge deal, but it took me a lot of time to figure out if it was a real problem or not, so it's worth fixing.

It took me three shots to kill this bird. I'm proposing 1 patch and 1 documentation update and 1 question ;):

1. The CDN will NOT cache the cdn_basic_farfuture_reverseproxy_test page callback reply because that callback doesn't override the drupal core default "no-cache", etc Cache-Control headers like the real farfuture callback does. I copied the relevant lines and cloudfront cached the reply. Patch coming

2. In my case, my installation is forwarding all http requests to https. In that case, amazon's "behavior" settings should be set to also redirect at the edge, otherwise caching will end up bypassed (and the annoying message will come up too :P). This should be documented. In general, there's no up-to-date document on the front page on what to set those settings to. Here's what I *think* it should say:
- viewer protocol policy: if you're redirecting http requests to https before hitting drupal (e.g. using aegir or your own .htaccess rule), set this to "redirect". Otherwise, match your site's policy.
- forward headers: None
- object caching: Use Origin Headers (to allow far future expiration)
- forward query strings: I don't know :P - I couldn't find a clear answer
- smooth streaming: I don't think so
- restrict user access: I don't think so
- compress objects automatically: I don't know

3. The SEO stuff also breaks this, because it again sends a redirect instead of a 200. The simplest (hackiest) way to handle this would be to just add a /file.txt to the request and move on with life, but if someone adds .txt to the extension list it would again break. That or we'd have to detect this router path. I won't submit a patch without feedback first, but I'm leaning toward hacky ;).

CommentFileSizeAuthor
#19 cdn-partial_rollback_of_reverseproxy_fix-2647830-19.patch461 bytescaptainack
#10 interdiff.txt1.12 KBWim Leers
#10 cdn-reverseproxy_test_callback_doesnt_cache-2647830-10.patch1.82 KBWim Leers
#2 cdn-reverseproxy_test_callback_doesnt_cache-2647830-2.patch653 bytescaptainack
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

captainack created an issue. See original summary.

captainack’s picture

captainack CreditAttribution: captainack commented
Status: Active » Needs review
FileSize
cdn-reverseproxy_test_callback_doesnt_cache-2647830-2.patch653 bytes

Patch for item 1.

captainack’s picture

captainack CreditAttribution: captainack commented
Title: uses the same web server as this Drupal site - CDN never perfect » farfuture, SEO, and "domain uses the same web server as this Drupal site" error message
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs review » Postponed (maintainer needs more info) 
+++ b/cdn.basic.farfuture.inc
@@ -164,6 +164,17 @@ function cdn_basic_farfuture_reverseproxy_test($token) {
+  header("Cache-Control: max-age=290304000, no-transform, public");
+  header("Expires: Tue, 20 Jan 2037 04:20:42 GMT");
+  header("Last-Modified: Wed, 20 Jan 1988 04:20:42 GMT");
+
   print REQUEST_TIME . '-' . md5(rand());

This is contradictory. We want this to NOT be cached (since we're printing the request time), yet the caching headers say the opposite.

The CDN will NOT cache the cdn_basic_farfuture_reverseproxy_test page callback reply because that callback doesn't override the drupal core default "no-cache", etc Cache-Control headers like the real farfuture callback does.

Like I said above, we specifically do not want this to be cached. Can you explain your reasoning for why this should be cached?

captainack’s picture

captainack CreditAttribution: captainack commented

So I'll focus on #1 first.

In cdn_admin_details_form_validate(), two requests are made, and they are compared. The form replies with "...is a CDN or a reverse proxy" ONLY if the two timestamp responses match. If you don't cache them, they'll never match (you'll get two different request times), and the admin form will always complain.

As I was reverse engineering the purpose of that, I figured you probably printed the request times specifically so you could ensure that the second request IS a cached version of the first. Did I guess wrong?

At any rate, the admin form currently complains that CloudFront isn't a CDN, and after reading your article defining CDN, I'm pretty sure it is ;). If I misunderstood your purpose for that callback, the fix is different.

Thanks a million again.

captainack’s picture

captainack CreditAttribution: captainack commented
Status: Postponed (maintainer needs more info) » Active
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Priority: Normal » Major

That… is totally and absolutely sensible. Thanks for taking the time to write that!

Clearly I've done a poor job properly documenting this, since #4 clearly shows I've managed to confuse myself, using my own code.

Thank you a million! :)

captainack’s picture

captainack CreditAttribution: captainack commented

Haha. It's okay. It took me some time to figure out how it was working, but once I did I thought "Oh, that's really clever!" Plus having to read through the CDN module code made me appreciate it even more!

So good point about the documentation - would you like me to add some comments to a reroll of the patch to make it a little clearer?

Also, do you have some guidance on how you'd prefer the fix for (3) from the description?

Thanks again!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Related issues: +#1060358: CDN and SEO

I'm glad you weren't frustrated by this then :)

So good point about the documentation - would you like me to add some comments to a reroll of the patch to make it a little clearer?

That would be splendid!

3. The SEO stuff also breaks this, because it again sends a redirect instead of a 200. The simplest (hackiest) way to handle this would be to just add a /file.txt to the request and move on with life, but if someone adds .txt to the extension list it would again break. That or we'd have to detect this router path. I won't submit a patch without feedback first, but I'm leaning toward hacky ;).

D'oh! I think /file.txt is fine (blacklisting *.txt would be a rare edge case).

Thanks so much!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Active » Needs review
FileSize
cdn-reverseproxy_test_callback_doesnt_cache-2647830-10.patch1.82 KB
interdiff.txt1.12 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs review » Fixed

Thanks again!

(I rerolled myself & committed it to be able to move forward faster, I wanted to tag 2.7-rc1… finally!)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Related issues: +#1886484: What is the problem with "Potentially problematic domains"

And this also allowed me to close #1886484: What is the problem with "Potentially problematic domains", yay.

captainack’s picture

captainack CreditAttribution: captainack commented

Awesome! Sorry you beat me to it! :-!

Thanks again!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

In return, please let me know whether everything is working correctly after updating to 2.7 :)

captainack’s picture

captainack CreditAttribution: captainack commented

2.7?! :D Awesome!

Ha! That's a great deal!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

captainack’s picture

captainack CreditAttribution: captainack commented
Status: Closed (fixed) » Needs work

I tested 2.7 and it looks like 2.7's farfuture is now broken :(.

It looks like it's because this patch had some accidental residue. Specifically, the hook_menu change to the farfuture router path. Now the farfuture callback gets the first directory in the path instead of the full path.

captainack’s picture

captainack CreditAttribution: captainack commented
Status: Needs work » Needs review
FileSize
cdn-partial_rollback_of_reverseproxy_fix-2647830-19.patch461 bytes
captainack’s picture

captainack CreditAttribution: captainack commented

Also... As promised... I tested it and the original problem looks to be fixed with your txt file thing :).

captainack’s picture

captainack CreditAttribution: captainack commented

Hi Wim,

This was tested by two other people over at #2667494: Follow-up for #2647830: version 2.7 broke Far Future expiration. Unfortunately the original reporter of that one (and someone who is affiliated with them I think) couldn't figure out how to patch it, but I think this will close that one too if it gets committed.

Thanks.

  • Wim Leers committed f5d0d74 on 7.x-2.x authored by captainack 
    Issue #2667494 by captainack, Oleksiy, muldrow, fgm: Follow-up for #...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs review » Closed (fixed)

Committed #2667494: Follow-up for #2647830: version 2.7 broke Far Future expiration.