s3|ecr: auto-delete-[objects|images] breaks on cloudformation rollback (suspected)

[kaizencc]

Based on the way the custom resource is implemented, it is likely that unexpected behavior happens on Cloudformation rollback, i.e. the custom resource will prematurely delete the objects.

Consider the following scenario:

UPDATE target resource (replacement, creates a new resource)
UPDATE custom resource (old -> new, objects in old bucket are deleted)
(...stuff happens...)
ERROR, triggers a rollback
UPDATE custom resource (new -> old)
DELETE target resource (deletes the new resource, remembers the existing one)

We will have deleted objects in the bucket that has been rolled back to in this scenario.

---

The correct way to handle this is what was done in synthetics:

aws-cdk/packages/@aws-cdk/custom-resource-handlers/lib/aws-synthetics-alpha/auto-delete-underlying-resources-handler/index.ts

Lines 26 to 36 in 3d6d042

	async function onUpdate(event: AWSLambda.CloudFormationCustomResourceEvent) {
	const updateEvent = event as AWSLambda.CloudFormationCustomResourceUpdateEvent;
	const newCanaryName = updateEvent.ResourceProperties?.CanaryName;
	// If the name of the canary has changed, CloudFormation will delete the canary
	// and create a new one with the new name. Returning a PhysicalResourceId that
	// differs from the event's PhysicalResourceId will trigger a `Delete` event
	// for this custom resource. Here, if `newCanaryName` differs from `event.PhysicalResourceId`
	// then this will trigger a `Delete` event.
	return { PhysicalResourceId: newCanaryName };
	}

As opposed to

aws-cdk/packages/@aws-cdk/custom-resource-handlers/lib/aws-s3/auto-delete-objects-handler/index.ts

Lines 23 to 35 in 3d6d042

	async function onUpdate(event: AWSLambda.CloudFormationCustomResourceEvent) {
	const updateEvent = event as AWSLambda.CloudFormationCustomResourceUpdateEvent;
	const oldBucketName = updateEvent.OldResourceProperties?.BucketName;
	const newBucketName = updateEvent.ResourceProperties?.BucketName;
	const bucketNameHasChanged = newBucketName != null && oldBucketName != null && newBucketName !== oldBucketName;
	/* If the name of the bucket has changed, CloudFormation will try to delete the bucket
	and create a new one with the new name. So we have to delete the contents of the
	bucket so that this operation does not fail. */
	if (bucketNameHasChanged) {
	return onDelete(oldBucketName);
	}
	}

[GavinZZ]

I can confirm that this is a reproducible issue. Here's the cdk code I used

const bucket = new s3.Bucket(this, 'Bucket', {
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      bucketName: <a bucket name that's not used>,
      autoDeleteObjects: true,
    });

    // Intentionally failure since `mybucket-1` exists
    const bucket2 = new s3.Bucket(this, 'Bucket2', {
      removalPolicy: cdk.RemovalPolicy.DESTROY,
      bucketName: <a bucket name that's not used>,
    });

    bucket2.node.addDependency(bucket);

Once the deployment is successful, add some random content to the bucket, then update the code so that the first bucket's bucketName is updated to another valid name. Update the second bucket's bucketName to be an existing bucket name, which will trigger a deployment failure hence roll back.

The stack is rolled back, but the content is the bucket is now deleted.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.