feat: Upgrade S3 module to support AWS Provider 4.x #138

robwittman · 2022-03-07T17:38:16Z

Description

The release of 4.x for the AWS provider brings a large number of breaking changes for the S3 bucket configuration. This PR uses the new explicit resources for various configuration options, while limiting changes on the module interface.

Motivation and Context

These changes are needed to support the 4.x and newer versions of the AWS provider. This module is currently locked on the 3.x branch until these are completed

Works to resolve #137

Breaking Changes

There are a few breaking changes around the interface itself, as well as the structure of some of the variables.

I will update with documentation around those soon.

How Has This Been Tested?

I have tested and validated these changes using one or more of the provided examples/* projects
I have edited the examples/complete workspace with the few backwards incompatible changes. I'm able to create the various s3 buckets, and the configuration I've checked looks good

We'll also be updating some of our own modules to deploy and test in a non-production environment

jeohist

I was working on this as well, had a few additions / comments. Great to see this PR!

I'm wondering whether this might be the time to get rid of var.create_bucket as well? With count in modules since 0.13 and moved blocks making the transition easier, as well as this upgrade requiring lots of state file surgery anyway... Could make things a bit easier to maintain.

jeohist · 2022-03-07T21:30:57Z

examples/complete/main.tf

  source = "../../"

  bucket = "cloudfront-logs-${random_pet.this.id}"
  acl    = null # conflicts with default of `acl = "private"` so set to null to use grants


There's no more default value, so I think it's safe to remove this now.

jeohist · 2022-03-07T21:32:09Z

variables.tf

 }

-variable "grant" {
+variable "access_control_policy" {


We should update the description of var.acl too :)

jeohist · 2022-03-07T21:33:57Z

main.tf

+# TODO: This should accept both ACL and Grant, exclusively
+resource "aws_s3_bucket_acl" "this" {


I think we can accomplish this with this hacky bit of code (my kingdom for an exclusive OR, HashiCorp...):

# If ACL and ACP: 0, if ACL or ACP: 1, if ACL nor ACP: 0 count = (var.acl != "null" && var.acl != null) && length(keys(try(jsondecode(var.access_control_policy), var.access_control_policy))) > 0 ? 0 : (var.acl != "null" && var.acl != null) || length(keys(try(jsondecode(var.access_control_policy), var.access_control_policy))) > 0 ? 1 : 0

From my understanding, aws_s3_bucket_acl still supports both acl and access_control_policy, just not at the same time. Would just using something like

count = (var.acl != null || var.access_control_policy != "") ? 1 : 0

get the expectedc result?

jeohist · 2022-03-07T21:35:22Z

main.tf

+    content {
+      allowed_methods = cors_rule.value.allowed_methods
+      allowed_origins = cors_rule.value.allowed_origins
+      allowed_headers = lookup(cors_rule.value, "allowed_headers", null)
+      expose_headers  = lookup(cors_rule.value, "expose_headers", null)
+      max_age_seconds = lookup(cors_rule.value, "max_age_seconds", null)
+    }


An optional id attribute is now supported, might be easy to add:

id = lookup(cors_rule.value, "id", null)

jeohist · 2022-03-07T21:37:43Z

main.tf

+      dynamic "filter" {
+        for_each = length(keys(lookup(rule.value, "filter", {}))) == 0 ? [] : [lookup(rule.value, "filter")]
+
+        content {


Maybe we can also add the newly added optional attributes?

object_size_greater_than = lookup(filter.value, "object_size_greater_than", null) object_size_less_than = lookup(filter.value, "object_size_less_than", null) prefix = lookup(filter.value, "prefix", null)

I'd be delighted to, though I'd like to get feedback on their usage of the and block for predicates. For each attribute that can conditionally end up in that and { } segment, the check across the different filters gets more complex. Could we hoist those to an evaluation in locals maybe? Is there a straightforward way to allow passing the desired filters from outside the module instead?

robwittman · 2022-03-07T22:23:36Z

I was working on this as well, had a few additions / comments. Great to see this PR!

I'm wondering whether this might be the time to get rid of var.create_bucket as well? With count in modules since 0.13 and moved blocks making the transition easier, as well as this upgrade requiring lots of state file surgery anyway... Could make things a bit easier to maintain.

I don't know that I have enough background info into what use-cases var.create_bucket is used for. It feels like something that could be removed, but I can't back that up at all :)

Regarding state surgery, their documentation says that since most of the new resources are PUT operations, they should mostly be no-ops, and one could safely upgrade directly to these new resources. They recommend importing where possible, but for simpler use cases, I don't think much state management will be needed

robwittman · 2022-03-08T15:24:10Z

Since this PR is already causing breaking changes, I went ahead and removed var.create_bucket as well. Any requirements around documentation for breaking changes and upgrades to support them?

antonbabenko · 2022-03-08T15:31:05Z

Hi guys! I have finished 90% of this update already by the 24th of February but then Russia invaded Ukraine and my life has changed 180 degrees.

Let me push what I have made and open a PR, hopefully, tonight, and someone can take a look and finish the remainings there (upgrade guide, proofread docs, verify examples).

🇺🇦

rblaine95 · 2022-03-09T15:16:44Z

I'm experimenting with this branch and it's been working really well so far!

I'm just having one issue when trying to set a lifecycle rule that automatically expires objects after 180 days and, for some reason, the days parameter keeps being set to 0

lifecycle_rule = [{
  id      = "Retain logs for 6 months"
  enabled = true
  expiration = {
    days = 180
  }
}]

  # module.example_s3_logs.aws_s3_bucket_lifecycle_configuration.this[0] will be updated in-place
  ~ resource "aws_s3_bucket_lifecycle_configuration" "this" {
        id     = "example-logs"
        # (1 unchanged attribute hidden)

      ~ rule {
            id     = "Retain logs for 6 months"
            status = "Enabled"

          ~ expiration {
              ~ days                         = 180 -> 0
                # (1 unchanged attribute hidden)
            }

            # (1 unchanged block hidden)
        }
        # (1 unchanged block hidden)
    }

Maybe I'm missing something stupid, but I can't seem to figure out if/what I'm doing wrong here 🤔

robwittman · 2022-03-09T18:19:00Z

@rblaine95 If you want to pull the latest commit, there was an issue with one of the lookups that should be fixed now. Wether or not expiration.days was set, the expiration block was getting set to for_each=[]

rblaine95 · 2022-03-10T07:02:26Z

@robwittman just tested, working perfectly!

antonbabenko · 2022-03-30T13:44:29Z

This issue has been resolved in version 3.0.0 🎉

github-actions · 2022-10-28T02:42:04Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

robwittman added 2 commits March 7, 2022 12:33

Upgrade S3 module to support AWS Provider 4.x

18f2deb

Apply complete example to test

fe50d04

robwittman changed the title ~~Upgrade S3 module to support AWS Provider 4.x~~ feat: Upgrade S3 module to support AWS Provider 4.x Mar 7, 2022

robwittman marked this pull request as ready for review March 7, 2022 20:21

Revert var.website to map(string)

c7950f2

jeohist reviewed Mar 7, 2022

View reviewed changes

robwittman added 2 commits March 7, 2022 17:35

Address feedback

33cbc55

Remove var.create_bucket

d834ded

Make aws_s3_bucket_acl conditional

d2a56eb

Make access_control_policy a string, fix ACL count check

c9698f6

antonbabenko mentioned this pull request Mar 8, 2022

feat!: Update to support AWS provider v3.75 and newer (including v4.x) #139

Merged

Fix evaluation of lifecycle.rule.expiration

76ad336

antonbabenko mentioned this pull request Mar 30, 2022

Argument is deprecated #142

Closed

antonbabenko closed this in #139 Mar 30, 2022

github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2022

		# TODO: This should accept both ACL and Grant, exclusively
		resource "aws_s3_bucket_acl" "this" {

Uh oh!

feat: Upgrade S3 module to support AWS Provider 4.x #138

feat: Upgrade S3 module to support AWS Provider 4.x #138

Uh oh!

Conversation

robwittman commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

Uh oh!

jeohist left a comment

Choose a reason for hiding this comment

Uh oh!

jeohist Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeohist Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeohist Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

robwittman Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeohist Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jeohist Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

robwittman Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

robwittman commented Mar 7, 2022

Uh oh!

robwittman commented Mar 8, 2022

Uh oh!

antonbabenko commented Mar 8, 2022

Uh oh!

rblaine95 commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robwittman commented Mar 9, 2022

Uh oh!

rblaine95 commented Mar 10, 2022

Uh oh!

antonbabenko commented Mar 30, 2022

Uh oh!

github-actions bot commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robwittman commented Mar 7, 2022 •

edited

Loading

rblaine95 commented Mar 9, 2022 •

edited

Loading