Problem/Motivation

Without the CNAME alias (CNAME checkbox in the UI) enabled, actionable S3 asset URIs take the form https://[S3origin]/bucketname/objectpath, where bucketname is the value of Default Bucket Name as specified through the UI. With CNAME aliasing enabled, actionable S3 asset URIs take the form https://[CNAMEAlias]/bucketname/objectpath, disallowing CNAME aliasing that provides the effect of including the bucketname in the alias https://[CNAMEAlias]/objectpath, where CNAME alias is formatted per the Amazon S3 documentation.

The basis for using such a CNAME alias is described in the S3 documentation at http://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html#VirtualHostingCustomURLs, which describes the scenario in which the bucketname and CNAME alias are intentionally made the same (say, images.example.net), with the alias target being declared as images.example.net.s3.amazonaws.com--that is, as [bucketname].[S3Origin], in which the value of [S3Origin] is appropriate to the region.

Proposed resolution

If using a CNAME alias merely as a synonym for S3 origin's FQDN is a valid use case AND using a CNAME alias all the way down to [S3Origin]/bucketname] is a valid use case, add in association with Enable CNAME a Use Bucket Name checkbox, the ON state of which enacts the first case, and the OFF state of enacts the second case. There is no workaround to the current code's hard inclusion of bucketname in asset URIs with CNAME aliasing enabled.

Remaining tasks

User interface changes

API changes

Data model changes

Original report by [username]

CommentFileSizeAuthor
#15 2508469-currently-failing-test.patch488 bytesIsland Usurper
#15 2508469-amazons3-cname-url-dedup.patch4.05 KBIsland Usurper
#12 2508469-amazons3-bucketname-cname-12.patch6.71 KBElijah Lynn
#10 2508469-amazons3-bucketname-cname-10.patch6.75 KBElijah Lynn
#7 2508469-amazons3-cname-bucketname-7.patch2.92 KBElijah Lynn
#7 interdiff-1-6.txt5.55 KBElijah Lynn
#6 2508469-amazons3-cname-bucketname-6.patch3.53 KBElijah Lynn
#4 2508469-amazons3-cname-bucketname-1.patch3.23 KBElijah Lynn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
Issue summary: View changes
dpnewkirk’s picture

dpnewkirk CreditAttribution: dpnewkirk commented
Issue summary: View changes
dpnewkirk’s picture

dpnewkirk CreditAttribution: dpnewkirk commented
Issue summary: View changes
Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
Status: Active » Needs review
FileSize
2508469-amazons3-cname-bucketname-1.patch3.23 KB

Here is a patch that assumes the CNAME record is setup as Amazon suggests. This means if you decide to use CNAME it will no longer prepend the bucket name to the path and it will just be http://bucketname.s3.amazonaws.com/objectpath.

This breaks the ability to use per field buckets so I disabled that for now with a #access = FALSE.

Also, this passes all tests locally but I didn't write a test for it. Will be happy to write a test with a little guidance.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot for NBCUniversal commented
Status: Needs review » Needs work 
  1. +++ b/src/StreamWrapper.php
@@ -245,7 +245,9 @@ class StreamWrapper extends \Aws\S3\StreamWrapper implements \DrupalStreamWrappe
-    // @codeCoverageIgnoreEnd

    This looks like we now have an ignore start marker without and end marker? 

  2. +++ b/src/StreamWrapper.php
@@ -500,6 +502,25 @@ class StreamWrapper extends \Aws\S3\StreamWrapper implements \DrupalStreamWrappe
+    $url = Url::factory($url);

    Why do we need to re-create the URL object? 

  3. +++ b/src/StreamWrapperConfiguration.php
@@ -264,6 +265,22 @@ class StreamWrapperConfiguration extends Collection {
+    return $this->data['cname'];

    This looks like it's actually returning a string?

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
Status: Needs work » Needs review
FileSize
2508469-amazons3-cname-bucketname-6.patch3.53 KB

Good catches! The reason why I called ::factory() again was because I just copied above from getCloudFrontUrl(). I didn't need to do that. Fixed all three.

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
FileSize
interdiff-1-6.txt5.55 KB
2508469-amazons3-cname-bucketname-7.patch2.92 KB

Whoops, ignore #6, forgot to use --relative.

Also, I can't make an interdiff right now, keep getting a weird error "1 out of 3 hunks FAILED -- saving rejects to file /tmp/user/1000/interdiff-1.H7wRIv.rej │-rw------- 1 root root 3.3K Jul 6 13:08 interdiff-2.st8hTK
interdiff: Error applying patch1 to reconstructed file "

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented

An somehow I attached my earlier interdiff to #7, so ignore that too. I am on a roll today ;D!!

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
FileSize
2508469-amazons3-bucketname-cname-10.patch6.75 KB

New patch that is on top of/includes #2514674: When using CNAME, make URI scheme configurable.

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
Related issues: +#2514674: When using CNAME, make URI scheme configurable
Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn at NBCUniversal commented
FileSize
2508469-amazons3-bucketname-cname-12.patch6.71 KB

Whoops, a couple of extra params in getCnameUrl() left over from a previous experiment in #10. Here is one without those unused params.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch at Lullabot commented

It's been ages here, but I think this needs a reroll now that I've merged #2514674: When using CNAME, make URI scheme configurable?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch at Lullabot commented
Status: Needs review » Needs work

I also think that we'll need new unit tests to cover this CNAME functionality.

Island Usurper’s picture

Island Usurper CreditAttribution: Island Usurper commented
Version: 7.x-2.0 » 7.x-2.x-dev
Status: Needs work » Needs review
FileSize
2508469-amazons3-cname-url-dedup.patch4.05 KB
2508469-currently-failing-test.patch488 bytes

Reapplied the patch to work on latest dev, and added a couple of minor tests. Not sure if they are enough, but the one in StreamWrapperTest.php fails without the patch, so I've added an extra patch that only applies that test. I suspect that all of the tests that check the host should also check the path for the expected values.

Note: the test-only patch doesn't use 'cname' => TRUE, in the config because I was lazy and didn't want to recreate it, but it also doesn't make a difference that the test fails. However, with the way the fix works, setting cname is necessary to get the test to pass.