RFC655: Enhanced L1s #657
RFC655: Enhanced L1s #657
Conversation
| Currently, L1 constructs in the CDK perform minimal validation on properties, mostly ensuring type correctness. We can extend these validations by leveraging additional data from our sources. Depending on the property type, various validation rules can be applied, as defined in the schema: | ||
|
|
||
| 1. String | ||
| 1. `minLength`: min length the string can be | ||
| 2. `maxLength`: max length the string can be | ||
| 3. `pattern`: the pattern the string must match (commonly used for arns) | ||
| 4. `enum`: the enum values the string can be, this likely won’t be part of the validation as we’ll have an enum type | ||
| 5. `const`: the constant the string must be | ||
| 2. Array | ||
| 1. `minItems`: min number of items in the array | ||
| 2. `maxItems`: max number of items in the array | ||
| 3. Integer | ||
| 1. `minimum` | ||
| 2. `maximum` |
There was a problem hiding this comment.
This will be a huge help. It's immensely frustrating to have so many checks in your CDK code only to have a simple validation check fail in Cloudformation. Thank you for recommending this!
| const func = new lambda.CfnFunction(this, 'test', { | ||
| code: { | ||
| ... | ||
| }, | ||
| handler: 'index.handler', | ||
| role: r.attrArn, | ||
| runtime_v2: lambda.CfnFunction.Runtime.PYTHON310, // new V2 property | ||
| // if both runtime and runtime_v2 are set we can throw an error | ||
| timeout: 30 | ||
| }) |
There was a problem hiding this comment.
I can't think of a better way to manage the backwards compatibility issues here, but I'm not crazy about the possibility of having runtime_v2 defined but not runtime. Can't think of specific examples but I get the impression validation becomes a little more difficult - if you must have a property defined, now you have to account for that property and its _v2 variant, which could introduce issues for certain use cases.
|
|
||
| ### POC | ||
|
|
||
| There were some internal experiments done around this idea before. We concluded that generating `ICfn<Resource>` is fairly straight forward but actually backporting the interface to our existing L2 API is manual and runs the risk of accidentally creating breaking changes e.x turning a public property `IBucket` into `ICfnBucket` (since users could be referencing `IBucket` -specific properties) or renaming a parameter to an API can cause breaking changes in Python. If we move forward with this we'll continue the work from the experiments while keeping the findings in mind. |
There was a problem hiding this comment.
Cautiously optimistic about this approach, and definitely curious about the findings.
|
I love everything being proposed here. This will bring significant improvements! Except for the I think this adds cognitive load to the developer, I have to now think about wether to use _v2 or not. Also, does this stay around forever, or does it get dumped in a future change? DO we end up with _v3 down the track? My suggestion: Bite the bullet, make a breaking change and bump the major version. A possible approach to help, if we release today
Some alternatives I can think of
|
|
I love the idea of having Interfaces for the CFN L1s and improving the L1-L2 interoperability. I agree with @johnf, it has been a few years since a major release and there are many Flags in cdk.json and @deprecated functions. Including this big change, it justifies a new major release for me. |
|
At re:Invent 2024 I had the opportunity to discuss this with a number of AWS Heroes: @hoegertn @Osterjour @pgarbe This is a write-up of this discussion from my perspective. Please add to it as required. It was questioned if enums are necessary in L1s as they don't seem to add much value and take L1s further away from what the CFN is. It was also noted that the use of The discussion then dived into what L1s are representing, what they should be representing and what they could be representing in future. There was consensus that L1 are currently roughly equal to CFN, however it was not clear what that means exactly. As an example, Validations were brought up which are currently not part of L1s but having them would make them arguably closer to what CFN does. Another example was IAM grants. While the group agreed that this is not currently part of CFN, it would be very useful if grants can be auto-generated from a data source. Based on this, the idea was proposed that L1s could be everything that can be auto generated (i.e. not hand-written) from a data source. It was suggested that maybe this should be separate layer and the proposed features in the RFC plus future features like grants could be introduced as a new layer between the current L1 and L2. However concerns were raised that this was also confusing. End of the discussion. |
|
|
||
| ### Recommended approach | ||
|
|
||
| Adding new property to the existing L1s would be the recommended approach. The suffix of `_v2` should make it clear which property to use and alleviate any confusion. We can also deprecate the older properties to signal to developers to use the new one. By deprecating the older properties rather than removing them, we ensure that existing code continues to function without breaking. Developers can still use the original properties, while being encouraged to adopt the new "_v2" version at their own pace. |
There was a problem hiding this comment.
I love the more thorough validations added with the enhanced L1s!
One question, how does this work when the original prop is required? For example in CfnTableProps, the keySchema property is required, something like below:
interface CfnTableProps {
keySchema: FooType
}After enhancing L1, we cannot make both original and v2 prop required, because users only need to set one.
// NOT WORK
interface CfnTableProps {
keySchema: FooType
keySchema_v2: FooType
}So I think we have to make the interface something like this:
interface CfnTableProps {
keySchema?: FooType
keySchema_v2?: FooType
}But imho this is actually worse than the original L1, because it lost the information that keySchema is a required property. (of course we can do runtime validation, but type check is faster.)
This is another reason why I'm not a big fan of option 1. I like option 3 better with the upgrade of aws-cdk-lib to v3 (and possibly aws-cdk v3) as @johnf said. With cdk v3, we can also remove all the feature flags that have been added since the v2 release, which has been a burden for construct library maintainers to validate their code with various possible flags.
|
This is an excellent suggestion. It would be especially nice to have the built-in validation in L1 by default. And most of my opinions have already been mentioned above. Introducing V2 for each parameter could lead to complexity (props become bloated, required properties become optional, etc.). Also, many users have created a mechanism to check against L1 resource properties using Aspects, and I'm concerned that the addition of this v2 property might break that (existing) mechanism. Therefore, I agree with the opinion to upgrade with CDK V3 (or aws-cdk-lib-v2 mentioned as option.3) as they say. |
This is a request for comments about enhancing L1s in CDK. See #655 for additional details.
APIs are signed off by @alias.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license