s3 module terraform warnings refactor

s3/main.tf

@@ -1,13 +1,8 @@
 resource "aws_s3_bucket" "log_bucket" {
   count         = var.access_logs == true && var.apply_resource == true ? 1 : 0
-  acl           = "log-delivery-write"
   bucket        = "${local.bucket_name}-logs"
   force_destroy = var.force_destroy
 
-  versioning {
-    enabled = true
-  }
-
   tags = merge(
     var.common_tags,
     tomap(
@@ -16,6 +11,23 @@ resource "aws_s3_bucket" "log_bucket" {
   )
 }
 
+resource "aws_s3_bucket_versioning" "log_bucket_versioning" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket = aws_s3_bucket.log_bucket[0].id
+
+  versioning_configuration {
+    status = "Enabled"
+  }
+}
+
+resource "aws_s3_bucket_acl" "log_bucket_acl" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket = aws_s3_bucket.log_bucket[count.index].id
+  acl    = "log-delivery-write"
+}
+
 resource "aws_s3_bucket_server_side_encryption_configuration" "bucket_encryption" {
   bucket = local.s3_bucket_id
 
@@ -39,9 +51,13 @@ resource "aws_s3_bucket_public_access_block" "log_bucket" {
 }
 
 resource "aws_s3_bucket_policy" "log_bucket" {
-  count      = var.access_logs == true && var.apply_resource == true ? 1 : 0
-  bucket     = aws_s3_bucket.log_bucket.*.id[0]
-  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0] })
+  count  = var.access_logs == true && var.apply_resource == true ? 1 : 0
+  bucket = aws_s3_bucket.log_bucket.*.id[0]
+  policy = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl",
+    {
+      bucket_name           = aws_s3_bucket.log_bucket.*.id[0],
+      canonical_user_grants = jsonencode(var.canonical_user_grants)
+  })
   depends_on = [aws_s3_bucket_public_access_block.log_bucket]
 }
 
@@ -55,76 +71,82 @@ resource "aws_s3_bucket_notification" "log_bucket_notification" {
   }
   depends_on = [aws_s3_bucket_policy.log_bucket]
 }
-
+# This module is to be deprecated
 resource "aws_s3_bucket" "bucket" {
   count         = var.apply_resource == true ? 1 : 0
   bucket        = local.bucket_name
-  acl           = length(var.canonical_user_grants) == 0 ? var.acl : null
   force_destroy = var.force_destroy
+  tags = merge(
+    var.common_tags,
+    tomap(
+      { "Name" = local.bucket_name }
+    )
+  )
+}
 
-  dynamic "grant" {
-    for_each = var.canonical_user_grants
-    content {
-      permissions = grant.value.permissions
-      type        = "CanonicalUser"
-      id          = grant.value.id
-    }
-  }
+resource "aws_s3_bucket_versioning" "bucket_versioning" {
+  count = var.apply_resource == true && length(var.canonical_user_grants) == 0 ? 1 : 0
 
-  versioning {
-    enabled = var.versioning
-  }
+  bucket = aws_s3_bucket.bucket[0].id
 
-  dynamic "lifecycle_rule" {
-    for_each = var.abort_incomplete_uploads == true ? ["include_block"] : []
-    content {
-      id                                     = "abort-incomplete-uploads"
-      enabled                                = true
-      abort_incomplete_multipart_upload_days = 7
-      expiration {
-        days                         = 0
-        expired_object_delete_marker = false
-      }
-    }
+  versioning_configuration {
+    status = "Enabled"
   }
+}
+
+resource "aws_s3_bucket_logging" "bucket_logging" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket        = aws_s3_bucket.bucket[0].id
+  target_bucket = aws_s3_bucket.log_bucket[0].id
+  target_prefix = "${local.bucket_name}/${data.aws_caller_identity.current.account_id}/"
+}
+
+resource "aws_s3_bucket_lifecycle_configuration" "bucket_lifecycle" {
+  count = var.apply_resource == true && var.abort_incomplete_uploads == true ? 1 : 0
+
+  bucket = aws_s3_bucket.bucket[0].id
+
+  rule {
+    id     = "abort-incomplete-uploads"
+    status = "Enabled"
 
-  dynamic "logging" {
-    for_each = var.access_logs == true ? ["include_block"] : []
-    content {
-      target_bucket = aws_s3_bucket.log_bucket.*.id[0]
-      target_prefix = "${local.bucket_name}/${data.aws_caller_identity.current.account_id}/"
+    abort_incomplete_multipart_upload {
+      days_after_initiation = 7
     }
-  }
 
-  dynamic "cors_rule" {
-    for_each = length(var.cors_urls) > 0 ? ["include-cors"] : []
-    content {
-      allowed_headers = ["*"]
-      allowed_methods = ["PUT", "POST", "GET"]
-      allowed_origins = var.cors_urls
-      expose_headers  = ["ETag", "x-amz-server-side-encryption", "x-amz-request-id", "x-amz-id-2"]
-      max_age_seconds = 3000
+    expiration {
+      days                         = 0
+      expired_object_delete_marker = false
     }
   }
+}
 
-  tags = merge(
-    var.common_tags,
-    tomap(
-      { "Name" = local.bucket_name }
-    )
-  )
+resource "aws_s3_bucket_cors_configuration" "bucket_cors" {
+  count = var.apply_resource == true && length(var.cors_urls) > 0 ? 1 : 0
+
+  bucket = aws_s3_bucket.bucket[0].id
+
+  cors_rule {
+    allowed_headers = ["*"]
+    allowed_methods = ["PUT", "POST", "GET"]
+    allowed_origins = var.cors_urls
+    expose_headers  = ["ETag", "x-amz-server-side-encryption", "x-amz-request-id", "x-amz-id-2"]
+    max_age_seconds = 3000
+  }
 }
 
 resource "aws_s3_bucket_policy" "bucket" {
   count  = var.apply_resource == true ? 1 : 0
   bucket = aws_s3_bucket.bucket.*.id[0]
   policy = local.environment == "mgmt" && contains(["log-data", "lambda_update"], var.bucket_policy) ? templatefile("./tdr-terraform-modules/s3/templates/${var.bucket_policy}.json.tpl",
     {
-      bucket_name        = aws_s3_bucket.bucket.*.id[0],
-      account_id         = data.aws_caller_identity.current.account_id,
-      external_account_1 = data.aws_ssm_parameter.intg_account_number.*.value[0],
-      external_account_2 = data.aws_ssm_parameter.staging_account_number.*.value[0],
-      external_account_3 = data.aws_ssm_parameter.prod_account_number.*.value[0]
+      bucket_name           = aws_s3_bucket.bucket.*.id[0],
+      account_id            = data.aws_caller_identity.current.account_id,
+      external_account_1    = data.aws_ssm_parameter.intg_account_number.*.value[0],
+      external_account_2    = data.aws_ssm_parameter.staging_account_number.*.value[0],
+      external_account_3    = data.aws_ssm_parameter.prod_account_number.*.value[0]
+      canonical_user_grants = jsonencode(var.canonical_user_grants)
     }) : templatefile("./tdr-terraform-modules/s3/templates/${var.bucket_policy}.json.tpl",
     {
       bucket_name                  = aws_s3_bucket.bucket.*.id[0],
@@ -134,6 +156,7 @@ resource "aws_s3_bucket_policy" "bucket" {
       environment                  = local.environment, title_environment = title(local.environment),
       read_access_roles            = var.read_access_role_arns,
       cloudfront_distribution_arns = jsonencode(var.cloudfront_distribution_arns)
+      canonical_user_grants        = jsonencode(var.canonical_user_grants)
   })
   depends_on = [aws_s3_bucket_public_access_block.bucket]
 }

s3/templates/cloudtrail.json.tpl

@@ -1,7 +1,7 @@
 {
   "Version": "2012-10-17",
   "Statement": [
-    {
+   {
       "Sid": "AWSCloudTrailAclCheck",
       "Effect": "Allow",
       "Principal": {

s3/templates/secure_transport.json.tpl

@@ -2,7 +2,26 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
+  %{ for grant in jsondecode(canonical_user_grants) ~}
     {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "CanonicalUser": "${grant.id}"
+      },
+        "Action": [
+        "s3:GetBucketAcl",
+        "s3:ListBucket",
+        "s3:PutBucketAcl",
+        "s3:PutObject"
+    ],
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+  %{ endfor ~}
+   {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
       "Effect": "Deny",
@@ -16,6 +35,6 @@
         }
       },
       "Principal": "*"
-    }
+   }
   ]
-}
+}

s3/variables.tf

@@ -23,7 +23,8 @@ variable "environment_suffix" {
 variable "acl" {
   default = "private"
 }
-
+# If this is set, the id will be used to set bucket policies for the user equivalent to 'FULL_CONTROL'.Permissions value ignored
+# Only specified for the log bucket and used in the secure_transport.json.tpl
 variable "canonical_user_grants" {
   description = "A list of canonical user IDs and their permissions. If this is set, you cannot use a canned ACL"
   type        = list(object({ id = string, permissions = list(string) }))

vpc/main.tf

@@ -65,7 +65,7 @@ resource "aws_route" "internet_access" {
 
 resource "aws_eip" "gw" {
   count      = var.az_count
-  vpc        = true
+  domain     = "vpc"
   depends_on = [aws_internet_gateway.gw]
 
   tags = merge(