Allow different CDN domain when using HTTPS [#1267902]

Comment	File	Size	Author
#21	1267902-21.patch	4.81 KB	wim leers
#19	cdn.https_3.patch	6.83 KB	andreiashu
#18	cdn.https_2.patch	5.58 KB	andreiashu
#17	cdn.https_1.patch	5.85 KB	andreiashu
#16	cdn.https_.patch	5.71 KB	andreiashu

Comment #1

wim leers

Ghent 🇧🇪🇪🇺

commented 7 September 2011 at 15:06

Status:

Active

» Postponed (maintainer needs more info)

I think this is a problem only if you want to use your own vanity domain name as an alias for the cloudfront domain name, no? Then why not just always use the cloudfront domain name?

Log in or register to post comments

Comment #2

wim leers

Ghent 🇧🇪🇪🇺

commented 7 September 2011 at 15:37

Oh, I just noticed this was documented at #1047182: Document that HTTPS breaks if one uses a vanity subdomain (CNAME) for a CDN that supports HTTPS requests. It's documented in the admin interface, but only in the dev snapshot, not yet in a release. That will change soon.

Log in or register to post comments

Comment #3

bendodd commented 7 September 2011 at 19:42

I use vanity domains so that both the code looks better with more connections are to servers people know and trust, but mostly to allow multiple domains to be used, and allow more simultaneous downloads: http://code.google.com/speed/page-speed/docs/rtt.html#ParallelizeDownloads

Log in or register to post comments

Comment #4

bendodd commented 7 September 2011 at 19:43

...this is not possible though for https, as cloudfront only provides one domain

Log in or register to post comments

Comment #5

wim leers

Ghent 🇧🇪🇪🇺

commented 7 September 2011 at 20:23

Status:

Postponed (maintainer needs more info)

» Active

I thought I'd read somewhere that you'd need unique IP addresses for that too work (meaning that what Google states there, and what many others state, is in fact wrong), but I can't find it. I guess I must have imagined that.

Your use case is obviously valid.

Obviously, I welcome patches. Secondly, I'm wondering what you propose in terms of UI?

Log in or register to post comments

Comment #6

bendodd commented 7 September 2011 at 20:30

Haven't really thought about the UI, but I'm guessing we need another 'CDN mapping' box (CDN HTTPS mapping?). But there is something odd about having to go to the following tab to indicate support for HTTPS. Perhaps we could move the 'CDN supports HTTPS' select to the previous/same tab?

Log in or register to post comments

Comment #7

wim leers

Ghent 🇧🇪🇪🇺

commented 8 September 2011 at 10:51

I know, but the problem is that most people don't need this, and hence we provide them with a boatload of settings they don't need…

Log in or register to post comments

Comment #8

bendodd commented 8 September 2011 at 12:54

I guess we only need to show the textarea if the https checkbox is checked? Or move to a sub-module?

Log in or register to post comments

Comment #9

bendodd commented 12 December 2011 at 17:06

Initial ideas for functionality here: https://github.com/bendodd/drupal-6-cdn/tree/help_https

Just need to move these changes to it's own patch...

Log in or register to post comments

Comment #10

wim leers

Ghent 🇧🇪🇪🇺

commented 18 January 2012 at 08:38

I just replied to #1204400: Supply different CDN hosts for HTTPS, which is strongly related to this.

Log in or register to post comments

Comment #11

bendodd commented 19 January 2012 at 11:13

We are working on a patch for this today and will hopefully have it to test tomorrow. It will use a checkbox to reveal a second textarea if needed:
http://drupal.org/files/cdn_admin_details.png

Log in or register to post comments

Comment #12

wim leers

Ghent 🇧🇪🇪🇺

commented 19 January 2012 at 12:14

I think this should be at admin/settings/other; there it could appear below the "CDN supports HTTPS" setting. Doesn't that also make more sense to you?

Log in or register to post comments

Comment #13

andreiashu commented 19 January 2012 at 12:59

@wim re #11, #12: how about having both HTTPS related options on the details page? I think it would make more sense to have the "CDN supports HTTPS" option show the "Select different mappings for HTTPS" option which in turn, when checked, will show the text area with mappings for https?

Log in or register to post comments

Comment #14

andreiashu commented 19 January 2012 at 13:01

I forgot to mention that I think they would be more useful on the details page because then you would have both http and https mappings on the same screen.

Log in or register to post comments

Comment #15

wim leers

Ghent 🇧🇪🇪🇺

commented 19 January 2012 at 13:40

#13, #14: Yet HTTPS mappings are rarely necessary. The UI is optimized for the common use case.

Log in or register to post comments

Comment #16

andreiashu commented 19 January 2012 at 14:08

Status:

Active

» Needs review

Status	File	Size
new	cdn.https_.patch	5.71 KB

I took bendodd's code, organised it a bit and now we have a patch.
I still need to move the options around in the admin settings area but I'd like a quick review from you guys.

Log in or register to post comments

Comment #17

andreiashu commented 19 January 2012 at 16:40

Status	File	Size
new	cdn.https_1.patch	5.85 KB

New patch attached which:
- has all the HTTPS related settings on the "other" tab;
- I got rid of the CDN_BASIC_MAPPING_HTTPS_ENABLED_VARIABLE var. We have one less variable_get(set) - on the details page we don't have a checkbox for the normal CDN mappings

Log in or register to post comments

Comment #18

andreiashu commented 19 January 2012 at 16:46

Status	File	Size
new	cdn.https_2.patch	5.58 KB

Minor amend to the previous patch so that it doesn't include whitespace change around '_cdn_ufi_deployment_id' function.

Log in or register to post comments

Comment #19

andreiashu commented 23 January 2012 at 10:59

Status	File	Size
new	cdn.https_3.patch	6.83 KB

Rerolled against latest 6.x-2.x branch

Log in or register to post comments

Comment #20

andreiashu commented 27 January 2012 at 18:19

bump

Log in or register to post comments

Comment #21

wim leers

Ghent 🇧🇪🇪🇺

commented 29 January 2012 at 13:47

Status	File	Size
new	1267902-21.patch	4.81 KB

Thanks for the patch! It contained *a lot* of flaws though.

- it contained changes from previous patches; this forced me to apply all of your changes manually
- the two new public functions you added to the .module file had weird logic checks
- cdn_get_domains() needs the *raw* mappings, but you're passing it the *parsed* mappings through the new cdn_basic_get_mapping() function you've introduced

Reroll attached. Against 6.x-2.x-dev.

Log in or register to post comments

Comment #22

andreiashu commented 29 January 2012 at 14:18

Hi Wim,

Thanks for re-rolling and fixing it. I'll give it a try on Monday.

Log in or register to post comments

Comment #23

andreiashu commented 30 January 2012 at 08:55

Status:

Needs review

» Reviewed & tested by the community

Works for me: it picks up the files from the https CDN when necessary.

Log in or register to post comments

Comment #24

bendodd commented 30 January 2012 at 09:45

With a fresh GIT checkout of the dev branch and this patch applied, I get:

Fatal error: Call to undefined function _cdn_requirements_get_integration_mechanism() in
#######/sites/all/modules/contributed/cdn/cdn.admin.inc on line 20
I have also confirm this on a fresh installation at Pantheon (let me know if you want access to the codebase/admin):
http://dev.comicrelief-test.gotpantheon.com

Fatal error: Call to undefined function _cdn_requirements_get_integration_mechanism() in /srv/bindings/2c15305bc7cd4bed8527316bb254d667/code/sites/all/modules/contrib/cdn/cdn.admin.inc on line 20