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.

Sign up for GitHub

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

fix s3 bucket versioning #77

Closed
wants to merge 3 commits into from
+36 −6
Closed

fix s3 bucket versioning #77

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84c5b48
fix s3 bucket versioning
Feb 16, 2021
e8cb547
update from upstream/master
Feb 23, 2021
efae4f0
Auto Format
cloudpossebot Feb 23, 2021
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: 4 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ Available targets:
| attributes | Additional attributes (e.g. `1`) | `list(string)` | `[]` | no |
| block\_public\_acls | Set to `false` to disable the blocking of new public access lists on the bucket | `bool` | `true` | no |
| block\_public\_policy | Set to `false` to disable the blocking of new public policies on the bucket | `bool` | `true` | no |
| bucket\_versioning | A state of versioning. Versioning is a means of keeping multiple variants of an object in the same bucket | `bool` | `true` | no |
| context | Single object for setting entire context at once.<br>See description of individual variables for details.<br>Leave string and numeric variables as `null` to use default value.<br>Individual variable settings (non-null) override settings in context object,<br>except for attributes, tags, and additional\_tag\_map, which are merged. | `any` | <pre>{<br> "additional_tag_map": {},<br> "attributes": [],<br> "delimiter": null,<br> "enabled": true,<br> "environment": null,<br> "id_length_limit": null,<br> "label_key_case": null,<br> "label_order": [],<br> "label_value_case": null,<br> "name": null,<br> "namespace": null,<br> "regex_replace_chars": null,<br> "stage": null,<br> "tags": {}<br>}</pre> | no |
| cors\_rule\_inputs | Specifies the allowed headers, methods, origins and exposed headers when using CORS on this bucket | <pre>list(object({<br> allowed_headers = list(string)<br> allowed_methods = list(string)<br> allowed_origins = list(string)<br> expose_headers = list(string)<br> max_age_seconds = number<br> }))</pre> | `null` | no |
| deeparchive\_transition\_days | Number of days after which to move the data to the glacier deep archive storage tier | `number` | `90` | no |
Expand All @@ -227,7 +228,9 @@ Available targets:
| enable\_standard\_ia\_transition | Enables the transition to STANDARD\_IA | `bool` | `false` | no |
| enabled | Set to false to prevent the module from creating any resources | `bool` | `null` | no |
| environment | Environment, e.g. 'uw2', 'us-west-2', OR 'prod', 'staging', 'dev', 'UAT' | `string` | `null` | no |
| expiration\_date | Specifies the date after which you want the corresponding action to take effect | `string` | `null` | no |
| expiration\_days | Number of days after which to expunge the objects | `number` | `90` | no |
| expiration\_expired\_object\_delete\_marker | Direct Amazon S3 to delete expired object delete markers. This cannot be specified with Days or Date in a Lifecycle Expiration Policy | `bool` | `false` | no |
| force\_destroy | A boolean string that indicates all objects should be deleted from the bucket so that the bucket can be destroyed without error. These objects are not recoverable | `bool` | `false` | no |
| glacier\_transition\_days | Number of days after which to move the data to the glacier storage tier | `number` | `60` | no |
| grants | An ACL policy grant. Conflicts with `acl`. Set `acl` to `null` to use this. | <pre>list(object({<br> id = string<br> type = string<br> permissions = list(string)<br> uri = string<br> }))</pre> | `null` | no |
Expand Down Expand Up @@ -257,7 +260,7 @@ Available targets:
| standard\_transition\_days | Number of days to persist in the standard storage tier before moving to the infrequent access tier | `number` | `30` | no |
| tags | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no |
| user\_enabled | Set to `true` to create an IAM user with permission to access the bucket | `bool` | `false` | no |
| versioning\_enabled | A state of versioning. Versioning is a means of keeping multiple variants of an object in the same bucket | `bool` | `true` | no |
| versioning\_enabled | A state of versioning. Controls versioning state enabled/suspended | `bool` | `true` | no |

## Outputs

Expand Down
5 changes: 4 additions & 1 deletion docs/terraform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
| attributes | Additional attributes (e.g. `1`) | `list(string)` | `[]` | no |
| block\_public\_acls | Set to `false` to disable the blocking of new public access lists on the bucket | `bool` | `true` | no |
| block\_public\_policy | Set to `false` to disable the blocking of new public policies on the bucket | `bool` | `true` | no |
| bucket\_versioning | A state of versioning. Versioning is a means of keeping multiple variants of an object in the same bucket | `bool` | `true` | no |
| context | Single object for setting entire context at once.<br>See description of individual variables for details.<br>Leave string and numeric variables as `null` to use default value.<br>Individual variable settings (non-null) override settings in context object,<br>except for attributes, tags, and additional\_tag\_map, which are merged. | `any` | <pre>{<br> "additional_tag_map": {},<br> "attributes": [],<br> "delimiter": null,<br> "enabled": true,<br> "environment": null,<br> "id_length_limit": null,<br> "label_key_case": null,<br> "label_order": [],<br> "label_value_case": null,<br> "name": null,<br> "namespace": null,<br> "regex_replace_chars": null,<br> "stage": null,<br> "tags": {}<br>}</pre> | no |
| cors\_rule\_inputs | Specifies the allowed headers, methods, origins and exposed headers when using CORS on this bucket | <pre>list(object({<br> allowed_headers = list(string)<br> allowed_methods = list(string)<br> allowed_origins = list(string)<br> expose_headers = list(string)<br> max_age_seconds = number<br> }))</pre> | `null` | no |
| deeparchive\_transition\_days | Number of days after which to move the data to the glacier deep archive storage tier | `number` | `90` | no |
Expand All @@ -54,7 +55,9 @@
| enable\_standard\_ia\_transition | Enables the transition to STANDARD\_IA | `bool` | `false` | no |
| enabled | Set to false to prevent the module from creating any resources | `bool` | `null` | no |
| environment | Environment, e.g. 'uw2', 'us-west-2', OR 'prod', 'staging', 'dev', 'UAT' | `string` | `null` | no |
| expiration\_date | Specifies the date after which you want the corresponding action to take effect | `string` | `null` | no |
| expiration\_days | Number of days after which to expunge the objects | `number` | `90` | no |
| expiration\_expired\_object\_delete\_marker | Direct Amazon S3 to delete expired object delete markers. This cannot be specified with Days or Date in a Lifecycle Expiration Policy | `bool` | `false` | no |
| force\_destroy | A boolean string that indicates all objects should be deleted from the bucket so that the bucket can be destroyed without error. These objects are not recoverable | `bool` | `false` | no |
| glacier\_transition\_days | Number of days after which to move the data to the glacier storage tier | `number` | `60` | no |
| grants | An ACL policy grant. Conflicts with `acl`. Set `acl` to `null` to use this. | <pre>list(object({<br> id = string<br> type = string<br> permissions = list(string)<br> uri = string<br> }))</pre> | `null` | no |
Expand Down Expand Up @@ -84,7 +87,7 @@
| standard\_transition\_days | Number of days to persist in the standard storage tier before moving to the infrequent access tier | `number` | `30` | no |
| tags | Additional tags (e.g. `map('BusinessUnit','XYZ')` | `map(string)` | `{}` | no |
| user\_enabled | Set to `true` to create an IAM user with permission to access the bucket | `bool` | `false` | no |
| versioning\_enabled | A state of versioning. Versioning is a means of keeping multiple variants of an object in the same bucket | `bool` | `true` | no |
| versioning\_enabled | A state of versioning. Controls versioning state enabled/suspended | `bool` | `true` | no |

## Outputs

Expand Down
12 changes: 9 additions & 3 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,12 @@ resource "aws_s3_bucket" "default" {
policy = var.policy
tags = module.this.tags

versioning {
enabled = var.versioning_enabled
dynamic "versioning" {
Copy link

Choose a reason for hiding this comment

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

Error Description: Ensure all data stored in the S3 bucket is securely encrypted at rest
Category: Storage | Severity: HIGH
Resource: aws_s3_bucket [default], lines: 1 - 156

Copy link

Choose a reason for hiding this comment

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

Error Description: Ensure all data stored in the S3 bucket have versioning enabled
Category: Storage | Severity: HIGH
Resource: aws_s3_bucket [default], lines: 1 - 156

for_each = var.bucket_versioning ? [1] : [0]
Copy link
Member

@nitrocode nitrocode Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why have two separate boolean variables here?

Let's use the original version_enabled in the dynamic and when setting the value

Copy link
Author

Choose a reason for hiding this comment

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

Long time ago, but as I remember... cause versioning can be enabled or disabled for bucket. In case versioning is enabled, it could be set to suspended.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's relevant here tho as you can still suspend versioning on the bucket by toggling version_enabled = false if versioning_enabled was used in the for_each instead of bucket_versioning.

Copy link
Member

@nitrocode nitrocode Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm after fully understanding the aws provider issue you linked to, perhaps I'm mistaken. This seems to be a workaround for an upstream bug but I don't know if we want to support this workaround or wait for the bug to be fixed upstream.

Copy link
Author

Choose a reason for hiding this comment

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

If you are referring to aws provider issue, well it's more then 3y old, I wouldn't expect it to be fixed. And as I mentioned there are 4 states of versioning for S3 buckets, aws provider seems reflects exactly that behavior. So I'm not sure if it's a bug at all..


content {
enabled = var.versioning_enabled
}
}

lifecycle_rule {
Expand Down Expand Up @@ -74,7 +78,9 @@ resource "aws_s3_bucket" "default" {
for_each = var.enable_current_object_expiration ? [1] : []

content {
days = var.expiration_days
days = var.expiration_days
date = var.expiration_date
expired_object_delete_marker = var.expiration_expired_object_delete_marker
}
}
}
Expand Down
20 changes: 19 additions & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,18 @@ variable "force_destroy" {
description = "A boolean string that indicates all objects should be deleted from the bucket so that the bucket can be destroyed without error. These objects are not recoverable"
}

variable "versioning_enabled" {
variable "bucket_versioning" {
type = bool
default = true
description = "A state of versioning. Versioning is a means of keeping multiple variants of an object in the same bucket"
}

variable "versioning_enabled" {
type = bool
default = true
description = "A state of versioning. Controls versioning state enabled/suspended"
}

variable "logging" {
type = object({
bucket_name = string
Expand Down Expand Up @@ -163,6 +169,18 @@ variable "expiration_days" {
description = "Number of days after which to expunge the objects"
}

variable "expiration_date" {
type = string
default = null
description = "Specifies the date after which you want the corresponding action to take effect"
}

variable "expiration_expired_object_delete_marker" {
type = bool
default = false
description = "Direct Amazon S3 to delete expired object delete markers. This cannot be specified with Days or Date in a Lifecycle Expiration Policy"
}

variable "abort_incomplete_multipart_upload_days" {
type = number
default = 5
Expand Down