Make S3 website private; add S3 origin #105

jmcgeheeiv · 2020-10-15T20:06:08Z

What

In this PR I added these enhancements:

Make the S3 website accessible only via CloudFront. You may prefer that I add a variable to make this optional. Let me know.
Add S3 origins, which are different from the currently-supported custom origins
Change http_port and https_port to numbers merely to dissuade observers from attempting to use null. null causes a very, very hard-to-diagnose error that does not reveal the line number.

I will further note that Terraform insists that the complete data structure appearing in variables.tf be specified for custom_origin and s3_origin variables. Thus lookup() with default values is immaterial.

Why

The reasons are included in the list of enhancements above. I hope it is more readable that way.

jmcgeheeiv · 2020-10-25T02:31:45Z

As I reported in #107, when using an S3 website origin, redirection from subdir/ to subdir/index.html works only when aliases is specified. I added a notation to that effect in the website_enabled documentation.

I am tempted to go further and make aliases a required variable, but that would break backward compatibility.

mergify · 2021-01-23T15:26:04Z

This pull request is now in conflict. Could you fix it @jmcgeheeiv? 🙏

nitrocode · 2021-03-26T14:43:40Z

@jmcgeheeiv can you fix the merge conflicts ?

Also, it looks like PR #108 put in the following

terraform-aws-cloudfront-s3-cdn/main.tf

Lines 249 to 255 in 0818b34

    
           dynamic "custom_header" { 
        
             for_each = var.custom_origin_headers 
        
             content { 
        
               name  = custom_header.value["name"] 
        
               value = custom_header.value["value"] 
        
             } 
        
           }

How would that work with this PR ? does that PR make this one no longer relevant ?

main.tf

nitrocode · 2021-03-27T22:50:33Z

/test all

mergify · 2021-03-28T16:38:49Z

This pull request is now in conflict. Could you fix it @jmcgeheeiv? 🙏

jmcgeheeiv · 2021-03-31T03:15:07Z

Okay @nitrocode, the code is tested with Terraform 0.13 and 0.14. The changes are in a single commit. I hope you can get this through before too many more commits come through.

nitrocode · 2021-03-31T03:29:24Z

/test all

nitrocode · 2021-03-31T03:30:37Z

It's not up to me. I'm not one of the approvers or in the engineering group. You'll have to be vocal in #pr-reviews channel to get this pr merged

nitrocode · 2021-03-31T03:40:59Z

Please fix the tests. You need to add the random provider with a version pin

mergify · 2021-03-31T05:32:04Z

This pull request is now in conflict. Could you fix it @jmcgeheeiv? 🙏

nitrocode · 2021-03-31T12:04:58Z

There are also a lot of prs going in and a recent bug i introduced. Let's get all the other stuff merged and come back to this one and then fix the conflicts. Otherwise you'll be driving yourself up a wall like me after every merge

jmcgeheeiv · 2021-03-31T20:43:16Z

Okay, holding until I hear from you, @nitrocode.

nitrocode · 2021-03-31T21:41:42Z

The bug was fixed in a now closed PR. Feel free to correct tests and fix conflicts. After you do, I'd post in #pr-reviews in slack to get attention to it.

Gowiem · 2021-04-07T22:49:17Z

/test all

Gowiem · 2021-04-07T23:01:10Z

@jmcgeheeiv looks like this one is close, but two things outstanding:

Bats tests are failing on the provider not being pinned properly:

not ok 6 check if terraform providers are properly pinned
# (in test file test/.test-harness/test/terraform/provider-pinning.bats, line 23)
#   `terraform-config-inspect --json . | jq '.required_providers | to_entries[] | "  - \(.key)|\(.value.version_constraints[])"' > $TMPFILE' failed with status 5
# /usr/local/bin/terraform-config-inspect
# jq: error (at <stdin>:1197): Cannot iterate over null (null)
not ok 7 check if terraform providers have explicit source locations for TF =>0.13
# (in test file test/.test-harness/test/terraform/provider-pinning.bats, line 55)
#   `exit 1' failed
# /usr/local/bin/terraform-config-inspect
# 
# * Cloud Posse requires all providers to use registry format introduced in Terraform 0.13, for example
#     aws = {
#        source  = "hashicorp/aws"
#        version = ">= 3.0"
#     }
# 
# * Please add constraints for these providers:
#   - random

You've got merge conflicts in the README. They seem easily so should be quick to knock out.

Get those updated and we'll give this a final pass and move it forward 🤙

* Add variable to specify S3 origins * URL subdir/ forwards to subdir/index.html (no Lambda function required!) * S3 website origin made private: CloudFront sends "referer" custom header to S3 website S3 website demands referer custom header * Merge my custom_headers with that added in #108 * In docs, warn the user to specify aliases with website_enabled * Change http_port and https_port from null to numbers. null causes a very, very hard-to-diagnose error

* Provide better description for var.origin_bucket * Auto Format * Update based on comments * Auto Format * Add example of reusing an s3 bucket * Auto Format * Correct naming order * Auto Format * Auto Format Co-authored-by: cloudpossebot <[email protected]>

Gowiem · 2021-04-16T20:39:31Z

@jmcgeheeiv something got gunked up with your PR / commits, so now we're seeing a changeset from a number of recent PRs instead of just your changes.

Sorry for this but can you do something along the following lines to get things back in order?

Check out a fresh origin/master
Create new branch
cherry-pick your commits onto that new branch
rename your old branch
name your new branch like your branch
force-push your new branch to your forked master

Feel free to go about that differently if your git-fu is better than mine (not hard obviously).

jmcgeheeiv · 2021-04-16T21:05:22Z

I force-pushed a better version.

@Gowiem I think I see the error I made. Last time, I did something similar to the six step procedure you describe. However, the local working master branch remains out of sync with origin/master. Therefore your procedure and I both need to remember that at the end we also need to update the local master in preparation for the next time:

git checkout master
git pull --force origin master

While I am certain there is a more elegant way to do this, this is what worked.

Gowiem · 2021-04-17T15:08:09Z

/test all

Gowiem

LGTM 👍

Gowiem · 2021-04-17T15:30:39Z

@jmcgeheeiv these bats tests are still failing -- Mind looking into them?

not ok 6 check if terraform providers are properly pinned
# (in test file test/.test-harness/test/terraform/provider-pinning.bats, line 23)
#   `terraform-config-inspect --json . | jq '.required_providers | to_entries[] | "  - \(.key)|\(.value.version_constraints[])"' > $TMPFILE' failed with status 5
# /usr/local/bin/terraform-config-inspect
# jq: error (at <stdin>:1175): Cannot iterate over null (null)
not ok 7 check if terraform providers have explicit source locations for TF =>0.13
# (in test file test/.test-harness/test/terraform/provider-pinning.bats, line 55)
#   `exit 1' failed
# /usr/local/bin/terraform-config-inspect
# 
# * Cloud Posse requires all providers to use registry format introduced in Terraform 0.13, for example
#     aws = {
#        source  = "hashicorp/aws"
#        version = ">= 3.0"
#     }
# 
# * Please add constraints for these providers:
#   - random
# 
#

nitrocode · 2021-04-21T15:27:39Z

/test all

* Add support for query_string_cache_keys Fixes #156 Co-authored-by: cloudpossebot <[email protected]>

jmcgeheeiv · 2021-04-21T18:57:05Z

/test all

Gowiem · 2021-04-21T21:17:34Z

/test all

jmcgeheeiv · 2021-04-21T21:36:35Z

@Gowiem, it seems that chatops /test all works only for CloudPosse personnel. Is this true?

Gowiem · 2021-04-21T21:49:41Z

@jmcgeheeiv that's correct 😄 Leads to some confusion, but I believe it's done because Cloud Posse is spending money on an AWS bill out of its own pocket each time tests are run so they like to guard the fact that it could potentially be abused and they'd end up paying the full bill.

Gowiem

I believe this looks good, but I want to get a second set of eyes on it as well before we call it merged since it's a bigger PR. @nitrocode you mind giving a second approval?

jmcgeheeiv · 2021-04-21T21:52:14Z

Quick, before cloudpossebot changes its mind!

jmcgeheeiv · 2021-04-21T22:37:49Z

@Gowiem, yes, I was thinking that since /test all is not free, not everyone should be able to do it. Quite reasonable.

jmcgeheeiv · 2021-04-26T18:28:39Z

@3h4x @woz5999 please review while it's clean and green!

woz5999 · 2021-04-26T20:59:34Z

the code changes lgtm but i've not super familiar with the implications of the changes, so i'd prefer to punt the actual merge to someone else.

Gowiem · 2021-04-26T23:26:14Z

@woz5999 haha I'm in the same boat and wanted another contributor to help me share the merge burden due to the large set of changes, but I shouldn't try to put that on others. I'll merge and deal with the fallout if this blows up this popular module. @jmcgeheeiv I'll be pinging you if that happens 😉

Gowiem · 2021-04-26T23:27:23Z

Thanks for the contribution @jmcgeheeiv (and working through our fun hoops on this one in particular)! Released as 0.61.0!

jmcgeheeiv requested a review from a team as a code owner October 15, 2020 20:06

jmcgeheeiv requested review from 3h4x and woz5999 and removed request for a team October 15, 2020 20:06

jmcgeheeiv mentioned this pull request Oct 15, 2020

S3 website distributed by CloudFront does not redirect subdir/ to subdir/index.html #107

Closed

jmcgeheeiv requested review from a team as code owners January 28, 2021 20:28

nitrocode reviewed Mar 27, 2021

View reviewed changes

main.tf Show resolved Hide resolved

nitrocode reviewed Mar 27, 2021

View reviewed changes

main.tf Outdated Show resolved Hide resolved

nitrocode reviewed Mar 27, 2021

View reviewed changes

main.tf Outdated Show resolved Hide resolved

nitrocode previously approved these changes Mar 31, 2021

View reviewed changes

jmcgeheeiv dismissed nitrocode’s stale review via 1ffc95e April 16, 2021 20:07

mediatek-aiot and others added 3 commits April 16, 2021 13:16

Auto Format

bd7f388

Auto Format

c99bc6a

Auto Format

db8cc62

Gowiem previously approved these changes Apr 17, 2021

View reviewed changes

Pin version of provider hashicorp/random

9f1c8c0

jmcgeheeiv dismissed Gowiem’s stale review via 9f1c8c0 April 17, 2021 18:13

gozer and others added 3 commits April 21, 2021 11:51

Add support for query_string_cache_keys (#157)

6b9f9af

* Add support for query_string_cache_keys Fixes #156 Co-authored-by: cloudpossebot <[email protected]>

Don't touch generated README.md after make readme

be988a9

Auto Format

e1364ab

Gowiem approved these changes Apr 21, 2021

View reviewed changes

Gowiem merged commit d378e4b into cloudposse:master Apr 26, 2021

syphernl mentioned this pull request May 12, 2021

fix: only create referer password when needed #159

Closed

Nuru mentioned this pull request May 12, 2021

Add deployers #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make S3 website private; add S3 origin #105

Make S3 website private; add S3 origin #105

jmcgeheeiv commented Oct 15, 2020 •

edited

Loading

jmcgeheeiv commented Oct 25, 2020 •

edited

Loading

mergify bot commented Jan 23, 2021

nitrocode commented Mar 26, 2021

nitrocode commented Mar 27, 2021

mergify bot commented Mar 28, 2021

jmcgeheeiv commented Mar 31, 2021

nitrocode commented Mar 31, 2021

nitrocode commented Mar 31, 2021

nitrocode commented Mar 31, 2021

mergify bot commented Mar 31, 2021

nitrocode commented Mar 31, 2021

jmcgeheeiv commented Mar 31, 2021

nitrocode commented Mar 31, 2021

Gowiem commented Apr 7, 2021

Gowiem commented Apr 7, 2021

Gowiem commented Apr 16, 2021

jmcgeheeiv commented Apr 16, 2021 •

edited

Loading

Gowiem commented Apr 17, 2021

Gowiem left a comment

Gowiem commented Apr 17, 2021

nitrocode commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021

Gowiem commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021 •

edited

Loading

Gowiem commented Apr 21, 2021

Gowiem left a comment

jmcgeheeiv commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021

jmcgeheeiv commented Apr 26, 2021

woz5999 commented Apr 26, 2021

Gowiem commented Apr 26, 2021

Gowiem commented Apr 26, 2021

Make S3 website private; add S3 origin #105

Make S3 website private; add S3 origin #105

Conversation

jmcgeheeiv commented Oct 15, 2020 • edited Loading

What

Why

jmcgeheeiv commented Oct 25, 2020 • edited Loading

mergify bot commented Jan 23, 2021

nitrocode commented Mar 26, 2021

nitrocode commented Mar 27, 2021

mergify bot commented Mar 28, 2021

jmcgeheeiv commented Mar 31, 2021

nitrocode commented Mar 31, 2021

nitrocode commented Mar 31, 2021

nitrocode commented Mar 31, 2021

mergify bot commented Mar 31, 2021

nitrocode commented Mar 31, 2021

jmcgeheeiv commented Mar 31, 2021

nitrocode commented Mar 31, 2021

Gowiem commented Apr 7, 2021

Gowiem commented Apr 7, 2021

Gowiem commented Apr 16, 2021

jmcgeheeiv commented Apr 16, 2021 • edited Loading

Gowiem commented Apr 17, 2021

Gowiem left a comment

Choose a reason for hiding this comment

Gowiem commented Apr 17, 2021

nitrocode commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021

Gowiem commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021 • edited Loading

Gowiem commented Apr 21, 2021

Gowiem left a comment

Choose a reason for hiding this comment

jmcgeheeiv commented Apr 21, 2021

jmcgeheeiv commented Apr 21, 2021

jmcgeheeiv commented Apr 26, 2021

woz5999 commented Apr 26, 2021

Gowiem commented Apr 26, 2021

Gowiem commented Apr 26, 2021

jmcgeheeiv commented Oct 15, 2020 •

edited

Loading

jmcgeheeiv commented Oct 25, 2020 •

edited

Loading

jmcgeheeiv commented Apr 16, 2021 •

edited

Loading

jmcgeheeiv commented Apr 21, 2021 •

edited

Loading