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

S3 Replication Improvements #93

Merged
merged 20 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,9 @@ Available targets:
| <a name="input_object_lock_configuration"></a> [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. | <pre>object({<br> mode = string # Valid values are GOVERNANCE and COMPLIANCE.<br> days = number<br> years = number<br> })</pre> | `null` | no |
| <a name="input_policy"></a> [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 |
| <a name="input_regex_replace_chars"></a> [regex\_replace\_chars](#input\_regex\_replace\_chars) | Regex to replace chars with empty string in `namespace`, `environment`, `stage` and `name`.<br>If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no |
| <a name="input_replication_rules"></a> [replication\_rules](#input\_replication\_rules) | Specifies the replication rules if S3 bucket replication is enabled | `list(any)` | `null` | no |
| <a name="input_replication_rules"></a> [replication\_rules](#input\_replication\_rules) | S3 replication rules | `list` | `[]` | no |
| <a name="input_replication_source_roles"></a> [replication\_source\_roles](#input\_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 |
| <a name="input_restrict_public_buckets"></a> [restrict\_public\_buckets](#input\_restrict\_public\_buckets) | Set to `false` to disable the restricting of making the bucket public | `bool` | `true` | no |
| <a name="input_s3_replica_bucket_arn"></a> [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | The ARN of the S3 replica bucket (destination) | `string` | `""` | no |
| <a name="input_s3_replication_enabled"></a> [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 |
| <a name="input_sse_algorithm"></a> [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no |
| <a name="input_stage"></a> [stage](#input\_stage) | Stage, e.g. 'prod', 'staging', 'dev', OR 'source', 'build', 'test', 'deploy', 'release' | `string` | `null` | no |
| <a name="input_tags"></a> [tags](#input\_tags) | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no |
Expand Down
5 changes: 2 additions & 3 deletions docs/terraform.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,9 @@
| <a name="input_object_lock_configuration"></a> [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. | <pre>object({<br> mode = string # Valid values are GOVERNANCE and COMPLIANCE.<br> days = number<br> years = number<br> })</pre> | `null` | no |
| <a name="input_policy"></a> [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 |
| <a name="input_regex_replace_chars"></a> [regex\_replace\_chars](#input\_regex\_replace\_chars) | Regex to replace chars with empty string in `namespace`, `environment`, `stage` and `name`.<br>If not set, `"/[^a-zA-Z0-9-]/"` is used to remove all characters other than hyphens, letters and digits. | `string` | `null` | no |
| <a name="input_replication_rules"></a> [replication\_rules](#input\_replication\_rules) | Specifies the replication rules if S3 bucket replication is enabled | `list(any)` | `null` | no |
| <a name="input_replication_rules"></a> [replication\_rules](#input\_replication\_rules) | S3 replication rules | `list` | `[]` | no |
| <a name="input_replication_source_roles"></a> [replication\_source\_roles](#input\_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 |
| <a name="input_restrict_public_buckets"></a> [restrict\_public\_buckets](#input\_restrict\_public\_buckets) | Set to `false` to disable the restricting of making the bucket public | `bool` | `true` | no |
| <a name="input_s3_replica_bucket_arn"></a> [s3\_replica\_bucket\_arn](#input\_s3\_replica\_bucket\_arn) | The ARN of the S3 replica bucket (destination) | `string` | `""` | no |
| <a name="input_s3_replication_enabled"></a> [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 |
| <a name="input_sse_algorithm"></a> [sse\_algorithm](#input\_sse\_algorithm) | The server-side encryption algorithm to use. Valid values are `AES256` and `aws:kms` | `string` | `"AES256"` | no |
| <a name="input_stage"></a> [stage](#input\_stage) | Stage, e.g. 'prod', 'staging', 'dev', OR 'source', 'build', 'test', 'deploy', 'release' | `string` | `null` | no |
| <a name="input_tags"></a> [tags](#input\_tags) | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no |
Expand Down
56 changes: 44 additions & 12 deletions main.tf
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
locals {
bucket_name = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id
bucket_name = var.bucket_name != null && var.bucket_name != "" ? var.bucket_name : module.this.id
replication_enabled = length(var.replication_rules) > 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
replication_enabled = length(var.replication_rules) > 0
# the following also takes into account the deprecated var.s3_replication_enabled variable (which is null by default)
replication_enabled = length(var.replication_rules) > 0 || var.s3_replication_enabled == true

bucket_arn = "arn:${data.aws_partition.current.partition}:s3:::${join("", aws_s3_bucket.default.*.id)}"
}
alexjurkiewicz marked this conversation as resolved.
Show resolved Hide resolved

resource "aws_s3_bucket" "default" {
Expand Down Expand Up @@ -139,7 +141,7 @@ 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
Expand All @@ -154,7 +156,7 @@ resource "aws_s3_bucket" "default" {
status = try(rules.value.status, null)

destination {
bucket = var.s3_replica_bucket_arn
bucket = rules.value.destination.bucket
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bucket = rules.value.destination.bucket
# The following takes into account the deprecated var.s3_replica_bucket_arn variable
bucket = try(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)
Expand Down Expand Up @@ -229,7 +231,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 = ["*"]
Expand All @@ -251,7 +253,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 = ["*"]
Expand All @@ -270,13 +272,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 = ["*"]
Expand All @@ -290,6 +289,39 @@ data "aws_iam_policy_document" "bucket_policy" {
}
}
}

dynamic "statement" {
for_each = length(var.replication_source_roles) > 0 ? [1] : []

content {
sid = "CrossAccountReplicationObjects"
actions = [
"s3:ReplicateObject",
"s3:ReplicateDelete",
alexjurkiewicz marked this conversation as resolved.
Show resolved Hide resolved
"s3:ReplicateTags",
"s3:GetObjectVersionTagging",
"s3:ObjectOwnerOverrideToBucketOwner"
]
resources = ["${local.bucket_arn}/*"]
principals {
type = "AWS"
identifiers = var.replication_source_roles
}
}
}
dynamic "statement" {
for_each = length(var.replication_source_roles) > 0 ? [1] : []

content {
sid = "CrossAccountReplicationBucket"
actions = ["s3:List*", "s3:GetBucketVersioning", "s3:PutBucketVersioning"]
resources = [local.bucket_arn]
principals {
type = "AWS"
identifiers = var.replication_source_roles
}
}
}
}

data "aws_iam_policy_document" "aggregated_policy" {
Expand Down
2 changes: 1 addition & 1 deletion outputs.tf
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}

Expand Down
12 changes: 6 additions & 6 deletions replication.tf
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
resource "aws_iam_role" "replication" {
count = module.this.enabled && var.s3_replication_enabled ? 1 : 0
count = module.this.enabled && 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 = module.this.enabled && local.replication_enabled ? 1 : 0

statement {
sid = "AllowPrimaryToAssumeServiceRole"
Expand All @@ -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 = module.this.enabled && 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 = module.this.enabled && local.replication_enabled ? 1 : 0

statement {
sid = "AllowPrimaryToGetReplicationConfiguration"
Expand All @@ -56,12 +56,12 @@ data "aws_iam_policy_document" "replication" {
"s3:ObjectOwnerOverrideToBucketOwner"
]

resources = ["${var.s3_replica_bucket_arn}/*"]
resources = toset([for rule in var.replication_rules : "${rule.destination.bucket}/*"])
}
}

resource "aws_iam_role_policy_attachment" "replication" {
count = module.this.enabled && var.s3_replication_enabled ? 1 : 0
count = module.this.enabled && local.replication_enabled ? 1 : 0
role = aws_iam_role.replication[0].name
policy_arn = aws_iam_policy.replication[0].arn
}
27 changes: 10 additions & 17 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -166,25 +166,14 @@ variable "restrict_public_buckets" {
description = "Set to `false` to disable the restricting of making the bucket public"
}

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`."
}

variable "s3_replica_bucket_arn" {
type = string
default = ""
description = "The ARN of the S3 replica bucket (destination)"
}

Copy link
Member

Choose a reason for hiding this comment

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

I will need to weigh in on @Nuru and @osterman to decide on whether removing these outright is the best upgrade path for our module's users.

In this particular case I feel like var.s3_replication_enabled could be deprecated instead of outright removed, whereas var.s3_replica_bucket_arn cannot be. But in that case, maybe it doesn't make sense to deprecate and keep one without the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know.

As someone who went through the change of lifecycle rules from individual variables to the lifecycle_rules combined structure, my experience was that the transition was significantly harder because the old variables still existed but did nothing. I think it's easier for users to simply break compatibility and describe the migration path in release notes.

Copy link
Member

Choose a reason for hiding this comment

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

Please add this to the bottom of variables.tf

# The variables below are DEPRECATED and should not be used anymore

variable "s3_replication_enabled" {
  type        = bool
  default     = null
  description = "DEPRECATED. Use `replication_rules` instead."
}

variable "s3_replica_bucket_arn" {
  type        = string
  default     = null
  description = "DEPRECATED. Use `replication_rules` instead."
}

variable "replication_rules" {
# type = list(object({
# id = string
# priority = number
# prefix = string
# status = string
# destination = object({
# bucket = string
# storage_class = string
# replica_kms_key_id = string
# access_control_translation = object({
Expand All @@ -202,10 +191,8 @@ variable "replication_rules" {
# tags = map(string)
# })
# }))

type = list(any)
default = null
description = "Specifies the replication rules if S3 bucket replication is enabled"
default = []
description = "S3 replication rules"
}

variable "bucket_name" {
Expand Down Expand Up @@ -235,4 +222,10 @@ variable "website_inputs" {
default = null

description = "Specifies the static website hosting configuration object."
}
}

variable "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)."
}