aws-cloudformation / cloudformation-cli

The CloudFormation Provider Development Toolkit allows you to author your own resource providers and modules that can be used by CloudFormation.
Apache License 2.0
319 stars 161 forks source link

Add current state identifiers to Read/Delete invocation #584

Open eduardomourar opened 4 years ago

eduardomourar commented 4 years ago

There are scenarios where the primary identifier is created by the resource provider itself. For those situations, you need to check another information from the current state (e.g. a name attribute) because usually the underlying AWS API does not know anything about the primary ID.

So I propose that the previous state property be filled with the current state by the upstream service, otherwise there is no real way to retrieve the state stored in CloudFormation during read.

You can find a little bit more context here: https://github.com/org-formation/aws-resource-providers/issues/26

eduardomourar commented 4 years ago

This will also apply when Delete is invoked, and to List most probably.

benbridts commented 4 years ago

Another solution to the same problem would be to offer managed state storage within the resource providers. Eg. by adding a ResourceContext key to the progressEvent, that works similar to the CalbackContext, but persists between handlers.

Neither solution works well if you want to be able to use the ReadHandler outside of CloudFormation (eg. in AWS Config), although the resourceContext would at least decouple it from a specific template.

eduardomourar commented 4 years ago

@ikben your suggestion makes sense for additional information outside the model definition. For the properties within model, we know the state is saved within CloudFormation itself, so it could/should be simple.

benbridts commented 4 years ago

@eduardomourar yes. Although with things like Import and ChangeSets, "data from the CloudFormation stack" is not always that straight forward. In general depending on the stack itself for state is tricky.

eduardomourar commented 4 years ago

I have run into a specific problem while implementing CodeCommit approval rule template resource type. That services stores a unique identifier, but most API calls you can only query using the name. So I just need a easy way to go from a primary id to a name, but the rest of the state is being retrieved directly from the target service and not from the stack itself.

benbridts commented 4 years ago

I'm not saying you can't do it the right way, just pointing out the possible limitations / user error when you rely on the resource being tied to a template

(in your example I would say it's a failure in the CodeCommit API - and you can probably use the Name as the PrimaryIdentifier, and the Id as an additionalIdentifier as a workaround)

eduardomourar commented 4 years ago

@ikben I completely agree with you and that the failure is in the CodeCommit API. The problem is that the Name can be changed (more info here), so I had to go with ID as primary identifier. Also, you can see from the other discussion around SSO principal assignments, where we have a more complex model, and there we need to store a list of UUIDs. So I believe we could still rely on CloudFormation to store a mapping between a hash (the primary identifier) and that set of UUIDs instead of having to create a S3 bucket for that reason.

benkehoe commented 4 years ago

While I'd like this in general, there's a major flaw here. The list handler is supposed to return primary identifiers based on at most a resource spec to filter down the outputs. Additionally, the read handler is intended to be used in resource import situations, where there is no input other than the primary identifier (which the user may have selected from the output of a list operation). So it's not possible to always have the read handler receive properties/state.

For https://github.com/org-formation/aws-resource-providers/issues/26 it would be mighty difficult to implement a list handler, because we'd have no idea of the kinds of grouping the user might want.

eduardomourar commented 4 years ago

For the list handler, I don't have a complete understanding of its use currently. But, during an import process, we could ask the user for the additional identifiers (in optional free text field), right?

benbridts commented 4 years ago

@eduardomourar Additional identifiers are seen as equivalent to the primary identifier, according to the contract:

The input to a read handler MUST contain either the primaryIdentifier or an additionalIdentifier. (emphasis mine, link)

One of the challenges is that it's required to have a read handler, while the list handler is not required. I think that in general, it's a good idea to require that (and it might be(come) needed by the service - see quote below), but it runs into issues with edge cases like this.

If the resource provider does not contain create, read, and delete handlers, CloudFormation cannot actually provision the resource. (link)

read: CloudFormation invokes this handler as part of a stack update operation when detailed information about the resource's current state is required (link)


@benkehoe

So it's not possible to always have the read handler receive properties/state

If the resource represents something that's "virtual" and only manageable by CloudFormation, that does not have to be a problem. It would still depend on a central place (either CFN managed or Resource Provider managed) to store state Eg.

In the case of the Community::SSO::AssignmentGroup, that would mean that it's either impossible to import existing assignments, or that it requires out-of-band access to the data store (to create an "orphaned" resource/state).

The same limitation exists today in AWS::Route53::RecordSetGroup, which will fail if one of the records already exists (it doesn't support import at all, at this point. neither does AWS::Route53::RecordSet)