RFC655: Enhanced L1s

[aaythapa]

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

[mikejgray]

It's exciting to see big changes being proposed in the CDK! Thank you for the work on this one.

[mikejgray]

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!

[mikejgray]

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.

[mikejgray]

Cautiously optimistic about this approach, and definitely curious about the findings.

[aaythapa]

You can look at this branch for a deeper look on this POC: https://github.com/aws/aws-cdk/tree/conroy/generate

[johnf]

I love everything being proposed here. This will bring significant improvements!

Except for the _v2 suggestion. I understand why this approach is being suggested, but it doesn't feel right to me.

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

• release aws-cdk-lib 3.174.0 and 2.174.0.
• Keep the versions in sync
• The only difference would be v3 has the prop_v2 logic written out to prop
• So this should be automatable

Some alternatives I can think of

• Make a breaking change and release a codemod to help everyone upgrade their code
• Typescript magic: pass in something like cdk_type_version: 2 into every prop, which gives me the new types - in the future, we could make this go away. I suspect there is some Typescript magic that might allow us to set something globally and have this still work

[rehanvdm]

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.

[mrgrain]

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 _v2 is confusing and annoying. In particular deploying this pattern for enums will make it a regular occurrence across the L1 library, as opposed to using the suffix only in situations when an upstream type change is breaking. In response to this, it was noted that old properties would be marked as deprecated and that it is an IDE and documentation concern to ensure that users are not flooded with multiple of the same props to choose from.

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.

[tmokmss]

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.

[aaythapa]

This is a good example, my thought was we could do runtime validation (making sure one of these is set) but as you mentioned type check is faster. I'll add this example to the cons for option 1

[eladb]

Damn this stuff is hard!

For option 1, I agree that the loss of strong typing (turning required properties to optional) is a major drawback.

But option 1 is tricky for another reason: you will likely want to deprecate the legacy properties and keep only the modern versions. In the above example, it means that you will need to delete keySchema and only leave keySchema_v2 which is not a good outcome. If you end up modernizing keySchema, you'll need to remove keySchema_v2 and then you will penalize the users who migrated to the modern version earlier by breaking or deprecating their code.

I would say the best approach would be to go with option 3 and couple this feature with a new major version (v3) of the CDK, which will allow you to break the existing APIs. When we planned v2 we had a GitHub issue where we accumulated these major breaking changes, and at some point we got to a critical mass that justified a new major version.

If v3 is too far away and there's a very strong push to introduce this capabilities sooner, I think option 2 is a reasonable tradeoff albeit there's a chance it will cause the library to be too large.

I am wondering if perhaps as an intermediate step (before a major version is released), you can just add better documentation and runtime (synth-time) checks that the value is valid. This will go a long way to shift-left errors without breaking. In TypeScript, you could even use a type union like "foo" | "bar" | "baz" to improve typings without impacting JSII languages.

[aaythapa]

If you end up modernizing keySchema, you'll need to remove keySchema_v2 and then you will penalize the users who migrated to the modern version earlier by breaking or deprecating their code.

By modernizing keySchema do you mean if/when we go to CDK V3 we'd have to go back to keySchema?

I am wondering if perhaps as an intermediate step (before a major version is released), you can just add better documentation and runtime (synth-time) checks that the value is valid.

Updating the docs is a great idea and the implementation should be fairly straight forward. For the runtime validation, is it possible to do without introducing a new property and without breaking compatibility?

When we planned v2 we had a GitHub issue where we accumulated these major breaking changes?

Do you have a link to this github issue? Couldn't find it through a quick search 😅

[eladb]

If you end up modernizing keySchema, you'll need to remove keySchema_v2 and then you will penalize the users who migrated to the modern version earlier by breaking or deprecating their code.

By modernizing keySchema do you mean if/when we go to CDK V3 we'd have to go back to keySchema?

Yes. In the long run that's the "right" name.

I am wondering if perhaps as an intermediate step (before a major version is released), you can just add better documentation and runtime (synth-time) checks that the value is valid.

Updating the docs is a great idea and the implementation should be fairly straight forward. For the runtime validation, is it possible to do without introducing a new property and without breaking compatibility?

I believe it should be possible to only add runtime validations instead of introducing new types.

When we planned v2 we had a GitHub issue where we accumulated these major breaking changes?

Do you have a link to this github issue? Couldn't find it through a quick search 😅

https://github.com/aws/aws-cdk-rfcs/blob/main/text/0079-cdk-2.0.md

That's the relevant RFC

[go-to-k]

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.

[aaythapa]

Thank you all for the feedback everyone!

The general consensus seems to be that the features are valuable and would make a great addition to the L1s. However, the proposed implementation (_v2 suffix on the new properties), compromises UX while aiming to maintain backward compatibility. Most would prefer moving forward with CDK V3 instead.

I'll share this feedback with the team and provide updates here as they come. In the meantime, please feel free to share any additional comments or suggestions. Thanks again

[eladb]

Where does cfn-lint get the data from? Any reason not to include this data source directly?

[jfuss]

cfn-lint already has a process built that pulls the data from the AWS SDK (specifically boto3). Idea is to not re-implement that ourselves. CFN Lint has similar data requirements/usage, so (imo) makes more sense to re-use. Ideally this data should just be in the CFN schemas per service directly, which is something we can work with cfn-lint and CFN on directly. That is outside of this, but helps with the full picture.

[eladb]

AFAIK AWS CLI is based on the Smithy service definitions. Would it make sense to pull these as a data source and cross reference with the CFN service spec?

[jfuss]

This is (more or less) what cfn-lint is doing (see above comment) but packaging it as CFN schemas that we can just leverage instead of doing ourselves.

[eladb]

If there's enough "meat" for CDKv3, then I would recommend choosing this approach, but bear in mind that a major version is a big project (v2 tool us almost two years to release).

It is also okay to punt this particular enhancement until you are ready to release v3 given its breaking nature.

[eladb]

Damn this stuff is hard!

For option 1, I agree that the loss of strong typing (turning required properties to optional) is a major drawback.

But option 1 is tricky for another reason: you will likely want to deprecate the legacy properties and keep only the modern versions. In the above example, it means that you will need to delete keySchema and only leave keySchema_v2 which is not a good outcome. If you end up modernizing keySchema, you'll need to remove keySchema_v2 and then you will penalize the users who migrated to the modern version earlier by breaking or deprecating their code.

I would say the best approach would be to go with option 3 and couple this feature with a new major version (v3) of the CDK, which will allow you to break the existing APIs. When we planned v2 we had a GitHub issue where we accumulated these major breaking changes, and at some point we got to a critical mass that justified a new major version.

If v3 is too far away and there's a very strong push to introduce this capabilities sooner, I think option 2 is a reasonable tradeoff albeit there's a chance it will cause the library to be too large.

I am wondering if perhaps as an intermediate step (before a major version is released), you can just add better documentation and runtime (synth-time) checks that the value is valid. This will go a long way to shift-left errors without breaking. In TypeScript, you could even use a type union like "foo" | "bar" | "baz" to improve typings without impacting JSII languages.

[eladb]

I am wondering if perhaps it would be sufficient to just add fromCfnXxx to all L2s instead of juggling all of these interfaces.

These conversion static methods would be straightforward to implement:

public static fromCfnBucket(bucket: CfnBucket): IBucket {
  return this.fromBucketArn(bucket.attrBucketArn);
}

Indeed, it won't allow users to directly pass an L1 everywhere an IBucket is needed but it feels like this would be good enough to improve the ergonomics without too many breaking changes.

[aaythapa]

Bucket L2 already has a fromCfnBucket that implements what you mentioned but I'm not sure how many other L2s have something similar or what the effort to add the missing ones would be. Could be worthwhile alternative but I don't think it fixes the root issue (and it becomes another thing we have to manually make rather than having it be auto generated :/ )

[eladb]

It should be easy to enforce the fromCfnXxx static methods using awslint so it's semi-manual.

[mrpackethead]

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 _v2 is confusing and annoying. In particular deploying this pattern for enums will make it a regular occurrence across the L1 library, as opposed to using the suffix only in situations when an upstream type change is breaking.

I may have missed the point here, but to me, having enums where there is a defined list of possible values, is very valuable. Its an almost every day occurrence having to look at the docs, to check on valid values.

Why would this not be valuable?