Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

saravanan30erd · 2018-05-16T19:01:00Z

Added new attribute bucket_regional_domain_name in aws_s3_bucket.

bflad

Thanks for this @saravanan30erd -- please see the below and let us know if you have any questions.

bflad · 2018-05-24T13:57:16Z

aws/resource_aws_s3_bucket.go

@@ -1441,6 +1449,18 @@ func bucketDomainName(bucket string) string {
 return fmt.Sprintf("%s.s3.amazonaws.com", bucket)
 }

+func bucketRegionalDomainName(bucket string, region string) string {


The AWS Go SDK contains an endpoints package which can automatically retrieve this information.

Here's the endpoints data set: https:/aws/aws-sdk-go/blob/master/aws/endpoints/defaults.go

Theoretically we should be able to fetch the endpoint from the SDK package based on service (s3) and region. This should prevent future maintainability issues.

used endpoints package to retrieve region-specific endpoint.

bflad · 2018-05-24T14:03:14Z

aws/resource_aws_s3_bucket_test.go

+ Check: resource.ComposeTestCheckFunc(
+ testAccCheckAWSS3BucketExists("aws_s3_bucket.bucket"),
+ resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "region", "ap-south-1"),
+ resource.TestCheckResourceAttr(


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.

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.

bflad · 2018-05-24T14:05:51Z

website/docs/r/s3_bucket.html.markdown

@@ -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. Will be of format `bucketname.s3-eu-west-1.amazonaws.com`. 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.


Rather than specifying bucketname.s3-eu-west-1.amazonaws.com here we should probably instead point to the AWS regions and endpoints documentation mentioning the domain name including the region name.

saravanan30erd · 2018-05-25T08:47:46Z

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_bucketRegionalDomainName'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSS3Bucket_bucketRegionalDomainName -timeout 120m
=== RUN TestAccAWSS3Bucket_bucketRegionalDomainName
--- PASS: TestAccAWSS3Bucket_bucketRegionalDomainName (67.64s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 67.687s

bflad

Since I need to create separate test config

Can you explain why this is necessary? A new single check in the existing _basic test should work just fine as it applies to all bucket resource configurations. testAccGetRegion() has the region information you need and on master is already available in the _basic test function. I would rebase the test file (as it currently shows a merge conflict) and simply just add something like the below rather than creating a new test and configuration:

resource.TestCheckResourceAttr("aws_s3_bucket.bucket", "bucket_regional_domain_name", testAccBucketRegionalDomainName(rInt, region)),

bflad · 2018-05-25T12:44:17Z

aws/resource_aws_s3_bucket_test.go

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

+func testAccFindRandomRegion() string {


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.

saravanan30erd · 2018-05-25T13:45:02Z

@bflad since I couldn't find testAccProviderRegion(), I thought it's better to provide region as input in separate test config and test the same. Now testAccGetRegion() fixed all the issues.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSS3Bucket_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSS3Bucket_basic -timeout 120m
=== RUN TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (104.41s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 104.451s

reverting changes due to conflict

saravanan30erd · 2018-05-30T04:45:06Z

@bflad is there anything I can do to push this?

bflad · 2018-05-30T19:36:24Z

@saravanan30erd there is currently a merge conflict in aws/resource_aws_s3_bucket_test.go and I'd like to add some quick unit testing for BucketRegionalDomainName. Fixing these on merge.

bflad

LGTM after fixing up the merge conflict! I went ahead and refactored the function to use endpoints.DefaultResolver().EndpointFor() to simplify it a bit:

// https://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region
func BucketRegionalDomainName(bucket string, region string) (string, error) {
	// Return a default AWS Commercial domain name if no region is provided
	// Otherwise EndpointFor() will return BUCKET.s3..amazonaws.com
	if region == "" {
		return fmt.Sprintf("%s.s3.amazonaws.com", bucket), nil
	}
	endpoint, err := endpoints.DefaultResolver().EndpointFor(endpoints.S3ServiceID, region)
	if err != nil {
		return "", err
	}
	return fmt.Sprintf("%s.%s", bucket, strings.TrimPrefix(endpoint.URL, "https://")), nil
}

Added unit testing:

func TestBucketRegionalDomainName(t *testing.T) {
	const bucket = "bucket-name"

	var testCases = []struct {
		ExpectedErrCount int
		ExpectedOutput   string
		Region           string
	}{
		{
			Region:           "",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.amazonaws.com",
		},
		{
			Region:           "custom",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.custom.amazonaws.com",
		},
		{
			Region:           "us-east-1",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.amazonaws.com",
		},
		{
			Region:           "us-west-2",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.us-west-2.amazonaws.com",
		},
		{
			Region:           "us-gov-west-1",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.us-gov-west-1.amazonaws.com",
		},
		{
			Region:           "cn-north-1",
			ExpectedErrCount: 0,
			ExpectedOutput:   bucket + ".s3.cn-north-1.amazonaws.com.cn",
		},
	}

	for _, tc := range testCases {
		output, err := BucketRegionalDomainName(bucket, tc.Region)
		if tc.ExpectedErrCount == 0 && err != nil {
			t.Fatalf("expected %q not to trigger an error, received: %s", tc.Region, err)
		}
		if tc.ExpectedErrCount > 0 && err == nil {
			t.Fatalf("expected %q to trigger an error", tc.Region)
		}
		if output != tc.ExpectedOutput {
			t.Fatalf("expected %q, received %q", tc.ExpectedOutput, output)
		}
	}
}

Everything looks good!

make test TEST=./aws
==> Checking that code complies with gofmt requirements...
go test ./aws -timeout=30s -parallel=4
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.830s

47 tests passed (all tests)
=== RUN   TestAccAWSS3BucketObject_content
--- PASS: TestAccAWSS3BucketObject_content (10.81s)
=== RUN   TestAccAWSS3BucketObject_contentBase64
--- PASS: TestAccAWSS3BucketObject_contentBase64 (10.96s)
=== RUN   TestAccAWSS3Bucket_importBasic
--- PASS: TestAccAWSS3Bucket_importBasic (11.22s)
=== RUN   TestAccAWSS3BucketObject_withContentCharacteristics
--- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (11.60s)
=== RUN   TestAccAWSS3BucketObject_source
--- PASS: TestAccAWSS3BucketObject_source (18.25s)
=== RUN   TestAccAWSS3BucketNotification_withoutFilter
--- PASS: TestAccAWSS3BucketNotification_withoutFilter (18.53s)
=== RUN   TestAccAWSS3BucketObject_sse
--- PASS: TestAccAWSS3BucketObject_sse (25.37s)
=== RUN   TestAccAWSS3BucketObject_updatesWithVersioning
--- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (26.34s)
=== RUN   TestAccAWSS3BucketMetric_basic
--- PASS: TestAccAWSS3BucketMetric_basic (31.70s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndMultipleTags (31.75s)
=== RUN   TestAccAWSS3Bucket_importWithPolicy
--- PASS: TestAccAWSS3Bucket_importWithPolicy (33.41s)
=== RUN   TestAccAWSS3BucketObject_kms
--- PASS: TestAccAWSS3BucketObject_kms (36.69s)
=== RUN   TestAccAWSS3BucketObject_storageClass
--- PASS: TestAccAWSS3BucketObject_storageClass (25.96s)
=== RUN   TestAccAWSS3BucketObject_updates
--- PASS: TestAccAWSS3BucketObject_updates (42.09s)
=== RUN   TestAccAWSS3BucketObject_tags
--- PASS: TestAccAWSS3BucketObject_tags (32.45s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterMultipleTags
--- PASS: TestAccAWSS3BucketMetric_WithFilterMultipleTags (46.79s)
=== RUN   TestAccAWSS3Bucket_namePrefix
--- PASS: TestAccAWSS3Bucket_namePrefix (28.52s)
=== RUN   TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (32.95s)
=== RUN   TestAccAWSS3BucketPolicy_basic
--- PASS: TestAccAWSS3BucketPolicy_basic (41.27s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefix
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefix (56.61s)
=== RUN   TestAccAWSS3Bucket_region
--- PASS: TestAccAWSS3Bucket_region (35.26s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (15.82s)
=== RUN   TestAccAWSS3BucketObject_acl
--- PASS: TestAccAWSS3BucketObject_acl (64.20s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterSingleTag (71.44s)
=== RUN   TestAccAWSS3Bucket_shouldFailNotFound
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (32.14s)
=== RUN   TestAccAWSS3Bucket_WebsiteRoutingRules
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (42.44s)
=== RUN   TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag
--- PASS: TestAccAWSS3BucketMetric_WithFilterPrefixAndSingleTag (87.95s)
=== RUN   TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (40.57s)
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (53.76s)
=== RUN   TestAccAWSS3BucketNotification_basic
--- PASS: TestAccAWSS3BucketNotification_basic (102.26s)
=== RUN   TestAccAWSS3Bucket_Logging
--- PASS: TestAccAWSS3Bucket_Logging (49.05s)
=== RUN   TestAccAWSS3Bucket_UpdateAcl
--- PASS: TestAccAWSS3Bucket_UpdateAcl (75.75s)
=== RUN   TestAccAWSS3Bucket_Website_Simple
--- PASS: TestAccAWSS3Bucket_Website_Simple (75.79s)
=== RUN   TestAccAWSS3Bucket_RequestPayer
--- PASS: TestAccAWSS3Bucket_RequestPayer (84.60s)
=== RUN   TestAccAWSS3Bucket_Cors
--- PASS: TestAccAWSS3Bucket_Cors (57.91s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (34.39s)
=== RUN   TestAccAWSS3BucketNotification_importBasic
--- PASS: TestAccAWSS3BucketNotification_importBasic (122.50s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (51.77s)
=== RUN   TestAccAWSS3BucketPolicy_policyUpdate
--- PASS: TestAccAWSS3BucketPolicy_policyUpdate (129.19s)
=== RUN   TestAccAWSS3Bucket_Versioning
--- PASS: TestAccAWSS3Bucket_Versioning (97.72s)
=== RUN   TestAccAWSS3Bucket_Lifecycle
--- PASS: TestAccAWSS3Bucket_Lifecycle (94.98s)
=== RUN   TestAccAWSS3Bucket_Policy
--- PASS: TestAccAWSS3Bucket_Policy (135.04s)
=== RUN   TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (142.10s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (128.29s)
=== RUN   TestAccAWSS3Bucket_generatedName
--- PASS: TestAccAWSS3Bucket_generatedName (161.80s)
=== RUN   TestAccAWSS3Bucket_LifecycleExpireMarkerOnly
--- PASS: TestAccAWSS3Bucket_LifecycleExpireMarkerOnly (122.43s)
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (135.17s)

saravanan30erd · 2018-05-31T03:25:52Z

Cool. The endpoints.DefaultResolver().EndpointFor is really simplifying the logic, I should have checked that.

bflad · 2018-05-31T04:07:42Z

This has been released in version 1.21.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-05T17:45:31Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

saravanan30erd added 3 commits May 16, 2018 19:15

issue hashicorp#4540 add new attribute bucket_regional_domain_name

1021e48

issue hashicorp#4540 add acceptance test

e1bd1dc

issue hashicorp#4540 updated documents

9c9195d

ghost added the size/M Managed by automation to categorize the size of a PR. label May 16, 2018

saravanan30erd mentioned this pull request May 16, 2018

Add s3 domain names with region so cloudfront origins outside of us-east-1 won't cause redirects immediately after setup #4540

Closed

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/s3 Issues and PRs that pertain to the s3 service. labels May 16, 2018

kitsunde mentioned this pull request May 21, 2018

Support S3 regional redirect (new AWS feature) cloudposse/terraform-aws-cloudfront-s3-cdn#16

Closed

bflad mentioned this pull request May 21, 2018

Fixing s3 bucket domain name in non-commercial regions #4599

Closed

bflad reviewed May 24, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label May 24, 2018

issue hashicorp#4540 using endpoints package

95f1fba

ghost added the size/M Managed by automation to categorize the size of a PR. label May 24, 2018

issue hashicorp#4540 acceptance test corrections

bf516ae

bflad reviewed May 25, 2018

View reviewed changes

issue hashicorp#4540 corrections in acceptance test

97f375e

reverting changes due to conflict

saravanan30erd force-pushed the issue-4540 branch from 698a426 to 97f375e Compare May 25, 2018 13:51

ghost added the size/M Managed by automation to categorize the size of a PR. label May 25, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label May 30, 2018

bflad approved these changes May 30, 2018

View reviewed changes

bflad added this to the v1.21.0 milestone May 30, 2018

bflad merged commit 97f375e into hashicorp:master May 30, 2018

bflad added a commit that referenced this pull request May 30, 2018

Update CHANGELOG for #4556

3cf12ce

saravanan30erd deleted the issue-4540 branch May 31, 2018 03:25

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

saravanan30erd commented May 16, 2018

bflad left a comment

bflad May 24, 2018

saravanan30erd May 25, 2018

bflad May 24, 2018

saravanan30erd May 25, 2018

bflad May 24, 2018

saravanan30erd May 25, 2018

saravanan30erd commented May 25, 2018

bflad left a comment

bflad May 25, 2018

saravanan30erd May 25, 2018

saravanan30erd commented May 25, 2018

saravanan30erd commented May 30, 2018

bflad commented May 30, 2018

bflad left a comment

saravanan30erd commented May 31, 2018

bflad commented May 31, 2018

ghost commented Apr 5, 2020

Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

Issue #4540 Add new attribute bucket_regional_domain_name in aws_s3_bucket #4556

Conversation

saravanan30erd commented May 16, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saravanan30erd commented May 25, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saravanan30erd commented May 25, 2018

saravanan30erd commented May 30, 2018

bflad commented May 30, 2018

bflad left a comment

Choose a reason for hiding this comment

saravanan30erd commented May 31, 2018

bflad commented May 31, 2018

ghost commented Apr 5, 2020