Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

Merged
merged 6 commits into from
May 30, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions aws/resource_aws_s3_bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/arn"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/hashicorp/errwrap"
"github.com/hashicorp/terraform/helper/hashcode"
Expand Down Expand Up @@ -51,6 +52,11 @@ func resourceAwsS3Bucket() *schema.Resource {
Computed: true,
},

"bucket_regional_domain_name": {
Type: schema.TypeString,
Computed: true,
},

"arn": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -1078,6 +1084,13 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error {
return err
}

// Add the bucket_regional_domain_name as an attribute
regionalEndpoint, err := BucketRegionalDomainName(d.Get("bucket").(string), region)
if err != nil {
return err
}
d.Set("bucket_regional_domain_name", regionalEndpoint)

// Add the hosted zone ID for this bucket's region as an attribute
hostedZoneID, err := HostedZoneIDForRegion(region)
if err != nil {
Expand Down Expand Up @@ -1441,6 +1454,23 @@ func bucketDomainName(bucket string) string {
return fmt.Sprintf("%s.s3.amazonaws.com", bucket)
}

func BucketRegionalDomainName(bucket string, region string) (string, error) {
// https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region

for _, partition := range endpoints.DefaultPartitions() {
for _, reg := range partition.Regions() {
if region == reg.ID() {
regionEndpointS3, err := reg.ResolveEndpoint(endpoints.S3ServiceID)
if err != nil {
return "", err
}
return fmt.Sprintf("%s.%s", bucket, strings.TrimPrefix(regionEndpointS3.URL, "https://")), nil
}
}
}
return "", fmt.Errorf("Regional endpoint not found for bucket %s", bucket)
}

func WebsiteEndpoint(bucket string, region string) *S3Website {
domain := WebsiteDomainUrl(region)
return &S3Website{Endpoint: fmt.Sprintf("%s.%s", bucket, domain), Domain: domain}
Expand Down
60 changes: 60 additions & 0 deletions aws/resource_aws_s3_bucket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import (
"bytes"
"encoding/json"
"fmt"
"math/rand"
"reflect"
"regexp"
"strconv"
"testing"
"text/template"
"time"

"github.com/hashicorp/terraform/helper/acctest"
"github.com/hashicorp/terraform/helper/resource"
Expand All @@ -18,6 +20,7 @@ import (

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/endpoints"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/hashicorp/terraform/helper/schema"
)
Expand Down Expand Up @@ -126,6 +129,28 @@ func TestAccAWSS3Bucket_region(t *testing.T) {
})
}

func TestAccAWSS3Bucket_bucketRegionalDomainName(t *testing.T) {
rInt := acctest.RandInt()
region := testAccFindRandomRegion()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSS3BucketDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSS3BucketConfigRegionalDomainName(rInt, region),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSS3BucketExists("aws_s3_bucket.bucket"),
resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "region", region),
resource.TestCheckResourceAttr(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other PR with a Computed: true attribute that is always available, this can be moved as a single check under the _basic test.

We also explicitly need the S3 testing to be able to run from AWS Commercial, AWS GovCloud (US), and AWS China which means that we cannot hard code a specific partition region. I believe there is a testAccProviderRegion() helper function which can aid us here.

If we want to ensure that specific regions are returning their expected domain names (e.g. cn-north-1 actually returning BUCKET.s3.cn-north-1.amazonaws.com.cn) I would highly recommend setting up unit testing for bucketRegionalDomainName as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I need to create separate test config, I used separate acceptance test TestAccAWSS3Bucket_bucketRegionalDomainName. I couldn't find testAccProviderRegion(), so I used the same endpoints package to choose region.

"aws_s3_bucket.bucket", "bucket_regional_domain_name", testAccBucketRegionalDomainName(rInt, region)),
),
},
},
})
}

func TestAccAWSS3Bucket_acceleration(t *testing.T) {
rInt := acctest.RandInt()

Expand Down Expand Up @@ -1331,6 +1356,26 @@ func testAccBucketDomainName(randInt int) string {
return fmt.Sprintf("tf-test-bucket-%d.s3.amazonaws.com", randInt)
}

func testAccFindRandomRegion() string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally in our acceptance testing we do not want to insert randomness as it leads to inconsistent testing behavior that makes troubleshooting harder than it needs to be.

Aside from that, as written this currently will enumerate a list of regions across all AWS partitions (Commercial, GovCloud (US), and China) while the testing credentials are only valid for one partition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

regions := []string{}
for _, partition := range endpoints.DefaultPartitions() {
for k, _ := range partition.Regions() {
regions = append(regions, k)
}
}
rand.Seed(time.Now().Unix())
return regions[rand.Intn(len(regions))]
}

func testAccBucketRegionalDomainName(randInt int, region string) string {
bucket := fmt.Sprintf("tf-test-bucket-%d", randInt)
regionalEndpoint, err := BucketRegionalDomainName(bucket, region)
if err != nil {
return fmt.Sprintf("Regional endpoint not found for bucket %s", bucket)
}
return regionalEndpoint
}

func testAccWebsiteEndpoint(randInt int) string {
return fmt.Sprintf("tf-test-bucket-%d.s3-website-us-west-2.amazonaws.com", randInt)
}
Expand Down Expand Up @@ -1431,6 +1476,21 @@ resource "aws_s3_bucket" "bucket" {
`, randInt)
}

func testAccAWSS3BucketConfigRegionalDomainName(randInt int, region string) string {
return fmt.Sprintf(`
provider "aws" {
alias = "custom"
region = "%s"
}

resource "aws_s3_bucket" "bucket" {
provider = "aws.custom"
bucket = "tf-test-bucket-%d"
region = "%s"
}
`, region, randInt, region)
}

func testAccAWSS3BucketWebsiteConfig(randInt int) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "bucket" {
Expand Down
2 changes: 1 addition & 1 deletion website/docs/r/cloudfront_distribution.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ resource "aws_s3_bucket" "b" {

resource "aws_cloudfront_distribution" "s3_distribution" {
origin {
domain_name = "${aws_s3_bucket.b.bucket_domain_name}"
domain_name = "${aws_s3_bucket.b.bucket_regional_domain_name}"
origin_id = "myS3Origin"

s3_origin_config {
Expand Down
1 change: 1 addition & 0 deletions website/docs/r/s3_bucket.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ The following attributes are exported:
* `id` - The name of the bucket.
* `arn` - The ARN of the bucket. Will be of format `arn:aws:s3:::bucketname`.
* `bucket_domain_name` - The bucket domain name. Will be of format `bucketname.s3.amazonaws.com`.
* `bucket_regional_domain_name` - The bucket region-specific domain name. The bucket domain name including the region name, please refer [here](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region) for format. Note: The AWS CloudFront allows specifying S3 region-specific endpoint when creating S3 origin, it will prevent [redirect issues](https://forums.aws.amazon.com/thread.jspa?threadID=216814) from CloudFront to S3 Origin URL.
* `hosted_zone_id` - The [Route 53 Hosted Zone ID](https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_website_region_endpoints) for this bucket's region.
* `region` - The AWS region this bucket resides in.
* `website_endpoint` - The website endpoint, if the bucket is configured with a website. If not, this will be an empty string.
Expand Down