Added optional regional s3 endpoint. Added Example #17

Jamie-BitFlight · 2018-05-30T10:49:09Z

what

Add support for Regional Endpoint

…ated working example.

…e destroyed if required.

kitsunde · 2018-05-30T11:56:18Z

variables.tf

+variable "use_regional_s3_endpoint" {
+  type        = "string"
+  description = "When set to 'true' the s3 origin_bucket will use the regional endpoint address instead of the global endpoint address"
+  default     = "false"


Is this just to not introduce a change? Since the regional endpoint is an improvement shouldn't this be true always?

Yeah. Backwards compatibility and non-breaking changes are good.

But the README can be updated to say that they should prefer it.

kitsunde · 2018-05-30T14:18:54Z

It is backwards compatible though right since all that should happen is the origin on CF gets modified in place?

________________________________ From: Jamie Nelson <[email protected]> Sent: Wednesday, May 30, 2018 10:14:10 PM To: cloudposse/terraform-aws-cloudfront-s3-cdn Cc: Kit Sunde; Comment Subject: Re: [cloudposse/terraform-aws-cloudfront-s3-cdn] Added optional regional s3 endpoint. Added Example (#17) @Jamie-BitFlight commented on this pull request.

________________________________ In variables.tf<#17 (comment)>:

@@ -46,6 +46,12 @@ variable "aliases" {

default = [] } +variable "use_regional_s3_endpoint" { + type = "string" + description = "When set to 'true' the s3 origin_bucket will use the regional endpoint address instead of the global endpoint address" + default = "false" Yeah. Backwards compatibility and non-breaking changes are good. But the README can be updated to say that they should prefer it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#17 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAC_RT5dccXeYed2QMYto8DPayfVNiJ8ks5t3qkygaJpZM4UTCl0>.

Jamie-BitFlight · 2018-05-30T14:23:51Z

It is. Lets see what @osterman and @aknysh think about it.

aknysh · 2018-05-31T14:50:22Z

README.md

@@ -85,6 +86,7 @@ This is a fundamental requirement of CloudFront, and you will need to request th
 | `origin_path`                  | ``                     | Element that causes CloudFront to request your content from a directory in your Amazon S3 bucket. Begins with `/`. CAUTION! Do not use bare `/` as `origin_path`. | No       |
 | `parent_zone_id`               | ``                     | ID of the hosted zone to contain this record  (or specify `parent_zone_name`)                                                                                     | Yes      |
 | `parent_zone_name`             | ``                     | Name of the hosted zone to contain this record (or specify `parent_zone_id`)                                                                                      | Yes      |
+| `use_regional_s3_endpoint`     | `"false"`              | Use a regional endpoint for the bucket instead of the global endpoint. Useful for speeding up the deploy process.                                                 | No       |


Useful to reduce data latency

aknysh · 2018-05-31T14:51:13Z

README.md

+
+## Workaround for Known Issues
+
+To use the regional endpoint name instead of the global bucket name in this module, then set `use_regional_s3_endpoint = "true"` in the module.


To use the regional endpoint name instead of the global bucket name in this module, set use_regional_s3_endpoint = "true"

aknysh · 2018-05-31T14:51:57Z

example/main.tf

@@ -0,0 +1,23 @@
+resource "aws_route53_zone" "primary" {
+  name          = "cloudposse-example.com"


example.cloudposse.com

aknysh · 2018-05-31T14:52:31Z

example/main.tf

+  namespace                = "cp"
+  stage                    = "dev"
+  name                     = "app-cdn"
+  aliases                  = ["assets.cloudposse-example.com"]


assets.example.cloudposse.com

aknysh · 2018-05-31T14:54:07Z

example/main.tf

+  origin_force_destroy     = "true"
+}
+
+resource "aws_s3_bucket_object" "index" {


this is a good idea to put the HTML file into the bucket as an example

aknysh · 2018-05-31T15:09:53Z

main.tf

-    bucket             = "${element(compact(concat(list(var.origin_bucket), aws_s3_bucket.origin.*.bucket)), 0)}"
-    bucket_domain_name = "${format(var.bucket_domain_format, element(compact(concat(list(var.origin_bucket), aws_s3_bucket.origin.*.bucket)), 0))}"
-  }
+# ec2-downloads-windows is a bucket owned by amazon that will perminantly exist


hmm...
not sure i like it for a few reasons:

The ec2-downloads-windows bucket can exist now, but can be changed later by AWS

Probably not a good idea to introduce this dependency - we'll have to maintain it

It's related to Windows (not a big deal, we just work with Linux :)

Data sources can introduce issue like race conditions etc.

From the code below, looks like that all we need from aws_s3_bucket.selected is the region. I suggest we provide the region in var.region as we do for many other modules. And remove data "aws_region" "current" {}. In this case, this module will not make any decisions on its own about regions and other data sources. That will be done in top-level modules, and we know the region we want to deploy to.

@osterman what do you think about this?

I think I'll be able to chime in once I understand the primary driver for using it.

The region has to match where the bucket is. Or it doesn't work.

During destruction if var.bucket is "" and the s3 bucket that would otherwise be created has already been destroyed. But for some reason terraform hits any other error during the destruction process. Then local.bucket can be left being "". This causes a following terraform destroy to fail because the data.aws_s3_bucket doesn't have a bucket to provide a region for.
While testing the module for each variable, i applied and destroyed tens of times, sometimes I got errors which I fixed. And thats how this workaround came to be.
If there is no bucket, then temporarily use a permanent aws public bucket during the destruction step.

aknysh

looks good, just a few comments
thanks guys

osterman · 2018-05-31T17:38:15Z

main.tf

+# ec2-downloads-windows is a bucket owned by amazon that will perminantly exist
+# It allows for the data source to be called during the destruction process without failing.
+# It doesn't get used for anything else.
+data "aws_s3_bucket" "selected" {


@Jamie-BitFlight can you explain the trick you're using?

osterman · 2018-06-26T20:39:12Z

main.tf

+# It allows for the data source to be called during the destruction process without failing.
+# It doesn't get used for anything else.
+data "aws_s3_bucket" "selected" {
+  bucket = "${local.bucket == "" ? "ec2-downloads-windows" : local.bucket}"


What if we created a new variable and called it permanent_s3_bucket that had a description of what you have above. Then we can reference permanent_s3_bucket everywhere we need the hack. I think this would make the hack very clear.

Maybe use the aws-cli bucket? That would maybe raise less eyebrows. https://docs.aws.amazon.com/cli/latest/userguide/awscli-install-bundle.html

osterman

I think I'm okay with the hack, just I think we should codify it in a variable name so that it's very clear what the symbolism of ec2-downloads-windows is for. Also, maybe use the aws-cli bucket instead.

CR addressed

Jamie Nelson added 2 commits May 30, 2018 11:42

Added the ability to specify a regional endpoint. Updated README. Cre…

4c9e59d

…ated working example.

Added output in example that provided instructions for beginners

405119f

Jamie-BitFlight mentioned this pull request May 30, 2018

Support S3 regional redirect (new AWS feature) #16

Closed

Jamie Nelson added 2 commits May 30, 2018 12:30

Added origin_force_destroy to all s3 buckets so that the module can b…

b154fda

…e destroyed if required.

Added origin_force_destroy to all s3 buckets so that the module can b…

356adc5

…e destroyed if required.

kitsunde reviewed May 30, 2018

View reviewed changes

Fixed a another destruction issue.

f08d511

osterman requested a review from aknysh May 31, 2018 04:14

osterman assigned aknysh May 31, 2018

osterman added enhancement New feature or request feature request labels May 31, 2018

aknysh reviewed May 31, 2018

View reviewed changes

aknysh previously requested changes May 31, 2018

View reviewed changes

osterman reviewed May 31, 2018

View reviewed changes

Jamie Nelson added 2 commits May 31, 2018 19:05

Updated for consistency

ed6e780

Updated for consistency

e1f842e

osterman reviewed Jun 26, 2018

View reviewed changes

osterman requested changes Jun 26, 2018

View reviewed changes

Jamie Nelson added 3 commits June 26, 2018 21:51

Changed the 'static' bucket

a4434ef

added variable for static_s3_bucket

3e7377f

added variable for static_s3_bucket

5263899

osterman approved these changes Jun 26, 2018

View reviewed changes

osterman merged commit 3e299e3 into cloudposse:master Jun 26, 2018

This was referenced Mar 27, 2021

BucketRegionError: incorrect region #101

Closed

Remove static bucket hack #143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added optional regional s3 endpoint. Added Example #17

Added optional regional s3 endpoint. Added Example #17

Jamie-BitFlight commented May 30, 2018 •

edited by osterman

Loading

kitsunde May 30, 2018

Jamie-BitFlight May 30, 2018

kitsunde commented May 30, 2018 via email

Jamie-BitFlight commented May 30, 2018

aknysh May 31, 2018

aknysh May 31, 2018

aknysh May 31, 2018

aknysh May 31, 2018

aknysh May 31, 2018

aknysh May 31, 2018

osterman May 31, 2018

Jamie-BitFlight May 31, 2018

aknysh left a comment

osterman May 31, 2018

osterman Jun 26, 2018

osterman Jun 26, 2018

osterman left a comment


		## Workaround for Known Issues

		To use the regional endpoint name instead of the global bucket name in this module, then set `use_regional_s3_endpoint = "true"` in the module.

		@@ -0,0 +1,23 @@
		resource "aws_route53_zone" "primary" {
		name = "cloudposse-example.com"

Added optional regional s3 endpoint. Added Example #17

Added optional regional s3 endpoint. Added Example #17

Conversation

Jamie-BitFlight commented May 30, 2018 • edited by osterman Loading

what

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsunde commented May 30, 2018 via email

Jamie-BitFlight commented May 30, 2018

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

Choose a reason for hiding this comment

aknysh 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

osterman left a comment

Choose a reason for hiding this comment

Jamie-BitFlight commented May 30, 2018 •

edited by osterman

Loading