From a6f994223c69473924b12e2195250d03303f6829 Mon Sep 17 00:00:00 2001 From: Alex Jurkiewicz Date: Tue, 29 Jun 2021 06:40:07 +1000 Subject: [PATCH] S3 Replication Improvements (#93) Co-authored-by: Nuru Co-authored-by: Yonatan Koren --- README.md | 20 ++- README.yaml | 12 +- docs/terraform.md | 8 +- ...est-1.tfvars => fixtures.us-east-2.tfvars} | 2 +- ...-west-1.tfvars => grants.us-east-2.tfvars} | 2 +- ...st-1.tfvars => lifecycle.us-east-2.tfvars} | 2 +- examples/complete/main.tf | 45 +++++++ ...-1.tfvars => object-lock.us-east-2.tfvars} | 2 +- examples/complete/outputs.tf | 15 +++ .../complete/replication.us-east-2.tfvars | 29 ++++ examples/complete/variables.tf | 5 + main.tf | 91 +++++++++---- outputs.tf | 2 +- replication.tf | 15 ++- test/src/Makefile | 9 +- test/src/examples_complete_test.go | 126 +++++++++++++++++- variables.tf | 31 ++++- 17 files changed, 353 insertions(+), 63 deletions(-) rename examples/complete/{fixtures.us-west-1.tfvars => fixtures.us-east-2.tfvars} (94%) rename examples/complete/{grants.us-west-1.tfvars => grants.us-east-2.tfvars} (96%) rename examples/complete/{lifecycle.us-west-1.tfvars => lifecycle.us-east-2.tfvars} (98%) rename examples/complete/{object-lock.us-west-1.tfvars => object-lock.us-east-2.tfvars} (95%) create mode 100644 examples/complete/replication.us-east-2.tfvars diff --git a/README.md b/README.md index 94ca7926..383a3887 100644 --- a/README.md +++ b/README.md @@ -28,15 +28,19 @@ --> -This module creates an S3 bucket with support of versioning, encryption, ACL and bucket object policy. +This module creates an S3 bucket with support of versioning, replication, encryption, ACL, and bucket object policy. If `user_enabled` variable is set to `true`, the module will provision a basic IAM user with permissions to access the bucket. -This basic IAM system user is suitable for CI/CD systems (_e.g._ TravisCI, CircleCI) or systems which are *external* to AWS that cannot leverage [AWS IAM Instance Profiles](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html). +This basic IAM system user is suitable for CI/CD systems (_e.g._ TravisCI, CircleCI) or systems which are *external* to AWS that cannot leverage +[AWS IAM Instance Profiles](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html) and +do not already have IAM credentials. Users or systems that have IAM credentials should either be granted access directly based on +their IAM identity or be allowed to assume an IAM role with access. We do not recommend creating IAM users this way for any other purpose. -It blocks public access to the bucket by default. -https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html +This module blocks public access to the bucket by default. See `block_public_acls`, `block_public_policy`, +and `ignore_public_acls` to change the settings. See [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html) +for more details. --- @@ -241,10 +245,12 @@ Available targets: | [object\_lock\_configuration](#input\_object\_lock\_configuration) | A configuration for S3 object locking. With S3 Object Lock, you can store objects using a `write once, read many` (WORM) model. Object Lock can help prevent objects from being deleted or overwritten for a fixed amount of time or indefinitely. | 
object({
    mode  = string # Valid values are GOVERNANCE and COMPLIANCE.
    days  = number
    years = number
  })
| `null` | no | | [policy](#input\_policy) | A valid bucket policy JSON document. Note that if the policy document is not specific enough (but still valid), Terraform may view the policy as constantly changing in a terraform plan. In this case, please make sure you use the verbose/specific version of the policy | `string` | `""` | no | | [regex\_replace\_chars](#input\_regex\_replace\_chars) | Regex to replace chars with empty string in `namespace`, `environment`, `stage` and `name`.
If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no | -| [replication\_rules](#input\_replication\_rules) | Specifies the replication rules if S3 bucket replication is enabled | `list(any)` | `null` | no | +| [replication\_rules](#input\_replication\_rules) | DEPRECATED: Use s3\_replication\_rules instead. | `list(any)` | `null` | no | | [restrict\_public\_buckets](#input\_restrict\_public\_buckets) | Set to `false` to disable the restricting of making the bucket public | `bool` | `true` | no | -| [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | The ARN of the S3 replica bucket (destination) | `string` | `""` | no | -| [s3\_replication\_enabled](#input\_s3\_replication\_enabled) | Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`. | `bool` | `false` | no | +| [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | A single S3 bucket ARN to use for all replication rules.
Note: The destination bucket can be specified in the replication rule itself
(which allows for multiple destinations), in which case it will take precedence over this variable. | `string` | `""` | no | +| [s3\_replication\_enabled](#input\_s3\_replication\_enabled) | Set this to true and specify `s3_replication_rules` to enable replication. `versioning_enabled` must also be `true`. | `bool` | `false` | no | +| [s3\_replication\_rules](#input\_s3\_replication\_rules) | Specifies the replication rules for S3 bucket replication if enabled. You must also set s3\_replication\_enabled to true. | `list(any)` | `null` | no | +| [s3\_replication\_source\_roles](#input\_s3\_replication\_source\_roles) | Cross-account IAM Role ARNs that will be allowed to perform S3 replication to this bucket (for replication within the same AWS account, it's not necessary to adjust the bucket policy). | `list(string)` | `[]` | no | | [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no | | [stage](#input\_stage) | Stage, e.g. 'prod', 'staging', 'dev', OR 'source', 'build', 'test', 'deploy', 'release' | `string` | `null` | no | | [tags](#input\_tags) | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no | diff --git a/README.yaml b/README.yaml index aade5f26..7f6a8e73 100644 --- a/README.yaml +++ b/README.yaml @@ -66,15 +66,19 @@ related: # Short description of this project description: |- - This module creates an S3 bucket with support of versioning, encryption, ACL and bucket object policy. + This module creates an S3 bucket with support of versioning, replication, encryption, ACL, and bucket object policy. If `user_enabled` variable is set to `true`, the module will provision a basic IAM user with permissions to access the bucket. - This basic IAM system user is suitable for CI/CD systems (_e.g._ TravisCI, CircleCI) or systems which are *external* to AWS that cannot leverage [AWS IAM Instance Profiles](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html). + This basic IAM system user is suitable for CI/CD systems (_e.g._ TravisCI, CircleCI) or systems which are *external* to AWS that cannot leverage + [AWS IAM Instance Profiles](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2_instance-profiles.html) and + do not already have IAM credentials. Users or systems that have IAM credentials should either be granted access directly based on + their IAM identity or be allowed to assume an IAM role with access. We do not recommend creating IAM users this way for any other purpose. - It blocks public access to the bucket by default. - https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html + This module blocks public access to the bucket by default. See `block_public_acls`, `block_public_policy`, + and `ignore_public_acls` to change the settings. See [AWS documentation](https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html) + for more details. # How to use this project usage: |- diff --git a/docs/terraform.md b/docs/terraform.md index ea3534ac..d38c3c49 100644 --- a/docs/terraform.md +++ b/docs/terraform.md @@ -69,10 +69,12 @@ | [object\_lock\_configuration](#input\_object\_lock\_configuration) | A configuration for S3 object locking. With S3 Object Lock, you can store objects using a `write once, read many` (WORM) model. Object Lock can help prevent objects from being deleted or overwritten for a fixed amount of time or indefinitely. | 
object({
    mode  = string # Valid values are GOVERNANCE and COMPLIANCE.
    days  = number
    years = number
  })
| `null` | no | | [policy](#input\_policy) | A valid bucket policy JSON document. Note that if the policy document is not specific enough (but still valid), Terraform may view the policy as constantly changing in a terraform plan. In this case, please make sure you use the verbose/specific version of the policy | `string` | `""` | no | | [regex\_replace\_chars](#input\_regex\_replace\_chars) | Regex to replace chars with empty string in `namespace`, `environment`, `stage` and `name`.
If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no | -| [replication\_rules](#input\_replication\_rules) | Specifies the replication rules if S3 bucket replication is enabled | `list(any)` | `null` | no | +| [replication\_rules](#input\_replication\_rules) | DEPRECATED: Use s3\_replication\_rules instead. | `list(any)` | `null` | no | | [restrict\_public\_buckets](#input\_restrict\_public\_buckets) | Set to `false` to disable the restricting of making the bucket public | `bool` | `true` | no | -| [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | The ARN of the S3 replica bucket (destination) | `string` | `""` | no | -| [s3\_replication\_enabled](#input\_s3\_replication\_enabled) | Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`. | `bool` | `false` | no | +| [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | A single S3 bucket ARN to use for all replication rules.
Note: The destination bucket can be specified in the replication rule itself
(which allows for multiple destinations), in which case it will take precedence over this variable. | `string` | `""` | no | +| [s3\_replication\_enabled](#input\_s3\_replication\_enabled) | Set this to true and specify `s3_replication_rules` to enable replication. `versioning_enabled` must also be `true`. | `bool` | `false` | no | +| [s3\_replication\_rules](#input\_s3\_replication\_rules) | Specifies the replication rules for S3 bucket replication if enabled. You must also set s3\_replication\_enabled to true. | `list(any)` | `null` | no | +| [s3\_replication\_source\_roles](#input\_s3\_replication\_source\_roles) | Cross-account IAM Role ARNs that will be allowed to perform S3 replication to this bucket (for replication within the same AWS account, it's not necessary to adjust the bucket policy). | `list(string)` | `[]` | no | | [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no | | [stage](#input\_stage) | Stage, e.g. 'prod', 'staging', 'dev', OR 'source', 'build', 'test', 'deploy', 'release' | `string` | `null` | no | | [tags](#input\_tags) | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no | diff --git a/examples/complete/fixtures.us-west-1.tfvars b/examples/complete/fixtures.us-east-2.tfvars similarity index 94% rename from examples/complete/fixtures.us-west-1.tfvars rename to examples/complete/fixtures.us-east-2.tfvars index 66c2d2f0..d9022d6b 100644 --- a/examples/complete/fixtures.us-west-1.tfvars +++ b/examples/complete/fixtures.us-east-2.tfvars @@ -1,6 +1,6 @@ enabled = true -region = "us-west-1" +region = "us-east-2" namespace = "eg" diff --git a/examples/complete/grants.us-west-1.tfvars b/examples/complete/grants.us-east-2.tfvars similarity index 96% rename from examples/complete/grants.us-west-1.tfvars rename to examples/complete/grants.us-east-2.tfvars index dcea155a..3df51ded 100644 --- a/examples/complete/grants.us-west-1.tfvars +++ b/examples/complete/grants.us-east-2.tfvars @@ -1,4 +1,4 @@ -region = "us-west-1" +region = "us-east-2" namespace = "eg" diff --git a/examples/complete/lifecycle.us-west-1.tfvars b/examples/complete/lifecycle.us-east-2.tfvars similarity index 98% rename from examples/complete/lifecycle.us-west-1.tfvars rename to examples/complete/lifecycle.us-east-2.tfvars index df9e7ac2..cf8f6434 100644 --- a/examples/complete/lifecycle.us-west-1.tfvars +++ b/examples/complete/lifecycle.us-east-2.tfvars @@ -1,4 +1,4 @@ -region = "us-west-1" +region = "us-east-2" namespace = "eg" diff --git a/examples/complete/main.tf b/examples/complete/main.tf index d06ae037..8f7b8d80 100644 --- a/examples/complete/main.tf +++ b/examples/complete/main.tf @@ -1,3 +1,15 @@ +locals { + replication_enabled = length(var.s3_replication_rules) > 0 + extra_rule = local.replication_enabled ? { + id = "replication-test-explicit-bucket" + status = "Enabled" + prefix = "/extra" + priority = 5 + destination_bucket = module.s3_bucket_replication_target_extra[0].bucket_arn + } : null + s3_replication_rules = local.replication_enabled ? concat(var.s3_replication_rules, [local.extra_rule]) : null +} + provider "aws" { region = var.region } @@ -15,6 +27,39 @@ module "s3_bucket" { allowed_bucket_actions = var.allowed_bucket_actions bucket_name = var.bucket_name object_lock_configuration = var.object_lock_configuration + s3_replication_enabled = local.replication_enabled + s3_replica_bucket_arn = join("", module.s3_bucket_replication_target.*.bucket_arn) + s3_replication_rules = local.s3_replication_rules context = module.this.context } + +module "s3_bucket_replication_target" { + count = local.replication_enabled ? 1 : 0 + + source = "../../" + + user_enabled = true + acl = "private" + force_destroy = true + versioning_enabled = true + s3_replication_source_roles = [module.s3_bucket.replication_role_arn] + + attributes = ["target"] + context = module.this.context +} + +module "s3_bucket_replication_target_extra" { + count = local.replication_enabled ? 1 : 0 + + source = "../../" + + user_enabled = true + acl = "private" + force_destroy = true + versioning_enabled = true + s3_replication_source_roles = [module.s3_bucket.replication_role_arn] + + attributes = ["target", "extra"] + context = module.this.context +} diff --git a/examples/complete/object-lock.us-west-1.tfvars b/examples/complete/object-lock.us-east-2.tfvars similarity index 95% rename from examples/complete/object-lock.us-west-1.tfvars rename to examples/complete/object-lock.us-east-2.tfvars index 791bd2d2..f8f57fba 100644 --- a/examples/complete/object-lock.us-west-1.tfvars +++ b/examples/complete/object-lock.us-east-2.tfvars @@ -1,4 +1,4 @@ -region = "us-west-1" +region = "us-east-2" namespace = "eg" diff --git a/examples/complete/outputs.tf b/examples/complete/outputs.tf index ce76e56f..80cc2777 100644 --- a/examples/complete/outputs.tf +++ b/examples/complete/outputs.tf @@ -13,6 +13,21 @@ output "bucket_arn" { description = "Bucket ARN" } +output "replication_bucket_id" { + value = local.replication_enabled ? join("", module.s3_bucket_replication_target.*.bucket_id) : null + description = "Bucket Name (aka ID)" +} + +output "replication_bucket_arn" { + value = local.replication_enabled ? join("", module.s3_bucket_replication_target.*.bucket_arn) : null + description = "Bucket ARN" +} + +output "replication_role_arn" { + value = module.s3_bucket.replication_role_arn + description = "The ARN of the replication IAM Role" +} + output "bucket_region" { value = module.s3_bucket.bucket_region description = "Bucket region" diff --git a/examples/complete/replication.us-east-2.tfvars b/examples/complete/replication.us-east-2.tfvars new file mode 100644 index 00000000..9fb2507f --- /dev/null +++ b/examples/complete/replication.us-east-2.tfvars @@ -0,0 +1,29 @@ +# Do not set "enabled", will be set by test framework +# enabled = true +region = "us-east-2" +namespace = "eg" +stage = "test" +name = "s3-replication-test" +acl = "private" +force_destroy = true +versioning_enabled = true +allow_encrypted_uploads_only = true +allowed_bucket_actions = [ + "s3:PutObject", + "s3:PutObjectAcl", + "s3:GetObject", + "s3:DeleteObject", + "s3:ListBucket", + "s3:ListBucketMultipartUploads", + "s3:GetBucketLocation", + "s3:AbortMultipartUpload", +] + +# Rules will be augmented with an additional bucket rule, so prefix cannot be "/" +s3_replication_rules = [ + { + id = "replication-test" + status = "Enabled" + prefix = "/main" + } +] \ No newline at end of file diff --git a/examples/complete/variables.tf b/examples/complete/variables.tf index 21562d42..b5fc93c0 100644 --- a/examples/complete/variables.tf +++ b/examples/complete/variables.tf @@ -61,6 +61,11 @@ variable "lifecycle_rules" { description = "A list of lifecycle rules." } +variable "s3_replication_rules" { + default = [] + description = "S3 replication rules" +} + variable "policy" { type = string default = "" diff --git a/main.tf b/main.tf index bcde03a6..7155bda1 100644 --- a/main.tf +++ b/main.tf @@ -1,5 +1,12 @@ locals { - bucket_name = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id + enabled = module.this.enabled + bucket_name = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id + replication_enabled = local.enabled && var.s3_replication_enabled + bucket_arn = "arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}" + + # Deprecate `replication_rules` in favor of `s3_replication_rules` to keep all the replication related + # inputs grouped under s3_replica[tion] + s3_replication_rules = var.replication_rules == null ? var.s3_replication_rules : var.replication_rules } resource "aws_s3_bucket" "default" { @@ -7,7 +14,7 @@ resource "aws_s3_bucket" "default" { #bridgecrew:skip=CKV_AWS_52:Skipping `Ensure S3 bucket has MFA delete enabled` due to issue in terraform (https://github.com/hashicorp/terraform-provider-aws/issues/629). #bridgecrew:skip=BC_AWS_S3_16:Skipping `Ensure S3 bucket versioning is enabled` because dynamic blocks are not supported by checkov #bridgecrew:skip=BC_AWS_S3_14:Skipping `Ensure all data stored in the S3 bucket is securely encrypted at rest` because variables are not understood - count = module.this.enabled ? 1 : 0 + count = local.enabled ? 1 : 0 bucket = local.bucket_name acl = try(length(var.grants), 0) == 0 ? var.acl : null force_destroy = var.force_destroy @@ -139,22 +146,27 @@ resource "aws_s3_bucket" "default" { } dynamic "replication_configuration" { - for_each = var.s3_replication_enabled ? [1] : [] + for_each = local.replication_enabled ? [1] : [] content { role = aws_iam_role.replication[0].arn dynamic "rules" { - for_each = var.replication_rules == null ? [] : var.replication_rules + for_each = local.s3_replication_rules == null ? [] : local.s3_replication_rules content { id = rules.value.id priority = try(rules.value.priority, 0) - prefix = try(rules.value.prefix, null) - status = try(rules.value.status, null) + # `prefix` at this level is a V1 feature, replaced in V2 with the filter block. + # `prefix` conflicts with `filter`, and for multiple destinations, a filter block + # is required even if it empty, so we always implement `prefix` as a filter. + # OBSOLETE: prefix = try(rules.value.prefix, null) + status = try(rules.value.status, null) destination { - bucket = var.s3_replica_bucket_arn + # Prefer newer system of specifying bucket in rule, but maintain backward compatibility with + # s3_replica_bucket_arn to specify single destination for all rules + bucket = try(length(rules.value.destination_bucket), 0) > 0 ? rules.value.destination_bucket : var.s3_replica_bucket_arn storage_class = try(rules.value.destination.storage_class, "STANDARD") replica_kms_key_id = try(rules.value.destination.replica_kms_key_id, null) account_id = try(rules.value.destination.account_id, null) @@ -178,11 +190,14 @@ resource "aws_s3_bucket" "default" { } } + # Replication to multiple destination buckets requires that priority is specified in the rules object. + # If the corresponding rule requires no filter, an empty configuration block filter {} must be specified. + # See https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket dynamic "filter" { - for_each = try(rules.value.filter, null) == null ? [] : [rules.value.filter] + for_each = try(rules.value.filter, null) == null ? ["empty"] : [rules.value.filter] content { - prefix = try(filter.value.prefix, null) + prefix = try(filter.value.prefix, try(rules.value.prefix, null)) tags = try(filter.value.tags, {}) } } @@ -210,7 +225,7 @@ module "s3_user" { source = "cloudposse/iam-s3-user/aws" version = "0.15.2" - enabled = module.this.enabled && var.user_enabled ? true : false + enabled = local.enabled && var.user_enabled s3_actions = var.allowed_bucket_actions s3_resources = ["${join("", aws_s3_bucket.default.*.arn)}/*", join("", aws_s3_bucket.default.*.arn)] @@ -220,7 +235,7 @@ module "s3_user" { data "aws_partition" "current" {} data "aws_iam_policy_document" "bucket_policy" { - count = module.this.enabled ? 1 : 0 + count = local.enabled ? 1 : 0 dynamic "statement" { for_each = var.allow_encrypted_uploads_only ? [1] : [] @@ -229,7 +244,7 @@ data "aws_iam_policy_document" "bucket_policy" { sid = "DenyIncorrectEncryptionHeader" effect = "Deny" actions = ["s3:PutObject"] - resources = ["arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}/*"] + resources = ["${local.bucket_arn}/*"] principals { identifiers = ["*"] @@ -251,7 +266,7 @@ data "aws_iam_policy_document" "bucket_policy" { sid = "DenyUnEncryptedObjectUploads" effect = "Deny" actions = ["s3:PutObject"] - resources = ["arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}/*"] + resources = ["${local.bucket_arn}/*"] principals { identifiers = ["*"] @@ -270,13 +285,10 @@ data "aws_iam_policy_document" "bucket_policy" { for_each = var.allow_ssl_requests_only ? [1] : [] content { - sid = "ForceSSLOnlyAccess" - effect = "Deny" - actions = ["s3:*"] - resources = [ - "arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}", - "arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}/*" - ] + sid = "ForceSSLOnlyAccess" + effect = "Deny" + actions = ["s3:*"] + resources = [local.bucket_arn, "${local.bucket_arn}/*"] principals { identifiers = ["*"] @@ -290,16 +302,49 @@ data "aws_iam_policy_document" "bucket_policy" { } } } + + dynamic "statement" { + for_each = length(var.s3_replication_source_roles) > 0 ? [1] : [] + + content { + sid = "CrossAccountReplicationObjects" + actions = [ + "s3:ReplicateObject", + "s3:ReplicateDelete", + "s3:ReplicateTags", + "s3:GetObjectVersionTagging", + "s3:ObjectOwnerOverrideToBucketOwner" + ] + resources = ["${local.bucket_arn}/*"] + principals { + type = "AWS" + identifiers = var.s3_replication_source_roles + } + } + } + dynamic "statement" { + for_each = length(var.s3_replication_source_roles) > 0 ? [1] : [] + + content { + sid = "CrossAccountReplicationBucket" + actions = ["s3:List*", "s3:GetBucketVersioning", "s3:PutBucketVersioning"] + resources = [local.bucket_arn] + principals { + type = "AWS" + identifiers = var.s3_replication_source_roles + } + } + } } data "aws_iam_policy_document" "aggregated_policy" { - count = module.this.enabled ? 1 : 0 + count = local.enabled ? 1 : 0 source_json = var.policy override_json = join("", data.aws_iam_policy_document.bucket_policy.*.json) } resource "aws_s3_bucket_policy" "default" { - count = module.this.enabled && (var.allow_ssl_requests_only || var.allow_encrypted_uploads_only || var.policy != "") ? 1 : 0 + count = local.enabled && (var.allow_ssl_requests_only || var.allow_encrypted_uploads_only || length(var.s3_replication_source_roles) > 0 || var.policy != "") ? 1 : 0 bucket = join("", aws_s3_bucket.default.*.id) policy = join("", data.aws_iam_policy_document.aggregated_policy.*.json) depends_on = [aws_s3_bucket_public_access_block.default] @@ -309,7 +354,7 @@ resource "aws_s3_bucket_policy" "default" { # https://www.terraform.io/docs/providers/aws/r/s3_bucket_public_access_block.html # for the nuances of the blocking options resource "aws_s3_bucket_public_access_block" "default" { - count = module.this.enabled ? 1 : 0 + count = local.enabled ? 1 : 0 bucket = join("", aws_s3_bucket.default.*.id) block_public_acls = var.block_public_acls diff --git a/outputs.tf b/outputs.tf index a657a4af..31681bb7 100644 --- a/outputs.tf +++ b/outputs.tf @@ -49,7 +49,7 @@ output "user_unique_id" { } output "replication_role_arn" { - value = module.this.enabled && var.s3_replication_enabled ? join("", aws_iam_role.replication.*.arn) : "" + value = module.this.enabled && local.replication_enabled ? join("", aws_iam_role.replication.*.arn) : "" description = "The ARN of the replication IAM Role" } diff --git a/replication.tf b/replication.tf index 96c8c8c6..cb0af93c 100644 --- a/replication.tf +++ b/replication.tf @@ -1,12 +1,12 @@ resource "aws_iam_role" "replication" { - count = module.this.enabled && var.s3_replication_enabled ? 1 : 0 + count = local.replication_enabled ? 1 : 0 name = format("%s-replication", module.this.id) assume_role_policy = data.aws_iam_policy_document.replication_sts[0].json } data "aws_iam_policy_document" "replication_sts" { - count = module.this.enabled && var.s3_replication_enabled ? 1 : 0 + count = local.replication_enabled ? 1 : 0 statement { sid = "AllowPrimaryToAssumeServiceRole" @@ -23,14 +23,14 @@ data "aws_iam_policy_document" "replication_sts" { } resource "aws_iam_policy" "replication" { - count = module.this.enabled && var.s3_replication_enabled ? 1 : 0 + count = local.replication_enabled ? 1 : 0 name = format("%s-replication", module.this.id) policy = data.aws_iam_policy_document.replication[0].json } data "aws_iam_policy_document" "replication" { - count = module.this.enabled && var.s3_replication_enabled ? 1 : 0 + count = local.replication_enabled ? 1 : 0 statement { sid = "AllowPrimaryToGetReplicationConfiguration" @@ -56,12 +56,15 @@ data "aws_iam_policy_document" "replication" { "s3:ObjectOwnerOverrideToBucketOwner" ] - resources = ["${var.s3_replica_bucket_arn}/*"] + resources = toset(concat( + try(length(var.s3_replica_bucket_arn), 0) > 0 ? ["${var.s3_replica_bucket_arn}/*"] : [], + [for rule in local.s3_replication_rules : "${rule.destination_bucket}/*" if try(length(rule.destination_bucket), 0) > 0], + )) } } resource "aws_iam_role_policy_attachment" "replication" { - count = module.this.enabled && var.s3_replication_enabled ? 1 : 0 + count = local.replication_enabled ? 1 : 0 role = aws_iam_role.replication[0].name policy_arn = aws_iam_policy.replication[0].arn } diff --git a/test/src/Makefile b/test/src/Makefile index dd2a4e9e..dd458292 100644 --- a/test/src/Makefile +++ b/test/src/Makefile @@ -14,13 +14,16 @@ init: .PHONY : test ## Run tests test: init - # go mod download - go test -v -timeout 60m -run TestExamplesComplete + # Verify we have valid AWS credential + if command -v aws >/dev/null; then \ + (unset AWS_PROFILE; aws sts get-caller-identity >/dev/null); \ + fi + go test -v -timeout 10m -run TestExamplesComplete ## Run tests in docker container docker/test: docker run --name terratest --rm -it -e AWS_ACCESS_KEY_ID -e AWS_SECRET_ACCESS_KEY -e AWS_SESSION_TOKEN -e GITHUB_TOKEN \ - -e PATH="/usr/local/terraform/0.12/bin:/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ + -e PATH="/usr/local/terraform/0.13/bin:/go/bin:/usr/local/go/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" \ -v $(CURDIR)/../../:/module/ cloudposse/test-harness:latest -C /module/test/src test .PHONY : clean diff --git a/test/src/examples_complete_test.go b/test/src/examples_complete_test.go index 97be8c05..4450a978 100644 --- a/test/src/examples_complete_test.go +++ b/test/src/examples_complete_test.go @@ -13,12 +13,13 @@ import ( // Test the Terraform module in examples/complete using Terratest. func TestExamplesComplete(t *testing.T) { + t.Parallel() rand.Seed(time.Now().UnixNano()) attributes := []string{strconv.Itoa(rand.Intn(100000))} rootFolder := "../../" terraformFolderRelativeToRoot := "examples/complete" - varFiles := []string{"fixtures.us-west-1.tfvars"} + varFiles := []string{"fixtures.us-east-2.tfvars"} tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) @@ -56,12 +57,13 @@ func TestExamplesComplete(t *testing.T) { // Test the Terraform module in examples/complete using Terratest for grants. func TestExamplesCompleteWithGrants(t *testing.T) { - rand.Seed(time.Now().UnixNano()) + t.Parallel() + rand.Seed(time.Now().UnixNano()+1) attributes := []string{strconv.Itoa(rand.Intn(100000))} rootFolder := "../../" terraformFolderRelativeToRoot := "examples/complete" - varFiles := []string{"grants.us-west-1.tfvars"} + varFiles := []string{"grants.us-east-2.tfvars"} tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) @@ -99,12 +101,13 @@ func TestExamplesCompleteWithGrants(t *testing.T) { // Test the Terraform module in examples/complete using Terratest for grants. func TestExamplesCompleteWithObjectLock(t *testing.T) { - rand.Seed(time.Now().UnixNano()) + t.Parallel() + rand.Seed(time.Now().UnixNano()+2) attributes := []string{strconv.Itoa(rand.Intn(100000))} rootFolder := "../../" terraformFolderRelativeToRoot := "examples/complete" - varFiles := []string{"object-lock.us-west-1.tfvars"} + varFiles := []string{"object-lock.us-east-2.tfvars"} tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) @@ -141,12 +144,13 @@ func TestExamplesCompleteWithObjectLock(t *testing.T) { } func TestExamplesCompleteWithLifecycleRules(t *testing.T) { - rand.Seed(time.Now().UnixNano()) + t.Parallel() + rand.Seed(time.Now().UnixNano()+3) attributes := []string{strconv.Itoa(rand.Intn(100000))} rootFolder := "../../" terraformFolderRelativeToRoot := "examples/complete" - varFiles := []string{"lifecycle.us-west-1.tfvars"} + varFiles := []string{"lifecycle.us-east-2.tfvars"} tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) @@ -181,3 +185,111 @@ func TestExamplesCompleteWithLifecycleRules(t *testing.T) { // Verify we're getting back the outputs we expect assert.Equal(t, expectedS3BucketId, s3BucketId) } + + +func TestExamplesCompleteWithReplication(t *testing.T) { + t.Parallel() + rand.Seed(time.Now().UnixNano()+4) + + attributes := []string{strconv.Itoa(rand.Intn(100000))} + rootFolder := "../../" + terraformFolderRelativeToRoot := "examples/complete" + varFiles := []string{"replication.us-east-2.tfvars"} + + tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) + + terraformOptions := &terraform.Options{ + // The path to where our Terraform code is located + TerraformDir: tempTestFolder, + Upgrade: true, + // Variables to pass to our Terraform code using -var-file options + VarFiles: varFiles, + Vars: map[string]interface{}{ + "attributes": attributes, + "enabled": "true", + }, + } + + // At the end of the test, run `terraform destroy` to clean up any resources that were created + defer terraform.Destroy(t, terraformOptions) + + // This will run `terraform init` and `terraform apply` and fail the test if there are any errors + terraform.InitAndApply(t, terraformOptions) + + // Run `terraform output` to get the value of an output variable + userName := terraform.Output(t, terraformOptions, "user_name") + + expectedUserName := "eg-test-s3-replication-test-" + attributes[0] + // Verify we're getting back the outputs we expect + assert.Equal(t, expectedUserName, userName) + + // Run `terraform output` to get the value of an output variable + s3BucketId := terraform.Output(t, terraformOptions, "bucket_id") + + expectedS3BucketId := "eg-test-s3-replication-test-" + attributes[0] + // Verify we're getting back the outputs we expect + assert.Equal(t, expectedS3BucketId, s3BucketId) + + // Run `terraform output` to get the value of an output variable + s3ReplicationBucketId := terraform.Output(t, terraformOptions, "replication_bucket_id") + + expectedReplicationS3BucketId := "eg-test-s3-replication-test-" + attributes[0] + "-target" + // Verify we're getting back the outputs we expect + assert.Equal(t, expectedReplicationS3BucketId, s3ReplicationBucketId) + + // Run `terraform output` to get the value of an output variable + s3ReplicationRoleArn := terraform.Output(t, terraformOptions, "replication_role_arn") + + // Verify we're getting back the outputs we expect + assert.NotEmptyf(t, s3ReplicationRoleArn, "If replication is enabled, we should get a Replication Role ARN.") + +} + +func TestExamplesCompleteDisabled(t *testing.T) { + t.Parallel() + rand.Seed(time.Now().UnixNano()+5) + + attributes := []string{strconv.Itoa(rand.Intn(100000))} + rootFolder := "../../" + terraformFolderRelativeToRoot := "examples/complete" + varFiles := []string{"replication.us-east-2.tfvars"} + + tempTestFolder := test_structure.CopyTerraformFolderToTemp(t, rootFolder, terraformFolderRelativeToRoot) + + terraformOptions := &terraform.Options{ + // The path to where our Terraform code is located + TerraformDir: tempTestFolder, + Upgrade: true, + // Variables to pass to our Terraform code using -var-file options + VarFiles: varFiles, + Vars: map[string]interface{}{ + "attributes": attributes, + "enabled": "false", + }, + } + + // At the end of the test, run `terraform destroy` to clean up any resources that were created + defer terraform.Destroy(t, terraformOptions) + + // This will run `terraform init` and `terraform apply` and fail the test if there are any errors + terraform.InitAndApply(t, terraformOptions) + + // Run `terraform output` to get the value of an output variable + userName := terraform.Output(t, terraformOptions, "user_name") + + // Verify we're getting back the outputs we expect + assert.Empty(t, userName, "When disabled, module should have no outputs.") + + // Run `terraform output` to get the value of an output variable + s3BucketId := terraform.Output(t, terraformOptions, "bucket_id") + + // Verify we're getting back the outputs we expect + assert.Empty(t, s3BucketId, "When disabled, module should have no outputs.") + + // Run `terraform output` to get the value of an output variable + s3ReplicationBucketId := terraform.Output(t, terraformOptions, "replication_bucket_id") + + // Verify we're getting back the outputs we expect + assert.Empty(t, s3ReplicationBucketId, "When disabled, module should have no outputs.") + +} diff --git a/variables.tf b/variables.tf index 295f484e..9a1a3366 100644 --- a/variables.tf +++ b/variables.tf @@ -169,21 +169,29 @@ variable "restrict_public_buckets" { variable "s3_replication_enabled" { type = bool default = false - description = "Set this to true and specify `s3_replica_bucket_arn` to enable replication. `versioning_enabled` must also be `true`." + description = "Set this to true and specify `s3_replication_rules` to enable replication. `versioning_enabled` must also be `true`." } variable "s3_replica_bucket_arn" { type = string default = "" - description = "The ARN of the S3 replica bucket (destination)" + description = <<-EOT + A single S3 bucket ARN to use for all replication rules. + Note: The destination bucket can be specified in the replication rule itself + (which allows for multiple destinations), in which case it will take precedence over this variable. + EOT } -variable "replication_rules" { +variable "s3_replication_rules" { # type = list(object({ # id = string # priority = number # prefix = string # status = string + # # destination_bucket is specified here rather than inside the destination object + # # to make it easier to work with the Terraform type system and create a list of consistent type. + # destination_bucket = string # destination bucket ARN, overrides s3_replica_bucket_arn + # # destination = object({ # storage_class = string # replica_kms_key_id = string @@ -197,6 +205,7 @@ variable "replication_rules" { # enabled = bool # }) # }) + # # filter.prefix overrides top level prefix # filter = object({ # prefix = string # tags = map(string) @@ -205,7 +214,19 @@ variable "replication_rules" { type = list(any) default = null - description = "Specifies the replication rules if S3 bucket replication is enabled" + description = "Specifies the replication rules for S3 bucket replication if enabled. You must also set s3_replication_enabled to true." +} + +variable "replication_rules" { + type = list(any) + default = null + description = "DEPRECATED: Use s3_replication_rules instead." +} + +variable "s3_replication_source_roles" { + type = list(string) + default = [] + description = "Cross-account IAM Role ARNs that will be allowed to perform S3 replication to this bucket (for replication within the same AWS account, it's not necessary to adjust the bucket policy)." } variable "bucket_name" { @@ -235,4 +256,4 @@ variable "website_inputs" { default = null description = "Specifies the static website hosting configuration object." -} \ No newline at end of file +}