Variable prevent_unencrypted_uploads is possibly poorly named

[pazaan]

Describe the Feature

Since apply_server_side_encryption_by_default is always set, and the EnforceTlsRequestsOnly policy is created, uploads will always be encrypted in transit and at rest.
The way I read the behavior of the code, prevent_unencrypted_uploads is simply enforcing that all uploads must specify an at-rest encryption key, and therefore bypass the default encryption.

Expected Behavior

Would a better name be prevent_default_encryption? Or modify the documentation in the README to better describe the functionality. The way it reads now, it sounds like without prevent_unencrypted_uploads, uploads would be unencrypted.

Use Case

When using the module, I couldn't upload files to the Terraform state without specifying a key (including using the AWS Console). When I disabled prevent_default_encryption, I confirmed that the file I uploaded was indeed encrypted with the default encryption key (SSE-S3).

Describe Ideal Solution

Either change the variable name, or the accompanying documentation in the README so that new users of the module don't need to read the code in order to understand the behavior.

Alternatives Considered

No response

Additional Context

No response

[Gowiem]

@pazaan an update to the description of the variable that helps clarify this sounds like a good call -- Mind putting up a PR that you think would address that?

We could change the name of the variable, but that'll introduce a major rev and that's something we like to avoid. Let's get better about the description and hopefully that will help others avoid confusion 👍

[Nuru]

@pazaan I believe you are wrong about the impact of the setting. Although I agree the setting has been made essentially irrelevant because now all S3 buckets are encrypted, the setting does not require that you use a specific KMS key. It merely requires that you specify encryption as part of the request. When set:

aws s3 cp test.txt s3://mybucket/test2.txt # fails
aws s3 cp test.txt s3://mybucket/test2.txt --sse # succeeds

With that, I am inclined to close this issue as "invalid", but remain open to persuasion.

[pazaan]

@Nuru I don't mind closing this for myself, because I've already gone through the rigmarole of determining the underlying behaviour. I just assumed that because I was confused by the naming of the variable that others might be too.
In the PR I submitted, the documentation has been modified to say exactly what you have stated above - it's just making it clear in the documentation rather than requiring end users to look at the code to determine the underlying behaviour.