(aws-autoscaling): notificationTarget shouldn't be compulsory in LifecycleHook

[abeer91]

The notificationTarget property can be unset and by default it will be able to use CWE to deliver Autoscaling Group notifications.

ASG API - https://docs.aws.amazon.com/cli/latest/reference/autoscaling/put-lifecycle-hook.html , keeps [--notification-target-arn <value>] as an optional param.

I was able to use L1 construct - CfnLifecycleHook as such without providing the notificationTarget

        const lcHook = new autoscaling.CfnLifecycleHook(this, "LCHook", {
            autoScalingGroupName: nodegroup.autoScalingGroupName,
            lifecycleTransition: LifecycleTransition.INSTANCE_TERMINATING,
            defaultResult: DefaultResult.CONTINUE,
            lifecycleHookName: "TerminateLifecycleHook",
            heartbeatTimeout: 300
        })

Reproduction Steps

Documentation shows property is mandatory - https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-autoscaling.LifecycleHook.html

What did you expect to happen?

notificationTarget should be optional or should have a way to use the default CWE integration.

What actually happened?

Could not use CWE integration which is how aws-node-termination-handler needs to be configured

Environment

• CDK CLI Version : 1.97.0
• Framework Version:
• Node.js Version: v15.9.0
• OS : Mac
• Language (Version): TypeScript

Other

---

This is 🐛 Bug Report

[peterwoodworth]

Sorry for the long reply here,

You're right. This isn't required from the Cfn side of things, so the L2 construct could have notificationTargets be an optional property. This seems to be the only place the property is used: lines 104 and 113

aws-cdk/packages/@aws-cdk/aws-autoscaling/lib/lifecycle-hook.ts

Lines 104 to 115 in 32f00eb

	const targetProps = props.notificationTarget.bind(this, this);
	const resource = new CfnLifecycleHook(this, 'Resource', {
	autoScalingGroupName: props.autoScalingGroup.autoScalingGroupName,
	defaultResult: props.defaultResult,
	heartbeatTimeout: props.heartbeatTimeout && props.heartbeatTimeout.toSeconds(),
	lifecycleHookName: this.physicalName,
	lifecycleTransition: props.lifecycleTransition,
	notificationMetadata: props.notificationMetadata,
	notificationTargetArn: targetProps.notificationTargetArn,
	roleArn: this.role.roleArn,
	});

I'm not sure why this is required in the first place, but I can't find a reason for making it required

[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.