resource/aws_s3_bucket: Mark replication_configuration rules id attribute as required

[bflad]

Closes #665

The rules under replication_configuration are currently TypeSet with the hash computed including the ID attribute when available. However, when creating a rule that omits the ID attribute (allowing AWS to generate the ID), the plan will always show a difference due to the hash changing. Rather than deal with a state migration to TypeList on a critical resource for seemingly little benefit, set this attribute as Required as the least effort option to make the resource configuration less surprising.

I do not believe this needs to be marked as a breaking change, due to the current behavior.

If there are better ways to handle this via the Set function or some other way, please let me know!

[radeksimko]

Hi @bflad
I'd be ok with making it Required if there's a way to modify the config to comply with this change. I don't believe there's an easy way at this point. I'd have to go to AWSCLI/API/Console to find the ID to paste it into config after the replication was enabled, unless I'm mistaken?

---

We have historically kept all of S3 bucket-related functionality inside aws_s3_bucket which has pros and cons.

Pros include easier protection against effects of eventual consistency (because all retries are in a single place and happen roughly at the same time sequentially: https://github.com/terraform-providers/terraform-provider-aws/pull/891/files ).

Cons include that it's trickier to address bugs like this one. Having a separate resource like aws_s3_bucket_replication_configuration would IMO help here.

We discussed this together in #916 in the past and this PR is bringing more arguments to that discussion - thank you! 👍

The transition towards the state won't be easy because we'll possibly have to deprecate a bunch of fields and/or revisit the idea of nested resources where the CRUD is shared.

---

Discussions aside I think in short-term we could theoretically make this field Computed and omit it from the Set hash, so it's not raising spurious diffs.

Let me know what you think.

[bflad]

I believe your assessment of how to "fix" the spurious diff when id is omitted is correct. Essentially we require it at the moment, either you setting it upfront or finding it after the fact and setting it in the configuration to prevent the diff.

Would changing the Set hash to omit id be a "breaking" change for folks who already have rules with id configured in their hash (e.g. plan diff to recreate the same rules with new hashes)? I was thinking that would require a state migration to not show the one time diff.

I'm certainly not opposed to doing it the better way here with state migration, just requires more effort and testing. :)

[bflad]

Closing this one out for now, I don't have time to properly adjust it.

[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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!