provider/aws: Add versioning option to S3 bucket

[kjmkznr]

This commit is allows to versioning configuration to S3 bucket resource.

resource "aws_s3_bucket" "bucket" {
    bucket = "bucket"
    acl = "private"
    versioning = true
}

[apparentlymart]

This seems like a reasonable change and implementation.

A couple thoughts:

• The S3 API for versioning includes another attribute called "MFADelete" that seems to provide extra protection around the permanent deletion of objects. Do you think we should make versioning a child configuration block that contains both of these options? If we make versioning a boolean now then adding support for mfa_delete in future would require it to be a top-level attribute. I don't know much about the S3 versioning feature so I don't know what configuration would be most intuitive to a user.
• In the underlying API it looks like S3 versioning can be in three different states: not enabled, enabled, or suspended. In your implementation you've collapsed "not enabled" and "suspended" into false. I'm not sure whether it's a good idea to abstract away the distinction between these two states, since it seems to have some implications for the behavior of the S3 bucket object operations. What do you think? Could it be better to make this attribute a string that can either be "" (meaning "disabled"), "enabled" or "suspended"?

Just some things to think about, since these decisions will be hard to change later without breaking backward compatibility.

[kjmkznr]

Thanks review.

The S3 API for versioning includes another attribute called "MFADelete" that seems to provide extra protection around the permanent deletion of objects. Do you think we should make versioning a child configuration block that contains both of these options? If we make versioning a boolean now then adding support for mfa_delete in future would require it to be a top-level attribute. I don't know much about the S3 versioning feature so I don't know what configuration would be most intuitive to a user.

If conform the AWS API, I think should make status and mfa_delete in versioning block.
And, I think intuitive versioning block, because "MFA Delete" depends versioning option.
I will try change this.

In the underlying API it looks like S3 versioning can be in three different states: not enabled, enabled,
or suspended. In your implementation you've collapsed "not enabled" and "suspended" into false.
I'm not sure whether it's a good idea to abstract away the distinction between these two states,
since it seems to have some implications for the behavior of the S3 bucket object operations.
What do you think? Could it be better to make this attribute a string that can either be "" (meaning "disabled"),
"enabled" or "suspended"?

I agree make attribute a string.
How about follow plan?

• status attribute can set nil(meaning disabled), "enabled", "suspended".

• If currently status value is "enabled" or "suspended", can't set nil.

  Display error when apply action.
  (I don't know way, how to write to fail in plan action when change to nil from not nil.)

[apparentlymart]

That sounds reasonable to me. IIRC schema attributes can't actually be nil, and if you try to set hem that way it will instead set the "zero value" of the type. So your unset case would actually be the empty string, not literally nil. Then just make sure the attribute is defined as Optional and that it has the empty string as the default.

[radeksimko]

Hi @kjmkznr
Thanks for sending this PR!

Would you mind rebasing last commits from upstream master & resolve conflicts in your branch?

In the underlying API it looks like S3 versioning can be in three different states: not enabled, enabled, or suspended. In your implementation you've collapsed "not enabled" and "suspended" into false. I'm not sure whether it's a good idea to abstract away the distinction between these two states, since it seems to have some implications for the behavior of the S3 bucket object operations.

I actually think that boolean here makes sense. The reason suspended exists is to differentiate between a state when versioning has never been enabled on that bucket (i.e. no past versions exist) and when we just "pause" tracking of versions. There is no way to get back to disabled state once you enable versioning, you can only suspend.
See http://docs.aws.amazon.com/AmazonS3/latest/dev/Versioning.html

Important
Once you version-enable a bucket, it can never return to an unversioned state. You can, however, suspend versioning on that bucket.

I think we should hide this detail away from user, hence I think that default state, when there'll be no versioning block could mean either disabled or suspended, but user doesn't need to be worried about the difference. Terraform should IMO figure out what to do.

versioning.mfa_delete can be a boolean as well and it would be nice to have it as well. I would not worry too much about dependencies between these two since we don't have mechanisms how to define & check this inside schema yet.

To clarify, this would be adequate to default state, when user doesn't define versioning at all:

versioning {
  enabled = false
  mfa_delete = false
}

Let me know if there's anything else you need to finish this up, so we can merge it. 😉

[radeksimko]

Any reason why keep this Computed: true?

[kjmkznr]

Sorry, I mistake it.

[kjmkznr]

@radeksimko
Thanks review.

I rebase upstream master and resolve conflicts.
And, changed to versioning block and enabled bool from single versioning bool value.

versioning.enabled has three states.

1. disabled
   This state is undefined versioning.enabled.
2. enabled
   This state is versioning.enabled=true
3. suspended
   This state is versioning.enabled=false or removed versioning block.

[radeksimko]

This is now reported by go vet:

builtin/providers/aws/resource_aws_s3_bucket.go:108: arg m["enabled"].(bool) for printf verb %d of wrong type: bool

the right modifier is %t.

[kjmkznr]

Fixed!

[radeksimko]

Schema looks ok, do you plan adding mfa_delete? If not that's 👌 , we can add it later.
Also it would be great if you could update related docs. 😉

Besides that one vet nitpick it's overall OK.

[kjmkznr]

@radeksimko

I updated S3 bucket resource document.

I don't have adding mfa_delete plan.
mfa_delete needs authentication device's serial number and displayed value.
I don't know how to pass value to command. Sorry.

[radeksimko]

LGTM 👍

[kjmkznr]

Thanks!

[ghost]

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.