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

feat: support bucket_key_enabled for server_side_encryption_configura… #82

Merged

Conversation

kumashun8
Copy link
Contributor

@kumashun8 kumashun8 commented Apr 13, 2021

…tion.rule

Description

Make server_side_encryption_configuration to be able to set to bucket_key_enabled .

Motivation and Context

Recently, bucket_key_enable was supported by terraform-provider-aws in the following PR.
so I'd like to reflect that here as well.

hashicorp/terraform-provider-aws#16581

Breaking Changes

bucket_key_enabled is a optional value, so compatibility won't be lost.

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects

current plan at example/complete result is below (extracted).

+ server_side_encryption_configuration {
          + rule {
              + bucket_key_enabled = false

              + apply_server_side_encryption_by_default {
                  + kms_master_key_id = (known after apply)
                  + sse_algorithm     = "aws:kms"
                }
            }
        }

and apply was done successfully (extracted).

...
Apply complete! Resources: 11 added, 0 changed, 0 destroyed.

Outputs:

this_s3_bucket_arn = "arn:aws:s3:::s3-bucket-mighty-tetra"
this_s3_bucket_bucket_domain_name = "s3-bucket-mighty-tetra.s3.amazonaws.com"
this_s3_bucket_bucket_regional_domain_name = "s3-bucket-mighty-tetra.s3.ap-northeast-1.amazonaws.com"
this_s3_bucket_hosted_zone_id = "Z2M4EHUR26P7ZW"
this_s3_bucket_id = "s3-bucket-mighty-tetra"
this_s3_bucket_region = "ap-northeast-1"
this_s3_bucket_website_domain = "s3-website-ap-northeast-1.amazonaws.com"
this_s3_bucket_website_endpoint = "s3-bucket-mighty-tetra.s3-website-ap-northeast-1.amazonaws.com"

next, edit example/complete/main.tf line:191-199 to add variable bucket_key_enabled like this.

server_side_encryption_configuration = {
    rule = {
      bucket_key_enabled = true
      apply_server_side_encryption_by_default = {
        kms_master_key_id = aws_kms_key.objects.arn
        sse_algorithm     = "aws:kms"
      }
    }
  }

plan result is below (extracted).

# module.s3_bucket.aws_s3_bucket.this[0] will be updated in-place
  ~ resource "aws_s3_bucket" "this" {
        id                          = "s3-bucket-mighty-tetra"
        tags                        = {
            "Owner" = "Anton"
        }
        # (11 unchanged attributes hidden)





      ~ server_side_encryption_configuration {
          ~ rule {
              ~ bucket_key_enabled = false -> true

                # (1 unchanged block hidden)
            }
        }


        # (8 unchanged blocks hidden)
    }

  # module.s3_bucket.aws_s3_bucket_policy.this[0] will be updated in-place
  ~ resource "aws_s3_bucket_policy" "this" {
        id     = "s3-bucket-mighty-tetra"
      ~ policy = jsonencode(
            {
              - Statement = [
                  - {
                      - Action    = "s3:*"
                      - Condition = {
                          - Bool = {
                              - aws:SecureTransport = "false"
                            }
                        }
                      - Effect    = "Deny"
                      - Principal = "*"
                      - Resource  = [
                          - "arn:aws:s3:::s3-bucket-mighty-tetra/*",
                          - "arn:aws:s3:::s3-bucket-mighty-tetra",
                        ]
                      - Sid       = "denyInsecureTransport"
                    },
                  - {
                      - Action    = "s3:ListBucket"
                      - Effect    = "Allow"
                      - Principal = {
                          - AWS = "arn:aws:iam::057575985710:role/terraform-20210413081143894800000001"
                        }
                      - Resource  = "arn:aws:s3:::s3-bucket-mighty-tetra"
                      - Sid       = ""
                    },
                ]
              - Version   = "2012-10-17"
            }
        ) -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 2 to change, 0 to destroy.

and apply complete successfully (extracted).

...
Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

Outputs:

this_s3_bucket_arn = "arn:aws:s3:::s3-bucket-mighty-tetra"
this_s3_bucket_bucket_domain_name = "s3-bucket-mighty-tetra.s3.amazonaws.com"
this_s3_bucket_bucket_regional_domain_name = "s3-bucket-mighty-tetra.s3.ap-northeast-1.amazonaws.com"
this_s3_bucket_hosted_zone_id = "Z2M4EHUR26P7ZW"
this_s3_bucket_id = "s3-bucket-mighty-tetra"
this_s3_bucket_region = "ap-northeast-1"
this_s3_bucket_website_domain = "s3-website-ap-northeast-1.amazonaws.com"
this_s3_bucket_website_endpoint = "s3-bucket-mighty-tetra.s3-website-ap-northeast-1.amazonaws.com"

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Please update versions.tf with a minimum version of Terraform AWS provider 3.36.0 in the module and in the examples.

Also, could you please add the same change to modules/object?

Thank you!

main.tf Outdated
@@ -196,6 +196,7 @@ resource "aws_s3_bucket" "this" {
for_each = length(keys(lookup(server_side_encryption_configuration.value, "rule", {}))) == 0 ? [] : [lookup(server_side_encryption_configuration.value, "rule", {})]

content {
bucket_key_enabled = lookup(rule.value, "bucket_key_enabled", false)
Copy link
Member

Choose a reason for hiding this comment

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

Leave the default value as null, please.

@kumashun8 kumashun8 requested a review from antonbabenko April 13, 2021 11:30
@@ -196,6 +196,7 @@ resource "aws_s3_bucket" "this" {
for_each = length(keys(lookup(server_side_encryption_configuration.value, "rule", {}))) == 0 ? [] : [lookup(server_side_encryption_configuration.value, "rule", {})]

content {
bucket_key_enabled = lookup(rule.value, "bucket_key_enabled", null)
Copy link
Member

Choose a reason for hiding this comment

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

Please do a similar update to the object submodule also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I misread the contents of the first review.
I'll do it.

@kumashun8 kumashun8 force-pushed the support-s3-bucket-key branch from 2aab7ac to d128396 Compare April 28, 2021 03:57
@kumashun8 kumashun8 requested a review from antonbabenko April 28, 2021 04:27
@antonbabenko antonbabenko merged commit a41d75f into terraform-aws-modules:master Apr 28, 2021
@antonbabenko
Copy link
Member

Thanks, @kumashun8 !

v2.1.0 has been just released.

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants