aws-s3: SkipDestinationValidation should not have default value when not used. It triggers CR's accidentally

[heikkis]

Describe the bug

#30914 introduced default value that is set always to false. The wanted behaviour would be that it would be omit when not defined.

Similar issue like this which was rollback: #30121

Regression Issue

• Select this option if this issue appears to be a regression.

Last Known Working CDK Version

2.154.1

Expected Behavior

No default value. Default value triggers all custom resources.

Current Behavior

Added new default value -> triggers CRs

Reproduction Steps

Update CDK, CRs are triggered which causes problems in many CR definitions in generally (not designed for re-run).

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.154.1

Framework Version

No response

Node.js Version

latest

OS

any

Language

TypeScript

Language Version

No response

Other information

No response

[heikkis]

#30914
https://github.com/aws/aws-cdk/pull/30916/files#diff-86b9ce1dfff6c730deb18027ed257d3bdaefce441ece0354d481081972c98af2R81
SkipDestinationValidation is not set but still added as false.

[ashishdhingra]

Findings:

• Looks like PR feat(s3): add skip destination validation property #30916 per per file diff:
  • Introduced a mandatory property skipDestinationValidation which sets the SkipDestinationValidation in createResourceOnce() while creating Custom::S3BucketNotifications custom resource.
  • It also modified the notifications-resource-handler to by default use the value of skipDestinationValidation as false, if not defined.
• If we don't want to set the property SkipDestinationValidation if not defined, then we would need to make the property skipDestinationValidation optional.
• Per Amazon S3 > PutBucketNotificationConfiguration API documentation, the value for x-amz-skip-destination-validation is either true or false.

@heikkis Please advise an end-to-end scenario with minimal reproducible CDK code which demonstrates that the custom resource would be triggered when SkipDestinationValidation is not set. SkipDestinationValidation is only used when adding notifications. If not defined, it would default to false per Amazon S3 > PutBucketNotificationConfiguration API.

Thanks,
Ashish

[pahud]

I think @heikkis 's concern is that - when it's not set, it should leave as undefined(implicit false) rather than an explicit false.

https://github.com/yerzhan7/aws-cdk/blob/26b21e1f8bc934449031796ce336a5ae1a69971a/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L895C9-L895C34

This passes an explicit false when undefined, which is arguably wrong.

And because of that, here would be passed an explicit false which cause unnecessary custom resource update.

Am I understanding this correct?

[gracelu0]

Thank you for bringing this to our attention. I can implement a similar fix to #30418 where we set the default of SkipDestinationValidation as undefined to prevent triggering an update of the custom resource. Can you confirm if this change would cause another update? If so we can either warn the user or introduce a feature flag for customers who wish to keep the existing behavior where SkipDestinationValidation is set to false by default (and avoid another update of their CR setting the value from false to undefined with the fix).

[heikkis]

No parameter changes (add or delete) should be added by default to CR's. I think if it is undefined it want be added at all => wanted way 👍🏼

[ryanwilliams83]

I wrote a work-around for this today by finding all stack resources of type Custom::S3BucketNotifications and using the .AddPropertyDeletionOverride() escape hatch to remove the offending line of CloudFormation.

public static Stack OverrideBucketNotificationProperties(this Stack instance)
{
    var bucketNotifications = instance
            .Node
            .FindAll()
            .Select(x => x.Node.DefaultChild)
            .OfType<CfnResource>()
            .Where(x => x.CfnResourceType == "Custom::S3BucketNotifications");

    foreach (var resource in bucketNotifications)
        resource.AddPropertyDeletionOverride("SkipDestinationValidation");

    return instance;
}

[mdw123]

@ryanwilliams83 not to ask too stupid of a question, but how do you apply your workaround? Thank you!

[ryanwilliams83]

@ryanwilliams83 not to ask too stupid of a question, but how do you apply your workaround? Thank you!

My workaround is code in the CDK stack that you add last in the constructor. It enumerates all nodes in the stack safe casting them from IResource to CfnResource. Then filter the list to find the node that is the CustomResource itself. Finally use the Amazon provided "escape hatch" function CfnResource.addPropertyDeletionOverride() to cause CDK to Delete a CloudFormation property from the synthesized output.

The problem arises because the CloudFormation synthesised with new versions of CDK contains the SkipValidation property whereas older versions did not. The fact that the CloudFormation has changed between the time you first provisioned your bucket trigger now causes the CR (Custom Resource) to be executed by CloudFormation which can result in the error about ambiguous triggers.

As you may be aware CloudFormation will attempt to provision new resources before cleaning up old ones while applying a "Change Set".

If you're still stuck paste my code into ChatGPT, tell it you found a workaround for the ambiguous bucket trigger problem introduced when the SkipValidation property was recently added, and ask it to write the equivalent work-around in your language of choice.

[megarach0]

@heikkis I'm working on a fix