Graduate @aws-cdk/custom-resources to stable

[iliapolo]

No description provided.

[richardhboyd]

#5874 (comment)

[iliapolo]

@richardhboyd see my response: #5874 (comment)

[jogold]

#6061 (comment)

[jogold]

#6375

[jogold]

@eladb @rix0rrr @iliapolo regarding the 15 minutes / 14 minutes and retry question raised today: CloudFormation invokes the Lambda asynchronously. This means that we can now use Lambda destinations: you can get rid of the onEventFunction synchronously invoking the user onEventHandler function => no more "strange" 14 minutes limit in the doc.

We will have the user function directly called by CloudFormation and a "framework" Lambda function set as destination for both onSuccess and onFailure. The event received by this "destination" Lambda function contains of course the responsePayload of the user function but also the requestPayload (see Example Invocation Record) which is exactly what we need to submit the response to CF, equivalent of:

aws-cdk/packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts

Lines 34 to 36 in 8d5ee04

	// merge the request and the result from onEvent to form the complete resource event
	// this also performs validation.
	const resourceEvent = createResponseEvent(cfnRequest, onEventResult);

In a way the framework could be simplified to providing only a destination function? I think that it's even possible for the case with IsComplete.

What do you think?

[iliapolo]

@jogold This is nice, looks like we should be leveraging some of these capabilities.

Though I don't think we should start this endeavor just now. If this doesn't result in a change to the API, then it definitely doesn't qualify as a graduation blocker. And if it does, it sounds a bit risky to do now and i think is more suitable for the upcoming v2 of the CDK.

[iliapolo]

Summary of the API review

Following the API review we conducted, here are the points we discussed.

AwsCustomResources uses default STAR (*) permissions

https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts#L202

This grants the lambda that invokes the user-defined SDK calls permissions to all resources for a given service::action.

This is considered bad practice. For example, if a user defined the s3::DeleteBucket, the lambda could theoretically delete buckets it didn't intend to.

Action Item

Ideally we would require specific resource ARNs, but the nature of the resource makes it impossible because there is no ARN when invoking the onCreate function.

We decided that we should at least make the users aware of this fact and make this behavior opt in.

So basically we would require the user to pass policyStatements, unless a new property called permissive (better names are welcome) is set to true.

Clarify Async Operations

The Provider Framework section states that it:

- Supports asynchronous handlers to enable long operations which can exceed the AWS Lambda timeout

This is a bit confusing because it doesn't actually allow for long synchronous computations/executions. That is, a lambda function doing time.sleep(16 * 60) will not work.

Action Item

Clarify that the intent here is to allow for long waiting periods, as opposed to long executions.

AwsSdkCall Physical Resource ID

We currently have two different properties for defining the resource id:

• physicalResourceIdPath - Extract the ID from the response data.
• physicalResourceId - Explicitly set the resource ID.

One of these must be configured for both the onCreate and the onUpdate calls. It makes the API a bit cumbersome and we usually use a Union type for stuff like this.

Action Item

Create a union type for these two properties, something like:

export class PhysicalResourceID {
  public static fromResponseData(path: string): PhysicalResourceID {}
  public static fromString(id: string): PhysicalResourceID {}
}

There is quirk here because even with this union type, we can't make it a required property because the onDelete call doesn't require it. It's caused because we have a somewhat unnatural connection between AwsSdkCall and AwsCustomResourceProps. This means we would still have a runtime validation. I think it's still worth doing because it does make the API safer and cleaner.

AwsCustomResource: getData() & getDataString()

The names are a bit confusing, as well as the docstring:

Returns response data for the AWS SDK call

Which call? This actually doesn't relate to any call but simply returns attributes of the custom resource that was created/updated by sdk calls.

Action Item

Think of a better name. Maybe just getAttr? Its essentially just delegating to the inner customResource.getAttr methods...

AwsSdkCall.catchErrorPattern

We talked about the name not being clear enough. It also surfaced the issue of using getData in conjunction with this property.

When using catchErrorPattern, we are intentionally ignoring errors in the sdk call. Since getData relies on the Data returned by the call, we will inevitably lookup non existing data.

Action Item

• Rename catchErrorPattern to ignoreErrorCodesMatching

• Validate what the behavior is when using catchErrorPattern with getData

• Create issue for disallowing the usage of catchErrorPattern along with getData and onCreate.

  We talked about adding a backlog item here, but this is actually a breaking change, so when did we think of adding it if not now? Are we assuming it already failing in unexpected ways now so it's not really a breaking change?

User defined state machines

@jogold mentioned that we could add the ability to specify custom state machines to allow for long running executions.

Action Item

@jogold Can you please open a ticket for this with some more details and context?

14 minute timeout

Our README states:

It is recommended not to exceed a 14 minutes timeout

We talked about maybe reducing this to 5-7 minutes because of retries that might happen our of our control.

Action Item

Dig a little into the invocation behavior and see if this number actually makes sense or not.

Package Restructuring

We talked about the fact that the custom-resources package contains things that are not exactly related to one another. The AwsCustomResource doesn't use the provider framework, but uses the cfn.CustomResourceProvider directly, which is also used by the provider framework. Its hard to navigate these 3 constructs to understand the exact structure and usage.

In addition, the name AwsCustomResource is also not very clear.

Action Item

We initially talked about simply extracting aws-custom-resource to another package as part of this graduation process.

However, @eladb and I were talking that this is probably too intrusive for now and wouldn't really provide our customers with much value. So the action item becomes:

• Create an issue to split aws-custom-resource to its own package as part of CDK v2.

Also, we would like to restructure the aws-cloudformation package and actually attempt to get rid of it because it doesn't really represent a resource library. All the stuff in it can probably either go in core or in custom-resources.

So another action item:

• Create an issue to cleanup aws-cloudformation and move its contents to more relevant packages, also as part of CDK v2.

[jogold]

@iliapolo I will try to work on the items for AwsCustomResource next week.

Think of a better name. Maybe just getAttr? Its essentially just delegating to the inner customResource.getAttr methods...

I think that we agreed on getResponseField

Create issue for disallowing the usage of catchErrorPattern along with getData and onCreate.

I think we can do this now, it's easy.

We talked about maybe reducing this to 5-7 minutes because of retries that might happen our of our control.

There are no retries, this function (the user function) is invoked synchronously.

[jogold]

Though I don't think we should start this endeavor just now. If this doesn't result in a change to the API, then it definitely doesn't qualify as a graduation blocker. And if it does, it sounds a bit risky to do now and i think is more suitable for the upcoming v2 of the CDK.

I think that it should be possible to do this as a refactor without bringing breaking changes.

[iliapolo]

@jogold wrote:

I think that it should be possible to do this as a refactor without bringing breaking changes.

Excellent, even better since it means we can do this after graduation. We want to make as little changes as possible for this graduation push.

@eladb thoughts? (Relates to #5873 (comment))

[eladb]

Yes, I like the idea, but definitely not something I would do right now. Let's open an issue with this info.

[iliapolo]

Summarizing action items for graduation:

• Enforce star permissions opt-int for auto-generated policies (feat(custom-resources): enforce user opt-in when auto-generating SDK call policies #6521)
• Replace physicalResourceId and physicalResourceIdPath with PhysicalResourceId union type (feat(custom-resources): physical resource id union type #6518)
• Rename catchErrorPattern to ignoreErrorCodesMatching and add validation that its not being used in combination with getData and onCreate. (feat(custom-resources): rename catchErrorPattern to ignoreErrorCodesMatching #6553)
• Rename getData to getResponseField (feat(custom-resources): rename getData* to getResponseField* #6556)
• README clarifications: (chore(custom-resources): README clarifications #6554)
  • Long waiting time vs long execution time.
  • 14 minutes retry - consider reducing it.
• Clear awslint exclusions (chore(custom-resources): clear awslint exclusions #6523)

@jogold thanks for stepping forward and offering to do some of these! But i think i'll have it all covered soon enough. So don't worry about it, you can focus on the bigger features if you want :)