feat(cfn-include): preserve properties of resources that are not in the current CFN schema #11822

skinny85 · 2020-12-02T02:13:16Z

Currently, if cloudformation-include encounters a property in one of the resources the CDK has an L1 for
that is not in the version of the CloudFormation resource schema the module uses,
the property will be silently dropped. Of course, that is not ideal.

Make changes to our L1 code generation to preserve all properties that are not in the CFN schema.

Fixes #9717

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2020-12-02T02:13:19Z

skinny85 · 2020-12-02T04:47:22Z

s3.generated.ts with the changes from this PR: https://gist.github.com/skinny85/b892306a9b027fc8738395a0dfcee4d6

rix0rrr

Money question I have after all of this:

Why not use CFN overrides to remember the properties and make sure they're rendered back out?

rix0rrr · 2020-12-02T08:34:52Z

packages/@aws-cdk/core/lib/cfn-parse.ts

+   * @returns a new object with all the entries in `object`,
+   *   except those whose keys are one of the names provided in `props`
+   */
+  public static omit(object: object, ...props: string[]): object {


The location of this function is a tough un' eh.

It strictly speaking has nothing to do with FromCloudFormation, so it should be a generic Util function. But on the other hand, it is only used there and it's nice and convenient to put it here. Oh well, I guess?

Yeah, it was put here more for convenience than anything else.

The only other place I can think of that makes sense for it is in runtime.ts in @aws-cdk/core.

Let me know what you think!

I think runtime.ts might make for a good location, yeah.

tools/cfn2ts/lib/codegen.ts

skinny85 · 2020-12-03T01:21:22Z

Money question I have after all of this:

Why not use CFN overrides to remember the properties and make sure they're rendered back out?

Excellent question. There is a reason, and it's nested properties.

While adding the property overrides would work for top-level properties, like this (present in the test of the initial version of this PR):

{
  "Resources": {
    "Bucket": {
      "Type": "AWS::S3::Bucket",
      "Properties": {
        "PropertyNotInCfnSchema": 1
      }
    }
  }
}

It would not work for unknown properties in nested complex types, like this:

{
  "Resources": {
    "Bucket": {
      "Type": "AWS::S3::Bucket",
      "Properties": {
        "AccelerateConfiguration": {
          "AccelerationStatus": "Enabled",
          "PropertyNotInCfnSchema": false
        }
      }
    }
  }
}

For that second case, we need to save the unrecognized property in the converters generated for each of the complex property types of the resource. I don't think that can be reasonably achieved with property overrides without huge changes to the current code generator.

(BTW, I've added this second case to the tests in this PR)

rix0rrr · 2020-12-03T09:30:31Z

packages/@aws-cdk/aws-cloudfront/lib/distribution.ts

@@ -264,7 +264,7 @@ export class Distribution extends Resource implements IDistribution {
    }

    const originId = this.addOrigin(props.defaultBehavior.origin);
-    this.defaultBehavior = new CacheBehavior(originId, { pathPattern: '*', ...props.defaultBehavior });
+    this.defaultBehavior = new CacheBehavior(originId, props.defaultBehavior);


Is this change safe?

rix0rrr

It would not work for unknown properties in nested complex types, like this:

But it would, as far as I'm aware (or at least it should). Properties can be dot-separated to indicate deep properties, like so:

cfnResource.addPropertyOverride('AccelerateConfiguration.PropertyNotInCfnSchema', false);

Am I missing something?

rix0rrr · 2020-12-03T09:31:51Z

packages/@aws-cdk/core/lib/cfn-parse.ts

+   * @returns a new object with all the entries in `object`,
+   *   except those whose keys are one of the names provided in `props`
+   */
+  public static omit(object: object, ...props: string[]): object {


I think runtime.ts might make for a good location, yeah.

…he current CFN schema Currently, if cloudformation-include encounters a property in one of the resources the CDK has an L1 for that is not in the version of the CloudFormation resource schema the module uses, the property will be silently dropped. Of course, that is not ideal. Make changes to our L1 code generation to preserve all properties that are not in the CFN schema. Fixes aws#9717

…Result<PropType>.

skinny85 · 2020-12-04T01:53:52Z

@rix0rrr this is ready for another review. I've changed to using property overrides, like we discussed. I kind of hate how the code looks because of that, especially the changes done in FromCloudFormation static methods from cfn-parse.ts. But let me know what you think.

The s3.generated.ts file generated with the changes from this PR: https://gist.github.com/skinny85/ed4dc3556c95b343f958f2e5e9689b44

rix0rrr · 2020-12-04T09:00:49Z

I agree that it currently doesn't look great. On the one hand, it's generated code so I really don't care what it looks like, only that it does the right thing.

But I see what you mean. We have to do 2 things (instead of 1) to the return value of every inner parser, which requires that we declare a variable to hold the intermediary result.

But what if we defined an additional function which does the 2 things that we need to a parameter? Then it becomes one line per property again, which is rather clean.

I added in one more convenience class which is intended to be used as an accumulator. It cleans this:

function CfnAccessPointPropsFromCloudFormation(properties: any): cfn_parse.FromCloudFormationResult<CfnAccessPointProps> {
    properties = properties || {};
    const bucketResult = cfn_parse.FromCloudFormation.getString(properties.Bucket);
    const creationDateResult = properties.CreationDate != null ? cfn_parse.FromCloudFormation.getString(properties.CreationDate) : undefined;
    const nameResult = properties.Name != null ? cfn_parse.FromCloudFormation.getString(properties.Name) : undefined;
    const networkOriginResult = properties.NetworkOrigin != null ? cfn_parse.FromCloudFormation.getString(properties.NetworkOrigin) : undefined;
    const policyResult = properties.Policy != null ? cfn_parse.FromCloudFormation.getAny(properties.Policy) : undefined;
    const policyStatusResult = properties.PolicyStatus != null ? cfn_parse.FromCloudFormation.getAny(properties.PolicyStatus) : undefined;
    const publicAccessBlockConfigurationResult = properties.PublicAccessBlockConfiguration != null ? CfnAccessPointPublicAccessBlockConfigurationPropertyFromCloudFormation(properties.PublicAccessBlockConfiguration) : undefined;
    const vpcConfigurationResult = properties.VpcConfiguration != null ? CfnAccessPointVpcConfigurationPropertyFromCloudFormation(properties.VpcConfiguration) : undefined;
    const ret = new cfn_parse.FromCloudFormationResult({
        bucket: bucketResult?.value,
        creationDate: creationDateResult?.value,
        name: nameResult?.value,
        networkOrigin: networkOriginResult?.value,
        policy: policyResult?.value,
        policyStatus: policyStatusResult?.value,
        publicAccessBlockConfiguration: publicAccessBlockConfigurationResult?.value,
        vpcConfiguration: vpcConfigurationResult?.value,
    });
    ret.appendExtraProperties('Bucket', bucketResult?.extraProperties);
    ret.appendExtraProperties('CreationDate', creationDateResult?.extraProperties);
    ret.appendExtraProperties('Name', nameResult?.extraProperties);
    ret.appendExtraProperties('NetworkOrigin', networkOriginResult?.extraProperties);
    ret.appendExtraProperties('Policy', policyResult?.extraProperties);
    ret.appendExtraProperties('PolicyStatus', policyStatusResult?.extraProperties);
    ret.appendExtraProperties('PublicAccessBlockConfiguration', publicAccessBlockConfigurationResult?.extraProperties);
    ret.appendExtraProperties('VpcConfiguration', vpcConfigurationResult?.extraProperties);
    for (const [key, val] of Object.entries(cfn_parse.FromCloudFormation.omit(properties, 'Bucket', 'CreationDate', 'Name', 'NetworkOrigin', 'Policy', 'PolicyStatus', 'PublicAccessBlockConfiguration', 'VpcConfiguration')))  {
        ret.appendExtraProperty(key, val);
    }
    return ret;
}

Up to:

function CfnAccessPointPropsFromCloudFormation(properties: any): cfn_parse.FromCloudFormationResult<CfnAccessPointProps> {
    properties = properties || {};
    const ret = new cfn_parse.FromCloudFormationPropertyObject<CfnAccessPointProps>();

    ret.addResult('bucket', 'Bucket', cfn_parse.FromCloudFormation.getString(properties.Bucket));
    ret.addResult('creationDate', 'CreationDate', properties.CreationDate != null ? cfn_parse.FromCloudFormation.getString(properties.CreationDate) : undefined);
    ret.addResult('name', 'Name', properties.Name != null ? cfn_parse.FromCloudFormation.getString(properties.Name) : undefined);
    ret.addResult('networkOrigin', 'NetworkOrigin', properties.NetworkOrigin != null ? cfn_parse.FromCloudFormation.getString(properties.NetworkOrigin) : undefined);
    ret.addResult('policy', 'Policy', properties.Policy != null ? cfn_parse.FromCloudFormation.getAny(properties.Policy) : undefined);
    ret.addResult('policyStatus', 'PolicyStatus', properties.PolicyStatus != null ? cfn_parse.FromCloudFormation.getAny(properties.PolicyStatus) : undefined);
    ret.addResult('publicAccessBlockConfiguration', 'PublicAccessBlockConfiguration', properties.PublicAccessBlockConfiguration != null ? CfnAccessPointPublicAccessBlockConfigurationPropertyFromCloudFormation(properties.PublicAccessBlockConfiguration) : undefined);
    ret.addResult('vpcConfiguration', 'VpcConfiguration', properties.VpcConfiguration != null ? CfnAccessPointVpcConfigurationPropertyFromCloudFormation(properties.VpcConfiguration) : undefined);

    for (const [key, val] of Object.entries(cfn_parse.FromCloudFormation.omit(properties, 'Bucket', 'CreationDate', 'Name', 'NetworkOrigin', 'Policy', 'PolicyStatus', 'PublicAccessBlockConfiguration', 'VpcConfiguration')))  {
        ret.appendExtraProperty(key, val);
    }

    return ret;
}

That's a lot better already, no?

And it occurs to me that the for-loop at the end contains a duplicate list of properties that have already been set on the accumulation object. So in fact we could even simplify to this:

function CfnAccessPointPropsFromCloudFormation(properties: any): cfn_parse.FromCloudFormationResult<CfnAccessPointProps> {
    properties = properties || {};
    const ret = new cfn_parse.FromCloudFormationPropertyObject<CfnAccessPointProps>();

    ret.addResult('bucket', 'Bucket', cfn_parse.FromCloudFormation.getString(properties.Bucket));
    ret.addResult('creationDate', 'CreationDate', properties.CreationDate != null ? cfn_parse.FromCloudFormation.getString(properties.CreationDate) : undefined);
    ret.addResult('name', 'Name', properties.Name != null ? cfn_parse.FromCloudFormation.getString(properties.Name) : undefined);
    ret.addResult('networkOrigin', 'NetworkOrigin', properties.NetworkOrigin != null ? cfn_parse.FromCloudFormation.getString(properties.NetworkOrigin) : undefined);
    ret.addResult('policy', 'Policy', properties.Policy != null ? cfn_parse.FromCloudFormation.getAny(properties.Policy) : undefined);
    ret.addResult('policyStatus', 'PolicyStatus', properties.PolicyStatus != null ? cfn_parse.FromCloudFormation.getAny(properties.PolicyStatus) : undefined);
    ret.addResult('publicAccessBlockConfiguration', 'PublicAccessBlockConfiguration', properties.PublicAccessBlockConfiguration != null ? CfnAccessPointPublicAccessBlockConfigurationPropertyFromCloudFormation(properties.PublicAccessBlockConfiguration) : undefined);
    ret.addResult('vpcConfiguration', 'VpcConfiguration', properties.VpcConfiguration != null ? CfnAccessPointVpcConfigurationPropertyFromCloudFormation(properties.VpcConfiguration) : undefined);

    ret.addUnknownPropertiesAsExtra(properties);

    return ret;
}

That also means we can get rid of the omit() helper, because that logic could just live inside FromCloudFormationPropertyObject (all it would do is need to hold a Set<> containing all keys it had already seen). Didn't do that yet because I wonder if it's going too far. But it sure deals with the ugliness some more.

What do you think?

rix0rrr

Leaving whether or not to clean up the final omit loop up to you. Also if you want to rename any classes, or simplify some more, that's up to you.

I'm happy with this approach. Provisional approval.

skinny85 · 2020-12-04T20:14:31Z

Thanks Rico. I've incorporated your comment about getting rid of omit(), by introducing the addUnknownPropertiesAsExtra() method (although I called it addUnrecognizedPropertiesAsExtra()).

Merging this in now!

mergify · 2020-12-04T20:17:43Z

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-cdk-automation · 2020-12-04T20:49:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4dc7b34
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-12-04T20:51:47Z

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).

…he current CFN schema (aws#11822) Currently, if cloudformation-include encounters a property in one of the resources the CDK has an L1 for that is not in the version of the CloudFormation resource schema the module uses, the property will be silently dropped. Of course, that is not ideal. Make changes to our L1 code generation to preserve all properties that are not in the CFN schema. Fixes aws#9717 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 requested a review from rix0rrr December 2, 2020 02:13

skinny85 self-assigned this Dec 2, 2020

github-actions bot added the @aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package label Dec 2, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Dec 2, 2020

rix0rrr reviewed Dec 2, 2020

View reviewed changes

skinny85 force-pushed the feat/cfn-include-missing-props branch 2 times, most recently from 1326cb8 to 2e26abc Compare December 3, 2020 00:45

skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 3, 2020

skinny85 requested a review from rix0rrr December 3, 2020 01:43

rix0rrr reviewed Dec 3, 2020

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Dec 3, 2020

rix0rrr reviewed Dec 3, 2020

View reviewed changes

skinny85 added 4 commits December 3, 2020 17:50

Property overrides, without handling nested properties.

df56c06

Phase 1 - change the FromCfn* converters to return FromCloudFormation…

a43000b

…Result<PropType>.

Phase 2 - remaining implementation.

1c98223

skinny85 force-pushed the feat/cfn-include-missing-props branch from 6a9da5e to 1c98223 Compare December 4, 2020 01:51

skinny85 requested a review from rix0rrr December 4, 2020 01:51

Add accumulation object to simplify property parsers

04c4aee

rix0rrr approved these changes Dec 4, 2020

View reviewed changes

Get rid of 'omit()' by introducing 'addUnrecognizedPropertiesAsExtra()'.

5d14ae5

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Dec 4, 2020

Merge branch 'master' into feat/cfn-include-missing-props

83bb060

Merge branch 'master' into feat/cfn-include-missing-props

4dc7b34

mergify bot merged commit 45677ca into aws:master Dec 4, 2020

skinny85 deleted the feat/cfn-include-missing-props branch December 4, 2020 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cfn-include): preserve properties of resources that are not in the current CFN schema #11822

feat(cfn-include): preserve properties of resources that are not in the current CFN schema #11822

skinny85 commented Dec 2, 2020

gitpod-io bot commented Dec 2, 2020 •

edited

Loading

skinny85 commented Dec 2, 2020

rix0rrr left a comment

rix0rrr Dec 2, 2020

skinny85 Dec 3, 2020

rix0rrr Dec 3, 2020

skinny85 commented Dec 3, 2020 •

edited

Loading

rix0rrr Dec 3, 2020

rix0rrr left a comment

rix0rrr Dec 3, 2020

skinny85 commented Dec 4, 2020

rix0rrr commented Dec 4, 2020 •

edited

Loading

rix0rrr left a comment

skinny85 commented Dec 4, 2020

mergify bot commented Dec 4, 2020

aws-cdk-automation commented Dec 4, 2020

mergify bot commented Dec 4, 2020

feat(cfn-include): preserve properties of resources that are not in the current CFN schema #11822

feat(cfn-include): preserve properties of resources that are not in the current CFN schema #11822

Conversation

skinny85 commented Dec 2, 2020

gitpod-io bot commented Dec 2, 2020 • edited Loading

skinny85 commented Dec 2, 2020

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Dec 2, 2020

Choose a reason for hiding this comment

skinny85 Dec 3, 2020

Choose a reason for hiding this comment

rix0rrr Dec 3, 2020

Choose a reason for hiding this comment

skinny85 commented Dec 3, 2020 • edited Loading

rix0rrr Dec 3, 2020

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr Dec 3, 2020

Choose a reason for hiding this comment

skinny85 commented Dec 4, 2020

rix0rrr commented Dec 4, 2020 • edited Loading

rix0rrr left a comment

Choose a reason for hiding this comment

skinny85 commented Dec 4, 2020

mergify bot commented Dec 4, 2020

aws-cdk-automation commented Dec 4, 2020

AWS CodeBuild CI Report

mergify bot commented Dec 4, 2020

gitpod-io bot commented Dec 2, 2020 •

edited

Loading

skinny85 commented Dec 3, 2020 •

edited

Loading

rix0rrr commented Dec 4, 2020 •

edited

Loading