Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cfn-include): correctly handle the 'AWS::CloudFormation::CustomResource' resource type #10415

Merged
merged 2 commits into from
Sep 21, 2020
Merged

fix(cfn-include): correctly handle the 'AWS::CloudFormation::CustomResource' resource type #10415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b181c99
fix(cfn-include): correctly handle the 'AWS::CloudFormation::CustomRe…
skinny85 Sep 17, 2020
2bdde85
Merge branch 'master' into fix/cfn-include-custom-cfn-resource
mergify[bot] Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/@aws-cdk/cloudformation-include/lib/cfn-include.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,11 @@ export class CfnInclude extends core.CfnElement {
l1Instance = this.createNestedStack(logicalId, cfnParser);
} else {
const l1ClassFqn = cfn_type_to_l1_mapping.lookup(resourceAttributes.Type);
if (l1ClassFqn) {
// The AWS::CloudFormation::CustomResource type corresponds to the CfnCustomResource class.
// Unfortunately, it's quite useless; it only has a single property, ServiceToken.
Comment on lines +637 to +638
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@DV8FromTheWorld DV8FromTheWorld Sep 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// For that reason, even the CustomResource class from @core doesn't use it!
// So, special-case the handling of this one resource type
if (l1ClassFqn && resourceAttributes.Type !== 'AWS::CloudFormation::CustomResource') {
const options: cfn_parse.FromCloudFormationOptions = {
parser: cfnParser,
};
Expand Down
4 changes: 2 additions & 2 deletions .../@aws-cdk/cloudformation-include/test/test-templates/custom-resource-with-attributes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@
"DependsOn": [ "CustomResource" ]
},
"CustomResource": {
"Type": "AWS::MyService::AnotherCustom",
"Type": "AWS::CloudFormation::CustomResource",
"Properties": {
"CustomProp": "CustomValue",
"ServiceToken": "CustomValue",
"CustomFuncProp": {
"Ref": "AWS::NoValue"
}
Expand Down