Moto: Implement S3 CloudFormation Update Support

[jweite]

On CloudFormation update_stack operation with a stack defining an S3 bucket:

AttributeError: type object 'FakeBucket' has no attribute 'update_from_cloudformation_json'

[jweite]

@bblommers, this one's got more worms in the can than I anticipated. AFAICT moto CloudFormation support doesn't know about the three different update behaviors (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-updating-stacks-update-behaviors.html). It seems to treat them all as _Replacements_:

https://github.com/spulec/moto/blob/6adee0cbaf7a1971fff8809cc07dc4b23a08cc2e/moto/cloudformation/parsing.py#L340 always slams-in a new resource name regardless of whether the template is using any Replacement-update-type properties.

Handing this flexibly would be an interesting challenge. Backend models would need an opportunity to weigh-in on the type of update a template's properties imply. I'm thinking to declare a well-known method name for this that the cfn code can call on the model to ask it to evaluate the template properties. If the method's not present (as it wouldn't be for most backend models) then moto assumes Replacement for backward compatibility.

1) Does this make sense to you?
2) Is there a better way?
3) How would others know to start providing this model method?

[jweite]

Alternatively, I considered whether the model class' update_from_cloudformation_json could make the decision. The rub is that it can't tell if the name attribute in the CloudFormation json was provided by the requester or added by moto. If the caller provided it, then we need to consider it when deciding if this is a replacment-update. (Name's usually are). If moto provided it, then we need to ignore it when deciding.

[bblommers]

@jweite

I'm thinking to declare a well-known method name for this that the cfn code can call on the model to ask it to evaluate the template properties

This is the best way to go, IMO. There are too many resource-types to have that logic centralized in one place.
There's a draft PR I started for another issue, that you might be able to use as a base: #3185
Would be good to make the 'well_known_method' (whatever the name will be) also part of the CloudFormationModel-class

[jweite]

@bblommers, I read through your draft PR commit and get where you're coming from. Makes sense to me, and I can see how it can cover a more delegated approach to other CloudFormation processing needs, like determining update-type.

While I'd like to push forward with this issue, I don't want us to be cursing each other out at merge time. Do you have any ETA for merging your PR 3127 work? As a draft I imagine you wanted to incubate the idea a while. I don't want to get ahead of that process by adding _CloudFormationModel_ (with an _evaluate_cloudformation_update_type()_ method) before you do.

Regardless of base class, CloudFormation code would still have to use duck-type methods to see if a model actually implemented evaluate_cloudformation_update_type() and fall back to today's behavior if it didn't, so I think I could conceivable proceed independently of your work.

---

In related things, I've been mulling the CloudFormation code's behavior that I described above, where it adds a _name_ attribute to the _cloudformation_json_ if it's not present, and I disagree with that approach: it prevents the model from knowing if the user provided the name param, or if moto did, which is important information when deciding the update type. It's also unnecessary, as a _new_resource_name_ parameter is passed to update_from_cloudformation_json().

To get a sense of how sprawling the dependencies are on this behavior I commented it out and ran the unit tests. Could be worse:

ERROR: tests.test_cloudformation.test_cloudformation_depends_on.test_create_stack_with_depends_on
ERROR: tests.test_cloudformation.test_cloudformation_depends_on.test_create_stack_with_depends_on_string
ERROR: tests.test_cloudformation.test_cloudformation_stack_crud_boto3.test_non_json_redrive_policy
ERROR: tests.test_cloudformation.test_cloudformation_stack_integration.test_autoscaling_propagate_tags
ERROR: tests.test_cloudformation.test_cloudformation_stack_integration.test_dynamodb_table_creation
ERROR: tests.test_cloudformation.test_stack_parsing.test_parse_stack_with_name_type_resource
ERROR: tests.test_cloudformation.test_stack_parsing.test_parse_stack_with_tabbed_json_template
ERROR: tests.test_cloudformation.test_stack_parsing.test_parse_stack_with_yaml_template
ERROR: test_elbv2.test_create_target_groups_through_cloudformation
FAIL: tests.test_cloudformation.test_cloudformation_depends_on.test_create_chained_depends_on_stack
FAIL: tests.test_cloudformation.test_cloudformation_stack_integration.test_iam_roles

What do you think about discontinuing the practice of adding a name to the cloudformation_json and relying on _new_resource_name_ where a generated name is needed?

[bblommers]

Hi @jweite, I created the draft PR as a starting point for another user, so I can't' comment on an ETA for their work.
As far as I'm concerned, you can just go ahead whenever - I'll resolve any merge conflicts afterwards, and promise not to curse :)

Regarding the new_resource_name: adding the name to the properties is done for both create and update. It's not necessary for updates, but it is necessary for creation - the name isn't always supplied by the user on creation.

If there's a way to split this logic cleanly, and only add it on create, I'm all for it.

[jweite]

Easily done by adding an _add_name_to_resource_json param_ to _parse_resource()_, defaulting it to True, and re-orging the conditional logic therein to respect it. When _parse_and_update_resource()_ is modified to add that param as False, I get my desired update behavior, existing create behavior remains, and all unit tests pass. I appreciate your guidance here! Made it easier than I'd imagined.

[bblommers]

This is now part of moto >= 1.3.15.dev1017