fix(cloudformation-module): Use physicalResourceId when not provided by custom resource

[msailes]

Issue #, if available:
#1081

Description of changes:

Use physicalResourceId when not provided by custom resource.

Checklist

• Meet tenets criteria
• Update tests
• Update docs
• PR title follows conventional commit semantics

Breaking change checklist

No breaking changes

Response.success() and Response.failed are deprecated

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[mriccia]

Please can we add the following tests:

• CREATE with a user provided physicalResourceId via Response.success(physicalResourceId) and Response.failed(physicalResourceId)- validate that the physicalResourceId is sent to CF
• UPDATE with a user provided physicalResourceId via Response.success(physicalResourceId) and Response.failed(physicalResourceId)- validate that the physicalResourceId is sent to CF
• DELETE with a user provided physicalResourceId via Response.success(physicalResourceId) and Response.failed(physicalResourceId)- validate that the physicalResourceId is sent to CF

Adding these tests will ensure that we are indeed sending the provided physicalResourceId to Cloudformation, and they will help us catch any future regressions

[mriccia]

A further improvement on this functionality would be to allow Powertools users to specify which physicalResourceId to use, in the case of an exception.
This may not be required for this PR, but worth considering in future improvements.

A few scenarios where this would be useful:

• during an UPDATE, the customer code may be trying to create a new resource. if an exception is thrown, a FAILED response is sent to CloudFormation with the previous physicalResourceId. During the rollback, the stack will trigger an UPDATE with the previous Custom Resource properties.

Example: a custom resource creates an S3 bucket and with a specified lifecycle rule for expiring objects.
When an UPDATE happens with a different AWS S3 bucket name, the Custom resource will need to create a new S3 bucket with the new bucket name.
if the Custom Resource throws an exception after the new bucket is created, the response to CF will be with the previous physicalResourceId. During the rollback, CF will invoke the custom resource with the previous bucket name and lifecycle rules, leaving the possibility of an orphaned S3 bucket.
Instead, if the user can specify a different physicalResourceId (because they are replacing the underlying resources), during the failure CloudFormation will be notified of this replacement and during the rollback will issue a DELETE of the new physicalResourceId.

• during a CREATE of a custom resource, there is an exception in the Handler code. physicalResourceId will not be present in the CloudFormation Event, therefore the LogStreamName is returned as the physicalResourceId. During the rollback, Cloudformation will issue a DELETE with the new physicalResourceId. The user code may not know what to do with physicalResourceId as it does not give it any context

[scottgerring]

@mriccia good idea! I've added tests to cover both SUCCESS/FAILED responses specifying physicalResourceId from the user's handler.

[scottgerring]

The implementation looks like it satisfies our goals of 1/ resolving #1081 while 2/ not introducing breaking changes; both extensive discussion and reviewing of the code and our new test coverage help ensure this.

I have one outstanding question inline about the logic for physicalResourceId choice being spread about. This might just how it has to be to maintain the interfaces, but I wanted to raise for discussion before we merge.

[mriccia]

Thanks for adding in the additional tests Scott.
We should make 1 modification before pushing, in CloudFormationResponse.java here and here we should also retrieve the physicalResourceId from the event if present.

Separately, I agree that we need to centralise the logic on assigning physicalResourceId to a single place, in my view this can be done in V2.

[mriccia]

LGTM

OK to move on