fix(cfn-include): correctly handle the 'AWS::CloudFormation::CustomResource' resource type #10415
Conversation
…source' resource type The resource type 'AWS::CloudFormation::CustomResource' corresponds to the class CfnCustomResource. However, that class is automatically generated, and quite useless; it only supports one property, ServiceToken. It does not support passing in an arbitrary collection of properties, like custom resources in CloudFormation do. As a result, cfn-include would "lose" all properties of resources of type 'AWS::CloudFormation::CustomResource' other than ServiceToken. Fix the problem by handling this resource type with the CfnResource class, that does support an arbitrary collection of properties.
| // The AWS::CloudFormation::CustomResource type corresponds to the CfnCustomResource class. | ||
| // Unfortunately, it's quite useless; it only has a single property, ServiceToken. |
There was a problem hiding this comment.
So one option is what you've done here -- mapping a custom resource to a CfnResource. Would the other be to fix the "useless" CfnCustomResource class? Could we add extensions to that class via the cfnspec additions? If so, it seems like that would solve this issue for more than just this one use case.
There was a problem hiding this comment.
One advantage of this escape hatch to CfnResource is that AWS::CloudFormation::CustomResource is handled the same as Custom::MyCoolResource instead of one being CfnResource and the other being CfnCustomResource.
Indeed it might be better to elaborate the CfnCustomResource interface, but I definitely think it is important to make sure that the 2 Type namings for Custom Resources result in the same construct.
There was a problem hiding this comment.
Unfortunately, CfnCustomResource is auto-generated, so there's not much we can do there 😕. There's simply not a concept of "accept an arbitrary collection of properties" in the CloudFormation resource specification, and so it can't really be translated to our generated L1s.
We can try to hack something on top of the resource specification (we've done that before, for example in #10316), but I'm not sure it's worth it in this case - like @DV8FromTheWorld said, it's actually more consistent in some sense to always return custom resources, regardless of the resource type they use, as instances of the CfnResource class.
There was a problem hiding this comment.
You know, maybe I spoke too soon. I see that the resource specification for AWS::CloudFormation::Resource contains an "AdditionalProperties": true field:
"AWS::CloudFormation::CustomResource": {
"AdditionalProperties": true,
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cfn-customresource.html",
"Properties": {
"ServiceToken": {
"Documentation": "http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-cfn-customresource.html#cfn-customresource-servicetoken",
"PrimitiveType": "String",
"Required": true,
"UpdateType": "Immutable"
}
}
},Not sure if it's possible to express that easily in TypeScript, but I can investigate.
Let me know what you think @njlynch.
There was a problem hiding this comment.
Thanks for the deeper investigation. I think the context I was missing was that we return the other custom resource types as CfnResource as well. For the sake of consistency, I think we can stick with this approach.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The resource type 'AWS::CloudFormation::CustomResource' corresponds to the class CfnCustomResource.
However, that class is automatically generated, and quite useless; it only supports one property, ServiceToken.
It does not support passing in an arbitrary collection of properties,
like custom resources in CloudFormation do.
As a result, cfn-include would "lose" all properties of resources of type 'AWS::CloudFormation::CustomResource'
other than ServiceToken.
Fix the problem by handling this resource type with the CfnResource class,
that does support an arbitrary collection of properties.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license