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

aws_s3_bucket_versioning cannot disable versioning #23718

Closed
skeggse opened this issue Mar 16, 2022 · 7 comments · Fixed by #23723
Closed

aws_s3_bucket_versioning cannot disable versioning #23718

skeggse opened this issue Mar 16, 2022 · 7 comments · Fixed by #23723
Labels
bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. upstream Addresses functionality related to the cloud provider.
Milestone

Comments

@skeggse
Copy link
Contributor

skeggse commented Mar 16, 2022

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform CLI and Terraform AWS Provider Version

Terraform v1.0.1
on darwin_amd64
+ provider registry.terraform.io/hashicorp/aws v4.5.0

Affected Resource(s)

  • aws_s3_bucket_versioning

Terraform Configuration Files

There is no valid Terraform configuration corresponding to

resource "aws_s3_bucket_versioning" "bucket" {
  bucket = aws_s3_bucket.bucket.bucket

  versioning_configuration {
    status = "Disabled"
  }
}

Debug Output

N/A

Panic Output

N/A

Expected Behavior

The resource should support a disabled state for bucket versioning. The only way to handle such buckets in v4 appears to be to omit the resource entirely, which results in the versioning being unmanaged rather than actively disabled. Despite the fact that the action operation to transition a bucket from versioned to unversioned isn't supported by AWS, it's still important to track these changes using the standardized tooling that Terraform provides. Moreover, this blocks imports of existing buckets during the migration process lazily forced upon Terraform users in #23106, and makes it impossible to check for drift in that portion of a bucket's configuration.

Actual Behavior

╷
│ Error: expected versioning_configuration.0.status to be one of [Enabled Suspended], got Disabled
│
│   with aws_s3_bucket_versioning.bucket,
│   on test.tf line 5, in resource "aws_s3_bucket_versioning" "bucket":
│    5:     status = "Disabled"

Steps to Reproduce

  1. terraform apply

Important Factoids

N/A

References

@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Mar 16, 2022
@anGie44 anGie44 added service/s3 Issues and PRs that pertain to the s3 service. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 16, 2022
@anGie44
Copy link
Contributor

anGie44 commented Mar 16, 2022

Hi @skeggse , thank you for raising this issue. From what I've gathered from the AWS documentation (https://docs.aws.amazon.com/AmazonS3/latest/userguide/Versioning.html#versioning-states) , versioning cannot be disabled but MfaDelete for example can.

Buckets can be in one of three states:

Unversioned (the default)

Versioning-enabled

Versioning-suspended

You enable and suspend versioning at the bucket level. After you version-enable a bucket, it can never return to an unversioned state. But you can suspend versioning on that bucket.

Perhaps versioning can be disabled via a request to AWS Support though.

@anGie44 anGie44 added the upstream Addresses functionality related to the cloud provider. label Mar 16, 2022
@skeggse
Copy link
Contributor Author

skeggse commented Mar 16, 2022

@anGie44 Thanks for clarifying. I see the problem, though I think I perhaps mis-stated my need. I'm less concerned about actually disabling bucket versioning, and more concerned about detecting that drift: sure, if we have to make the change manually, we'll do that, but if we don't even know that a drift has occurred, then we don't know that an action needs to be taken.

Can we add a disabled state anyway, even though we know it won't work? That way, Terraform's core functionality - the diff - still works.

As an aside, I'm also seeing that we can't import a versioning resource if the bucket is unversioned, which is a related problem - I'd like to be able to import these resources for unversioned buckets.

@anGie44
Copy link
Contributor

anGie44 commented Mar 17, 2022

I see what you mean now, thanks for the additional info @skeggse . So the GetBucketVersioning API method will actually return a non-nil output when the bucket is unversioned/disabled and has an empty string status which we can for our purposes rebrand to something like "Unversioned". We can include this new "state" as a valid target state, that way every time the resource is refreshed, versioning_configuration will be stored in state as the following instead of timing out with Error: error waiting for S3 Bucket Versioning status for bucket (tf-test-bucket-yada-yada): timeout while waiting for state to become 'Enabled, Suspended' (timeout: 1m0s)

 "versioning": [
130               {
131                 "enabled": false,
132                 "mfa_delete": false
133               }
134             ],

I wonder though, if we can better represent the "disabled" state of bucket versioning by using a data source instead? that way you could detect a change and not worry about the singular data source erroring out.

Because the change described above would affect the UX for create/update vs. import, e.g.

  1. we'll need to allow versioning_configuration to be Optional to support terraform plan/apply after import but it cannot be left unspecified during create/update or alternatively,
  2. we could just add "Unversioned" as a valid state to the versioning_configiuration.status parameter, but it cannot be used on "update", and on create we'll need to skip the "PutBucketVersioning" call

, I'd like wait and see more community interest before working on potential code changes within the aws_s3_bucket_versioning and am also happy to hear other potential solutions you or the community may have.

@skeggse
Copy link
Contributor Author

skeggse commented Mar 17, 2022

I wonder though, if we can better represent the "disabled" state of bucket versioning by using a data source instead? that way you could detect a change and not worry about the singular data source erroring out.

I'm not familiar with this pattern of tracking resource changes - if the data source has changed, Terraform will still say that there are no changes, because it's a data source instead of a resource, right? So Terraform would prompt no action, and the change would go unnoticed.

If I'm not mistaken, this is a regression from the aws provider v3, because we used to be able to see this change:

Terraform will perform the following actions:

  # aws_s3_bucket.bucket will be updated in-place
  ~ resource "aws_s3_bucket" "bucket" {
        # (11 unchanged attributes hidden)

      ~ versioning {
          ~ enabled    = true -> false
            # (1 unchanged attribute hidden)
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

I'm not sure I understand why we overhauled the buckets in v4, and it seems weird that we would have done that while also taking steps backwards.

In light of this being a regression, I'd like to consider a quicker fix. We're trying to upgrade from v3 to v4, and this is a blocker. It's odd to me that we shipped this regression.

@anGie44
Copy link
Contributor

anGie44 commented Mar 17, 2022

In light of this being a regression, I'd like to consider a quicker fix. We're trying to upgrade from v3 to v4, and this is a blocker. It's odd to me that we shipped this regression.

Not to worry @skeggse , please refer to #23106. We're currently working on moving the aws_s3_bucket resource back to how it was previously as suggested in #2 of Currently Investigating (e.g versioning in this example will be configurable again), so the upgrade shoudn't be as painful for practitioners as is currently.

In the meantime, I'll keep this open as a bug . do you mind updating the description to better reflect the problem around drift detection/import?

Update: Fix ready for review after looking more into the previous versions of the s3 bucket and changes made in #4494. @skeggse , if you have any feedback on the documentation changes as well , reach out 👍

@github-actions
Copy link

This functionality has been released in v4.6.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented May 7, 2022

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/s3 Issues and PRs that pertain to the s3 service. upstream Addresses functionality related to the cloud provider.
Projects
None yet
2 participants