aws-cloudformation / aws-cloudformation-resource-providers-cloudformation

The CloudFormation Resource Provider Package For AWS CloudFormation
https://aws.amazon.com/cloudformation/
Apache License 2.0
48 stars 35 forks source link

AWS::CloudFormation::ResourceVersion - Define createOnlyProperties #24

Closed opalelement closed 3 years ago

opalelement commented 3 years ago

Description of changes:

Every registration of a CloudFormation Resource Provider creates a new version, which is identified by a version-specific ARN. This means there isn't technically an "update" of a resource provider; each registration is a new distinct resource. This was accounted for by excluding an UpdateHandler for this AWS::CloudFormation::ResourceVersion resource provider.

The schema for this resource provider does not define any createOnlyProperties. Therefore, any attempt to use this resource provider to register another resource provider would only work during the CREATE; any attempt to UPDATE the resource (e.g. changing SchemaHandlerPackage to an updated source package) would try to update the resource rather than triggering a replacement, which would fail due to the lack of an UpdateHandler (the resource would enter the UPDATE_FAILED state with a message of "Internal Failure", and no logs would be present in CloudWatch Logs).

Since changing any of the writable Properties of this resource provider results in a new physical resource (identified in the resource provider as the versioned Arn), all of these properties must be createOnlyProperties to force a replacement rather than an update. In essence, this resource is logically very similar to AWS::ECS::TaskDefinition - both resources use a versioned identifier, and any change results in a new version. Any change to a TaskDefinition results in resource replacement, followed by unregistration of the previous version during stack cleanup. This PR results in AWS::CloudFormation::ResourceVersion logically behaving the same.

One caveat is that AWS::CloudFormation::ResourceVersion really needs to be used in tandem with the AWS::CloudFormation::ResourceDefaultVersion resource to automatically update the default version. Otherwise, if a created ResourceVersion is the default version for that type (e.g. the CREATE was the first registration of a new/deleted resource type, or SetTypeDefaultVersion was used externally), an update will fail to delete the previous resource during the stack cleanup stage, resulting in an error of This type has more than one active version. Please deregister non-default active versions before attempting to deregister the type..

I also re-ordered the readOnlyProperties to be alphabetical, which matches the order in which they are defined in /properties.

ugudip commented 3 years ago

And regarding the resource version being the default version when created for the first time, yes I agree with you that the cleanup won't be successful here, but this is expected and the resource that failed to be deleted is the leaked resource. That's why we are expected to create a default version for every type created. Also, this doesn't cause any update failure because the creation was successful, only the clean up was not.

opalelement commented 3 years ago

@ugudip: I need a small clarification on this line "any attempt to UPDATE the resource (e.g. changing SchemaHandlerPackage to an updated source package) would try to update the resource rather than triggering a replacement, which would fail due to the lack of an UpdateHandler". I don't think this is happening because absence of an update handler will call replacement every time, we need not explicitly make all properties "createOnly" for this. The resource should not enter UPDATE_FAILED state here.

This does appear to be what should happen according to the documentation, but it doesn't happen in practice.

The AWS Resource Type Schema documentation states under handlers:

Specifies the provisioning operations which can be performed on this resource type. The handlers specified determine what provisioning actions CloudFormation takes with respect to the resource during various stack operations.

  • If the resource type does not contain create, read, and delete handlers, CloudFormation cannot actually provision the resource.
  • If the resource type does not contain an update handler, CloudFormation cannot update the resource during stack update operations, and will instead replace it.

So it seems like CloudFormation should make the assumption that an UPDATE isn't possible if no update handler (#/handlers/update) is declared, and the AWS::CloudFormation::ResourceVersion resource schema doesn't declare one. However, I did two tests, one with the current master code from this repository and the other using the code from this PR, using the following steps for both:

  1. Change the reserved AWS:: prefix in the resource type name to AWSOpenSource::
  2. Build, package, and register the AWSOpenSource::CloudFormation::ResourceVersion resource provider to my own AWS account
  3. Deploy a new stack (test-stack) with a single AWSOpenSource::CloudFormation::ResourceVersion resource, registering a new resource provider (logical id TestResourceVersion)
  4. Re-deploy that stack with a change to the SchemaHandlerPackage (same exact zip as before but under a new S3 key)
At step 4, the current master code resulted in the following events: Logical ID Type Status Status Reason
test-stack AWS::CloudFormation::Stack UPDATE_IN_PROGRESS User Initiated
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion UPDATE_FAILED Internal Failure
test-stack AWS::CloudFormation::Stack UPDATE_ROLLBACK_IN_PROGRESS The following resource(s) failed to update: [TestResourceVersion].
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion UPDATE_COMPLETE -
test-stack AWS::CloudFormation::Stack UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS -
test-stack AWS::CloudFormation::Stack UPDATE_ROLLBACK_COMPLETE -
Whereas using code in this PR resulted in the following events: Logical ID Type Status Status Reason
test-stack AWS::CloudFormation::Stack UPDATE_IN_PROGRESS User Initiated
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion UPDATE_IN_PROGRESS Requested update requires the creation of a new physical resource; hence creating one.
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion UPDATE_IN_PROGRESS Resource creation Initiated
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion UPDATE_COMPLETE -
test-stack AWS::CloudFormation::Stack UPDATE_COMPLETE_CLEANUP_IN_PROGRESS -
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion DELETE_IN_PROGRESS -
TestResourceVersion AWSOpenSource::CloudFormation::ResourceVersion DELETE_COMPLETE -
test-stack AWS::CloudFormation::Stack UPDATE_COMPLETE -

The RPDK doesn't push anything to CloudWatch Logs outside of the handler so we don't get any further details indicating what happened, but it's been very repeatable; multiple tests over multiple days were able to CREATE in every single instance, but every single UPDATE would fail in exactly the same way, so it's highly unlikely it was a transient issue.

Therefore it appears as though CloudFormation is using the createOnlyProperties to determine if a change requires a CREATE or UPDATE event, then sends that event to the resource provider regardless of whether a matching handler is declared. The RPDK embedded in the resource provider subsequently parses the message and sends it to the relevant action handler, throwing an exception if that handler doesn't exist (locally this shows the RPDK throwing an uncaught java.lang.RuntimeException: Unknown action UPDATE, which results in a InternalFailure error code).

opalelement commented 3 years ago

@ugudip or @xiwhuang can I get a re-review on this?

opalelement commented 3 years ago

@ammokhov as the person who initially added @ugudip and @xiwhuang as reviewers to this PR, are there other reviewers you might be able to add that have availability to review this?

tylersouthwick commented 3 years ago

I'm getting an "Internal Error" when I try to update a ResourceVersion using the resource type that is available in AWS accounts. Is there an ETA on this getting merged?

tylersouthwick commented 3 years ago

Is this still a valid change due to #51 ? It seems to work as expected now

ugudip commented 3 years ago

Yes, this PR can be closed now as the changes were made in https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-cloudformation/pull/51.

jarreds commented 3 years ago

We're seeing CloudFormation "Internal Failure" errors on update for our AWS::CloudFormation::ResourceVersion too. Is #51 live? And if not, any ideas on a timeline? The CloudFormation docs don't seem to reflect the change either.

ugudip commented 3 years ago

Yes, https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-cloudformation/pull/51 is live. You should not get any Internal Failures now. Were you using Changesets for update?

jarreds commented 3 years ago

Yep. Pasted below.

Our CICD system tags CloudFormation stacks on deploy. Looks like these tags are cascading down to the AWS::CloudFormation::ResourceVersion and that's causing the failure. In other words, we don't have any explicit tags on the AWS::CloudFormation::ResourceVersion -- they're inherited from the stack. Unfortunately, disabling tag updates on the CloudFormation stack is not possible for us.

{
    "resourceChange": {
      "logicalResourceId": "ClientResourceDefaultVersion",
      "action": "Modify",
      "physicalResourceId": "arn:aws:cloudformation:us-east-1:#########:type/resource/#######",
      "resourceType": "AWS::CloudFormation::ResourceDefaultVersion",
      "replacement": "False",
      "moduleInfo": null,
      "details": [
        {
          "target": {
            "name": null,
            "requiresRecreation": "Never",
            "attribute": "Tags"
          },
          "causingEntity": null,
          "evaluation": "Static",
          "changeSource": null
        },
        {
          "target": {
            "name": "TypeVersionArn",
            "requiresRecreation": "Never",
            "attribute": "Properties"
          },
          "causingEntity": "ClientResourceVersion",
          "evaluation": "Dynamic",
          "changeSource": "ResourceReference"
        }
      ],
      "changeSetId": null,
      "scope": [
        "Properties",
        "Tags"
      ]
    },
    "hookInvocationCount": null,
    "type": "Resource"
  }
jarreds commented 3 years ago

Oops, included the AWS::CloudFormation::ResourceDefaultVersion not the AWS::CloudFormation::ResourceVersion.

  {
    "resourceChange": {
      "logicalResourceId": "ClientResourceVersion",
      "action": "Modify",
      "physicalResourceId": "arn:aws:cloudformation:us-east-1:###########:type/resource/#########/00000007",
      "resourceType": "AWS::CloudFormation::ResourceVersion",
      "replacement": "Conditional",
      "moduleInfo": null,
      "details": [
        {
          "target": {
            "name": "LoggingConfig",
            "requiresRecreation": "Always",
            "attribute": "Properties"
          },
          "causingEntity": "ResourceLogRole.Arn",
          "evaluation": "Dynamic",
          "changeSource": "ResourceAttribute"
        },
        {
          "target": {
            "name": "ExecutionRoleArn",
            "requiresRecreation": "Always",
            "attribute": "Properties"
          },
          "causingEntity": "ResourceRole.Arn",
          "evaluation": "Dynamic",
          "changeSource": "ResourceAttribute"
        },
        {
          "target": {
            "name": null,
            "requiresRecreation": "Never",
            "attribute": "Tags"
          },
          "causingEntity": null,
          "evaluation": "Static",
          "changeSource": null
        }
      ],
      "changeSetId": null,
      "scope": [
        "Properties",
        "Tags"
      ]
    },
    "hookInvocationCount": null,
    "type": "Resource"
  }
jarreds commented 3 years ago

@ugudip any word? Would you like me to open a new issue for tag updates causing the "internal failure" error?

ugudip commented 3 years ago

yes, that would be helpful. Can you pls add all the details in the request?

jarreds commented 3 years ago

Created in #58