elbv2: ApplicationTargetGroup should validate that port is specified (when required)

[fogfish]

ApplicationTargetGroupProps defines all properties optional. However, it fails at runtime if port property is not defined.

Reproduction Steps

import * as alb from '@aws-cdk/aws-elasticloadbalancingv2'

new alb.ApplicationTargetGroup(this, 'DefaultTarget', {});

Error Log

A port must be specified (Service: AmazonElasticLoadBalancingV2; Status Code: 400; Error Code: ValidationError; Request ID: xxx)

Environment

• CLI Version :1.13.1 (build 96cfc63)
• Framework Version:1.13.1 (build 96cfc63)
• OS :MacOS
• Language :TypeScript

---

This is 🐛 Bug Report

[nmussy]

Hello @fogfish,

The port parameter not being required is due to the following, as described in the docs:

If the target is a Lambda function, this parameter does not apply.

Same with Protocol and VpcId.

We could add some client-side checks (with exceptions being thrown), but I'm not sure it's possible to do it with types given the current API.

[fogfish]

Yes, this is definitely make sense. Thank you for hints! May be similar statement shall be applied to CDK docs
https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-elasticloadbalancingv2.ApplicationTargetGroup.html

Feel free to close the issue!

One side comment. I've notice that similar issue is reflected to other Props. I know it would not be closed as part of this bug report. However, have you considered to declare a narrow Props that concern a particular use-case. It would make API a typesafe good for developers but of course it would require maintenance from your side. In typescript, this is called 'Discriminated Unions'.

[rix0rrr]

Unfortunately we're not currently allowing type unions, since not all languages we're targeting with jsii support them natively and we haven't narrowed down a way to represent them in those languages.

We can still keep this issue open, even if there is no static checking, there should be runtime validation in the construct so you don't need to wait for a CloudFormation deployment to detect this situation.

[fogfish]

Oh... True... TypeScript is not a central language for AWS CDK. I'll wish it will be the only one ;-)

Thank you for feedback!

[nmussy]

I can take the PR if no one wants it 👍

[kaizencc]

I took a look into this, and it looks like the validation method is already there, introduced in #3348:

aws-cdk/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts

Lines 313 to 330 in 7ec598e

	protected validate(): string[] {
	const ret = super.validate();
	if (this.targetType !== undefined && this.targetType !== TargetType.LAMBDA
	&& (this.protocol === undefined || this.port === undefined)) {
	ret.push('At least one of \'port\' or \'protocol\' is required for a non-Lambda TargetGroup');
	}
	if (this.healthCheck && this.healthCheck.protocol && !ALB_HEALTH_CHECK_PROTOCOLS.includes(this.healthCheck.protocol)) {
	ret.push([
	`Health check protocol '${this.healthCheck.protocol}' is not supported. `,
	`Must be one of [${ALB_HEALTH_CHECK_PROTOCOLS.join(', ')}]`,
	].join(''));
	}
	return ret;
	}
	}

Since targetType is not required by the resource, I don't think we can do any additional validation if targetType is undefined, which is why this becomes an error only when deployed.

Outside of the fact that I'm pretty sure the error message ought to read "Both port and protocol is required" based on the logic, I don't think there's much to do here.

@rix0rrr perhaps you can double check but I think this can just be closed.

[madeline-k]

Please re-open if you are experiencing this issue

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