search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.72k stars 3.94k forks source link

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

Closed fogfish closed 2 years ago

fogfish commented 5 years ago

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

This is :bug: Bug Report

nmussy commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I can take the PR if no one wants it 👍

kaizencc commented 4 years ago

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

https://github.com/aws/aws-cdk/blob/7ec598eff0cbdc88c6a80d5d23d533e892f868e8/packages/%40aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-target-group.ts#L313-L330

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 commented 2 years ago

Please re-open if you are experiencing this issue

github-actions[bot] commented 2 years ago

⚠️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.