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.

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

Expose props as read/write properties in CFN resources and remove propertyOverrides #2100

Closed
rix0rrr opened this issue Mar 27, 2019 · 9 comments · Fixed by MechanicalRock/tech-radar#14 · 4 remaining pull requests
Closed

Expose props as read/write properties in CFN resources and remove propertyOverrides #2100

rix0rrr opened this issue Mar 27, 2019 · 9 comments · Fixed by MechanicalRock/tech-radar#14 · 4 remaining pull requests
Labels
package/cfn Related to the CFN layer (L1)

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Mar 27, 2019

Perfectly reasonable application of aspects came up on Gitter:

Hello, I'm trying to write a custom IAspect which sets the RoleName of any IAM Role in a stack to
include a particular prefix (for resource matching in IAM permissions). Unfortunately, this overrides
any RoleName that is manually set in the stack itself.
Is there a way for an IAspect to check whether an IAM Role already has RoleName set?

To enable this we would need to allow read/write access to the configured properties of a CfnResource, in a typed fashion (so it needs to be done in cfn2ts).

We need to think about types and JSII though.

A RoleProps struct will have readonly properties for evertyhing, so cannot be mutated. So the syntax will have to be something like:

cfnrole.properties = {
   ...cfnrole.properties, 
   roleName: 'MyPrefix-' + cfnrole.properties.roleName
};

Not great...

Same story across JSII, except in Javaland it will be even more annoying to copy the existing properties into a new builder object. We need to generate a "copy constructor" as well then.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Mar 27, 2019

We could say "overrides!" but in that case it looks like:

cfnrole.propertyOverrides.roleName = 'MyPrefix-' + cfnrole.properties.roleName

Which I agree is slightly better, but super non-obvious.

Also if propertyOverrides is typed as the same struct type the readonly is going to disallow this as well.

@Doug-AWS
Copy link
Contributor

Is this yet another version of an escape hatch?

@eladb eladb added the package/cfn Related to the CFN layer (L1) label Apr 15, 2019
@eladb eladb changed the title Expose properties of CfnResource in a read/write fashion Remove strong-typed propertyOverrides and expose properties of CfnResource in a read/write fashion Apr 15, 2019
@eladb
Copy link
Contributor

eladb commented Apr 15, 2019

The proposal is to expose all top-level L1 props as simple read/write properties of the construct (note that deep structs will still be readonly, but that should be fine).

So for example:

class interface CfnBucketProps {
  // ...
  readonly corsConfiguration?: CorsConfigurationProperty;
  readonly bucketEncryption?: BucketEncryptionProperty;
  // ...
}

export class CfnBucket {
  // ...
  public corsConfiguration?: CorsConfigurationProperty;
  public bucketEncryption?: BucketEncryptionProperty;

  // ...
}

Also, remove propertyOverrides (it's no longer useful).

Then, in _toCloudFormation rendering should happen from the properties of course to allow overrides to take effect.

@moofish32 might pick this up.

Discussed with @rix0rrr and @RomainMuller

@eladb eladb changed the title Remove strong-typed propertyOverrides and expose properties of CfnResource in a read/write fashion Expose props as properties in CFN resources and remove propertyOverrides Apr 15, 2019
@eladb eladb changed the title Expose props as properties in CFN resources and remove propertyOverrides Expose props as mutable properties in CFN resources and remove propertyOverrides Apr 15, 2019
@eladb eladb changed the title Expose props as mutable properties in CFN resources and remove propertyOverrides Expose props as read/write properties in CFN resources and remove propertyOverrides Apr 15, 2019
@eladb
Copy link
Contributor

eladb commented Apr 15, 2019

A little note - we hope 🤞 that this does not conflict with attribute property names because they begin with the type name (e.g. bucketArn)

@RomainMuller
Copy link
Contributor

We could also code generate an IInterface for that mutable property. This should also feature an arbitrary accessor ([key: string]: any) so people could leverage properties that did t make it to the spec yet.

It'd maintain the namespace-like encapsulation that prevents name collisions from being a problem, and retain the strong typed overrides. It may need a JSII feature to support objects with an index signature.

@eladb
Copy link
Contributor

eladb commented Apr 16, 2019

I am not sure it's worth the effort, and the developer experience is less attractive:

bucket.corsConfiguration = { boom! };

versus:

bucket.props.corsConfiguration = { bam! };

I doubt we'll ever have conflicts and if we do, and we can should implement this logic to mangle the property name if there's an attribute with exactly the same name (e.g. add something like a "Config" postfix). At any rate, if this will happen, it will be a very fringy edge case.

@moofish32
Copy link
Contributor

Just to verify I'm thinking in line with the team a starting point might be:

  1. Modify code generation to create a class property for each CloudFormation Property currently in the immutable Cfn<resource>Props interface
  2. Keep the constructor with the immutable Cfn<resource>Props (no change here)
  3. After class initialization assign & read all properties from the class members do not persist the properties class member (renderProperties method should now assemble the properties back into an object?)
  4. Delete all references to the propertyOverrides solution

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Apr 19, 2019

Yep, that's about it what we were thinking as well.

@moofish32
Copy link
Contributor

I'm digging in here, the code needs a little unwinding. Hopefully I'll have something in a couple of days.

@eladb eladb removed the package/awscl Cross-cutting issues related to the AWS Construct Library label May 1, 2019
moofish32 added a commit to moofish32/aws-cdk that referenced this issue Jun 17, 2019
…aws#2372)

fixes aws#2100

Modify the generation of CFN resources to create Class members for each
CloudFormation Property. This removes the need for the property override
solution implemented previously. CloudFormation Resource attributes are
now prefixed with `attr` instead of resource name. The RefKind patches
are removed. All L2 resources have been updated to consume the new
attribute names. The Ref attributes are removed and the new `refAsString`
is used.

Added tagging support for AppSync, AppMesh, StepFunctions.

Token now supports `number` types, so the generated code no longer
treats number as non-tokenizable.

BREAKING CHANGE: All L1 (CFN) Resources have attributes prefixed with
`attr`. For example, in S3 `bucket.bucketArn` is now `bucket.attrArn`.
The property overrides for typed properties has been removed, instead
users can now directly access CloudFormation properties on the class.
moofish32 added a commit to moofish32/aws-cdk that referenced this issue Jun 17, 2019
…aws#2372)

Modify the generation of CFN resources to create Class members for each
CloudFormation Property. This removes the need for the property override
solution implemented previously. CloudFormation Resource attributes are
now prefixed with `attr` instead of resource name. The RefKind patches
are removed. All L2 resources have been updated to consume the new
attribute names. The Ref attributes are removed and the new `refAsString`
is used.

Added tagging support for AppSync, AppMesh, StepFunctions.

Token now supports `number` types, so the generated code no longer
treats number as non-tokenizable.

BREAKING CHANGE: All L1 (CFN) Resources have attributes prefixed with
`attr`. For example, in S3 `bucket.bucketArn` is now `bucket.attrArn`.
The property overrides for typed properties has been removed, instead
users can now directly access CloudFormation properties on the class.

Fixes aws#2100
@eladb eladb closed this as completed in aa61dfb Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment