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

Dynamic block for versioning added #118

Merged
merged 2 commits into from
Nov 12, 2021
Merged

Dynamic block for versioning added #118

merged 2 commits into from
Nov 12, 2021

Conversation

nnsense
Copy link
Contributor

@nnsense nnsense commented Nov 12, 2021

what

Enabling versioning on a bucket is a permanent action that cannot be disabled. For this reason, when versioning attribute is added to the s3 resource, the bucket is prepared to be versioned and put in suspended mode. The only way to avoid this and keep the versioning disabled is to not add the versioning attribute at all.

We were discussing this in this bug and @aknysh posted a snipped which is removing the attribute, making it possible to set versioning off instead of enabled but suspended.

I'm just adding that snippet, there's another PR which is apparently changing more than just the versioning and it seems abandoned (opened in February 2021, had no updates from August).

Note: there's a comment into this module's main:

#bridgecrew:skip=BC_AWS_S3_16:Skipping `Ensure S3 bucket versioning is enabled` because dynamic blocks are not supported by checkov

But I see that some basic handling for dynamic blocks has been added in checkov
bridgecrewio/checkov#836

So if you're using checkov it would be interesting to remove the comment and see if it works now

why

  • In a very quick deployment, where versioning is less important than speed, having a delay before an object can be written could be an issue (See the note here)
  • If the buckets are created by terraform and deleted by a script, the versioned bucket's deletion is much more complex than a non-versioned one.
  • User's preference

@nnsense nnsense requested review from a team as code owners November 12, 2021 19:02
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found infrastructure configuration errors in this PR ⬇️

main.tf Show resolved Hide resolved
main.tf Show resolved Hide resolved
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to 86c9124 - Auto Format - 1 new error was added

Change details

Error ID Change Path Resource
BC_AWS_NETWORKING_52 Added /main.tf aws_s3_bucket.default

@aknysh
Copy link
Member

aknysh commented Nov 12, 2021

/test all

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @nnsense

@aknysh aknysh added the patch A minor, backward compatible change label Nov 12, 2021
@aknysh aknysh merged commit 6947cac into cloudposse:master Nov 12, 2021
@nnsense
Copy link
Contributor Author

nnsense commented Nov 12, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants