feat(cdk): Expose props as read/write properties in CFN resources #2372

moofish32 · 2019-04-25T07:12:44Z

This description will be updated before merging:

This is an initial pass only fixing the generation of property class
members.

There are a good number of collisions with attributes (e.g. Fn::GetAtt)
with the names of the properties. In this implementation I chose to
append Prop to all property class members. I'm open to other
suggestions, but my thinkig was attributes have precedence on this name
and the use of L1 property manipulation should be less common.

I have not removed the propertyOverrides feature yet. I am seeking
feedback on the approach I took for minimal impact on generation. Please
see my code comments below.

WIP: #2100

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

moofish32

added questions about my current design, seeking some early feedback.

tools/cfn2ts/lib/codegen.ts

moofish32 · 2019-04-25T07:21:59Z

tools/cfn2ts/lib/codegen.ts

+  Class = 'CLASS',
+}
+
+interface EmitPropertyProps {


this is dying

tools/cfn2ts/lib/codegen.ts

eladb

Would be useful (for the PR) to see an example of some generated code

tools/cfn2ts/lib/codegen.ts

eladb · 2019-05-01T06:03:07Z

Makes me concerned now that mixing up these changes is going to fail miserably and will potentially really confuse people. userName is a config while userArn is an attribute and ref is actually the user name attribute.

@rix0rrr @RomainMuller @skinny85 what do you guys think?

I still think we should get rid of RefKinds for the reasons described in get rid of L1 RefKinds #2345, so basically bucketName becomes ref.
I still think we should expose props as top-level mutable properties in CFN resources.
We need to differentiate between attribute properties and prop properties.

Here's my proposal: let's add a postfix Attr to all attribute properties, so it's clear those are deploy-time values (tokens), and also replace the RefKind with a generic xxxRefAttr. So we'll have bucketArnAttr, bucketRefAttr.

Do you think we should maybe remove the bucket prefix (bucket.arnAttr, bucket.refAttr?).

moofish32 · 2019-05-01T15:37:28Z

Here's my proposal: let's add a postfix Attr to all attribute properties, so it's clear those are deploy-time values (tokens), and also replace the RefKind with a generic xxxRefAttr. So we'll have bucketArnAttr, bucketRefAttr.

To make sure I understand, specifically bucketRefAttr no longer has name because the only way we knew it was name was from RefKind patch in the specification.

Also I suppose CFN updating the spec to include this information is off the table?

eladb · 2019-05-01T15:41:45Z

Yes, you are correct.

Also I suppose CFN updating the spec to include this information is off the table?

Not at the moment.

skinny85 · 2019-05-01T19:02:12Z

Here's my proposal: let's add a postfix Attr to all attribute properties, so it's clear those are deploy-time values (tokens), and also replace the RefKind with a generic xxxRefAttr. So we'll have bucketArnAttr, bucketRefAttr.

I like that.

Do you think we should maybe remove the bucket prefix (bucket.arnAttr, bucket.refAttr?).

I agree with that as well.

moofish32 · 2019-05-04T19:53:36Z

So now that I get into this fix I'm starting to think this fix is like drawing the short straw ... this is going to break a lot of things requiring a lot name changes. I think I have a couple of ideas on some ways to shortcut that, but this one could be ugly.

So @rix0rrr @skinny85 @eladb @RomainMuller let's get agreement on the proposal above

Here's my proposal: let's add a postfix Attr to all attribute properties, so it's clear those are deploy-time values (tokens), and also replace the RefKind with a generic xxxRefAttr. So we'll have bucketArnAttr, bucketRefAttr.

Do you think we should maybe remove the bucket prefix (bucket.arnAttr, bucket.refAttr?).

This is what role would generate like under that change. Everything referencing role.roleId is going to need an update. Before I start fixing everything do we agree that we like this or do we want to change it?

export class CfnRole extends cdk.CfnResource {
    /**
     * The CloudFormation resource type name for this resource class.
     */
    public static readonly resourceTypeName = "AWS::IAM::Role";

    /**
     * @cloudformationAttribute Arn
     */
    public readonly arnAttr: string;

    /**
     * @cloudformationAttribute RoleId
     */
    public readonly roleIdAttr: string;

    /**
     * @cloudformationReference Ref
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html
     */
    public readonly refAttr: string;

    /**
     * `AWS::IAM::Role.AssumeRolePolicyDocument`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-assumerolepolicydocument
     */
    public assumeRolePolicyDocument: object | cdk.Token;

    /**
     * `AWS::IAM::Role.ManagedPolicyArns`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-managepolicyarns
     */
    public managedPolicyArns: string[];

    /**
     * `AWS::IAM::Role.MaxSessionDuration`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-maxsessionduration
     */
    public maxSessionDuration: number | cdk.Token;

    /**
     * `AWS::IAM::Role.Path`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-path
     */
    public path: string;

    /**
     * `AWS::IAM::Role.PermissionsBoundary`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-permissionsboundary
     */
    public permissionsBoundary: string;

    /**
     * `AWS::IAM::Role.Policies`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-policies
     */
    public policies: Array<CfnRole.PolicyProperty | cdk.Token> | cdk.Token;

    /**
     * `AWS::IAM::Role.RoleName`
     * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-iam-role.html#cfn-iam-role-rolename
     */
    public roleName: string;

    /**
     * Create a new `AWS::IAM::Role`.
     *
     * @param scope - scope in which this resource is defined
     * @param id    - scoped id of the resource
     * @param props - resource properties
     */
    constructor(scope: cdk.Construct, id: string, props: CfnRoleProps) {
        super(scope, id, { type: CfnRole.resourceTypeName, properties: props });
        cdk.requireProperty(props, 'assumeRolePolicyDocument', this);
        this.arnAttr = this.getAtt('Arn').toString();
        this.roleIdAttr = this.getAtt('RoleId').toString();
        this.refAttr = this.ref.toString();
    }
// ....

eladb · 2019-05-05T09:10:07Z

Looks good to me, and indeed you are correct that this is going to be break all of our L2 code.

I am wondering if it makes sense to use a common prefix for all attributes instead of a common suffix:

role.attrRef
role.attrRoleId
role.attrArn

I think this will be more organized and easier to discover, and also clearly distinguishes all attributes from props.

Alternatively, we could also just leave the type prefix ("roleArnAttr") and just append attr. Both options are okay in my view.

@skinny85 @RomainMuller @rix0rrr any preferences?

Option A: roleRefAttr, roleArnAttr,roleIdAttr
Option B: attrRef, attrArn, attrRoleId (notice the inconsistency between RoleID and ARN because the CFN attribute names are inconsistent in that manner).
Option C: refAttr, arnAttr, roleIdAttr.

My vote I think is option B as it is the most true to CFN and removes all heuristics from our generator, but I don't have a hugely strong opinion - I am fine with any of these options.

moofish32 · 2019-05-05T21:27:19Z

I think I like B as well. The prefix organizes better in my mind and maps directly to CFN (even it I don't agree with it, it's been around a long time).

rix0rrr · 2019-05-08T14:46:11Z

If I let go how ugly everything is, I suppose I prefer B as well.

moofish32 · 2019-05-14T05:59:16Z

@eladb I might have missed this, do we want to "remap" the values in the L2 or make them match?

error: resources must represent all attributes as properties [resource-attribute:@aws-cdk/aws-iam.IRole.attrArn]

Right now I was keeping the L2 as roleArn, but this means I have to do some flips in the awslint code and that made me wonder if wanted the L2s to match directly?

moofish32 · 2019-05-14T06:07:56Z

I pushed an example change of the renaming for aws-iam. Yes, I'm aware more codegen work will be necessary for the renderProperties( ... ) function etc, but the naming seems like most likely re-work area.

packages/@aws-cdk/aws-iam/lib/lazy-role.ts

tools/awslint/lib/rules/resource.ts

tools/cfn2ts/lib/codegen.ts

moofish32 · 2019-05-25T23:07:44Z

UPDATE -- see next comment

@eladb @rix0rrr -- looking for a little guidance here:

I think JSII reflect has an issue where the base class hides the interface that holds the attributes (cfn attributes/refs).

See here: https://github.com/awslabs/aws-cdk/blob/master/packages/@aws-cdk/aws-iam/lib/group.ts#L54

Versus Role that does a lot of code duplicate here: https://github.com/awslabs/aws-cdk/blob/master/packages/@aws-cdk/aws-iam/lib/role.ts#L112

GroupBase I think is only used to DRY up that import call, but I believe it hides the @attribute tags that I've ensured are on the IGroup interface during the aws-lint. It's hard for me to be certain on the details in JSII reflect from my minimal spelunking, but does this sound like I've hit the problem? If so should I remove GroupBase or focus on solving the problem differently?

moofish32 · 2019-05-26T20:21:26Z

I was wrong! The issue is unlike the base class for other resources Group does not export GroupBase. What is the right convention here, export the abstract base class for Group (and create one for role)?

eladb · 2019-05-26T23:14:16Z

Preferably we shouldn't export the base class, and jsii will copy the declarations (incl. any interface definitions and docstrings) from the base class to the derived classes. Can you describe the issue you are running into?

moofish32 · 2019-05-27T06:06:57Z

The issue I'm hitting is that the @attribute docTag is on the interface IGroup. The L2 for IAM Group does not directly implement IGroup instead it extends GroupBase which does. Since the GroupBase is not exported reflection does not properly find the properties and specifically the CFN attributes and therefore we fail aws-lint. If we export the base class JSII sees it and reflect works.

moofish32 · 2019-06-07T05:13:40Z

I've fixed aws-iam and aws-kms package. I've also changed the design a bit. I'm now creating all the class members for CFN resources and if they are optional I'm enabling | undefined. This enables us to pass the props objects directly to the class members and effectively never return to the props object.

eladb

Looks good. Can you somehow include in the PR (or just upload to gist) an example of a .generated.ts file? I want to see the resulting API...

packages/@aws-cdk/cdk/lib/cfn-resource.ts

tools/cfn2ts/lib/codegen.ts

eladb · 2019-06-07T08:50:00Z

tools/awslint/lib/rules/resource.ts

    for (const p of this.construct.classType.allProperties) {
      if (p.protected) {
        continue; // skip any protected properties
      }

-      // an attribute property is a property which starts with the type name
-      // (e.g. "bucketXxx") and/or has an @attribute doc tag.
+      // an attribute property is a property which starts with `attr`


This is not true for L2s, only L1s

still open -- will fix next

Still open - will update next.

updated to only L1 resources

tools/awslint/lib/rules/resource.ts

eladb · 2019-06-07T08:51:38Z

tools/awslint/lib/rules/resource.ts

@@ -152,7 +153,12 @@ resourceLinter.add({
  message: 'resources must represent all cloudformation attributes as attribute properties. missing property: ',
  eval: e => {
    for (const name of e.ctx.cfn.attributeNames) {


Feels like we should make attributeNames the clean attribute names without the attr prefix and then we won't need all this manipulation.

So I naively tried to do this, but what do you do for all the situations where attributes and property names collide? For example, CfnCapacityReservation availabiltyZone is both attribute and property (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-capacityreservation.html#cfn-ec2-capacityreservation-availabilityzone). No easy answer here with the naming convention from CloudFormation that I see? We can eliminate all attrRef and just use ref.

What I mean here is that I still want generated L1 attribute properties to have the “attr” prefix but the array “attributeNames” in awslint should be the canonical attribute names without the prefix so it will be easy to work with in awslint

alright -- I was I little dense there, I think I made the changes you wanted now.

tools/cfn2ts/lib/codegen.ts

tools/cfn2ts/lib/genspec.ts

moofish32 · 2019-06-08T23:54:59Z

Gist of current kms.generated.ts

moofish32 · 2019-06-09T14:06:00Z

@eladb starting to look at the name collisions: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-flowlog.html#cfn-ec2-flowlog-resourcetype

The resourceType property collides with CfnResource.resourceType. This happens to be the property. We didn't prefix these for readability, but now I'm questioning either a prefix or containing object (again).?

eladb · 2019-06-09T18:07:40Z

I prefer not to prefix for these (hopefully rare) edge cases. The base classes should have a pretty minimal API, which I am open to renaming. For example, we can add a “cfn” prefix so it becomes cfnResouceType. What else do we have there?

eladb

The codegen looks good, so I think you can start with the overall fix. A few comments remaining on linter

eladb · 2019-06-09T18:21:10Z

packages/@aws-cdk/cdk/lib/cfn-resource.ts

            DependsOn: ignoreEmpty(renderDependsOn(this.dependsOn)),
-            CreationPolicy:  capitalizePropertyNames(this, this.options.creationPolicy),
+            CreationPolicy: capitalizePropertyNames(this, this.options.creationPolicy),
            UpdatePolicy: capitalizePropertyNames(this, this.options.updatePolicy),
            UpdateReplacePolicy: capitalizePropertyNames(this, this.options.updateReplacePolicy),
            DeletionPolicy: capitalizePropertyNames(this, this.options.deletionPolicy),
            Metadata: ignoreEmpty(this.options.metadata),
            Condition: this.options.condition && this.options.condition.logicalId
          }, props => {


Curious why we moved renderProperies outside of PostResolve. I suspect the reasoning hasn’t changed or am I missing something?

Also feel like it should be inside.

moved back, but had to re-separate properties and validator/mapper. So the real issue is in the render call we have the mapper/validators and those need to operate on resolved fields.

packages/@aws-cdk/cdk/test/test.resource.ts

eladb · 2019-06-09T18:22:55Z

tools/awslint/lib/rules/cfn-resource.ts

@@ -67,13 +66,6 @@ export class CfnResourceReflection {

    this.namespace = fullname.split('::').slice(0, 2).join('::');

-    // special-case
-    const basename = this.basename


We still need these special cases for rendering the L2 linter

yes - I thought they would come back, but I was in a clean everything out.

eladb · 2019-06-09T18:28:36Z

tools/awslint/lib/rules/resource.ts

@@ -152,7 +153,9 @@ resourceLinter.add({
  message: 'resources must represent all cloudformation attributes as attribute properties. missing property: ',
  eval: e => {
    for (const name of e.ctx.cfn.attributeNames) {
-      const found = e.ctx.attributes.find(a => a.names.includes(name));
+      const lookup = name.startsWith(e.ctx.basename) ?


Something here still feels wrong. The purpose of this linter rule is to verify that the L2 has a property for each CFN resource attribute with the format “” (I.e bucketArn, topicName,...).

Let’s first render the desired L2 property name in the ResourceReflection class with all the heuristics and special cases (previously those were at the CfnResourceReflection), and then just check that the class has all these properties.

tools/cfn2ts/lib/codegen.ts

rix0rrr · 2019-06-11T06:54:22Z

packages/@aws-cdk/cdk/lib/cfn-resource.ts

            DependsOn: ignoreEmpty(renderDependsOn(this.dependsOn)),
-            CreationPolicy:  capitalizePropertyNames(this, this.options.creationPolicy),
+            CreationPolicy: capitalizePropertyNames(this, this.options.creationPolicy),
            UpdatePolicy: capitalizePropertyNames(this, this.options.updatePolicy),
            UpdateReplacePolicy: capitalizePropertyNames(this, this.options.updateReplacePolicy),
            DeletionPolicy: capitalizePropertyNames(this, this.options.deletionPolicy),
            Metadata: ignoreEmpty(this.options.metadata),
            Condition: this.options.condition && this.options.condition.logicalId
          }, props => {


Also feel like it should be inside.

packages/@aws-cdk/cdk/lib/cfn-resource.ts

moofish32 requested a review from a team as a code owner April 25, 2019 07:12

moofish32 force-pushed the inspect-properties branch from f2b737c to af39691 Compare April 25, 2019 07:15

moofish32 commented Apr 25, 2019

View reviewed changes

eladb reviewed Apr 30, 2019

View reviewed changes

tools/cfn2ts/lib/codegen.ts Show resolved Hide resolved

tools/cfn2ts/lib/codegen.ts Show resolved Hide resolved

tools/cfn2ts/lib/codegen.ts Outdated Show resolved Hide resolved

eladb suggested changes May 14, 2019

View reviewed changes

packages/@aws-cdk/aws-iam/lib/lazy-role.ts Outdated Show resolved Hide resolved

tools/awslint/lib/rules/resource.ts Outdated Show resolved Hide resolved

tools/cfn2ts/lib/codegen.ts Outdated Show resolved Hide resolved

moofish32 force-pushed the inspect-properties branch from 4808108 to f823eb2 Compare May 27, 2019 06:04

moofish32 force-pushed the inspect-properties branch from 8356baf to 6193da3 Compare June 7, 2019 04:44

eladb reviewed Jun 7, 2019

View reviewed changes

packages/@aws-cdk/cdk/lib/cfn-resource.ts Show resolved Hide resolved

eladb reviewed Jun 7, 2019

View reviewed changes

moofish32 force-pushed the inspect-properties branch from ac60000 to abfdd77 Compare June 8, 2019 23:53

eladb reviewed Jun 9, 2019

View reviewed changes

rix0rrr reviewed Jun 11, 2019

View reviewed changes

snyk-bot mentioned this pull request Aug 22, 2019

[Snyk] Upgrade @aws-cdk/aws-ecs from 0.18.1 to 0.39.0 udinachmany/trivia-api#5

Open

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

NOUIY mentioned this pull request Oct 2, 2024

[Snyk] Upgrade @aws-cdk/aws-apigateway from 0.0.0 to 0.39.0 NOUIY/aws-solutions-constructs#132

Open

feat(cdk): Expose props as read/write properties in CFN resources #2372

feat(cdk): Expose props as read/write properties in CFN resources #2372

Conversation

moofish32 commented Apr 25, 2019 • edited Loading

Pull Request Checklist

moofish32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb commented May 1, 2019

moofish32 commented May 1, 2019

eladb commented May 1, 2019

skinny85 commented May 1, 2019

moofish32 commented May 4, 2019

eladb commented May 5, 2019 • edited Loading

moofish32 commented May 5, 2019

rix0rrr commented May 8, 2019

moofish32 commented May 14, 2019

moofish32 commented May 14, 2019

moofish32 commented May 25, 2019 • edited Loading

moofish32 commented May 26, 2019

eladb commented May 26, 2019

moofish32 commented May 27, 2019

moofish32 commented Jun 7, 2019

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Jun 8, 2019

moofish32 commented Jun 9, 2019

eladb commented Jun 9, 2019

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moofish32 commented Apr 25, 2019 •

edited

Loading

eladb commented May 5, 2019 •

edited

Loading

moofish32 commented May 25, 2019 •

edited

Loading

moofish32 Jun 11, 2019 •

edited

Loading