fix s3 bucket versioning

[imunhatep]

Fixes #76

yet this change will break BC and there is no possibility to suspend bucket versioning.

updated: now it should be back-compatible

[bridgecrew]

Bridgecrew has found 2 infrastructure configuration errors in this PR ⬇️

[bridgecrew]

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

[bridgecrew]

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

[mergify]

This pull request is now in conflict. Could you fix it @imunhatep? 🙏

[mergify]

This pull request is now in conflict. Could you fix it @imunhatep? 🙏

[nitrocode]

Why have two separate boolean variables here?

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

[imunhatep]

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.

[nitrocode]

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.

[nitrocode]

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.

[imunhatep]

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..

[nitrocode]

I think I understand this PR a little more after some discussion with the author.

I'd like to see some changes before I'm comfortable with this PR.

• Clearly show why there are 2 input variables
• Explain in each variable why the other must be toggled
• Mention the suspended state
• Perhaps we can rename bucket_versioning to something more descriptive too.
• I wonder what the use case is that we want bucket_versioning enabled and version_enabled disabled ? We could document that as well.

@imunhatep if you have time.

[korenyoni]

@imunhatep thank you for the submission and sorry for not being able to merge this in (it's been almost a year).

A few things:

1. The extension of the expiration block would need to be updated following Add support for multiple lifecycle rules #85
2. The main purpose of this PR — to solve S3 buckets created with versioning #76 — has already been superseded by Dynamic block for versioning added #118.

I'm going to close this because of 2.

However, this PR also extends the expiration block. Because of this, I've created #130. Feel free to open another PR for #130 , if I do not get to it myself.